Convert E_RECOVERABLE_ERROR to an Exception

Clearly that's a valid problem. Seems like we're treating RECOVERABLE terribly. Granted its pretty akward to handle generically anyways. I'd have to look at this closer but my gut doesn't like the solution though. RECOVERABLE is its own brokenish thing and it doesn't seem like exceptions matches it correctly.

NOTE: The RFC this was based on didn't pass either. https://wiki.php.net/rfc/engine_exceptions.

Ok well looking at the patch I've got these observations.

1. +++ b/core/includes/errors.inc
   @@ -64,6 +64,24 @@ function _drupal_error_handler_real($error_level, $message, $filename, $line, $c
        _drupal_log_error(array(
   

   This log_error references E_RECOVERABLE but it will never be reached because we're throwing an exception now.

2. +++ b/core/includes/errors.inc
   @@ -64,6 +64,24 @@ function _drupal_error_handler_real($error_level, $message, $filename, $line, $c
   +      throw new ErrorException($message, $error_level, 0, $filename, $line);
   

   This subtly changes PHP's behavior too because it stops code execution. Granted, code relying on being able to run after a E_RECOVERABLE happens is probably trouble but it was one of the reasons for blocking the RFC and it really is hard to expect how external libraries work.

+++ b/core/modules/system/lib/Drupal/system/Tests/System/ErrorHandlerTest.php
@@ -91,6 +91,15 @@ function testErrorHandler() {
+    $this->verbose($this->content);

The request is already logged with verbose so I don't think this is needed.

Bah, while I'm still not a fan, it seems you're correct. This is actually a regression in functionality.

http://drupalcode.org/project/drupal.git/blob/refs/heads/7.x:/includes/e...

@neclimdul: Except it's not a regression. http://drupalcode.org/project/drupal.git/blob/refs/heads/7.x:/includes/e...

Yeah, that's exactly the calling code triggering the exit that stops code flow.

), $error_level == E_RECOVERABLE_ERROR);

This passes $fatal as true causing the code I linked to trigger exiting and stopping code flow.

Thanks for creating this issue! — I ran into this scenario a couple of times myself already (especially in the installer), but didn't take the time to figure out what's going on/wrong :)

1. +++ b/core/includes/errors.inc
   @@ -64,6 +64,24 @@ function _drupal_error_handler_real($error_level, $message, $filename, $line, $c
   +    if ($error_level == E_RECOVERABLE_ERROR) {
   ...
   +      throw new ErrorException($message, $error_level, 0, $filename, $line);
   

   I'm a bit confused: The error/exception handler handles an E_RECOVERABLE_ERROR and throws an ErrorException with code E_RECOVERABLE_ERROR — doesn't that result in infinite recursion?

   I guess PHP has some built-in magic for this use-case?

   Perhaps more interesting: Aren't we losing the (back)trace information, or is that retained as well?

   Lastly, the example usage on http://php.net/manual/en/class.errorexception.php passes the $error_level value as the third parameter instead of the second, which appears to comply with ErrorException::__construct():

   public __construct ([ string $message = "" [, int $code = 0 [, int $severity = 1 ...
   

   The example passes 0 as $code, and $error_level as $severity.

2. +++ b/core/includes/errors.inc
   @@ -64,6 +64,24 @@ function _drupal_error_handler_real($error_level, $message, $filename, $line, $c
   +      // Initialize a maintenance theme if the bootstrap was not complete.
   +      // Do it early because drupal_set_message() triggers a drupal_theme_initialize().
   +      if (drupal_get_bootstrap_phase() != DRUPAL_BOOTSTRAP_FULL) {
   +        unset($GLOBALS['theme']);
   +        if (!defined('MAINTENANCE_MODE')) {
   +          define('MAINTENANCE_MODE', 'error');
   +        }
   +        drupal_maintenance_theme();
   +      }
   

   Actually, I wonder whether this is still necessary in D8...?

   DRUPAL_BOOTSTRAP_FULL ought to be a thing of the past, no? ;)

   I'd be eager to simply remove it and see what happens, but happy to defer that to a separate issue ;)

3. +++ b/core/includes/errors.inc
   @@ -178,15 +196,6 @@ function error_displayable($error = NULL) {
    function _drupal_log_error($error, $fatal = FALSE) {
   

   It looks like the $fatal argument is obsolete with this change?

4. +++ b/core/modules/system/tests/modules/error_test/lib/Drupal/error_test/Controller/ErrorTestController.php
   @@ -30,6 +31,22 @@ public function generateWarnings($collect_errors = FALSE) {
   +    } catch (\ErrorException $e) {
   ...
   +    }
   +    catch (\Exception $e) {
   

   I have a clear preference (Drupal's) and I normally wouldn't comment on this in favor of making progress, but within a single patch, it would be great to decide on one style? ;-)

All this is really messed up, and in dire need of clean-up.

Let just bite the bullet and transform all errors into exceptions, and remove all errors.inc completely.

Let just bite the bullet and transform all errors into exceptions, and remove all errors.inc completely.

That's more or less what I've done in #1247666-5: Replace @function calls with try/catch ErrorException blocks, so if we want to go that direction, then I think this issue should be marked as duplicate.

Rerolled the patch to latest Release.

Code has been re rolled and committed as patch referring #18 patch

Queuing for review

https://wiki.php.net/rfc/engine_exceptions_for_php7

PHP 7 now throws specific exceptions for these errors. Perhaps it would be best to write a handler for these that throws the same exception as PHP 7 is going to throw in the PHP 5.x environment, allowing further error handling and logging to work in the same manner. For the assert tools patch I already wrote code to convert runtime assertions in this manner, but it would make sense to expand the scope of this out as much as possible.

As far as I can tell all E_RECOVERABLE_ERRORs are now Type Exceptions. So perhaps that is the route to take?

In PHP 5.x though, was E_RECOVERABLE_ERROR used for anything but the type hints? I've been looking for awhile and haven't found an example.

This was a bugsmash triage issue of the day. It was discussed by lendude, longwave and DanielVeza.

They agreed this issue is a task. And longwave questioned if this is still valid, read comments #27

The question to determine if this is still valid.

Yes, I would say this can be closed as outdated. This issue and all comments up thru #29 date from before php 7.0.0 was released, and by now php has converted almost all cases of E_RECOVERABLE_ERROR to throwing a Throwable - which is caught by Drupal\Core\EventSubscriber\ExceptionLoggingSubscriber or, in cases where that event subscriber is not in play, by _drupal_exception_handler().

I tested one of the few remaining E_RECOVERABLE_ERROR still in existence with this code: $var = (bool) gmp_mul(1, 1);
The error is logged ("Recoverable fatal error: Object of class GMP could not be converted to bool") and the browser gets a 500 page reading "The website encountered an unexpected error. Please try again later."

The error handler could convert this to an ErrorException, allowing the calling code to catch it, but unless someone has a good reason to do so, it seems safer to wait for php itself to figure out if/how to convert it. Since I don't see any bug/task left to solve here, and this issue was inactive for so long, I'll go ahead and close, but feel free to reopen if I missed something.