Handle uncaught exceptions

Subscribing. This makes MySQL fatal errors impossible to debug, so bumping to critical.

See also #710142: Handle exceptions in shutdown functions, maybe we should report these exceptions in a similiar way?

That could mean:
- Use _drupal_decode_exception as in the other issue to display the exception instead of simply printing them
- Check the error_level in _drupal_shutdown_function() as you did here

Fixes the issue, the way it does it is pretty self-documenting. RTBC. Would be nice not to see any more "Exception thrown without a stack frame in line 0".

Could we get a quick reroll minus the whitespace?

One last bit of whitespace removed, otherwise it's the same patch.

Since Dries just asked for a no-whitespace re-roll, and this fixes the most flamingly critical bug I can think of, committed to HEAD. Great sleuthing work, jbrown!

Oh, though one question. Is it possible to write tests for this at all so we don't EVER have this problem again?

I get this message in all tests now, when the default language isn't english. (here german)

Ein AJAX-HTTP-Fehler ist aufgetreten HTTP-Rückgabe-Code: 500 Im Folgenden finden Sie Debugging-Informationen. Pfad: /drupal7/batch?render=overlay&id=23&op=do StatusText: Service unavailable (with message) ResponseText: Additional uncaught exception thrown while handling exception.OriginalPDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupal7.simpletest253844locales_source' doesn't exist: SELECT s.source, s.context, t.translation, t.language FROM {locales_source} s LEFT JOIN {locales_target} t ON s.lid = t.lid AND t.language = :language WHERE s.textgroup = 'default' AND s.version = :version AND LENGTH(s.source) de [:version] => 7.0-dev ) in locale() (line 656 of F:\xampplite\htdocs\drupal7\modules\locale\locale.module).AdditionalPDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupal7.simpletest253844locales_source' doesn't exist: SELECT s.lid, t.translation, s.version FROM {locales_source} s LEFT JOIN {locales_target} t ON s.lid = t.lid AND t.language = :language WHERE s.source = :source AND s.context = :context AND s.textgroup = 'default'; Array ( [:language] => de [:source] => %type: %message in %function (line %line of %file). [:context] => ) in locale() (line 674 of F:\xampplite\htdocs\drupal7\modules\locale\locale.module). Fatal error: Exception thrown without a stack frame in Unknown on line 0

Same with a clean HEAD and only enabled german as default language.

An AJAX HTTP error occurred. HTTP Result Code: 500 Debugging information follows. Path: /drupal7/batch?render=overlay&id=2&op=do StatusText: Service unavailable (with message) ResponseText: Additional uncaught exception thrown while handling exception.OriginalPDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupal7.simpletest748206locales_source' doesn't exist: SELECT s.source, s.context, t.translation, t.language FROM {locales_source} s LEFT JOIN {locales_target} t ON s.lid = t.lid AND t.language = :language WHERE s.textgroup = 'default' AND s.version = :version AND LENGTH(s.source) de [:version] => 7.0-dev ) in locale() (line 656 of F:\xampplite\htdocs\drupal7\modules\locale\locale.module).AdditionalPDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupal7.simpletest748206locales_source' doesn't exist: SELECT s.lid, t.translation, s.version FROM {locales_source} s LEFT JOIN {locales_target} t ON s.lid = t.lid AND t.language = :language WHERE s.source = :source AND s.context = :context AND s.textgroup = 'default'; Array ( [:language] => de [:source] => %type: %message in %function (line %line of %file). [:context] => ) in locale() (line 674 of F:\xampplite\htdocs\drupal7\modules\locale\locale.module). Fatal error: Exception thrown without a stack frame in Unknown on line 0

Without t() , see drupal_742246.txt, I get this.

An AJAX HTTP error occurred. HTTP Result Code: 500 Debugging information follows. Path: /drupal7/batch?render=overlay&id=3&op=do StatusText: Service unavailable (with message) ResponseText: When called from JavaScript, simply output the error message. Fatal error: Exception thrown without a stack frame in Unknown on line 0

I officially disagree with the idea of "demoting to reduce the critical issue count".

Please demote if it does not qualify as critical, but not to make our numbers look better. Maybe that's what you're saying in your second sentence. But please, if this is critical (and I suspect it may be) leave it as critical.

http://drupal.org/node/314328:

Critical: This status is used to designate broken functionality that makes the project unusable.

And, of course, that we need ungodly ugly code ($error_level == ERROR_REPORTING_DISPLAY_ALL || (defined('MAINTENANCE_MODE') && MAINTENANCE_MODE == 'update')) { such reflects my thoughts from #673050: Database bugs became fatals: we added exceptions and we have no strategy to really handle them. This issue IS critical and NOT just session.inc. Recently matt2000 raised the question in IRC: what happens if an exception is thrown in .install? Now that we will add fields in .install that will happen often and then you get a fatal error and then you have a broken install. And then what else do we have that we have not thought of?

Once again: catch database exceptions where PDO would throw them, remove every other exception throwing and switch core to non fatal static error collections before it's too late.

Or if someone has a better overarching strategy then come ahead with it. "exceptions are awesome" is not a strategy to handle the sudden influx of fatals. In Drupal 6 a bad query was not a fatal it was a warning.

+1 chx in that I think it warrants further attention. IMHO whether a database query should be fatal depends on how critical that particular query is. *I* think it should be up to the caller to decide.

I don't see how that ugly code (no discussion there, it *is* ugly) is related to using exceptions or not. It is just necessary to decide if we want to print an error message or not. We would need something similar if we want to properly handle errors in that function without exceptions there.

Re catching PDO exceptions and convert them to errors: This would imho totally break transaction handling as we have it now (Rollback if an exception happens during node_save() and other functions that use it), just to name an example. Also, queries are IMHO not supposed to fail, and if you really expect that they do, you can always catch the exception and handle it. I fail to see how it is bad that an broken query is fatal?

Oh, and I am very aware that our current situation is not perfect, there *are* issues with exceptions, both in PHP (that is why this issue exists) and in Drupal. But I don't think "remove all of them" is a valid solution, not at this point.

@32: And how is that not possible right now? The default simply changed from a warning to a fatal error, you can always catch the exception and handle it in any way you want.

/me shrugs. Be it. I have already won't fixed my other issue and won't repeat all that I said there. I can only hope that someone in Drupal 8 will create a proper solution for this.

error_displayable surely can be written without a switch.

Almost. I do not think it worths saving a variable_get in a rarely called function like that. And so then what about

function error_displayable($error = NULL) {
  $error_level = variable_get('error_level', ERROR_REPORTING_DISPLAY_ALL);
  return 
    (defined('MAINTENANCE_MODE') && MAINTENANCE_MODE == 'update') ||
    ($error_level == ERROR_REPORTING_DISPLAY_ALL) || 
    ($error_level == ERROR_REPORTING_DISPLAY_SOME && isset($error) && $error['%type'] != 'Notice' && $error['%type'] != 'Strict warning'));
}

I already RTBC'd this when it was uglier...

Please read doc style document.

Newline between @param and @return block.
Function doc has to start with 3th person verb.

Still rtbc

Uh oh :)

+++ includes/errors.inc	11 May 2010 16:48:11 -0000
@@ -139,6 +139,27 @@
+ * When in mainenance mode or when error_level is ERROR_REPORTING_DISPLAY_ALL, ¶
+ * all errors should be displayed. For ERROR_REPORTING_DISPLAY_SOME, $error ¶
+ * will be examined to determine if it should be displayed.
+ *
+ * @param $error
+ *   Optional error to examine for ERROR_REPORTING_DISPLAY_SOME.
+ *
+ * @return
+ *   TRUE if an error should be displayed.
+ */
+function error_displayable($error = NULL) {
+  $error_level = variable_get('error_level', ERROR_REPORTING_DISPLAY_ALL);
+
+  return (defined('MAINTENANCE_MODE') && MAINTENANCE_MODE == 'update') ||
+    ($error_level == ERROR_REPORTING_DISPLAY_ALL) || ($error_level == ERROR_REPORTING_DISPLAY_SOME && ¶
+    isset($error) && $error['%type'] != 'Notice' && $error['%type'] != 'Strict warning');

There are trailing spaces...

Powered by Dreditor.

Better, this was already RTBC multiple times :)

Wow, that error_displayable() wrapper function is certainly a vast improvement in terms of readability. I also asked catch to take a look, as resident benchmarker of extra function calls, and he's cool with it.

The only thing is the PHPDoc for that function needs a bit of love. For example, there is nothing there that tells me when I need to call it. And I can't quite figure out why drupal_shutdown_function and drupal_write_session use it, but other places in bootstrap do not from reading the code.

#43: catch-session-exceptions.patch queued for re-testing.

edited the unholy return statement of error_displayable() a bit, no other changes.

If there was a drupal centric Seinfeld, this would have been a episode.

There is a typo in the errors.inc file. Maintenance is spelled incorrectly (missing a t) - "when in mainenance mode..."

Index: includes/errors.inc
===================================================================
RCS file: /cvs/drupal/drupal/includes/errors.inc,v
retrieving revision 1.5
diff -u -r1.5 errors.inc
--- includes/errors.inc	11 Apr 2010 18:33:43 -0000	1.5
+++ includes/errors.inc	13 May 2010 15:50:05 -0000
@@ -139,6 +139,29 @@
 }
 
 /**
+ * Determines whether an error should be displayed.
+ *
+ * When in mainenance mode or when error_level is ERROR_REPORTING_DISPLAY_ALL,
+ * all errors should be displayed. For ERROR_REPORTING_DISPLAY_SOME, $error
+ * will be examined to determine if it should be displayed.
+ *
+ * @param $error
+ *   Optional error to examine for ERROR_REPORTING_DISPLAY_SOME.
+ *
+ * @return
+ *   TRUE if an error should be displayed.
+ */
+function error_displayable($error = NULL) {
+  $error_level = variable_get('error_level', ERROR_REPORTING_DISPLAY_ALL);
+  $updating = (defined('MAINTENANCE_MODE') && MAINTENANCE_MODE == 'update');
+  $all_errors_displayed = ($error_level == ERROR_REPORTING_DISPLAY_ALL);
+  $error_needs_display = ($error_level == ERROR_REPORTING_DISPLAY_SOME &&
+    isset($error) && $error['%type'] != 'Notice' && $error['%type'] != 'Strict warning');
+
+  return ($updating || $all_errors_displayed || $error_needs_display);
+}

This fixes the typo; no other changes.

Nice clean-up and fixes a critical problem. Committed to CVS HEAD.