-
Notifications
You must be signed in to change notification settings - Fork 7.9k
[Observer] Fatal error support #6114
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
Conversation
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.
Looks good to me.
Does this work with ext/soap or other exts that throw fatal errors and then catch the bailout? |
@nikic Yes it should be, |
@beberlei My concern is the other way around: The fatal error will be caught by soap and execution will continue, exiting some frames that have already been exited. |
@nikic Very good point. I can confirm that the end handlers get fired twice in the case of ext/soap fatal errors. I'll see if I can come up with an elegant solution to handle the edge cases that extensions like ext/soap present. 🤔 Tangent: Ideally this PR would address all |
9241436
to
687b7c1
Compare
Here's a possible solution to extensions that juggle errors like ext/soap: 687b7c1. The idea is that for fatal errors, functions are "unobserved" after the end handlers fire for the first time. This causes all the observer handlers to fire and close prematurely for these specific edge cases, but ensures they only fire once. The long term solution would be: fix each individual edge case of fatal error juggling. |
@nikic I've looked at this a lot of different ways to see if we can get something akin to I'm open to other ideas of addressing |
Closing this in favor of #6377 which provides a more generic solution for handling |
This is another follow up PR to #5857.
This fixes an issue where fatal errors would not call the observer end handlers. And also adds a few missing tests.