Skip to content

Also don't call other magic for uninitialized typed properties #4974

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

Closed
wants to merge 1 commit into from

Conversation

nikic
Copy link
Member

@nikic nikic commented Dec 6, 2019

We already changed the behavior for __set() in f1848a4. However, it seems that this is also a problem for all the other property magic, see bug #78904.

This commit makes the behavior of all the property magic consistent: Magic will not be triggered for uninitialized typed properties, only explicit unset() ones. This brings behavior more in line how non-typed properties behave and avoid WTF.

As this is a non-trivial behavior change, we should land this ASAP (in 7.4.1), otherwise our hands may be tied.

These cases are much less important, but for consistency we should
treat all magic property methods the same with regard to how they
handle uninitialized typed properties.
@nikic
Copy link
Member Author

nikic commented Dec 6, 2019

I have verified that this does not break ProxyManager (cc @Ocramius), presumably it always unsets all properties, regardless whether they're typed or not.

@nikic
Copy link
Member Author

nikic commented Dec 6, 2019

@derickr I'd like to land this change for 7.4.1. If we make this change it should happen ASAP, before people start to rely on the behavior in a big way.

@nikic
Copy link
Member Author

nikic commented Dec 9, 2019

Per discussion in chat, going forward with this.

@php-pulls php-pulls closed this in 84354c6 Dec 9, 2019
vvval pushed a commit to cycle/proxy-factory that referenced this pull request Dec 24, 2019
See php/php-src#4974
Now ONLY FOR 7.4.0 <= php version < 7.4.1 proxy does not unset public typed properties with defaults
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants