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

Switch proxies back to Doctrine Persistence and LazyGhostTrait #2692

Open
wants to merge 14 commits into
base: 3.0.x
Choose a base branch
from

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Oct 19, 2024

Q A
Type improvement
BC Break yes
Fixed issues -

Summary

This PR replaces the proxy implementation from friendsofphp/proxy-manager-lts (which requires laminas/laminas-code) with doctrine/persistence and the LazyGhostTrait from symfony/var-exporter.

Copy the proxy implementation from Doctrine ORM: https://github.com/doctrine/orm/blob/3.3.x/src/Proxy/ProxyFactory.php

The Doctrine\Persistence\Proxy class was added in doctrine/persistence: 3.1, we already require 3.2.

This is a breaking change, so I open this PR for 3.0. The opposite move was done between 1.x and 2.0: #1875, but that was before Nicolas work on lazy ghost objects.

@GromNaN GromNaN changed the base branch from 2.10.x to 3.0.x October 19, 2024 00:46
Comment on lines +504 to +510
self::assertInstanceOf(Proxy::class, $profile);
$this->expectException(DocumentNotFoundException::class);
$this->expectExceptionMessage(
'The "Documents\Profile" document with identifier ' .
'"abcdefabcdefabcdefabcdef" could not be found.',
);
$profile->__load();
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand why there was no exception for this missing document, while there is for previous tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

This class my not be required for this PR, but it enable serialization/unserialization of proxy objects. The doc need to be duplicated for the ODM: https://www.doctrine-project.org/projects/doctrine-orm/en/3.3/reference/advanced-configuration.html#autoloading-proxies

Copy link
Member Author

Choose a reason for hiding this comment

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

This interface could be deprecated in 2.10.x. I don't think we need more than a single ProxyFactory.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or maybe this will be useful again, if we can have an optimized ProxyFactory for PHP 8.4 with Lazy Objects.

@GromNaN GromNaN requested a review from alcaeus October 19, 2024 02:10
@GromNaN GromNaN added this to the 3.0.0 milestone Oct 19, 2024
@malarzm
Copy link
Member

malarzm commented Oct 19, 2024

Wouldn't it be better to wait for PHP's native proxies? Or change from Symfony's implementation to native one will be transparent for end users?

@GromNaN
Copy link
Member Author

GromNaN commented Oct 19, 2024

My main motivation is to get rid of laminas code dependency and converge to the same implementation as the ORM. This change is compatible with every supported PHP version whereas the new native lazy objects are PHP 8.4+ only.

I'm novice in the lazy ghost objects domain. Maybe @nicolas-grekas would have a better insight.

My guess is that we are going to have 2 ProxyFactory classes: one for PHP 8.4+ and one for older versions. But both would use var-exporter and doctrine/persistence.

@nicolas-grekas
Copy link
Member

I think it should be possible to delegate all the native proxy handling to var-exporter (+doctrine/persistence). That's where the choice of the best implement should happen.
Then, when you'll have PHP 8.4 as a minimum dependency, the lib should go native-only.
The only thing to keep in mind is making as much things internal as possible to make the switch to native-only easier.

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

Successfully merging this pull request may close these issues.

3 participants