Repository navigation
Breaking Change: set_exception_handler no longer returns the exception handler within itself #13446
Description
Description
The following code:
<?php
set_exception_handler(function ($e): void {
var_dump(set_exception_handler('is_int'));
});
throw new \Exception;Resulted in this output:
NULL
But I expected this output instead:
object(Closure)#1 (1) {
["parameter"]=>
array(1) {
["$e"]=>
string(10) "<required>"
}
}
PHP Version
8.3.0 and up
Operating System
No response
Activity
ndossche commentedon Feb 20, 2024
Reverting the change in
zend_user_exception_handlerfrom b3e33be seems to restore the old behaviour.cc @iluuu1994 is this behaviour expected, based on what I remember and read it seems unintended but I could be wrong?
iluuu1994 commentedon Feb 20, 2024
Yes, this is expected behavior. Exception handlers are not called repeatedly, so they are unregistered before they are called. If you want to catch errors within an error handler, install a new one. You should probably avoid registering the same one, as this will lead to infinite recursion if there's a bug in your error handler. This was previously not done because errors in your error handler were never handled.
ndossche commentedon Feb 21, 2024
Ok thanks for checking. In that case I'll close this.
greg0ire commentedon Feb 21, 2024
Note that this also affects shutdown functions: https://3v4l.org/fXeHc#v8.3.3, and I think that's what really causes symfony/symfony#53946: my understanding is that with the shutdown function, Symfony is trying to reuse its exception handler to handle fatal errors.
ndossche commentedon Feb 21, 2024
FTR, the behaviour change was to implement #10695, but I see how this causes issues.
Not sure what to do with this, if the existing code could still be supported, I'll defer to @iluuu1994 as they're more familiar
greg0ire commentedon Feb 21, 2024
I think it makes sense to unregister the exception handler when you are inside it, but maybe it should be restored after exiting it, and before entering the shutdown function. Doesn't make a lot of sense though… when I said Symfony is reusing the exception handler, it's not like they are throwing an exception and letting the handler handle it… instead, they are invoking it manually. So I'd say it's not a super legitimate use case. This looks like a breaking change though.
nicolas-grekas commentedon Feb 23, 2024
Hi, this is a significant BC break, please reconsider. See symfony/symfony#53946, which is nasty.
iluuu1994 commentedon Feb 27, 2024
@nicolas-grekas
So, just so I understand this correctly,
ErrorHandler::handleFatalErrordepends on finding itself in the error handler stack. Because calling the current exception handler also removes it as the current exception handler, it cannot be recovered with the current API, asset_exception_handler('var_dump')immediately returnsnull. TBH the$handler = set_exception_handler('var_dump')andrestore_exception_handler()pattern is probably not what this API was meant for. Nonetheless, let's find a way this continues working.The reason the exception handler is removed is to avoid infinite recursion when an exception handler accidentally throws. Another way to solve this is to only re-trigger the exception handler only if a new one has been registered. It's worth noting that your code is potentially subject to infinite recursion with that definition, as you push/pop exception handlers to iterate the stack. This was not previously the case because registering exception handlers within exception handlers had no effect, as they would never be called anyway.
A more sturdy solution might be to offer better APIs for the underlying error handler stack (
EG(user_exception_handlers)).I'm not fully grasping the Symfony code, e.g. I don't understand why
handleFatalErroris not just an instance method that would make it unnecessary to find the correct instance within the handler stack. That just to say that my understanding of the issue may be incomplete.greg0ire commentedon Feb 27, 2024
I don't know either, but in case it helps remembering/understanding, that was originally the case, and changed in symfony/symfony#10921
nicolas-grekas commentedon Mar 7, 2024
Yes, that's the real problem for Symfony: when an exception is handled, the exception handler stack is emptied after the userland exception handlers ran.
Yes it is. This code in the shutdown function turns fatal errors into fake exceptions that are then reinjected in the exception handling stack. Most fatal errors have been turned into exceptions in the recent years, but some remain, and this logic makes the behavior more consistent. That's why we need access to the exception handlers stack at shutdown.
This is the reproducer:
greg0ire commentedon Mar 7, 2024
Sorry, I didn't use the right words. I meant to say that although there is a need for sure, the fact that it is achieved by calling the exception handler manually makes it easy to overlook and break by changes in the language, and might point to a shortcoming in PHP. Maybe PHP should handle exceptions thrown inside the shutdown function, so that there is no need to handle inconsistencies in userland?
nicolas-grekas commentedon Mar 12, 2024
That's fine, error handling is an edgy domain anyway.
That's the fix I'm looking for :)
minor #54189 [ErrorHandler] Fix exit code when an exception occurs an…
iluuu1994 commentedon Mar 12, 2024
@nicolas-grekas I'll have another look today. I was thinking that maybe it makes sense to push a
nullhandler during the duration of the handler. This would avoid triggering it recursively, while keeping the current handler available in the stack. After the handler finishes, we can pop thenullhandler again. Potentially, some additional rules are necessary to avoid popping thenullvalue if the hander has modified the handler stack, i.e. if the top isn'tnullanymore. I'll need to think about this in a bit more detail. WDYT?Restore error handler after running it
Restore error handler after running it