Support for non-nullable typed properties #685
Conversation
| }, $this, 'ProxyManagerTestAsset\\ClassWithMixedProperties')->__invoke(); | ||
|
|
||
| $class = new \ReflectionObject($localizedObject); | ||
| $this->bindProxyProperty($localizedObject, $class, 'publicProperty0'); |
There was a problem hiding this comment.
It would be better to inline these, instead of generating $this->bindProxyProperty().
Remember that this is generated code, so we don't really want to abstract it, but rather let the optimizer do as much work as possible.
In additon to that, bindProxyProperty is a too generic name, and will likely collide with parent class implementation.
|
|
||
| $bodyTemplate = <<<'CODE' | ||
| $originalClass = $class; | ||
| while (! $class->hasProperty($propertyName)) { |
There was a problem hiding this comment.
This operation should be performed during compilation, rather than during runtime: it's quite expensive to traverse ancestors.
There was a problem hiding this comment.
@Ocramius unfortunately I don't understand how to perform this during compilation: different properties would have different declaring classes. So how this can be done during compilation?
There was a problem hiding this comment.
Done - I'm generating a $classesMap variable at build time, which has instances of ReflectionClass for private properties, and then use it when localizing private props.
| } | ||
| $property = $class->getProperty($propertyName); | ||
| $property->setAccessible(true); | ||
| if (!$property->isInitialized($localizedObject)) { |
There was a problem hiding this comment.
If I understand this correctly, the difference here is that we aren't creating an "empty shell", but proxying when the other class is correctly initialized?
There was a problem hiding this comment.
Yes: for now proxy can be created only when properties are nullable and have default value in class itself.
Thus this library tries to be responsible for user classes and objects.
This PR moves responsibility of initializing object to the user of the library: class may have non-nullable typed properties, but as long as concrete instance being proxied is properly initialized - it can be proxied.
| ) | ||
| ); | ||
| } | ||
| if (!$property->isPrivate()) { |
There was a problem hiding this comment.
This call can be prevented by inspecting the reflection information for a property upfront
| }, $this, 'ProxyManagerTestAsset\\ClassWithMixedReferenceableTypedProperties')->__invoke(); | ||
|
|
||
| $expectedCode = <<<'PHP' | ||
| $class = new \ReflectionObject($localizedObject); |
There was a problem hiding this comment.
Ideally, we should be able to avoid runtime reflection for all properties that do not have a declared type or do have a default null value
There was a problem hiding this comment.
Done - I've returned original version of code generation for untyped properties and nullable properties with default values.
Other properties ($nonReferenceableProperties) are then used to generate localization code with ReflectionProperty::isInitialized() checks.
|
@Ocramius seems like in the last commit I've adressed all issues you mentioned. Could you please give it a new review? Thanks! |
|
@Ocramius sorry for bothering you, but will you have time to review this PR in the coming weeks? |
|
@petr-buchin possibly throwing it at the weekend - this week is packed with work. |
|
@Ocramius thank you very much - it would be great to have this reviewed in the coming weeks whenever you have time for it. |
|
@Ocramius may I ask you to look into this when you have time? We have entire framework depending on this feature, and it would be great to use this library instead of fork (as we do now). Thanks in advance! |
|
Indeed, I should try and verify this locally, but did also not get to it as I was hoping, sorry :( |
…d-properties-support
…rScopeLocalizer proxy
…tr-buchin/ProxyManager into non-nullable-typed-properties-support
|
Dear @Ocramius, is there any chance you can look at this PR ? Our company uses fork of ProxyManager repo, that includes this PR, for almost a year. Our 2 opensource repositories are dependent on this fork functionality. But now we need to install
Could you please look at this PR? Maybe I can do something for you to consider this PR? Thanks in advance! |
|
Hey @petr-buchin, I'll put it on the milestone for the next minor, which includes PHP 8.1.x support, but I haven't actively worked on As for the clients using this, I don't really care much, an mentioning that they're the biggest banks in the world only reduces my will to throw unpaid time at it :D I'll see when I can work on this :) |
|
@Ocramius I do understand you :) If it matters - I don't work for a bank, banks are clients of company where I work :)
Сan I pay for you time to look at this? If I do, please email me at Thanks very much! |
|
It'd be great rebasing + squashing this PR in one commit on top of 2.15.x |
| $localizedPropertyCode .= PHP_EOL; | ||
|
|
||
| if (! $property->isPublic()) { | ||
| $localizedPropertyCode .= '$property->setAccessible(true);' . PHP_EOL; |
There was a problem hiding this comment.
can be removed since https://wiki.php.net/rfc/make-reflection-setaccessible-no-op
|
We're running into a support requirement for this as well and will be using this PR branch. But it would be awesome if it could be merged in directly 😄 |
Fixes #683 and #682
This PR adds support for non-nullable typed properties when using AccessInterceptorScopeLocalizer proxy.
@Ocramius I tried to start from tests, as you said in #683 (comment)
I added class
ClassWithNonNullableTypedProperties, but as far as I see, existing tests are pretty informative already when used with this class, since all I want to achieve is to have this class proxied (which is being done in tests) and to have proxy being able to act like an original object (being in sync with it), which is also covered by existing tests.So if you can think of any other tests that I need to write for this PR, please request changes and I'll do it.
Thanks in advance!