Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:3.4fromalekitto:3.4
Feb 3, 2020

Conversation

@alekitto
Copy link
Contributor

QA
Branch?3.4
Bug fix?yes
New feature?no
Deprecations?no
TicketsFix#35544
LicenseMIT

Resolve bug#35544.

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

returnnull;
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.

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

@greedyivangreedyivanFeb 1, 2020•
edited
Loading

Choose a reason for hiding this comment

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

There is a discussionPHP 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
ContributorAuthor

@alekittoalekittoFeb 1, 2020•
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

ThehasType 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

@greedyivangreedyivanFeb 1, 2020•
edited
Loading

Choose a reason for hiding this comment

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

ThehasType 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

@greedyivangreedyivanFeb 1, 2020•
edited
Loading

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
ContributorAuthor

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.

alekitto reacted with thumbs up emoji
@nicolas-grekasnicolas-grekas added this to the3.4 milestoneFeb 1, 2020
Copy link
Member

@xabbuhxabbuh 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 isunset 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.

alekitto reacted with thumbs up emoji

Copy link
Member

@nicolas-grekasnicolas-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
ContributorAuthor

Reasonable, isn't it?

Yes, i've corrected the comments.

@fabpot
Copy link
Member

Thank you@alekitto.

fabpot added a commit that referenced this pull requestFeb 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       | MITResolve 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
@fabpotfabpot merged commit427bc3a intosymfony:3.4Feb 3, 2020
This was referencedFeb 29, 2020
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@xabbuhxabbuhxabbuh approved these changes

+1 more reviewer

@greedyivangreedyivangreedyivan left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

3.4

Development

Successfully merging this pull request may close these issues.

6 participants

@alekitto@greedyivan@fabpot@nicolas-grekas@xabbuh@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp