Check and restore error/exception global handlers #5619

mvorisek · 2023-12-18T18:38:44Z

related with #5592

no functional change to any test which does not add/remove error/exception handlers (or remove exactly the ones the test added)

inspired by output buffer TestCase::stopOutputBuffering() code

codecov · 2023-12-18T18:43:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (549f23c) 89.01% compared to head (0647107) 88.59%.

Additional details and impacted files

@@             Coverage Diff              @@
##               10.5    #5619      +/-   ##
============================================
- Coverage     89.01%   88.59%   -0.43%     
+ Complexity     6363     6315      -48     
============================================
  Files           673      673              
  Lines         20296    20197      -99     
============================================
- Hits          18067    17893     -174     
- Misses         2229     2304      +75

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

sebastianbergmann · 2023-12-19T06:47:16Z

Can you explain which use cases you have in mind? Thanks.

mvorisek · 2023-12-19T09:59:05Z

Any code which sets (adds) & restores (removes) error/exception handlers. They are tricky to debug if some set (added) handlers are not restored (removed) properly by a test.

When PHPUnit is executed, there is usually no error/exception handler. Then one is set by the PHPUnit runner. So the performance of fetching the handlers is fast, as they are newer than a few handlers active.

nunomaduro · 2023-12-27T14:19:23Z

@sebastianbergmann, this pull request introduced a BC issue that is currently impacting all existing Laravel 10 users: laravel/framework#49502. I wonder if you could consider reverting this change and porting it to PHPUnit 11?

sebastianbergmann · 2023-12-27T14:34:15Z

@nunomaduro Can youprovide a minimal, self-contained (without Laravel), reproducing test case that shows the problem?

@mvorisek What do you think? I am inclined to follow @nunomaduro's suggestion of reverting this change for PHPUnit 10.5.

nunomaduro · 2023-12-27T15:13:54Z

@sebastianbergmann Here you go:

git clone [email protected]:nunomaduro/phpunit-issue-global-handlers.git
cd phpunit-issue-global-handlers
composer update
./vendor/bin/phpunit

I understand the reasoning behind this pull request. However, currently there are dozens of widely used packages (including Laravel itself) that statically register their own error|exception handler just once, to prevent them from being registered on each test method and avoid memory leaks.

Ideally, the best scenario for us would be to have this feature as an opt-in, so we don't even have to deal with it. If that's not possible, could you port it to PHPUnit 11? This would give us time to manage the changes.

sebastianbergmann · 2023-12-27T15:15:56Z

I have already prepared PHPUnit 10.5.5 which will revert this change. The tag will be pushed shortly.

sebastianbergmann · 2023-12-27T15:25:41Z

This was reverted on both the 10.5 and main branches and PHPUnit 10.5.5 has been tagged.

nunomaduro · 2023-12-27T15:28:32Z

Thank you!

mvorisek · 2023-12-27T16:51:45Z

In laravel/framework#49502 the issue seems to be in Lavarel, it should either not set error/exception handler at all, or set it, but it must remove it before the test ends.

@sebastianbergmann this is expected when test code leave handlers in an unexpected state - can in be relanded in phpunit 10.6?

sebastianbergmann · 2023-12-28T05:46:37Z

There will not be a PHPUnit 10.5, but this could go into PHPUnit 11.

sebastianbergmann · 2023-12-28T06:28:17Z

Ideally, the best scenario for us would be to have this feature as an opt-in, so we don't even have to deal with it. If that's not possible, could you port it to PHPUnit 11? This would give us time to manage the changes.

I plan to simply revert 23c16c1 (reverting the revert of this PR) for PHPUnit 11.

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 <[email protected]> Reviewed-by: Simon Schaufelberger <[email protected]> Tested-by: core-ci <[email protected]> Reviewed-by: Oliver Klee <[email protected]> Reviewed-by: Anja Leichsenring <[email protected]> Tested-by: Simon Schaufelberger <[email protected]> Tested-by: Stefan Bürk <[email protected]> Reviewed-by: Stefan Bürk <[email protected]> Tested-by: Anja Leichsenring <[email protected]>

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 <[email protected]> Reviewed-by: Anja Leichsenring <[email protected]> Tested-by: Oliver Klee <[email protected]> Tested-by: core-ci <[email protected]> Tested-by: Simon Schaufelberger <[email protected]> Tested-by: Stefan Bürk <[email protected]> Tested-by: Anja Leichsenring <[email protected]> Reviewed-by: Stefan Bürk <[email protected]> Reviewed-by: Simon Schaufelberger <[email protected]>

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 <[email protected]> Reviewed-by: Simon Schaufelberger <[email protected]> Tested-by: core-ci <[email protected]> Reviewed-by: Oliver Klee <[email protected]> Reviewed-by: Anja Leichsenring <[email protected]> Tested-by: Simon Schaufelberger <[email protected]> Tested-by: Stefan Bürk <[email protected]> Reviewed-by: Stefan Bürk <[email protected]> Tested-by: Anja Leichsenring <[email protected]>

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 <[email protected]> Reviewed-by: Anja Leichsenring <[email protected]> Tested-by: Oliver Klee <[email protected]> Tested-by: core-ci <[email protected]> Tested-by: Simon Schaufelberger <[email protected]> Tested-by: Stefan Bürk <[email protected]> Tested-by: Anja Leichsenring <[email protected]> Reviewed-by: Stefan Bürk <[email protected]> Reviewed-by: Simon Schaufelberger <[email protected]>

Check and restore error/exception global handlers

0647107

mvorisek changed the base branch from main to 10.5 December 18, 2023 18:38

sebastianbergmann merged commit 93aa92b into sebastianbergmann:10.5 Dec 20, 2023
23 of 25 checks passed

mvorisek deleted the backup_restore_global_handlers branch December 20, 2023 06:20

nunomaduro mentioned this pull request Dec 27, 2023

PHPUnit warning: Test code or tested code did not remove its own exception handlers laravel/framework#49502

Closed

This was referenced Dec 29, 2023

5.x: fix behavior change introduced by PHPUnit 10.5.5 cakephp/cakephp#17509

Closed

5.next: fix behavior change introduced in phpunit 10.5.5 cakephp/cakephp#17510

Merged

5.x: adjust PHPUnit version range to <= 10.5.3 cakephp/cakephp#17511

Merged

driesvints mentioned this pull request Jan 1, 2024

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

Merged

mvorisek mentioned this pull request Jan 23, 2024

fix: Setting error handler inside an isolated test may cause test failure on PHP 8.3 #5677

Closed

siketyan mentioned this pull request Jan 23, 2024

[10.x] fix: Restore the original error/exception handler on terminating app laravel/framework#49791

Closed

This was referenced Feb 6, 2024

[Framework bundle] Cleanup error and exception handlers on Kernel shutdown symfony/symfony#53812

Closed

Add a way to cleanup error and exception handlers getsentry/sentry-php#1698

Closed

soyuka mentioned this pull request Jul 3, 2024

Configure global exception/error handlers checks #5888

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check and restore error/exception global handlers #5619

Check and restore error/exception global handlers #5619

mvorisek commented Dec 18, 2023 •

edited

Loading

codecov bot commented Dec 18, 2023

sebastianbergmann commented Dec 19, 2023

mvorisek commented Dec 19, 2023 •

edited

Loading

nunomaduro commented Dec 27, 2023

sebastianbergmann commented Dec 27, 2023

nunomaduro commented Dec 27, 2023

sebastianbergmann commented Dec 27, 2023

sebastianbergmann commented Dec 27, 2023

nunomaduro commented Dec 27, 2023

mvorisek commented Dec 27, 2023 •

edited

Loading

sebastianbergmann commented Dec 28, 2023

sebastianbergmann commented Dec 28, 2023

Check and restore error/exception global handlers #5619

Check and restore error/exception global handlers #5619

Conversation

mvorisek commented Dec 18, 2023 • edited Loading

codecov bot commented Dec 18, 2023

Codecov Report

sebastianbergmann commented Dec 19, 2023

mvorisek commented Dec 19, 2023 • edited Loading

nunomaduro commented Dec 27, 2023

sebastianbergmann commented Dec 27, 2023

nunomaduro commented Dec 27, 2023

sebastianbergmann commented Dec 27, 2023

sebastianbergmann commented Dec 27, 2023

nunomaduro commented Dec 27, 2023

mvorisek commented Dec 27, 2023 • edited Loading

sebastianbergmann commented Dec 28, 2023

sebastianbergmann commented Dec 28, 2023

mvorisek commented Dec 18, 2023 •

edited

Loading

mvorisek commented Dec 19, 2023 •

edited

Loading

mvorisek commented Dec 27, 2023 •

edited

Loading