PHPUnit's test runner overwrites custom error handler registered using set_error_handler() in bootstrap script

[marmichalski]

Q	A
PHPUnit version	10.5.4+
PHP version	N/A
Installation Method	Composer

Summary

There seems to be a change in behaviour starting with PHPUnit 10.5.4 when a custom error handler is used.
I have prepared a minimal repository with the issue here, handler is registered in bootstrap.php, and you can see failing tests here.
This seems to be caused by #5592.

Current behavior

PHPUnit swallows triggered error.

How to reproduce

Run tests in the repo.

Expected behavior

PHPUnit not taking precedence over custom error handler.

[sebastianbergmann]

CC @mvorisek

[mvorisek]

The current behaviour is expected - in https://github.com/marmichalski/phpunit-err-handler/blob/e4994c26/tests/TriggerTest.php#L19 you expect an exception beeing thrown because of https://github.com/marmichalski/phpunit-err-handler/blob/e4994c26/bootstrap.php#L7-L13 but the PHPUnit handler (registered since PHPUnit 10.x) prevents that because PHP handlers are not nested. So the behaviour is like when there would be no custom global handler - https://3v4l.org/KZVq4.

If you want a custom error handler to be used, use WithoutErrorHandler attribute or setUp and tearDown in your test class to setup and remove the custom error handler respectively.

[marmichalski]

I am not arguing it is incorrect. I am saying it changed in a patch release, see that 10.5.3 respected that and all tests passed.

[marmichalski]

Although, on the other hand, one could see it as incorrect and a bug instead.
If your application has a global error handler registered and it is used throughout that application, in every environment it is deployed to, why the behaviour would differ only in phpunit tests? Seems like you now have to disable PHPUnit's error handler for every test to have it behave in a reliable way.

You should probably also update the docs if that's the way to go https://docs.phpunit.de/en/10.5/error-handling.html#error-handling (re Other error handlers).

[mvorisek]

If your application has a global error handler registered

This is the same when your app would be used as a dependency which register it's own handler as well. It will be masked too.

In theory, we can emulate nested PHP error handler support in PHPUnit error handler, but I do not support this idea - PHPUnit should detect "non-execution-breaking warnings", not silence the side effects (like error_get_last()), but in my opinion, also not pass the warning further, imagine like theoretical PHPUnit in PHPUnit usecase, the main PHPUnit should not be informed about warnings already detected, ie. effectively handled, by the child PHPUnit.

Seems like you now have to disable PHPUnit's error handler for every test to have it behave in a reliable way.

If you rely on some global specifics in **unit** testing, you should set these specifics using before test hook.

[marmichalski]

If your application has a global error handler registered

This is the same when your app would be used as a dependency which register it's own handler as well. It will be masked too.

As application I've meant the actual final product here, not a library, but a project that can't be a dependency of anything.

Seems like you now have to disable PHPUnit's error handler for every test to have it behave in a reliable way.

If you rely on some global specifics in unit testing, you should set these specifics using before test hook.

Those global specifics are set globally in a bootstrap file, though, and the same file is included using PHPUnit's own bootstrap config.

Anyway, it seems that up until 10.5.3 PHPUnit was not overriding error_handler and started to do so in 10.5.4, which seems like a breaking change, right? Couldn't there be a better way of checking whether it should override the error handler or not, possibly by verifying if it came from a bootstrap file, if feasible?

[cracksalad]

Stuck with the same problem as @marmichalski
In my eyes this is definitly a breaking change, since my tests are not successfull anymore after updating to a backwards compatible (refering to SemVer) patch release.
Additionally the docs are wrong about this behavior as already mentioned.

What about a global WithoutErrorHandler attribute in the XML config? It would still be a breaking change, but the fix would not be to edit every single test case but just adding one line to the config. What do you think, would that be feasible? @mvorisek

[mvorisek]

Anyway, it seems that up until 10.5.3 PHPUnit was not overriding error_handler

@marmichalski as long as you did not use/used WithoutErrorHandler, this is not true, can you explain why https://github.com/marmichalski/phpunit-err-handler/blob/e4994c26/tests/TriggerTest.php#L19 was passing? Maybe because of #5566 issue?

Couldn't there be a better way of checking whether it should override the error handler or not, possibly by verifying if it came from a bootstrap file, if feasible?

In post above, in the paragraph starting with "In theory, we can emulate nested PHP error handler support in PHPUnit error handler"... I have discussed it there. PHPUnit should probably not propagate this "handled/logged warnings".

Stuck with the same problem as @marmichalski

@cracksalad Please provide a minimal repro of your usecase.

What about a global WithoutErrorHandler attribute in the XML config?

I do not support global WithoutErrorHandler attribute, as it will prevent PHPUnit to log these issues.

I might mildly support an option to allow to emulate the error handler nesting, which will solve this "app with bootstrapped error handler" usecase. But I still think there are better solutions like setting the own error handler before and after each test using PHPUnit hook.

[marmichalski]

Anyway, it seems that up until 10.5.3 PHPUnit was not overriding error_handler

@marmichalski as long as you did not use/used WithoutErrorHandler, this is not true, can you explain why https://github.com/marmichalski/phpunit-err-handler/blob/e4994c26/tests/TriggerTest.php#L19 was passing? Maybe because of #5566 issue?

https://github.com/marmichalski/phpunit-err-handler/blob/e4994c26aed29cdb86e135a5dc5ada930e6b699f/phpunit.xml.dist#L13 ?

[cracksalad]

@cracksalad Please provide a minimal repro of your usecase.

It is exactly the same as the one described above. I am setting a custom error handler in PHPUnit's bootstrap file to convert all the E_* errors and warnings into ErrorExceptions to be able to handle them properly. Some PHP internal functions still make use of those errors.

---

But I still think there are better solutions like setting the own error handler before and after each test using PHPUnit hook.

Even if that is the better solution in theory, it means a lot of work to put into existing test cases (there are a lot of them) right now - for a patch release!
Whatever the final solution will be, I think there should probably be an intermediate solution in another patch release to make this behavior change backwards compatible again (kind of).

[mvorisek]

Anyway, it seems that up until 10.5.3 PHPUnit was not overriding error_handler

@marmichalski as long as you did not use/used WithoutErrorHandler, this is not true, can you explain why https://github.com/marmichalski/phpunit-err-handler/blob/e4994c26/tests/TriggerTest.php#L19 was passing? Maybe because of #5566 issue?

https://github.com/marmichalski/phpunit-err-handler/blob/e4994c26aed29cdb86e135a5dc5ada930e6b699f/phpunit.xml.dist#L13 ?

I looked into the PHPUnit code:

Since PHPUnit 10.0.0 the PHPUnit error handler was always registered in

https://github.com/sebastianbergmann/phpunit/blob/10.5.3/src/Runner/ErrorHandler.php#L161

but **only if there was no other error handler**. This was of course not good as as discussed above, PHPUnit was not able to detect any warnings.

[cracksalad]

Since PHPUnit 10.0.0 the PHPUnit error handler was always registered but only if there was no other error handler.

This is at least documented:

When PHPUnit’s test runner becomes aware (after it called set_error_handler() to register its error handler) that another error handler was registered then it immediately unregisters its error handler so that the previously registered error handler remains active.

source: https://docs.phpunit.de/en/10.5/error-handling.html

[mvorisek]

Since PHPUnit 10.0.0 the PHPUnit error handler was always registered but only if there was no other error handler.

This is at least documented:

When PHPUnit’s test runner becomes aware (after it called set_error_handler() to register its error handler) that another error handler was registered then it immediately unregisters its error handler so that the previously registered error handler remains active.

source: https://docs.phpunit.de/en/10.5/error-handling.html

https://github.com/sebastianbergmann/phpunit/blob/10.5.3/src/Runner/ErrorHandler.php#L158-L164 is not strictly needed for/in #5592 and can be reverted for 10.x. Feel free to submit a PR, add an E2E test and ping me.

For 11.x, the current (10.5.4+) behaviour should be default as warnings should be always logged by PHPUnit with possibly a global option with options:

• always replace the error handler
• replace the error handler, log the warnings in PHPUnit, but call the parent error handler too
• replace the error handler, but do not log the warnings in PHPUnit and call the parent error handler too (effectively the same as WithoutErrorHandler)

[marmichalski]

@sebastianbergmann can this issue be reopened?

[sebastianbergmann]

https://github.com/sebastianbergmann/phpunit/blob/10.5.3/src/Runner/ErrorHandler.php#L158-L164 is not strictly needed for/in #5592 and can be reverted for 10.x

@mvorisek Then please send a PR for 10.5 that reverts this. Thanks!

[sebastianbergmann]

@mvorisek This would be the revert for 10.5 you're talking about, right?

diff --git a/src/Runner/ErrorHandler.php b/src/Runner/ErrorHandler.php
index ea3acfa5d..e1c5b8a72 100644
--- a/src/Runner/ErrorHandler.php
+++ b/src/Runner/ErrorHandler.php
@@ -166,7 +166,13 @@ public function enable(): void
             return;
         }
 
-        set_error_handler($this);
+        $oldErrorHandler = set_error_handler($this);
+
+        if ($oldErrorHandler !== null) {
+            restore_error_handler();
+
+            return;
+        }
 
         $this->enabled                     = true;
         $this->originalErrorReportingLevel = error_reporting();

@marmichalski @cracksalad Can you please test whether your test suites would work again with this change? Thanks!

[sebastianbergmann]

For 11.x, the current (10.5.4+) behaviour should be default as warnings should be always logged by PHPUnit with possibly a global option with options:

• always replace the error handler

• replace the error handler, log the warnings in PHPUnit, but call the parent error handler too

• replace the error handler, but do not log the warnings in PHPUnit and call the parent error handler too (effectively the same as WithoutErrorHandler)

PHPUnit 11.0 will be released in less that two weeks. I think that we should keep the current behaviour of main as the default for PHPUnit 11.0. I agree that looking into configuration options such as the ones you propose to provide even more flexibility when it comes to handling E_* issues. However, I do not want to rush this and therefore will not do this for PHPUnit 11.0.

[mvorisek]

@sebastianbergmann #5676 (comment) - yes, and I agree it should be reverted for 10.5.x only.

[marmichalski]

@marmichalski @cracksalad Can you please test whether your test suites would work again with this change? Thanks!

Can confirm that this works with the example repo, phpunit 10.5.8 (manually applying the patch).

[sebastianbergmann]

Thank you, @mvorisek and @marmichalski. I will take care of 10.5 ASAP.

[sebastianbergmann]

I have reverted the code in question in f38fc88 for PHPUnit 10.5 on the 10.5 branch and added a test that documents the fact that this is the intended behaviour.

I have reverted the revert in 26e8bf4 for PHPUnit 11 on the main branch and added a test that documents the new intended behaviour in 99d2828. This change in PHPUnit 11 was documented in f714b83 and sebastianbergmann/phpunit-documentation-english@692bad3.

[sebastianbergmann]

I have reverted the revert in 26e8bf4 for PHPUnit 11 on the main branch and added a test that documents the new intended behaviour in 99d2828. This change in PHPUnit 11 was documented in f714b83 and sebastianbergmann/phpunit-documentation-english@692bad3.

The change of behaviour in PHPUnit 11 (compared to PHPUnit 10) has been reverted via 9c05f71, 7cea991, and 4bbb3ca.

I have opened #5686 for tracking how I would like to improve the current situation.