E525 [PropertyAccess] Fix accessing dynamic properties by andreyserdjuk · Pull Request #37622 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[PropertyAccess] Fix accessing dynamic properties#37622

Merged
fabpot merged 1 commit intosymfony:5.1from
andreyserdjuk:master
Aug 26, 2020
Merged

[PropertyAccess] Fix accessing dynamic properties#37622
fabpot merged 1 commit intosymfony:5.1from
andreyserdjuk:master

Conversation

@andreyserdjuk
Copy link
Copy Markdown
Contributor
@andreyserdjuk andreyserdjuk commented Jul 20, 2020 •
edited by nicolas-grekas
Loading
Q A
Branch? 5.1 (to be switched when merging)
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #37026
License MIT
Doc PR no

@andreyserdjuk andreyserdjuk mentioned this pull request Jul 20, 2020
Closed
@nicolas-grekas
Copy link
Copy Markdown
Member
nicolas-grekas commented Jul 22, 2020

/cc @joelwurtz @Korbeil WDYT?

On my side, I'm wondering about the perf impact of the implementation. Isn't this the same as removing the $object instanceof \stdClass check?

Please also add tests.

@nicolas-grekas nicolas-grekas added this to the 5.1 milestone Jul 22, 2020
@nicolas-grekas nicolas-grekas changed the title Updated PropertyAccessor to access public properties for other classes [PropertyAccess] Fix accessing dynamic properties Jul 22, 2020
@andreyserdjuk
Copy link
Copy Markdown
Contributor Author
andreyserdjuk commented Jul 22, 2020 •
edited
Loading

@nicolas-grekas it will fail when trying to access protected/private property ($access is null but we dunno why: it's a dynamic object property or class protected/private inaccessible property). The only reason why \stdClass check was written before - to be 100% sure that class property is dynamic and cannot be private/protected - inaccessible.
To improve performance I suggest having PropertyReadInfo in any case to make a decision looking on PropertyReadInfo::$visibility. But it requires some refactoring.
BTW setValue() has the same logic. So I guess it also should support dynamic value?

@andreyserdjuk
Copy link
Copy Markdown
Contributor Author
andreyserdjuk commented Jul 23, 2020

Update: unit-test added. But only PropertyAccessor::getValue() is covered with getting dynamic value. setValue() - I didn't cover and as I see from unit-tests, the dynamic value cannot be set by design - at least setter method should be.
@dunglas , could you please tell (you created ReflectionExtractor), whether support of dynamic values getting/setting by ref evolutionally (and intentionally) was removed? I just want to be sure not to harm the idea of this tool.

@joelwurtz
Copy link
Copy Markdown
Contributor
joelwurtz commented Jul 29, 2020

I'm not sure about perf also, it may be slower (but need benchmark for that)

Another possible implementation would be to follow @nicolas-grekas suggestion by only removing the instanceof part and add a try catch when getting the value (to catch error if property is not accessible : protected / private), maybe it would keep perf reasonable and would also allow a better error message (like property exists but is not accessible)

@andreyserdjuk
Copy link
Copy Markdown
Contributor Author
andreyserdjuk commented Jul 31, 2020

@nicolas-grekas please review the last approach suggested by @joelwurtz .

joelwurtz
joelwurtz reviewed Aug 1, 2020
View reviewed changes
src/Symfony/Component/PropertyAccess/PropertyAccessor.php Outdated
}
} catch (\Error $e) {
if (!$ignoreInvalidProperty) {
throw new NoSuchPropertyException(sprintf('Can\'t read protected or private property "%s" in class "%s".', $property, $class));
Copy link
Copy Markdown
Contributor
@joelwurtz joelwurtz Aug 1, 2020

Choose a reason for hiding this comment

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

you can add the error in the parent exception

Copy link
Copy Markdown
Contributor Author
@andreyserdjuk andreyserdjuk Aug 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I've added a previous exception as an argument, hope I've got you right.

nicolas-grekas
nicolas-grekas reviewed Aug 1, 2020
View reviewed changes
@fabpot fabpot changed the base branch from master to 5.1 August 26, 2020 13:25
@fabpot
Copy link
Copy Markdown
Member
fabpot commented Aug 26, 2020

Thank you @andreyserdjuk.

@fabpot fabpot merged commit 92cb709 into symfony:5.1 Aug 26, 2020
@fabpot
Copy link
Copy Markdown
Member
fabpot commented Aug 30, 2020

Reverting this PR as it breaks things by bypassing a lot of logic. One such example is when you have a class with a private property AND a __get method to access it (like TestClassMagicGet in our test suite). Before this PR, if you disable magic __get method, you will get an expected NoSuchPropertyException. But now, the property_exists() call will return true, and $object->$property will get the value because there is a __get method (which the code explicitly disallow).

fabpot added a commit that referenced this pull request Aug 30, 2020
@fabpot
Revert "bug #37622 [PropertyAccess] Fix accessing dynamic properties …
47b9b5c 
…(andreyserdjuk)"

This reverts commit 92cb709, reversing
changes made to fc3095f.
fabpot added a commit that referenced this pull request Aug 30, 2020
@fabpot
Merge branch '5.1'
0885ebb 
* 5.1:
  Revert "bug #37622 [PropertyAccess] Fix accessing dynamic properties (andreyserdjuk)"
@fabpot fabpot mentioned this pull request Aug 31, 2020
Merged
@xabbuh xabbuh mentioned this pull request Feb 26, 2021
Closed
@Voetloos
Copy link
Copy Markdown
Voetloos commented Mar 18, 2021

Issue #37615 is again an issue since this is reverted. Any update on that? @fabpot @xabbuh @nicolas-grekas

@xabbuh xabbuh mentioned this pull request Dec 3, 2021
Merged
@renatogcarvalho renatogcarvalho mentioned this pull request Jan 4, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@fabpot fabpot fabpot approved these changes

@dunglas dunglas Awaiting requested review from dunglas

@jderusse jderusse Awaiting requested review from jderusse

@lyrixx lyrixx Awaiting requested review from lyrixx

@sroze sroze Awaiting requested review from sroze

@xabbuh xabbuh Awaiting requested review from xabbuh

+2 more reviewers

@joelwurtz joelwurtz joelwurtz left review comments

@maxhelias maxhelias maxhelias approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Bug PropertyAccess Status: Needs Review

Projects

None yet

Milestone

5.1

Development

Successfully merging this pull request may close these issues.

Recover support to write properties for non stdClass objects

8 participants

@andreyserdjuk @nicolas-grekas @joelwurtz @fabpot @Voetloos @maxhelias @xabbuh @carsonbot
0