Fix horrible things in the error reporting

I thinks this is enough.

This would make it even more robust.

Upon further testing, we don't even need to fatal the exceptions occuring inside the handler.

I still can't catch exceptions occurring inside module_implements() for some reasons.

Ok, this one passes the "chx" test. It turns out I was calling theme('page') even in maintenance mode.

The good patch.

Oh, and removed debugging cruft.

Here is a summary:
- this patch makes the new error and exception handler more robust to exceptions thrown during the error processing
- and make it works in MAINTENANCE_MODE

The "Fatal error: Exception thrown without a stack frame in Unknown on line 0" was the sign of PHP dying in horrible pain when an exception is thrown when running inside an error or exception handler.

When an exception occurs during a BatchAPI processing, you will see a cryptic error message about a 500 error. That was also one issue chx was trying to solve with his patch #1 above. We will need to teach the BatchAPI how to decode our new fatal error pages. That's another issue (#329015: BatchAPI should be able to read fatal error and exception messages).

From an API's point of view it would be cleaner and more consistent to use 'type' instead of '%type'. Given that _drupal_log_error() is an internal function, I'm cool with using '%xxx' though.

The terminology 'maintenance mode' really falls short here. We're not really in maintenance mode when we are printing an error message. For a while, that confused me (again). I wonder if that is something we can address in a future patch.

Rerolled the same patch :)

GO TESTING BOT, GO!! :D

It's right. BlogAPI and XML-RPC tests fail spectacularly with this patch.

Fixed the test failures (the simpletest error reporter was not updated).

Interestingly: there are notices during the XML-RPC process, that doesn't show up in our tests results. It's because that this process is called via drupal_http_request(), not via cURL, and we don't currently process X-Drupal-Assertion headers in drupal_http_request.

Committed to CVS HEAD. Thanks.

     $assertion = array(
-      $message,
-      $type,
-      $caller
+      $error['%message'],
+      $error['%type'],
+      $error['%function'],

This changes the third element from an array with 'function', 'file' and 'line' keys to a string containing only the function name, but without changing drupal_web_test_case::curlHeaderCallback() correspondingly.

This patch adds type hinting to error() and _assert() (this would have made debugging this error easier), and makes _drupal_report_error() pass the file name and line number as well. The Doxygen comment for error() is elaborated (based on the comment for _assert()).

This was originally raised in #243532: Catch notices, warnings, errors and fatal errors from the tested side comment #56 as a follow-up, but it turns out that it belongs here instead.

The standardisation for type hinting seems to be on Array (with an uppercase A), so rerolled the same patch. Added an unit test case for the error collection feature.

+   *   By default, the error comes from a function which names start with
+   *   'test'. 

That's not proper grammar. Possibly "whose" name "starts" with 'test'?

+      'description' => t("Performs tests on the Simpletest error and exception collecter."),

No interpolation -- use single quotes.

+  /**
+   * Collected errors.
+   */

Indented one too many spaces.

+  $awesomely_big = 1/0;
+  db_query("SELECT * FROM bananas_are_awesome");

In the interest of friendliness to "outsiders," can we please not have tests in here with inside jokes known only to like 10 people? :P $divide_by_zero and nonexistent_table would be fine. :P

+    if (count($this->collected_errors) == 2) {
+      ...
+    }
+    else {
+      if (count($this->collected_errors) != 2) {

The latter if is redundant.

Did you consider putting the test in modules/simpletest/simpletest.test? DrupalErrorCollectionUnitTest tests SimpleTest's error handling, while DrupalErrorHandlerUnitTest tests Drupal's error handling (which is also indicated by getInfo() returning 'group' => 'SimpleTest'), so I think it rather belongs in simpletest.test. But it isn't entirely clear to me how tests in modules/simpletests/tests/* are grouped wrt files and getInfo().

+ $this->assertEqual($error['group'], $group, t("Found '%group'", array('%group' => $group)));
The wording seems a bit awkward when the test actually compares two strings (rather than searching for a string using assertText()). I suggest e.g. t("Group was %group", ...).
Also the quotes around %group seems redundant when used with the % that adds -tags.

  /**
   * Collected errors.
   */
  protected $collected_errors;

The comment doesn't add anything useful and should either be omitted or elaborated.
Class members are usually declared at the top of the file.

Looks like this needs some work still.

Marked #337055: Filename, line and function not reported properly on PHP notice exceptions as a duplicate of this bug.

subscribe

Here, taking care of comments from #20 and #21.

I kept the private jokes, as I think there are pretty harmless.

catch fixed some typos.

Oh, I also kept the test in system.test, because it is very similar to the general error handling test, and both uses resources from system_test.module.

Reviewed this in irc and ran the tests., everything looks good now. I also think the in-jokes are harmless, not to mention banana is pretty standard as a silly word to use.

+function system_test_generate_warnings($collect_errors = FALSE) {
+  if (!$collect_errors) {
+    define('SIMPLETEST_DONT_COLLECT_ERRORS', TRUE);
+  }

Ow, my head. Can we please make this:

+function system_test_generate_warnings($collect_errors = FALSE) {
+  if (!$collect_errors) {
+    define('SIMPLETEST_COLLECT_ERRORS', FALSE);
+  }

...so we don't get into double-negatives? ;) It's also very unconventional to name a constant or function something you will NOT be doing.

I realize you're just moving code around, but it'd be nice to fix this as long as we're here. (mew!)

+
+  /**
+   * Overrides DrupalWebTestCase->error().
+   */
+  protected function error($message = '', $group = 'Other', Array $caller = NULL) {

....
+  function assertError($error, $group, $function, $file, $message = NULL) {

Either both of these should be uncommented or they both need PHPDoc. (I assume assertError is overriding DrupalWebTestCase->assertError(). Although I don't like it, it seems like the convention that's developed is not commenting overridden functions in sub-classes.

    * @param $group
    *   WHich group this assert belongs to.

Not part of your patch, but you fixed the comment right below this so why not? (mew.)

This issue has been promoted by webchick.

Fixed grammar.

This patch fixed my tests for several patches.

Revised patch that changes the case on the type hinting from "Array" to "array" since #318016: Type hint array parameters was committed.

This patch saved my life again. I was running SimpleTests and was booted out of the tests with a blank screen and an error like "Function does not exist: block_help in menu.inc" which was completely not helpful at all. With the patch, I was no longer booted out of the tests and could see where things were going wrong very easily.

This looks wrong:
+ $this->assertEqual(count($this->collectedErrors), 3, t('Two errors were collected'));

I changed 'Two' to 'Three' and committed this patch to CVS HEAD. Thanks.