Problem/Motivation

If the test suite is run in versions of PHP 7 or above, there are some situations where exceptions happen, but they are not caught by the custom error handler set in DrupalTestCase and the exit code is 0 (success), even with clear errors.

This leads to false positives in tests.

Steps to reproduce

See successful run with clear errors: https://git.drupalcode.org/issue/scheduler-3387331/-/jobs/159694
- See the "*** Status: 0" which is debug added to the script.
- We can also see HTML thrown into the error output and the test summary full of fails and exceptions, yet the status code is still 0

To force this error, you just need to call $this->drupalLogin(FALSE);

Proposed resolution

Newer versions of PHP throw other type of errors, not just "Exception", but they are all "Throwable", so try to catch that and then default into "Exception".

          try {
            $this->$method();
            // Finish up.
          }
          catch (Throwable $e) {
            // PHP7+ versions
            $this->exceptionHandler($e);
          }
          catch (Exception $e) {
            // PHP5.6 version
            $this->exceptionHandler($e);
          }

Suggested here: https://www.php.net/manual/en/class.throwable.php#124604

We can see that if the suggestion above is put in place, the exception is now caught and the exit code and results are as excepted. See here: https://git.drupalcode.org/issue/scheduler-3387331/-/jobs/159786

Remaining tasks

MR

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3393147

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

fjgarlin created an issue. See original summary.

fjgarlin’s picture

fjgarlin commented
Issue summary: View changes

fjgarlin opened merge request !4984

fjgarlin’s picture

fjgarlin commented
Status: Active » Needs review

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

mcdruid’s picture

mcdruid commented

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.

fjgarlin’s picture

fjgarlin commented

I've made the change suggested in #5.

poker10’s picture

poker10 commented
Status: Needs review » Reviewed & tested by the community

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.

  • mcdruid committed 5fa9cc2d on 7.x 
    Issue #3393147 by fjgarlin, mcdruid, poker10: Exceptions ignored in...
mcdruid’s picture

mcdruid commented
Status: Reviewed & tested by the community » Fixed

Great, thanks for fixing this!

jonathan1055’s picture

jonathan1055 commented

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 :-)

Status: Fixed » Closed (fixed)

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

poker10’s picture

poker10 commented

Updating credits for @jonathan1055 as per #10.

jonathan1055’s picture

jonathan1055 commented

Thank you @poker10

nicxvan closed merge request !4984