Skip to content

Fix GH-12695: skip __get() when __isset() materialised the property#22181

Merged
iluuu1994 merged 1 commit into
php:masterfrom
nicolas-grekas:gh12695-isset-materialization-fix
Jul 20, 2026
Merged

Fix GH-12695: skip __get() when __isset() materialised the property#22181
iluuu1994 merged 1 commit into
php:masterfrom
nicolas-grekas:gh12695-isset-materialization-fix

Conversation

@nicolas-grekas

Copy link
Copy Markdown
Contributor

Fixes #12695.

After __isset() returns true under ?? or empty(), the engine re-checks the property table before calling __get(). When __isset() has materialised the property (a pattern used by lazy proxies), its value is returned directly and __get() is skipped.

isset() is unchanged (it never called __get()).

Tests under Zend/tests/magic_methods/gh12695*.phpt cover:

  • the fix for ?? and empty()
  • non-regression: when __isset() doesn't materialise, __get() is still called and ??'s null-check still applies
  • isset()'s unchanged behaviour

@iluuu1994 iluuu1994 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks functionally correct. I'd be happy merging this to master if the rest of internals are happy.

Comment thread Zend/zend_object_handlers.c Outdated
Comment thread Zend/zend_object_handlers.c Outdated
Comment thread Zend/zend_object_handlers.c Outdated
@nicolas-grekas
nicolas-grekas force-pushed the gh12695-isset-materialization-fix branch from 1c11fe3 to 5d92fdc Compare May 29, 2026 14:17

@arnaud-lb arnaud-lb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks right to me otherwise

Comment thread Zend/zend_object_handlers.c
Comment thread Zend/zend_object_handlers.c
Comment thread Zend/zend_object_handlers.c Outdated
@nicolas-grekas
nicolas-grekas force-pushed the gh12695-isset-materialization-fix branch from 5d92fdc to 0e4cd8a Compare May 29, 2026 14:33
@nicolas-grekas
nicolas-grekas force-pushed the gh12695-isset-materialization-fix branch from 0e4cd8a to c47e4ec Compare May 29, 2026 14:46

@iluuu1994 iluuu1994 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks! LGTM. Given this is on verge of bug and feature, it would be good to post on the list again in a separate thread about this PR, and clarify that this is proposed to be merged unless there are objections. We usually wait for at least 2 weeks for such changes to be merged. If there are concerns, this will need to go through the RFC process nonetheless.

@TimWolla TimWolla left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Only looked at the tests and NEWS/UPGRADING. I trust the implementation review of Arnaud and Ilija.

Comment thread Zend/tests/magic_methods/gh12695.phpt
Comment thread Zend/tests/magic_methods/gh12695_no_materialization.phpt Outdated
@nicolas-grekas

nicolas-grekas commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Almost two weeks and no pushback on internals, be ready to merge ;)

@nicolas-grekas
nicolas-grekas force-pushed the gh12695-isset-materialization-fix branch from 70aaed7 to 335c0fc Compare July 20, 2026 08:57
@nicolas-grekas

Copy link
Copy Markdown
Contributor Author

PR rebased with minor conflicts resolved. Anybody to push the merge button?

@iluuu1994

Copy link
Copy Markdown
Member

Thanks for the reminder. I will handle this today.

After __isset() returns true under ?? or empty(), re-check the
property table before calling __get(). When __isset() materialised
the property (a pattern used by lazy proxies), its value is returned
directly. isset() itself is unchanged.

Closes phpGH-12695.
@iluuu1994
iluuu1994 force-pushed the gh12695-isset-materialization-fix branch from 335c0fc to 762ec32 Compare July 20, 2026 19:35
@iluuu1994
iluuu1994 merged commit 0641165 into php:master Jul 20, 2026
14 of 18 checks passed
@iluuu1994

Copy link
Copy Markdown
Member

Thanks Nicolas!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wrong magic methods sequence with ?? operator

4 participants