Skip to content

Fix GH-19720: Assertion failure when error handler throws when accessing a deprecated constant #19723

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Fix GH-19720: Assertion failure when error handler throws when accessing a deprecated constant #19723

wants to merge 1 commit into from

Conversation

nielsdos
Copy link
Member

@nielsdos nielsdos commented Sep 5, 2025

When deprecation causes an exception, we should return NULL instead of continuing.

@nielsdos
Fix phpGH-19720: Assertion failure when error handler throws when acc…
9c02027 
…essing a deprecated constant

When deprecation causes an exception, we should return NULL instead of
continuing.
@nielsdos nielsdos requested a review from dstogov as a code owner September 5, 2025 16:54
@nielsdos nielsdos linked an issue Sep 5, 2025 that may be closed by this pull request
Assertion failure when error handler throws when accessing a deprecated constant #19720
Closed
DanielEScherzer
DanielEScherzer approved these changes Sep 5, 2025
View reviewed changes
Copy link
Member

@DanielEScherzer DanielEScherzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For 8.4 we should have a test with E_STRICT so that it can be run without needing zend_test, and for 8.5 we would then also be able to confirm that the attributes don't mess with things, but the extra test can be done as a follow-up after this gets merged

@nielsdos
Copy link
Member Author

nielsdos commented Sep 5, 2025

For 8.4 we should have a test with E_STRICT so that it can be run without needing zend_test, and for 8.5 we would then also be able to confirm that the attributes don't mess with things, but the extra test can be done as a follow-up after this gets merged

The fact that it can use attributes is not really relevant. The ZEND_ACC flag matters, not how it was set.
I prefer to just test this via zend_test, sure we can test via E_STRICT but once E_STRICT is removed we would be forced to change the test again to use a different constant in the test.

@nielsdos nielsdos closed this in 9d69ab9 Sep 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DanielEScherzer DanielEScherzer DanielEScherzer approved these changes

@dstogov dstogov Awaiting requested review from dstogov dstogov is a code owner

Assignees

No one assigned

Labels

Category: Engine Extension: zend_test

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Assertion failure when error handler throws when accessing a deprecated constant

2 participants

@nielsdos @DanielEScherzer