Fix #63206: Fully support error/exception_handler stacking #5206
Conversation
… null or inside the handler Always push the current user_error/exception_handler to the stack, even when it is empty, so restore_error_handler() always works as expected. The user_error_handler is especially temporarily empty when we are inside the error handler, which caused inconsistent behaviour before.
There was a problem hiding this comment.
Looks good. I looked at some set_error_handler(null) uses in libraries, and it looks like they are either not affected by this change, or fixed by it (there were set_error_handler(null) + restore_error_handler() occurrences, which, if I understand right, will not do what is expected before this change.)
Eh, thinking about this again, that particular case should work fine already, as the null handler doesn't get pushed. But still, I don't think this change will cause issues. |
|
Yes, I don't think, it will cause real issues - the backward-incompatibility is more of theoretical nature as it never worked as expected in this edge-case. The only internal thing, I was not sure, is the cleanup of user_error_handlers ... but zval_ptr_dtor() seems to be safe for UNDEF values. And I didn't find any places where UNDEF causes problems in the C code. |
Yes, zval_ptr_dtor() is safe for IS_UNDEF. For master we may want to replace the use of UNDEF here with NULL. I don't think there's a reason to use UNDEF, and using it just means that we have to perform extra checks and mapping between undef <-> null. |
|
Cool, maybe for master it makes sense to additionally implement the "in-error-handler" flag instead of removing the error-handler temporarily to avoid recursion (as you suggested in https://bugs.php.net/bug.php?id=63206). But maybe that is not so easy, because setting a custom error handler inside the error-handler should still work. I didn't think it through, anyway ;-) |
|
Merged as 8c6a7c3 into 7.3.
Yes, I think we would have to track the current "depth" of the error handler for that. May not be worthwhile after this change. |
This PR was merged into the 3.4 branch. Discussion ---------- [Debug] fix for PHP 7.3.16+/7.4.4+ | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | - | License | MIT | Doc PR | - Should make tests green. Follows php/php-src#5206, which is part of PHP 7.4.4 and 7.3.16. The same patch is needed on the ErrorHandler component in 4.4 of course. Commits ------- b3d9a8a [Debug] fix for PHP 7.3.16+/7.4.4+
... even with
nullor inside the handler.Always push the current user_error/exception_handler to the stack,
even when it is empty, so
restore_error_handler()always works asexpected.
The
user_error_handleris especially temporarily empty when we are insidethe error handler, which caused inconsistent behaviour before.
See attached test-cases for details.
PHP-Bug: https://bugs.php.net/bug.php?id=63206