Error handler swallows catchable fatal errors

I totally agree, even we made progress recently, this function (_drupal_"log"_error()) is still horrible.

This patch logs the fatal error a little bit earlier, so that we ensure that also CLI/AJAX requests has that advantage.
I doubt that this will be green because we now should ensure each message which is error_log(ged()) which is a really great thing!

Quick tip: If you put
$config['system.logging']['error_level'] = 'verbose';into settings.php we also print out the error/exception message + backtrace.

Also adding a related issue which also makes the exception handling a little bit more sane.

So the second problem seems to be indeed a bug which was not caught before.

What about

    if ($fatal) {
      $message = 'The website encountered an unexpected error. Please try again later.' . '<br />' . $message;
      $message .= '<!-- Error Code: ' . $code . ' on line ' . $line . ' -->';
      $response->setContent($message);
      $response->setStatusCode(500, '500 Service unavailable (with message)');
      $response->send();
      exit;
    }

Mh, I don't think we want to display fatal errors unless configured.

I'm adding a bit of code to have the error handler just throw E_RECOVERABLE_ERROR as a TypeException to match the behavior of PHP 7. That may fix this as a side effect.

I'm adding a bit of code to have the error handler just throw E_RECOVERABLE_ERROR as a TypeException to match the behavior of PHP 7. That may fix this as a side effect.

Yeah why not.

Reroll.

This issue certainly improves the developer experience, this is for sure.

Another reroll

.

Ha, let's try again.

So in fact the error handler swallows at least one bug: #2587913: Passing non ascii/special character can cause a database exception in the menu active trail

The other failure is easy to fix.

Be careful with the overall premise of mapping E_RECOVERABLE to PHP 7 Exceptions. E_Recoverable usually maps to PHP 7's TypeError exception, but not always. I was going over a grep of PHP 5.6's source code to find all the reasons E_RECOVERABLE can be thrown - it's not something easily done.

@Aki Tendo
Well, for this issue I'm really more interested in logging the errors, so people can fix their problems. Do you think doing just that is sane? We should talk about converting those errors to exceptions in another isuse.

Logging them should work. Also, PHP 7 has to be told to throw exceptions in many cases in it's config files for backwards compatibility reasons.

Now also with test coverage.

Thanks for tagging this for rc target triage! To have committers consider it for inclusion in RC, we should add a statement to the issue summary of why we need to make this change during RC, including what happens if we do not make the change and what any disruptions from it are. We can add a section <h3>Why this should be an RC target</h3> to the summary.

Re-roll of #24.

From what I can tell, the fail is actually a valid failure that is only unmasked by this change. All tests are passing, but the following exception is thrown:

The website encountered an unexpected error. Please try again later.
Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[22007]: Invalid datetime format: 1366 Incorrect string value: '\xC3\xA9\xC3\xB8\xC3\xAF...' for column 'name' at row 1: INSERT INTO {semaphore} (name, value, expire) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2); Array ( [:db_insert_placeholder_0] => active-trail:route:menu_test.exotic_path:route_parameters:a:1:{s:6:"exotic";s:51:" -._~!$'"()*@[]?&+%#,;=:#%&+/?éøïвβ中國書۞";}:Drupal\Core\Cache\CacheCollector [:db_insert_placeholder_1] => 14548410155637ec62e417b5.50014883 [:db_insert_placeholder_2] => 1446505600.9342 ) in Drupal\Core\Cache\CacheCollector->updateCache() (line 236 of core/lib/Drupal/Core/Cache/CacheCollector.php).
Drupal\Core\Cache\CacheCollector->updateCache()
Drupal\Core\Cache\CacheCollector->destruct()
Drupal\Core\EventSubscriber\KernelDestructionSubscriber->onKernelTerminate(Object, 'kernel.terminate', Object)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.terminate', Object)
Symfony\Component\HttpKernel\HttpKernel->terminate(Object, Object)
Stack\StackedHttpKernel->terminate(Object, Object)
Drupal\Core\DrupalKernel->terminate(Object, Object)

When I run that test without this patch, that error is also printed on the test route, but it is not causing the test to fail:

Oh yeah it totally is, we have a unknown failure in our testsuite. @catch would know the issue which converts the CID to a hash .

I think it's #2501191: CacheCollector can result in an exception is cid + classname is too long to be used as a lock name.

This is postponed on the fix in #2501191: CacheCollector can result in an exception is cid + classname is too long to be used as a lock name, which could use review :)

Yeah the other issue landed! Let's get see whether this is green again.

Unilaterally tagging RC target because this is causing hidden failures (both test and real-world) in core, and doesn't touch anything outside the error handler or tests.

1. +++ b/core/includes/errors.inc
   @@ -119,8 +119,10 @@ function error_displayable($error = NULL) {
   + *     - A recoverable fatal error, which is a fatal error
   

   Missing trailing period.

   I think I vaguely remember that "recoverable fatal errors" are treated differently in PHP7; IIRC they become a special kind of exceptions there. If my memory is accurate, I think we need to call that out here too?

2. +++ b/core/includes/errors.inc
   @@ -169,6 +171,12 @@ function _drupal_log_error($error, $fatal = FALSE) {
   +  // Ensure to log out to error log for fatals, so developers can find the
   +  // problems.
   

   Log fatal errors, so developers can debug them.

3. +++ b/core/modules/simpletest/src/TestBase.php
   @@ -831,6 +831,43 @@ protected function assertNoErrorsLogged() {
   +   * Asserts that a specific error have been logged to the PHP error.log.
   

   s/have/has/

4. +++ b/core/modules/simpletest/src/TestBase.php
   @@ -831,6 +831,43 @@ protected function assertNoErrorsLogged() {
   +   * @see TestBase::prepareEnvironment()
   

   Does this need a FQCN?

5. +++ b/core/modules/simpletest/src/TestBase.php
   @@ -831,6 +831,43 @@ protected function assertNoErrorsLogged() {
   +    debug($content);
   

   Debugging leftover.

   (IIRC we don't use debug(), the global function, for logging errors into test output. I could be wrong though.)

I think I vaguely remember that "recoverable fatal errors" are treated differently in PHP7; IIRC they become a special kind of exceptions there. If my memory is accurate, I think we need to call that out here too?

I think those would throw basically an exception so we would deal with it like that but yeah in case we care about them, we need a general strategy. Pretty sure akitendo has an issue about that out there.

Log fatal errors, so developers can debug them.

Well, its really about finding in the first place, so let's use both.

Debugging leftover.

Yeah that is a leftover.

+++ b/core/includes/errors.inc
@@ -169,6 +171,12 @@ function _drupal_log_error($error, $fatal = FALSE) {
+  // Ensure to log out to error log for fatals, so developers can find and debug
+  // the problems

So, I suggested an alternative sentence because:
- the verb "ensure" can be removed here without losing any meaning
- "log out to" sounds very strange
- "debug the problems" also sounds strange
- missing trailing period

So, then I suggest:

Log fatal errors, so developers can find and debug them.

Thank you.

Most recoverable fatal errors are TypeExceptions in PHP 7 if PHP 7 is configured to throw exceptions in those scenarios. For BC it doesn't do so by default.

So can we concentrate on this particular issue now?

Just a nit and a remark

1. +++ b/core/includes/errors.inc
   @@ -119,8 +119,10 @@ function error_displayable($error = NULL) {
   + *   An error is fatal in case:
   + *     - An exception is thrown and not caught by something else.
   + *     - A recoverable fatal error, which is a fatal error.
   ...
      $is_installer = drupal_installation_attempted();
   

   The indent is 2 spaces too big. The dash should align with "An error...". See https://www.drupal.org/node/1354#lists

2. +++ b/core/modules/simpletest/src/TestBase.php
   @@ -831,6 +831,42 @@ protected function assertNoErrorsLogged() {
   +    $rows = explode("\n", $content);
   ...
   +    file_put_contents($error_log_filename, implode("\n", $rows));
   

   I'm not sure if PHP is using \n regardless of platform or is OS aware when writing to log. I tried to look in the docs and php-src but no luck to find an answer.

There's the PHP_EOL constant.

Alright, let's use that, good point.

This was pretty much nitpicked to death; hence looks super clean now :)

+++ b/core/includes/errors.inc
@@ -119,8 +119,10 @@ function error_displayable($error = NULL) {
+ *   An error is fatal in case:
+ *   - An exception is thrown and not caught by something else.
+ *   - A recoverable fatal error, which is a fatal error.

Sorry to nitpick a dead patch then, but I don't actually understand the documentation here at all. I think "in case" is maybe a mistranslation? Would this documentation be correct?

TRUE for:
- An exception that is thrown and not caught by something else.
- A recoverable fatal error.
Non-recoverable fatal errors cannot be logged by Drupal.

Yeah maybe this makes more sense? Here is an undead patch.

Back to RTBC.

BE WARE OF THE ZOMBIE PATCH

Zombie patch?

Committed/pushed to 8.0.x, thanks!