Support PHP7 EngineExceptions in the error handler

From the RFC:

There is some concern that by extending EngineException directly from Exception, previously fatal errors may be accidentally caught by existing catch(Exception $e) blocks (aka Pokemon exception handling). To alleviate this concern it is possible to introduce a new BaseException type with the following inheritance hierarchy:

BaseException (abstract)
 +- EngineException
 +- ParseException
 +- Exception
     +- ErrorException
     +- RuntimeException
         +- ...
     +- ...

As such engine/parse exceptions will not be caught by existing catch(Exception $e) blocks.

Whether such a hierarchy (with a new BaseException type) should be adopted will be subject to a secondary vote.

The secondary vote passed, so maybe we can use BaseException for the PHP7 type hint and \Exception for PHP<7? This would require two different error handler definitions depending on PHP version...

Just so we don't have to copypaste full classes, we could do something like this...

class DummyError {
  // common code
}

if (version_compare(PHP_VERSION, '7.0.0-dev') >= 0) {
  class Error extends DummyError {
    // copy-paste code that used to use \Exception in type hints
  }
}
else {
  class Error extends DummyError {
    //  code that uses \Exception in type hints
  }
}

Seems like on the mailing list they had the same idea:
http://www.serverphorums.com/read.php?7,1170469,1172542

See also https://github.com/php/php-src/pull/1095

Removing the type hint obviously would solve the issue as well :)

If we do want to keep it, I could write a test that parses the source code of both the PHP 5 class and the PHP 7 class and assures they're identical line by line (other than the type hint)?

I discussed with @dawehner and @Berdir on IRC and they lean toward dropping the type hint.

Upon doing a search for occurences of "Exception" type hints, I found the following:

./modules/migrate/src/Exception/RequirementsException.php: public function __construct($message = "", array $requirements = [], $code = 0, Exception $previous = NULL) {
./core/includes/bootstrap.inc:function watchdog_exception($type, Exception $exception, $message = NULL, $variables = array(), $severity = RfcLogLevel::ERROR, $link = NULL) {
./core/lib/Drupal/Component/Plugin/Exception/InvalidPluginDefinitionException.php: public function __construct($plugin_id, $message = '', $code = 0, \Exception $previous = NULL) {
./core/lib/Drupal/Component/Plugin/Exception/PluginNotFoundException.php: public function __construct($plugin_id, $message = '', $code = 0, \Exception $previous = NULL) {
./core/lib/Drupal/Core/Authentication/AuthenticationManager.php: public function challengeException(Request $request, \Exception $previous) {
./core/lib/Drupal/Core/Authentication/AuthenticationProviderChallengeInterface.php: public function challengeException(Request $request, \Exception $previous);
./core/lib/Drupal/Core/Database/RowCountException.php: public function __construct($message = NULL, $code = 0, \Exception $previous = NULL) {
./core/lib/Drupal/Core/Form/EnforcedResponseException.php: public function __construct(Response $response, $message = "", $code = 0, \Exception $previous = NULL) {
./core/lib/Drupal/Core/Installer/Exception/InstallerException.php: public function __construct($message, $title = 'Error', $code = 0, \Exception $previous = NULL) {
./core/modules/basic_auth/src/Authentication/Provider/BasicAuth.php: public function challengeException(Request $request, \Exception $previous) {
./core/modules/migrate/src/MigrateException.php: public function __construct($message = NULL, $code = 0, \Exception $previous = NULL, $level = MigrationInterface::MESSAGE_ERROR, $status = MigrateIdMapInterface::STATUS_FAILED) {
./lib/Drupal/Core/Cache/DatabaseBackend.php: protected function catchException(\Exception $e, $table_name = NULL) {
./lib/Drupal/Core/Cache/DatabaseCacheTagsChecksum.php: protected function catchException(\Exception $e) {
./lib/Drupal/Core/Form/EnforcedResponse.php: public static function createFromException(\Exception $e) {
./lib/Drupal/Core/Utility/Error.php: public static function decodeException(\Exception $exception) {
./lib/Drupal/Core/Utility/Error.php: public static function renderExceptionSafe(\Exception $exception) {
./modules/migrate/src/MigrateExecutable.php: protected function handleException(\Exception $exception, $save = TRUE) {
./modules/migrate/tests/src/Unit/TestMigrateExecutable.php: public function handleException(\Exception $exception, $save = TRUE) {

Then there are also the catch statements in _drupal_shutdown_function and _drupal_exception_handler...

function _drupal_shutdown_function() {
  $callbacks = &drupal_register_shutdown_function();

  // Set the CWD to DRUPAL_ROOT as it is not guaranteed to be the same as it
  // was in the normal context of execution.
  chdir(DRUPAL_ROOT);

  try {
    while (list($key, $callback) = each($callbacks)) {
      call_user_func_array($callback['callback'], $callback['arguments']);
    }
  }
  catch (Exception $exception) {
    // If using PHP-FPM then fastcgi_finish_request() will have been fired
    // preventing further output to the browser.
    if (!function_exists('fastcgi_finish_request')) {
      // If we are displaying errors, then do so with no possibility of a
      // further uncaught exception being thrown.
      require_once __DIR__ . '/errors.inc';
      if (error_displayable()) {
        print '<h1>Uncaught exception thrown in shutdown function.</h1>';
        print '<p>' . Error::renderExceptionSafe($exception) . '</p><hr />';
      }
    }

function _drupal_exception_handler($exception) {
  require_once __DIR__ . '/errors.inc';

  try {
    // Log the message to the watchdog and return an error page to the user.
    _drupal_log_error(Error::decodeException($exception), TRUE);
  }
  catch (Exception $exception2) {
    // Another uncaught exception was thrown while handling the first one.
    // If we are displaying errors, then do so with no possibility of a further uncaught exception being thrown.
    if (error_displayable()) {
      print '<h1>Additional uncaught exception thrown while handling exception.</h1>';
      print '<h2>Original</h2><p>' . Error::renderExceptionSafe($exception) . '</p>';
      print '<h2>Additional</h2><p>' . Error::renderExceptionSafe($exception2) . '</p><hr />';
    }

This removes type hints from the Error class, and catches any BaseExceptions that may occur in the shutdown function, or during handling or the first one.

Oops, missed your comment. How do they require different handling specifically (in this case)?

As written in #2454439: [META] Support PHP 7, there fewer errors/warnings in the error handling tests, but they still fail. I'm not sure if we want to fix that here or in a separate issue, but being able to pass the two relevant tests on php7 (ErrorHandlerTest and SimpleTestErrorCollectorTest) might be a good indicator to have things working? What I'm a bit worried about is those might require php5/7 specific code/assertions.

@Berdir what are the specific fails, does this patch fix any of them?

@neclimdul just to address your two questions:

Should it be second since its less likely?

Well EngineExceptions happening during exception handling may be purely theoretical, but catching a BaseException is still more likely regardless, as Exceptions are BaseExceptions as well :)

Should we make a class for 5.X so BaseException actually exists even if it won't be used?

I did consider this but in this specific case I don't see the added value? Apparently PHP is fine with trying to catch an exception of a nonexistent class, and I don't think we'll use BaseExceptions elsewhere... Anything that's not an actual Exception will be what used to be a parse error/fatal error, and those will be dealt with by the error handler.

On a side note, if this patch gets OK'd we'll have to fix some spelling:

• backwards compatibility
• Displays errors for any additional errors while handling an exception.

This fixes the error in ErrorHandlerTest:

Drupal test run
---------------

Tests to be run:
  - Drupal\system\Tests\System\ErrorHandlerTest

Test run started:
  Friday, April 10, 2015 - 14:38

Test summary
------------

Drupal\system\Tests\System\ErrorHandlerTest                   56 passes                                      

Test run duration: 13 sec

Nice, did you test the error collector test too?

@Berdir the error collector test didn't give me any errors, but I don't believe it gave me any errors on my baseline D8 either in PHP7. May be because I have a different setup.

If it's not a PHP bug, I guess it's a Drupal bug as we're supposed to support PHP7?

1. +++ b/core/includes/bootstrap.inc
   @@ -690,14 +690,32 @@ function _drupal_exception_handler($exception) {
   + * @param $exception
   + *   The first exception object that was thrown.
   + * @param $exception
   + *   The second exception object that was thrown.
   

   You can't type hint it, but for the @param, you can actually explicitly document it's \Exception|\BaseException

2. +++ b/core/includes/bootstrap.inc
   @@ -1078,18 +1096,35 @@ function _drupal_shutdown_function() {
   + * @param $exception
   + *   The exception object that was thrown.
   

   same here.

3. +++ b/core/includes/bootstrap.inc
   @@ -1078,18 +1096,35 @@ function _drupal_shutdown_function() {
   + * @see _drupal_shutdown_function
   

   @see functions need () at the end.

4. +++ b/core/lib/Drupal/Core/Utility/Error.php
   @@ -33,13 +33,15 @@ class Error {
   -   * @param \Exception $exception
   -   *   The exception object that was thrown.
   +   * @param $exception
   

   Here too, maybe the docblock below can be simplified then?

Yeah, still failing for me. In a *very* funny way :)

Drupal\system\Tests\System\ErrorHandlerTest                   52 passes   3 fails  -1 exceptions

Latest php7-master (I had to recompile from scratch, got messed up otherwise)

Sorry, forget about that, it's working, I messed up.

+++ b/core/modules/system/src/Tests/System/ErrorHandlerTest.php
@@ -49,8 +49,12 @@ function testErrorHandler() {
-      '!message' => 'Argument 1 passed to Drupal\error_test\Controller\ErrorTestController::Drupal\error_test\Controller\{closure}() must be of the type array, string given, called in ' . \Drupal::root() . '/core/modules/system/tests/modules/error_test/src/Controller/ErrorTestController.php on line 66 and defined',
+      '!message' => 'Argument 1 passed to Drupal\error_test\Controller\ErrorTestController::Drupal\error_test\Controller\{closure}() must be of the type array, string given, called in ' . \Drupal::root() . '/core/modules/system/tests/modules/error_test/src/Controller/ErrorTestController.php on line 66',

That is no longer line 66, but line 70 I believe :).

That should make that green again.

--

Besides that: Looks great!

Actually this patch will fail again, it is still line 66 I think as we don't change ErrorTestController

RTBC, looks great now!

It would be nice to get an answer on http://news.php.net/php.internals/85613 before proceeding with this.

Their interface doesnt show replies somehow, see http://www.serverphorums.com/read.php?7,1172307 for that

My other concern with this issue is that currently our exception handler is totally avoided if the front controller is used - see index.php. I'm not sure what we should about this.Adding yet another catch there is awful.

Personally I think we should remove the exception catch from index.php and put this information in the output of _drupal_exception_handler() but this is hard because _drupal_log_error() has too many dependencies.

Right, but is that really related to this issue? We're not changing any of that here, just making the same code work on PHP 7 as well.

Agreed that we should either drop the global exception handler or the stuff in index.php and not keep both. Exceptions that happen in controllers are handled separtely anyway.

There are also issues to try and remove our custom exception handling with whoops or a similar library.

I think we should go ahead with the patch here. Given they (PHP) need the BC break, dropping the typehinting is the most sensible thing we can do - unless we want to do conditional class loading based on PHP version ...

--

index.php only catches HttpExceptionInterface type exceptions.

RuntimeExceptions hence are never caught by that (they are converted from normal Exceptions inside the application however).

BUT: What is now BaseException has before been a fatal exit(1) WSOD error, so nothing a front-controller needed to handle anyway.

Therefore back to RTBC.

@Fabianx I do see a second catch in index.php for regular Exceptions...

Maybe we should still do a followup issue for removing those from index.php somehow?

Yes, but that Exception is re-thrown.

So while for consistency it is not good to not have \BaseException caught in the front-controller, it is also not a problem, because those have been WSOD in previous PHP versions anyway, so never reached that point and are now handled by the error handler.

So I think, yes lets follow-up on that to remove that and instead install our error handler way earlier (which is what this tries to do).

Similar to how the assertion patch works ...

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 700499c and pushed to 8.0.x. Thanks!

Sorry for changing the status, but you might interested in the ongoing RFC that removes `BaseException` and adds base class named `Error`. I'm not sure but probably also using `Drupal\Core\Utility\Error` would be hard if it won't be renamed.

RFC voting period ends on June 24th, 2015, but from current votes I assume it will pass.

Thanks, created #2505315: Catch PHP7 Throwable objects instead of BaseExceptions in the error handler as a follow up