Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add ExceptionHandlingTrait and Exceptions::(un)register #8380

Closed
wants to merge 2 commits into from

Conversation

paulbalandan
Copy link
Member

Description
This PR introduces a way to unregister the error and exception handlers originally registered in Services::exceptions()->initialize() by calling the unregister() method. Moreover, the initialize() method is deprecated in favor of the register() method simply for consistency with the unregister method.

Motivation
PHPUnit 10 registers its own error handler. This may cause issues if the SUT is registering its own error handler (such as the issues encountered in #8069 ).

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.
https://docs.phpunit.de/en/10.5/error-handling.html

To solve this, we have 2 possible solutions:

  1. Use the #[WithoutErrorHandler] attribute on each test case. This is problematic as this attribute is on a TARGET_METHOD so it needs to be applied to each test method.
  2. Let PHPUnit's own error handler do the job and unregister the SUT's own error handling.

This uses the 2nd approach by introducing the ExceptionHandlingTrait intended to be used in tests. The trait just provides shortcut access to the register/unregister methods.

This PR is intended to fix the failures in #8069 .

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@paulbalandan paulbalandan marked this pull request as draft December 28, 2023 15:39
@kenjis
Copy link
Member

kenjis commented Dec 29, 2023

1) CodeIgniter\Debug\ExceptionsTest::testUnregisteringUsesPhpUnitsOwnHandler
PHPUnit\Framework\Exception: PHP Fatal error:  Uncaught Error: Attempt to modify property "aliases" on null in /home/runner/work/CodeIgniter4/CodeIgniter4/tests/_support/Config/Filters.php:22
Stack trace:
#0 Standard input code(1177): require_once()
#1 {main}
  thrown in /home/runner/work/CodeIgniter4/CodeIgniter4/tests/_support/Config/Filters.php on line 22

https://github.com/codeigniter4/CodeIgniter4/actions/runs/7348347348/job/20006326903?pr=8380

@paulbalandan
Copy link
Member Author

The weird thing is that this error comes out when running vendor/bin/phpunit --group Others but not on vendor/bin/phpunit --filter ExceptionsTest

@kenjis
Copy link
Member

kenjis commented Dec 29, 2023

$ vendor/bin/phpunit tests/system/Debug/ExceptionsTest.php 
PHPUnit 9.6.15 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.2.13
Configuration: /Users/kenji/work/codeigniter/official/CodeIgniter4-4.5/phpunit.xml

.........                                                                                                       9 / 9 (100%)

Time: 00:00.501, Memory: 16.00 MB

OK (9 tests, 37 assertions)
$ vendor/bin/phpunit tests/system/Debug/
PHPUnit 9.6.15 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.2.13
Configuration: /Users/kenji/work/codeigniter/official/CodeIgniter4-4.5/phpunit.xml

..................E....................                                                                       39 / 39 (100%)

Time: 00:05.049, Memory: 20.00 MB

There was 1 error:

1) CodeIgniter\Debug\ExceptionsTest::testUnregisteringUsesPhpUnitsOwnHandler
PHPUnit\Framework\Exception: ERROR: 
PHP Fatal error:  Uncaught TypeError: CodeIgniter\CLI\CLI::write(): Argument #1 ($text) must be of type string, null given, called in /Users/kenji/work/codeigniter/official/CodeIgniter4-4.5/app/Views/errors/cli/error_404.php on line 6 and defined in /Users/kenji/work/codeigniter/official/CodeIgniter4-4.5/system/CLI/CLI.php:454
Stack trace:
#0 /Users/kenji/work/codeigniter/official/CodeIgniter4-4.5/app/Views/errors/cli/error_404.php(6): CodeIgniter\CLI\CLI::write(NULL)
#1 Standard input code(701): require_once('/Users/kenji/wo...')
#2 {main}
  thrown in /Users/kenji/work/codeigniter/official/CodeIgniter4-4.5/system/CLI/CLI.php on line 454

ERRORS!
Tests: 39, Assertions: 228, Errors: 1.
$ vendor/bin/phpunit --group=Others
PHPUnit 9.6.15 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.2.13
Configuration: /Users/kenji/work/codeigniter/official/CodeIgniter4-4.5/phpunit.xml

.........................................................................................................  105 / 5112 (  2%)
...
........................................................................                                  5112 / 5112 (100%)

...


Time: 01:27.991, Memory: 126.50 MB

There was 1 error:

1) CodeIgniter\Debug\ExceptionsTest::testUnregisteringUsesPhpUnitsOwnHandler
PHPUnit\Framework\Exception: PHP Fatal error:  Uncaught Error: Attempt to modify property "aliases" on null in /Users/kenji/work/codeigniter/official/CodeIgniter4-4.5/tests/_support/Config/Filters.php:22
Stack trace:
#0 Standard input code(1144): require_once()
#1 {main}
  thrown in /Users/kenji/work/codeigniter/official/CodeIgniter4-4.5/tests/_support/Config/Filters.php on line 22

ERRORS!
Tests: 5112, Assertions: 8649, Errors: 1.

@paulbalandan paulbalandan marked this pull request as ready for review December 31, 2023 13:55

if (Exceptions::isUsingPhpUnitErrorHandlingInSeparateProcess()) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't still understand what's happening.
Can you explain what is the exact cause of the errors?
This hack is very bad, because the view file should not know about PHPUnit process isolation.
Can't we avoid to do like this?

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this provide us over a simple environment check in register() that would short circuit during testing?

@kenjis kenjis added 4.5 enhancement PRs that improve existing functionalities labels Feb 6, 2024
@kenjis
Copy link
Member

kenjis commented Feb 26, 2024

@paulbalandan The following patch makes CodeIgniterTest pass. (You can see the errors: #8069 (comment))
So it is true that the CI's Error Handler does something wrong.

But I don't understand:

  • why on Wiindows there are no errors
  • why only PHP 8.3 causes the errors
--- a/system/Debug/Exceptions.php
+++ b/system/Debug/Exceptions.php
@@ -109,7 +109,7 @@ class Exceptions
     public function initialize()
     {
         set_exception_handler([$this, 'exceptionHandler']);
-        set_error_handler([$this, 'errorHandler']);
+        //set_error_handler([$this, 'errorHandler']);
         register_shutdown_function([$this, 'shutdownHandler']);
     }
 

@paulbalandan paulbalandan deleted the handling branch March 10, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement PRs that improve existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants