Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add watchdog entry if excpetion was thrown #522

Merged
merged 2 commits into from
Jun 1, 2015
Merged

Add watchdog entry if excpetion was thrown #522

merged 2 commits into from
Jun 1, 2015

Conversation

amitaibu
Copy link
Member

@@ -527,6 +527,9 @@ function restful_menu_process_callback($resource_name, $version = NULL) {
foreach ($e->getHeaders() as $header_name => $header_value) {
drupal_add_http_header($header_name, $header_value);
}

$severity = $e->getCode() < 500 ? WATCHDOG_NOTICE : WATCHDOG_ERROR;
watchdog('restful', $e->getMessage(), array(), $severity);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably use watchdog_exception.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't know about watchdog_exception :) I think we should use it only for the below case and when status code >= 500.

@amitaibu amitaibu changed the title WIP: Add watchdog entry if excpetion was thrown Add watchdog entry if excpetion was thrown May 28, 2015
@@ -527,6 +527,17 @@ function restful_menu_process_callback($resource_name, $version = NULL) {
foreach ($e->getHeaders() as $header_name => $header_value) {
drupal_add_http_header($header_name, $header_value);
}

if ($e->getCode() < 500) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we simplify this as:

// Even though it's an exception, it's in fact not a server error - it
// might be just access denied, or a bad request, so we just want to log
// it, but without marking it as an actual exception.
// If the code is 5XX then it is an actual server exception.
$severity = $e->getCode() < 500 ? WATCHDOG_NOTICE : WATCHDOG_ERROR;
watchdog_exception('restful', $e, NULL, array(), $severity);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the "problem" with watchdog_exception() is that it adds the line number where exception was thrown. However for exceptions < 500, we don't really need that info, since it's not really a server error, but just the way RESTful is taking advantage of exceptions to short circuit the flow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me.

e0ipso added a commit that referenced this pull request Jun 1, 2015
Add watchdog entry if excpetion was thrown
@e0ipso e0ipso merged commit e75c2e3 into 7.x-1.x Jun 1, 2015
@e0ipso e0ipso deleted the 521 branch June 1, 2015 04:26
@e0ipso
Copy link
Member

e0ipso commented Nov 7, 2015

This was ported to 7.x-2.x in #726.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants