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

Support for non-nullable typed properties #685

Open
wants to merge 21 commits into
base: 2.15.x
Choose a base branch
from

Conversation

petr-buchin
Copy link

@petr-buchin petr-buchin commented Apr 12, 2021

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)

do start from the test suite, by adding a class that looks like what you are trying to proxy, and making it work from there

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!

}, $this, 'ProxyManagerTestAsset\\ClassWithMixedProperties')->__invoke();

$class = new \ReflectionObject($localizedObject);
$this->bindProxyProperty($localizedObject, $class, 'publicProperty0');
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Done


$bodyTemplate = <<<'CODE'
$originalClass = $class;
while (! $class->hasProperty($propertyName)) {
Copy link
Owner

Choose a reason for hiding this comment

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

This operation should be performed during compilation, rather than during runtime: it's quite expensive to traverse ancestors.

Copy link
Author

Choose a reason for hiding this comment

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

@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?

Copy link
Author

Choose a reason for hiding this comment

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

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)) {
Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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()) {
Copy link
Owner

Choose a reason for hiding this comment

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

This call can be prevented by inspecting the reflection information for a property upfront

Copy link
Author

Choose a reason for hiding this comment

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

Done.

}, $this, 'ProxyManagerTestAsset\\ClassWithMixedReferenceableTypedProperties')->__invoke();

$expectedCode = <<<'PHP'
$class = new \ReflectionObject($localizedObject);
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Author

@petr-buchin petr-buchin Apr 13, 2021

Choose a reason for hiding this comment

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

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.

@petr-buchin
Copy link
Author

petr-buchin commented Apr 13, 2021

@Ocramius seems like in the last commit I've adressed all issues you mentioned.

Could you please give it a new review?

Thanks!

@petr-buchin
Copy link
Author

@Ocramius sorry for bothering you, but will you have time to review this PR in the coming weeks?

@Ocramius
Copy link
Owner

@petr-buchin possibly throwing it at the weekend - this week is packed with work.

@petr-buchin
Copy link
Author

@Ocramius thank you very much - it would be great to have this reviewed in the coming weeks whenever you have time for it.
So no hurry.

@petr-buchin
Copy link
Author

@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!

@Ocramius
Copy link
Owner

Ocramius commented Jun 9, 2021

Indeed, I should try and verify this locally, but did also not get to it as I was hoping, sorry :(

@petr-buchin petr-buchin changed the base branch from 2.12.x to 2.14.x January 31, 2022 16:44
@petr-buchin
Copy link
Author

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.
Our clients are the biggest banks in a world, so this code is "battle-tested" for almost a year in production and did not cause any problems or performance-related troubles.

But now we need to install symfony/orm-pack, which depends on friendsofphp/proxy-manager-lts, which is a fork from your repository.

friendsofphp/proxy-manager-lts replaces ocramius/ProxyManager lib via it's Composer settings, and your repository is upstream for it, thus to use it, I must have this PR merged here and updated from upstream in friendsofphp/proxy-manager-lts.

Could you please look at this PR? Maybe I can do something for you to consider this PR?

Thanks in advance!

@Ocramius Ocramius added this to the 2.14.0 milestone Jan 31, 2022
@Ocramius
Copy link
Owner

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 ocramius/proxy-manager, other than bugfixes, for a while now.

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 :)

@petr-buchin
Copy link
Author

petr-buchin commented Jan 31, 2022

@Ocramius I do understand you :)

If it matters - I don't work for a bank, banks are clients of company where I work :)

an mentioning that they're the biggest banks in the world only reduces my will to throw unpaid time at it

Сan I pay for you time to look at this?

If I do, please email me at petrbuchyn@gmail.com.

Thanks very much!

@Ocramius Ocramius self-requested a review February 22, 2022 15:18
@Ocramius Ocramius modified the milestones: 2.14.0, 2.15.0 Feb 28, 2022
@Ocramius Ocramius changed the base branch from 2.14.x to 2.15.x February 28, 2022 13:59
@nicolas-grekas
Copy link
Contributor

It'd be great rebasing + squashing this PR in one commit on top of 2.15.x

Copy link
Contributor

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

Shouldn't this be able to support readonly properties also?
Since they are readonly, using a reference to bind instances is not possible, but copying the value of these properties in the proxy would be just fine, since... they would be readonly :)

$localizedPropertyCode .= PHP_EOL;

if (! $property->isPublic()) {
$localizedPropertyCode .= '$property->setAccessible(true);' . PHP_EOL;
Copy link
Contributor

Choose a reason for hiding this comment

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

@ErvinSabic
Copy link

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 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allowing non-nullable properties for AccessInterceptorScopeLocalizer
5 participants