Tweak error/exception handler registration #58372
Conversation
|
@derrabus FYI as you used the same workaround for Twig |
|
I'm wondering about merging this in 6.4 🤔 |
9a97c62 to
f1d51d6
Compare
|
Rebased on 6.4 |
| $exceptionHandler = set_exception_handler('var_dump'); | ||
| restore_exception_handler(); |
There was a problem hiding this comment.
We need a PHP RFC to get get_exception_handler() 👀
|
Thank you. I'm a bit reluctant to merge this as a bugfix because of the change in behavior though. |
|
Then again, not having this change on 6.4 means we need to keep those bootstrap files for low-deps testing. Difficult decision. 🙈 |
f1d51d6 to
607cd94
Compare
2775afb to
97c08ad
Compare
| @@ -91,7 +92,16 @@ class FrameworkBundle extends Bundle | |||
| { | |||
| public function boot() | |||
| { | |||
| ErrorHandler::register(null, false)->throwAt($this->container->getParameter('debug.error_handler.throw_at'), true); | |||
| if (class_exists(SymfonyRuntime::class)) { | |||
There was a problem hiding this comment.
When symfony/runtime is installed, we assume it will register the ErrorHandler when appropriate.
That's not the case right now, but this PR fixes it.
| $handler = set_error_handler('var_dump'); | ||
| restore_error_handler(); | ||
| } else { | ||
| $handler = [ErrorHandler::register(null, false)]; |
There was a problem hiding this comment.
When symfony/runtime is not found, the index.php file registers ErrorHandler only in debug mode, so we need to register it to not break prod mode.
There was a problem hiding this comment.
shouldn't we check whether it is already registered ? Maybe some devs have changed the index.php file.
And wouldn't this still cause issues with PHPUnit 11 then when booting a kernel during tests ?
There was a problem hiding this comment.
The false argument is here to skip registering if ErrorHandler is already registered.
This line preserves the current behavior when symfoy/runtime isn't installed so I wouldn't add more logic here.
| @@ -86,6 +86,7 @@ | |||
| "symfony/mime": "<4.4", | |||
| "symfony/property-info": "<4.4", | |||
| "symfony/property-access": "<5.3", | |||
| "symfony/runtime": "<5.4.45|>=6.0,<6.4.13|>=7.0,7.1.6", | |||
There was a problem hiding this comment.
ensures that we symfony/runtime is installed, it's a version that always registers ErrorHandler
| } else { | ||
| $_SERVER[$debugKey] = $_ENV[$debugKey] = '0'; | ||
| } | ||
|
|
||
| if (false !== $errorHandler = ($options['error_handler'] ?? BasicErrorHandler::class)) { |
There was a problem hiding this comment.
untightening the error_handler option from the debug one
97c08ad to
f1d6a64
Compare
|
The test failures are related, aren't they? |
704e252 to
6d0b67d
Compare
6d0b67d to
af9c035
Compare
yes they are, this line is deprecated indeed when $debug = false: PR updated |
| } | ||
|
|
||
| if (0 <= \ini_get('zend.assertions')) { | ||
| ini_set('zend.assertions', (int) $debug); |
There was a problem hiding this comment.
@nicolas-grekas why are you disabling assertions in non-debug mode ? We are relying on assertions in production mode and this change breaks our application.
This should allow removing the custom bootstrap file from #58370 /cc @xabbuh
The change on FrameworkBundle leads to a tweaked behavior: we don't override the previous error handler in case it's not the Symfony one. This shouldn't change anything in practice since the error handler is already registered by the runtime component.
The rest is closer to bug fixes.