Exceptions ignored in errorHandler for DrupalTestCase

MR is ready for review and the issue summary contains the findings and links with the fix in place too.

Interesting, thanks!

It probably doesn't make much difference, but it might be better to catch Throwable first as we'd hope earlier versions of PHP are now in the minority?

The examples given are that way around, but the MR currently adds catch (Throwable $e) after catch (Exception $e).

I'd probably then have a comment only in the Exception section to note that it's catering for older PHP.

These are nits though; the overall idea seems a good one.

I've made the change suggested in #5.

I think this looks good, thanks!

I have done a quick research and it seems like other frameworks used the same try-catch construction for cross-compatibility between PHP 5.6 and PHP 7. For example cakePHP (https://github.com/cakephp/cakephp/pull/11462/files).

DrupalCI is green on PHP 5.6 and I tested this also manually to see if the catch (Throwable $e) can cause any errors on PHP 5.6 and lower (where the Throwable interface is not defined). But it seem to work correctly.

The change of order should be correct, as starting in PHP 7, the Exception class also implements Throwable, so the catch (Exception $e) will be only a fallback for PHP 5.6 and thus better to be a second.

I am not sure if it is possible to add a test for this, because the code from DrupalTestCase::run():

try {
  $this->$method();
  // Finish up.
}
catch (Exception $e) {
  $this->exceptionHandler($e);
}

is intended to catch only exceptions triggered by the testing methods itself and therefore it is not run when testing DrupalErrorHandlerTestCase::testExceptionHandler(). So I am moving this to RTBC.

Great, thanks for fixing this!

Thanks for fixing this. I discovered it whilst helping to implement gitlab pipelines for D7 Contrib testing. But @fjgarlin did the work to fix it.

On #3391902: [D7] Phpunit job ends green OK, hiding a fatal php error or exception → I am also proposing a fix to the gitlab template to replicate the commit above, by patching the core file, see MR58 so that we get the benefit immediately, not having to wait for D7.99 to be released. It is unnerving having tests appear to pass green, when in fact they are not working at all :-)

Automatically closed - issue fixed for 2 weeks with no activity.

Updating credits for @jonathan1055 as per #10.

Thank you @poker10