Fix error handler to not silence `error_get_last()` result #5592

mvorisek · 2023-11-30T16:19:05Z

fix #5587 (and older #5428)

The updated PHPUnit error handler still supresses the output and still logs the errors as before.

This PR implies no BC break for existing PHPUnit 10.x tests /w and /wo WithoutErrorHandler attribute.

The WithoutErrorHandler attribute should be deprecated in the next minor version, as the new error handler impl. does not effectively imply any side effects so the behaviour /w and /wo PHPUnit error handler enabled is newly the same. In the next major version, the attribute should be completely removed and related c2c8dbb and e676667 commits reverted.

In addition to fixing the linked issues:

error_clear_last() is called before every test to clear error_get_last()
E_USER_ERROR aborts script execution by default, so newly the updated PHPUnit error handled does the same

src/Runner/ErrorHandler.php

codecov · 2023-11-30T16:24:22Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (870d925) 89.00% compared to head (b424341) 89.01%.

Additional details and impacted files

@@             Coverage Diff              @@
##               10.5    #5592      +/-   ##
============================================
+ Coverage     89.00%   89.01%   +0.01%     
+ Complexity     6363     6362       -1     
============================================
  Files           673      673              
  Lines         20293    20294       +1     
============================================
+ Hits          18061    18065       +4     
+ Misses         2232     2229       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sebastianbergmann · 2023-11-30T16:28:44Z

This will not go into a minor version such as PHPUnit 10.5. Please target main, thank you.

sebastianbergmann · 2023-11-30T16:36:28Z

@jrfnl What do you think?

sebastianbergmann · 2023-12-04T11:16:07Z

This still targets the 10.5 branch.

mvorisek · 2023-12-04T11:46:38Z

Based on #5592 (comment) I was/am waiting for @jrfnl response.

I would be happy if this fix could land into 10.x. In 9.x and lower, there was no error handler with return true, thus the error_get_last() was working.

The original fix landed in 10.3 requires WithoutErrorHandler attribute to be added in each test which is complicated workaround to a bug and it also degrades the testing as all warnings are then unchecked.

The changed LOC is minimal, most of the changed/added LOC are because of added tests.

As there is no BC break and it is a bugfix, is there any reason to not land it into 10.x?

sebastianbergmann · 2023-12-04T12:04:21Z

Maybe I am wrong, but this change feels "too big" for me to land in a bugfix release. I will need to think about this.

mvorisek · 2023-12-14T17:55:31Z

Rebase fixed to account for #5599 changes.

PHPUnit has changed the behavior of the error handler in several ways, for example introducing a PHP attribute to disable the PHPUnit error handler for tests dealing with `error_get_last()` [1], throwing an exception and stopping the script execution on `E_*_ERROR` or clearing the error result [2] - which breaks the ability to use the native `error_get_last()` method to test custom error handler implementation. Something which TYPO3 needs to do for the provided custom error handler. An additional change [3] to check and restore error handlers with levels has been reverted due to issues in the Laravel world. This change modifies the related test to use the introduced PHP attribute `#[WithoutErrorHandler]` and raises PHPUnit. Note: PHPUnit does not recognize the `@test` doc-block annotation if the `#[WithoutErrorHandler]` attribute is used. Therefore, the attribute counterparts for `@test` and `@dataProvider` has been added on top and not exclusively for now, albeit looking weird and fishy. This needs to be addressed in a more generic way in a dedicated change. Used command(s): > composer update typo3/testing-framework > composer require --dev "phpunit/phpunit":"^10.5.5" [1] sebastianbergmann/phpunit#5430 [2] sebastianbergmann/phpunit#5592 [3] sebastianbergmann/phpunit#5619 Resolves: #102724 Releases: main, 12.4 Change-Id: I77f750ef2eba53b5f8caa51651f36321ae5d1224 Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/82282 Tested-by: Oliver Klee <typo3-coding@oliverklee.de> Reviewed-by: Simon Schaufelberger <simonschaufi+typo3@gmail.com> Tested-by: core-ci <typo3@b13.com> Reviewed-by: Oliver Klee <typo3-coding@oliverklee.de> Reviewed-by: Anja Leichsenring <aleichsenring@ab-softlab.de> Tested-by: Simon Schaufelberger <simonschaufi+typo3@gmail.com> Tested-by: Stefan Bürk <stefan@buerk.tech> Reviewed-by: Stefan Bürk <stefan@buerk.tech> Tested-by: Anja Leichsenring <aleichsenring@ab-softlab.de>

PHPUnit has changed the behavior of the error handler in several ways, for example introducing a PHP attribute to disable the PHPUnit error handler for tests dealing with `error_get_last()` [1], throwing an exception and stopping the script execution on `E_*_ERROR` or clearing the error result [2] - which breaks the ability to use the native `error_get_last()` method to test custom error handler implementation. Something which TYPO3 needs to do for the provided custom error handler. An additional change [3] to check and restore error handlers with levels has been reverted due to issues in the Laravel world. This change modifies the related test to use the introduced PHP attribute `#[WithoutErrorHandler]` and raises PHPUnit. Note: PHPUnit does not recognize the `@test` doc-block annotation if the `#[WithoutErrorHandler]` attribute is used. Therefore, the attribute counterparts for `@test` and `@dataProvider` has been added on top and not exclusively for now, albeit looking weird and fishy. This needs to be addressed in a more generic way in a dedicated change. Used command(s): > composer require --dev \ "phpunit/phpunit":"^10.5.5" \ "typo3/testing-framework":"^8.0.8" [1] sebastianbergmann/phpunit#5430 [2] sebastianbergmann/phpunit#5592 [3] sebastianbergmann/phpunit#5619 Resolves: #102724 Releases: main, 12.4 Change-Id: I77f750ef2eba53b5f8caa51651f36321ae5d1224 Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/82283 Reviewed-by: Oliver Klee <typo3-coding@oliverklee.de> Reviewed-by: Anja Leichsenring <aleichsenring@ab-softlab.de> Tested-by: Oliver Klee <typo3-coding@oliverklee.de> Tested-by: core-ci <typo3@b13.com> Tested-by: Simon Schaufelberger <simonschaufi+typo3@gmail.com> Tested-by: Stefan Bürk <stefan@buerk.tech> Tested-by: Anja Leichsenring <aleichsenring@ab-softlab.de> Reviewed-by: Stefan Bürk <stefan@buerk.tech> Reviewed-by: Simon Schaufelberger <simonschaufi+typo3@gmail.com>

PHPUnit has changed the behavior of the error handler in several ways, for example introducing a PHP attribute to disable the PHPUnit error handler for tests dealing with `error_get_last()` [1], throwing an exception and stopping the script execution on `E_*_ERROR` or clearing the error result [2] - which breaks the ability to use the native `error_get_last()` method to test custom error handler implementation. Something which TYPO3 needs to do for the provided custom error handler. An additional change [3] to check and restore error handlers with levels has been reverted due to issues in the Laravel world. This change modifies the related test to use the introduced PHP attribute `#[WithoutErrorHandler]` and raises PHPUnit. Note: PHPUnit does not recognize the `@test` doc-block annotation if the `#[WithoutErrorHandler]` attribute is used. Therefore, the attribute counterparts for `@test` and `@dataProvider` has been added on top and not exclusively for now, albeit looking weird and fishy. This needs to be addressed in a more generic way in a dedicated change. Used command(s): > composer update typo3/testing-framework > composer require --dev "phpunit/phpunit":"^10.5.5" [1] sebastianbergmann/phpunit#5430 [2] sebastianbergmann/phpunit#5592 [3] sebastianbergmann/phpunit#5619 Resolves: #102724 Releases: main, 12.4 Change-Id: I77f750ef2eba53b5f8caa51651f36321ae5d1224 Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/82282 Tested-by: Oliver Klee <typo3-coding@oliverklee.de> Reviewed-by: Simon Schaufelberger <simonschaufi+typo3@gmail.com> Tested-by: core-ci <typo3@b13.com> Reviewed-by: Oliver Klee <typo3-coding@oliverklee.de> Reviewed-by: Anja Leichsenring <aleichsenring@ab-softlab.de> Tested-by: Simon Schaufelberger <simonschaufi+typo3@gmail.com> Tested-by: Stefan Bürk <stefan@buerk.tech> Reviewed-by: Stefan Bürk <stefan@buerk.tech> Tested-by: Anja Leichsenring <aleichsenring@ab-softlab.de>

PHPUnit has changed the behavior of the error handler in several ways, for example introducing a PHP attribute to disable the PHPUnit error handler for tests dealing with `error_get_last()` [1], throwing an exception and stopping the script execution on `E_*_ERROR` or clearing the error result [2] - which breaks the ability to use the native `error_get_last()` method to test custom error handler implementation. Something which TYPO3 needs to do for the provided custom error handler. An additional change [3] to check and restore error handlers with levels has been reverted due to issues in the Laravel world. This change modifies the related test to use the introduced PHP attribute `#[WithoutErrorHandler]` and raises PHPUnit. Note: PHPUnit does not recognize the `@test` doc-block annotation if the `#[WithoutErrorHandler]` attribute is used. Therefore, the attribute counterparts for `@test` and `@dataProvider` has been added on top and not exclusively for now, albeit looking weird and fishy. This needs to be addressed in a more generic way in a dedicated change. Used command(s): > composer require --dev \ "phpunit/phpunit":"^10.5.5" \ "typo3/testing-framework":"^8.0.8" [1] sebastianbergmann/phpunit#5430 [2] sebastianbergmann/phpunit#5592 [3] sebastianbergmann/phpunit#5619 Resolves: #102724 Releases: main, 12.4 Change-Id: I77f750ef2eba53b5f8caa51651f36321ae5d1224 Reviewed-on: https://review.typo3.org/c/Packages/TYPO3.CMS/+/82283 Reviewed-by: Oliver Klee <typo3-coding@oliverklee.de> Reviewed-by: Anja Leichsenring <aleichsenring@ab-softlab.de> Tested-by: Oliver Klee <typo3-coding@oliverklee.de> Tested-by: core-ci <typo3@b13.com> Tested-by: Simon Schaufelberger <simonschaufi+typo3@gmail.com> Tested-by: Stefan Bürk <stefan@buerk.tech> Tested-by: Anja Leichsenring <aleichsenring@ab-softlab.de> Reviewed-by: Stefan Bürk <stefan@buerk.tech> Reviewed-by: Simon Schaufelberger <simonschaufi+typo3@gmail.com>

driesvints · 2024-01-01T15:57:13Z

Hi @sebastianbergmann @mvorisek. It seems this PR breaks Lumen's test suite: https://github.com/laravel/lumen-framework/actions/runs/7373447479/job/20062657751

Details

Run vendor/bin/phpunit
PHPUnit 10.5.5 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.1.27
Configuration: /home/runner/work/lumen-framework/lumen-framework/phpunit.xml.dist

........................................................FF....... 65 / 67 ( 97%)
..                                                                67 / 67 (100%)

Time: 00:00.109, Memory: 26.00 MB

There were 2 failures:

1) HandleExceptionsTest::testEnsuresDeprecationsDriver
null does not match expected type "array".

/home/runner/work/lumen-framework/lumen-framework/tests/HandleExceptionsTest.php:119

2) HandleExceptionsTest::testEnsuresNullDeprecationsDriver
Failed asserting that null matches expected 'NullHandler'.

/home/runner/work/lumen-framework/lumen-framework/tests/HandleExceptionsTest.php:144

FAILURES!
Tests: 67, Assertions: 129, Failures: 2.
Error: Process completed with exit code 1.

I can confirm the changes in this PR cause this because if I revert them locally, tests pass again. At this time I can't provide a way to reproduce or exactly pinpoint why this PR causes the tests to fail as I'm too unfamiliar with PHPUnit's internals.

@sebastianbergmann would you maybe consider reverting this PR in a similar fashion as was done with #5619 ?

sebastianbergmann · 2024-01-01T16:06:48Z

@driesvints Yes, I would consider that.

I am travelling for the next two weeks and will only be able to work on and off. Therefore, it would help me a lot to get this done quickly if you could send a pull request for 10.5 that restores the previous behaviour. Once I have that, I should be able to tag a release ASAP.

mvorisek · 2024-01-01T16:10:33Z

Reverting this will cause issues for users relying on this bugfix.

@driesvints please provide a minimal repro as a testcase and I can look into it.

driesvints · 2024-01-01T17:04:48Z

@sebastianbergmann Thank you, we appreciate that! I've sent in the PR here: #5637

please provide a minimal repro as a testcase and I can look into it.

I don't know how sorry.

mvorisek · 2024-01-01T18:40:22Z

Isolate https://github.com/laravel/lumen-framework/blob/v10.0.2/tests/HandleExceptionsTest.php#L119 test into a repro repository with minimal deps/sources.

The issue is probably in https://github.com/laravel/lumen-framework/blob/v10.0.2/src/Concerns/RegistersExceptionHandlers.php#L73 as it relies on specific error_reporting() - set that prior your test like error_reporting(error_reporting() | \E_USER_DEPRECATED).

Or consider changing your code to always handle errors no matter what the error_reporting() value. Unreported errors do not mean they should not be handled/logged, see proper detection of @ operator in https://github.com/sebastianbergmann/phpunit/blob/10.5.5/src/Runner/ErrorHandler.php#L58.

Because of this, I ask you to retract #5637 as it will break other users relying on this bugfix.

driesvints · 2024-01-02T07:45:44Z

I'm sorry but I don't understand why we should change our code. This has worked perfectly fine for us for years with using PHPUnit. If PHPUnit were to change this in a new major version we'd could look into adopting the new way of doing things. But right now I feel this is a breaking change, sorry.

mvorisek · 2024-01-02T09:43:11Z

@driesvints see the two issues in this PR description, there was big breaking change introduced in PHPUnit 10.0.0 and this PR fixes it - your code relies on specific error_reporting error level. I understand you might argue this is breaking change - we cannot satisfy both worlds.

The reason is we MUST NOT HANDLE the php warnings but instead LOG them only. This is what this PR does. To prevent php printing the unhandled php warnings, the error_reporting must be set to a lower level. I do not think there is other way to do it. I spent more than a day on figuring the php internals to understand the error handling limitations - if you have any question about error handling or even have an idea how to do things better, feel free to share.

…which makes tests fail

mvorisek commented Nov 30, 2023

View reviewed changes

src/Runner/ErrorHandler.php Show resolved Hide resolved

mvorisek changed the base branch from main to 10.5 November 30, 2023 16:21

mvorisek force-pushed the fix_error_get_last_5587 branch from 0a28bd2 to 6cc6dad Compare November 30, 2023 16:27

mvorisek force-pushed the fix_error_get_last_5587 branch from 6cc6dad to b67a9dc Compare November 30, 2023 16:29

sebastianbergmann added this to the PHPUnit 11.0 milestone Dec 1, 2023

sebastianbergmann added type/enhancement A new idea that should be implemented feature/test-runner CLI test runner labels Dec 1, 2023

sebastianbergmann force-pushed the fix_error_get_last_5587 branch from b67a9dc to 7dfa49b Compare December 14, 2023 13:59

sebastianbergmann mentioned this pull request Dec 15, 2023

Implement expectUserDeprecationMessage() and expectUserDeprecationMessageMatches() to expect E_USER_DEPRECATED issues #5605

Closed

sebastianbergmann removed this from the PHPUnit 11.0 milestone Dec 18, 2023

mvorisek added 8 commits December 18, 2023 07:18

Fix ErrorHandler to not silent error_get_last() results

a7ae68d

test in test must still open own error handler

4986438

replace supressed detection of @ (3739 issue)

6ad7e60

handle E_USER_ERROR but abort further test execution

33c1ece

improve WithoutErrorHandler test

24fc7c2

clear error_clear_last() before every test

9a8a8ed

add tests (5428 and 5587 issues)

479d181

fix rebase

b424341

sebastianbergmann force-pushed the fix_error_get_last_5587 branch from 041a32c to b424341 Compare December 18, 2023 06:18

sebastianbergmann changed the title ~~Fix ErrorHandler to not silent error_get_last() results~~ Fix error handler to not silence error_get_last() result Dec 18, 2023

sebastianbergmann merged commit b424341 into sebastianbergmann:10.5 Dec 18, 2023
31 checks passed

mvorisek deleted the fix_error_get_last_5587 branch December 18, 2023 08:35

sebastianbergmann mentioned this pull request Dec 18, 2023

Triggered E_USER_ERROR is displayed as . (dot) #5566

Closed

This was referenced Dec 18, 2023

Check and restore error/exception global handlers #5619

Merged

error_get_last() is not set when errors are logged #5587

Closed

This was referenced Jan 1, 2024

Revert PR 5592 #5636

Closed

Revert PR 5592 #5637

Closed

marmichalski mentioned this pull request Jan 20, 2024

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

Closed

Jean85 added a commit to facile-it/paraunit that referenced this pull request Feb 2, 2024

Bump minimum PHPUnit requirement due to sebastianbergmann/phpunit#5592 …

6c36c03

…which makes tests fail

Jean85 added a commit to facile-it/paraunit that referenced this pull request Feb 2, 2024

Bump minimum PHPUnit requirement due to sebastianbergmann/phpunit#5592 …

6feb6ab

…which makes tests fail

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix error handler to not silence `error_get_last()` result #5592

Fix error handler to not silence `error_get_last()` result #5592

mvorisek commented Nov 30, 2023

codecov bot commented Nov 30, 2023 •

edited

Loading

sebastianbergmann commented Nov 30, 2023

sebastianbergmann commented Nov 30, 2023

sebastianbergmann commented Dec 4, 2023

mvorisek commented Dec 4, 2023

sebastianbergmann commented Dec 4, 2023

mvorisek commented Dec 14, 2023

driesvints commented Jan 1, 2024

sebastianbergmann commented Jan 1, 2024

mvorisek commented Jan 1, 2024

driesvints commented Jan 1, 2024

mvorisek commented Jan 1, 2024 •

edited

Loading

driesvints commented Jan 2, 2024

mvorisek commented Jan 2, 2024

Fix error handler to not silence error_get_last() result #5592

Fix error handler to not silence error_get_last() result #5592

Conversation

mvorisek commented Nov 30, 2023

codecov bot commented Nov 30, 2023 • edited Loading

Codecov Report

sebastianbergmann commented Nov 30, 2023

sebastianbergmann commented Nov 30, 2023

sebastianbergmann commented Dec 4, 2023

mvorisek commented Dec 4, 2023

sebastianbergmann commented Dec 4, 2023

mvorisek commented Dec 14, 2023

driesvints commented Jan 1, 2024

sebastianbergmann commented Jan 1, 2024

mvorisek commented Jan 1, 2024

driesvints commented Jan 1, 2024

mvorisek commented Jan 1, 2024 • edited Loading

driesvints commented Jan 2, 2024

mvorisek commented Jan 2, 2024

Fix error handler to not silence `error_get_last()` result #5592

Fix error handler to not silence `error_get_last()` result #5592

codecov bot commented Nov 30, 2023 •

edited

Loading

mvorisek commented Jan 1, 2024 •

edited

Loading