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

Leverage LazyGhostTrait when possible #10187

Merged
merged 1 commit into from
Dec 9, 2022

Conversation

nicolas-grekas
Copy link
Member

This PR replaces Doctrine Common's proxies by VarExporter's LazyGhostTrait.

It requires running composer require symfony/var-exporter:^6.2@dev doctrine/persistence:^3.1@dev for now until stable versions of both packages are released (will happen in one month for var-exporter.)

As you can see from the changes on the documentation, the benefit is quite clear: this removes many limitations of the current proxy implementation.

From what I've understood through many interactions around doctrine/common, deprecating everything in the Doctrine\Common\Proxy namespace is highly desired and this PR provides a critical step in this direction. Thanks to the design design + maintenance policies of LazyGhostTrait, this should also make it way simpler to support newer versions of PHP in the future (this is already the case for PHP 8.1 which is fully supported by the trait.).

The target version on ORM is 2.14. This should help move away from Common's proxies quite quickly, and will allow deprecating them ASAP. In turn, Common's Proxies wouldn't need to add support for newer versions of PHP (which is non trivial, as highlighted by the work still happening to support PHP 8.1.), making maintenance easier for the Doctrine project. Targeting ORM v3 wouldn't allow this.

Another reason to target ORM v2 is that the change is mostly backward compatible, so it doesn't need to target v3. In my experience, this can help adoption of ORM v3 when it will be released, by reducing the number of dependencies that one would have to update in the process of upgrading.

Still, enabling the new proxies is opt-in for a few reasons:

  • ORM v2 supports PHP >= 7.1 while symfony/var-exporter requires PHP >= 8.1, so we cannot make it a hard dep yet;
  • generated proxies do not implement the (deprecated) Doctrine\ORM\Proxy\Proxy interface anymore so ppl need to replace their instanceof checks to target Doctrine\Persistence\Proxy instead (which is implemented by both new and old proxy implems.)
  • __wakeup() is not called anymore when initializing entities that implement it (this should be transparent if ppl implemented it as advised in the doc - aka not doing anything when the identifier is not yet known.)

One needs to call Configuration::setLazyGhostObjectEnabled(true) to turn on the new proxies.

For cross-ref, this takes some inspiration+changes from #6719.

As I see for now, the steps after merging this could be to:

  1. deprecate everything in Doctrine\Common\Proxy
  2. deprecate NOT enabling the new proxies.
  3. make Configuration::setLazyGhostObjectEnabled() a no-op in ORM v3 (throwing when called with false as argument)
  4. deprecate Configuration::setLazyGhostObjectEnabled() but not before ORM v3.1 to ease the transition (we cannot remove the method in v3.0 since we cannot first deprecate the method in v2).

For inspiration for other projects, the plan on Symfony should be to add a new config option to DoctrineBundle to allow opting-in, to deprecate not setting it to true, and to update the default recipe to make new projects opt-in by default.

lib/Doctrine/ORM/Proxy/ProxyFactory.php Outdated Show resolved Hide resolved
lib/Doctrine/ORM/UnitOfWork.php Outdated Show resolved Hide resolved
@nicolas-grekas nicolas-grekas force-pushed the ve-proxy-2 branch 2 times, most recently from c95fb77 to 0590939 Compare October 28, 2022 15:15
beberlei
beberlei previously approved these changes Oct 28, 2022
@beberlei
Copy link
Member

Great work, ready to merge from my POV. There are some changes in UoW that are maybe a bit risky, but nothing stands out as buggy from my side. The normalizeIdentifier function is unfortunate to have, as it adds even more looping inside looping, but we can always look to improve that later.

@derrabus
Copy link
Member

Thank you very much! I haven't had time to test this yes, but the changes look promising. Since the PR does not deprecate the old proxy from the Common package (yet), we could ship this change as an experimental feature in 2.14. For 3.0 however, our goal should be to have only one proxy implementation.

@beberlei
Copy link
Member

beberlei commented Nov 1, 2022

Yes since 3.0 will bump requirement to php 8.1 we could go with the new implementation only.

@nicolas-grekas nicolas-grekas force-pushed the ve-proxy-2 branch 3 times, most recently from 5f3b45e to 7d61cfd Compare December 8, 2022 13:19
@nicolas-grekas
Copy link
Member Author

This PR is now ready, using only stable dependencies! 🚀

@greg0ire greg0ire merged commit aa4b62c into doctrine:2.14.x Dec 9, 2022
@greg0ire
Copy link
Member

greg0ire commented Dec 9, 2022

Thanks @nicolas-grekas !

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.

5 participants