php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #63463 ReflectionProperty::setValue and ::getValue trigger magic methods
Submitted: 2012-11-08 06:15 UTC Modified: 2012-12-18 23:11 UTC
From: ocramius at gmail dot com Assigned:
Status: Not a bug Package: Reflection related
PHP Version: 5.4.8 OS: Irrelevant
Private report: No CVE-ID: None
 [2012-11-08 06:15 UTC] ocramius at gmail dot com
Description:
------------
When unset properties are requested, ReflectionProperty::setValue and ReflectionProperty::getValue trigger __get and __set magic methods.

Test script:
---------------
https://gist.github.com/4037155

Expected result:
----------------
The test script should report NULL for each of the requested values, and never call __set nor __get

Actual result:
--------------
__set and __get are called when ReflectionProperty tries to read or write to the requested properties.

Patches

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2012-11-08 06:22 UTC] ocramius at gmail dot com
I've added the failing test case as a PR at https://github.com/php/php-src/pull/230
 [2012-12-10 01:51 UTC] stas@php.net
I'm not sure why you think this is what should be happening. When you unset 
properties, they are no longer set. And when you access unset properties, magic 
methods kick in. So where's the problem here?
 [2012-12-10 01:51 UTC] stas@php.net
-Status: Open +Status: Feedback
 [2012-12-10 02:04 UTC] ocramius at gmail dot com
-Status: Feedback +Status: Open
 [2012-12-10 02:04 UTC] ocramius at gmail dot com
@stas let me put it into a more "practical" use case I currently have:

I use reflection to populate instances of proxy objects that have "lazy" marked properties unset at instantiation time. This is good to achieve lazy loading of public properties, but any r/w access to the property itself via reflection triggers `__get` or `__set`.

This makes it impossible to use reflection to populate the property. I worked around it by disabling `__get` in particular situations (see https://github.com/Ocramius/common/blob/DCOM-96/lib/Doctrine/Common/Reflection/RuntimePublicReflectionProperty.php), but at a terrible cost in performance terms and broken behaviour in rare cases.

Back to the issue itself:

I consider Reflection as my last resource to access and modify status of objects without affecting anything else within my environment. Having reflection trigger any logic during an assignment is an unwanted and dangerous behaviour in my opinion.
 [2012-12-18 21:15 UTC] stas@php.net
-Status: Open +Status: Not a bug
 [2012-12-18 21:15 UTC] stas@php.net
I am sorry, but I think your opinion is incorrect. Reflection works the same way and 
uses the same engine methods as other ways of accessing properties. Making 
Reflection work differently from the rest of the object model would require special 
exceptions in all object engine for Reflection and seems to make little sense to me 
in general. Reflection is just another way of doing the same thing as regular 
property access. I am sorry that your code does not work as you wanted it to, and 
you are welcome to propose ways to improve it, including participating in 
getters/setters discussion ongoing, but I do not think in this particular case 
reflection is doing something wrong. In any case, it is not a bug as the code is 
doing exactly as the intent of the code was for it to do.
 [2012-12-18 23:11 UTC] ocramius at gmail dot com
Isn't reflection able to handle properties in a different way from how userland code acts? I wouldn't consider Reflection at the same level of other code, or at least I'm not aware of any way of achieving the same functionality without having access to internal APIs.

I'm not denying the fact that this is probably a design issue and that it would require major refactoring to workaround it, but the fact that such effort is needed doesn't mean that this is not a bug.

Want to mark it as improvement instead? Or as a documentation issue and therefore language limitation (with relative improvement issue)?
 [2013-03-14 16:37 UTC] evan at digitalflophouse dot com
It would seem to me that the ideal solution here would be for 
ReflectionProperty::setValue and ::getValue to support an optional parameter 
indicating whether magic methods should be triggered (the default) or not.
 [2018-12-13 12:13 UTC] benjamin dot morel at gmail dot com
I was just bitten by the same "feature"; I guess my use case is similar to Ocramius': proxies.

I'm unset()ing properties in the proxy constructor, and using ReflectionProperty::getValue() to read the properties, set or not.

Unfortunately, doing so triggers __get(), which initializes my proxy.


Both points of views expressed above are valid IMO:

- viewing Reflection as just another way to access the property (Stas)
- viewing Reflection as a low-level tool to access properties without triggering the usual behaviour (Ocramius)


The problem with the current implementation is that it doesn't help create lazy initialization scenarios, a feature that's good for the PHP ecosystem.

Evan's suggestion is excellent IMO, as it fixes this issue without breaking BC:

ReflectionProperty::setValue() and getValue() could support an optional parameter 
indicating whether magic methods should be triggered (the default) or not.

Would you consider it?
 [2018-12-13 12:19 UTC] ocramius at gmail dot com
I managed to lazy-load everything without relying on reflection for hydration: maybe bring this offline and let's help each other out?

The current behavior is extremely locked in by existing consumers, so I don't think that changing it further is a good idea.
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Sat Nov 23 04:01:28 2024 UTC