Skip to content
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

[Validator] check for __get method existence if property is uninitialized #35546

Merged
merged 1 commit into from Feb 3, 2020

Conversation

alekitto
Copy link
Contributor

Q A
Branch? 3.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #35544
License MIT

Resolve bug #35544.

On PHP 7.4, check if object implements __get magic method if property is reported as uninitialized before returning null.

return null;
if (\PHP_VERSION_ID >= 70400 && $reflProperty->hasType() && !$reflProperty->isInitialized($object)) {
// There is no way to check if a property has been unset or it is uninitialized.
// When trying to access an uninitialized property, __get method is triggered in PHP 7.4.0, but not in 7.4.1+.
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that there was a bug in PHP 7.4.0 and __get() should never have been called? In this case, I am not convinced that we should do anything here as the behaviour is inline with what happens in PHP 7.4.1+.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

or should we call __get() directly?

Copy link
Contributor

@greedyivan greedyivan Feb 1, 2020

Choose a reason for hiding this comment

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

There is a discussion PHP Bug # 78904.

This is a PHP related problem, I think. Framework should not implement any hack for this. ProxyManager and others are currently not compatible with PHP 7.4.1+.

Copy link
Contributor Author

@alekitto alekitto Feb 1, 2020

Choose a reason for hiding this comment

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

__get must be called when accessing a property if the property has been explicitly unset, but not if it is uninitialized.
On the other side ReflectionProperty::isInitialized is returning false for both situations and there is no safe way to distinguish one state from the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The hasType has been added as if the property is not typed, no additional check should be made.
I think that \Error is too generic. Per typed properties RFC \TypeError is raised in case of access an uninitialized non-nullable typed property.

Copy link
Contributor

@greedyivan greedyivan Feb 1, 2020

Choose a reason for hiding this comment

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

The hasType has been added as if the property is not typed, no additional check should be made.

Agreed. hasType is just a zero cost check.

I think that \Error is too generic. Per typed properties RFC \TypeError is raised in case of access an uninitialized non-nullable typed property.

https://3v4l.org/lmTYH

Don't trust docs blindly.

Copy link
Contributor

@greedyivan greedyivan Feb 1, 2020

Choose a reason for hiding this comment

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

https://3v4l.org/lmTYH

According that, additional tests should be added.

  • typed uninitialized propery with no unset in constructor and with __get method.

This test would validate a correct way of \Error catching. In case if it will be changed to \TypeError in future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just added a check. Too bad that RFC is not respected about the \TypeError

Copy link
Contributor

Choose a reason for hiding this comment

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

Too bad that RFC is not respected about the \TypeError

Doc Bug #79206
Let's see, it is a doc bug or not.

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Feb 1, 2020
Copy link
Member

@xabbuh xabbuh left a comment

Choose a reason for hiding this comment

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

I am strongly against merging this as is as this would now work differently than how PHP behaves in 7.4.1+. I suggest to wait until a final decision has been made for PHP and then decide whether or not we have to adapt the validator code.

@greedyivan
Copy link
Contributor

@xabbuh this would now work differently than how PHP behaves in 7.4.1+.

Here we trying to solve one special case, when typed property is unset in constructor. In that state __get method is invoked when the property is accessed.

Only difference with 7.4.0 that this magic method was invoked in uninitialized state also.

This is normal (invoking __get for unset properties) but not clearly documented feature. ProxyManager uses it, and previous bugfix broke this special case.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Reasonable, isn't it?

@alekitto
Copy link
Contributor Author

alekitto commented Feb 3, 2020

Reasonable, isn't it?

Yes, i've corrected the comments.

@fabpot
Copy link
Member

fabpot commented Feb 3, 2020

Thank you @alekitto.

fabpot added a commit that referenced this pull request Feb 3, 2020
…s uninitialized (alekitto)

This PR was merged into the 3.4 branch.

Discussion
----------

[Validator] check for __get method existence if property is uninitialized

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #35544
| License       | MIT

Resolve bug #35544.

On PHP 7.4, check if object implements `__get` magic method if property is reported as uninitialized before returning null.

Commits
-------

427bc3a [Validator] try to call __get method if property is uninitialized
@fabpot fabpot merged commit 427bc3a into symfony:3.4 Feb 3, 2020
This was referenced Feb 29, 2020
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.

None yet

6 participants