Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Tweak error/exception handler registration#58372
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Tweak error/exception handler registration#58372
Uh oh!
There was an error while loading.Please reload this page.
Conversation
@derrabus FYI as you used the same workaround for Twig |
I'm wondering about merging this in 6.4 🤔 |
9a97c62 tof1d51d6CompareRebased on 6.4 |
| $exceptionHandler =set_exception_handler('var_dump'); | ||
| restore_exception_handler(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We need a PHP RFC to getget_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 to607cd94CompareUh oh!
There was an error while loading.Please reload this page.
2775afb to97c08adCompareUh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I rebased this on 5.4 and did a fix on symfony/runtime so that we have simpler setup of the error-handler register.
This should unlock moving to phpunit11 (deps=low included)
| publicfunctionboot() | ||
| { | ||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 ?
nicolas-grekasSep 25, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thefalse argument is here to skip registering ifErrorHandler is already registered.
This line preserves the current behavior when symfoy/runtime isn't installed so I wouldn't add more logic here.
| "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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ensures that we symfony/runtime is installed, it's a version that always registers ErrorHandler
| $_SERVER[$debugKey] =$_ENV[$debugKey] ='0'; | ||
| } | ||
| if (false !==$errorHandler = ($options['error_handler'] ?? BasicErrorHandler::class)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
untightening the error_handler option from the debug one
Uh oh!
There was an error while loading.Please reload this page.
97c08ad tof1d6a64CompareThe test failures are related, aren't they? |
704e252 to6d0b67dCompare6d0b67d toaf9c035Compare
yes they are, this line is deprecated indeed when $debug = false: PR updated |
de1a6ac intosymfony:5.4Uh oh!
There was an error while loading.Please reload this page.
| } | ||
| if (0 <=\ini_get('zend.assertions')) { | ||
| ini_set('zend.assertions', (int)$debug); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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.
Uh oh!
There was an error while loading.Please reload this page.
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.