Switch proxies to `LazyGhostTrait` by GromNaN · Pull Request #2700 · doctrine/mongodb-odm

GromNaN · 2024-11-23T21:26:07Z

Q	A
Type	improvement
BC Break	no
Fixed issues	Replace #2692

Summary

Use the LazyGhostTrait from symfony/var-exporter to generate lazy proxy classes.

Add LazyGhostProxyFactory that implements the ProxyFactory interface and use it by default
Add configuration to switch back to Proxy Manager, and run the test suite with this it
Deprecate using proxy manager, to be removed in 3.0
Add documentation (including Autoloading Proxies)
Review implementation to ensure property hooks and native lazy ghost will not require any breaking change. Using the ORM PRs as reference implementation Add support for ReflectionClass::newLazyGhost of PHP 8.4 orm#11652 and Necessary refactorings for Property hooks orm#11659
Fix job names for the new matrix variable lazy-ghost-objects

GromNaN · 2024-11-23T22:01:39Z

lib/Doctrine/ODM/MongoDB/Proxy/Resolver/LazyGhostProxyClassNameResolver.php

+use function substr;
+
+/** @internal */
+class LazyGhostProxyClassNameResolver implements ClassNameResolver, ProxyClassNameResolver


This class is copied from Doctrine\ORM\Proxy\DefaultProxyClassNameResolver.

GromNaN · 2024-11-23T22:02:08Z

lib/Doctrine/ODM/MongoDB/Proxy/InternalProxy.php

+ * @template T of object
+ * @template-extends Proxy<T>
+ */
+interface InternalProxy extends Proxy


This interface is copied from Doctrine\ORM\Proxy\InternalProxy.

GromNaN · 2024-11-23T22:03:33Z

lib/Doctrine/ODM/MongoDB/Configuration.php

+        $proxyManagerConfiguration = new ProxyManagerConfiguration();
+        $proxyManagerConfiguration->setProxiesTargetDir($this->getProxyDir());
+        $proxyManagerConfiguration->setProxiesNamespace($this->getProxyNamespace());
+
+        switch ($this->getAutoGenerateProxyClasses()) {
+            case self::AUTOGENERATE_FILE_NOT_EXISTS:
+                $proxyManagerConfiguration->setGeneratorStrategy(new FileWriterGeneratorStrategy(
+                    new FileLocator($proxyManagerConfiguration->getProxiesTargetDir()),
+                ));
+
+                break;
+            case self::AUTOGENERATE_EVAL:
+                $proxyManagerConfiguration->setGeneratorStrategy(new EvaluatingGeneratorStrategy());
+
+                break;
+            default:
+                throw new InvalidArgumentException('Invalid proxy generation strategy given - only AUTOGENERATE_FILE_NOT_EXISTS and AUTOGENERATE_EVAL are supported.');
+        }
+
+        return $proxyManagerConfiguration;


Creating ProxyManagerConfiguration on-demand as the class is not required when LazyGhostObjects are used.

GromNaN · 2024-11-23T22:04:48Z

lib/Doctrine/ODM/MongoDB/Proxy/Autoloader.php

+ *
+ * @internal
+ */
+final class Autoloader


This class is copied from Doctrine\ORM\Proxy\Autoloader.

GromNaN · 2024-11-23T22:06:14Z

lib/Doctrine/ODM/MongoDB/Proxy/Factory/LazyGhostProxyFactory.php

+        return static function (InternalProxy $proxy, mixed $identifier) use ($persister, $classMetadata, $factory): void {
+            $original = $persister->load([$classMetadata->identifier => $identifier], $proxy);
+
+            if (! $original && ! $factory->lifecycleEventManager->documentNotFound($proxy, $identifier)) {
+                throw DocumentNotFoundException::documentNotFound($classMetadata->getName(), $identifier);
+            }
+
+            // phpcs:ignore SlevomatCodingStandard.ControlStructures.EarlyExit.EarlyExitNotUsed
+            if ($proxy instanceof NotifyPropertyChanged) {
+                $proxy->addPropertyChangedListener($factory->uow);
+            }
+        };


Initialization code copied from StaticProxyFactory.

GromNaN · 2024-11-23T22:08:19Z

lib/Doctrine/ODM/MongoDB/Proxy/Factory/ProxyFactory.php

     * @template T of object
     */
-    public function getProxy(ClassMetadata $metadata, $identifier): GhostObjectInterface;
+    public function getProxy(ClassMetadata $metadata, $identifier): object;


The interface is modified, but child classes can be more restrictive. This is not a BC break for implementers of the interface, but it can be for users of the interface.

GromNaN · 2024-11-23T22:24:22Z

tests/Doctrine/ODM/MongoDB/Tests/BaseTestCase.php

+    public static function isLazyObject(object $document): bool
+    {
+        return $document instanceof InternalProxy || $document instanceof LazyLoadingInterface;
+    }


Most test changes are related to check interfaces to assert if the class is a lazy proxy object. I wonder if this method should be in UOW.

Later, it will use ReflectionClass::getLazyInitializer() to detect native lazy object.

GromNaN · 2024-11-23T22:32:00Z

lib/Doctrine/ODM/MongoDB/Configuration.php

+
+    /**
+     * Generate proxy classes using Symfony VarExporter's LazyGhostTrait if true.
+     * Otherwise, use ProxyManager's LazyLoadingGhostFactory (deprecated)
+     */
+    public function setUseLazyGhostObject(bool $flag): void
+    {
+        if ($flag === false) {
+            if (! class_exists(ProxyManagerConfiguration::class)) {
+                throw new LogicException('Package "friendsofphp/proxy-manager-lts" is required to disable LazyGhostObject.');
+            }
+
+            trigger_deprecation(
+                'doctrine/mongodb-odm',
+                '2.6',
+                'Using "friendsofphp/proxy-manager-lts" is deprecated. Use "symfony/var-exporter" LazyGhostObjects instead.',
+            );
+        }
+
+        $this->useLazyGhostObject = $flag;
+    }
+
+    public function isLazyGhostObjectEnabled(): bool
+    {
+        return $this->useLazyGhostObject ?? true;
+    }


Should we use the same method names as the ORM: isLazyGhostObjectEnabled and setLazyGhostObjectEnabled?

lib/Doctrine/ODM/MongoDB/Proxy/Factory/LazyGhostProxyFactory.php

lib/Doctrine/ODM/MongoDB/Configuration.php

SenseException · 2024-12-30T23:00:10Z

Considering the unchecked checkboxes in your initial PR description, is this a WIP?

GromNaN · 2024-12-31T11:28:47Z

I've just rebased this PR.

The bundle will need an update to register the autoloader.

Regarding native lazy objects and prop hooks PR on ODM, I think the approaches are not conflicting this this PR.

alcaeus

Just some minor comments on this. I think the "BC Break" in the ProxyFactory interface is fine - proxies are an implementation detail and as long as we communicate that we change the default proxy mechanism we'll be fine. People shouldn't rely on a specific proxy behaviour, other than "it behaves as if it was an instance of the original class".

alcaeus · 2025-01-21T12:30:03Z

lib/Doctrine/ODM/MongoDB/Configuration.php

    }

+    /** @deprecated */
    public function getProxyManagerConfiguration(): ProxyManagerConfiguration


Note that this will return a new configuration instance every time this is called. I don't recall if there were any issues with that approach (e.g. due to unique identifiers in directories), so it's worth checking that creating a separate configuration doesn't cause the proxy manager to look for proxy files in different directories.

I updated to keep the instance in memory so that it's still possible to customize the configuration of the proxy manager directly. But it is reset each time a configuration is set on the ODM configuration class.

lib/Doctrine/ODM/MongoDB/Configuration.php

lib/Doctrine/ODM/MongoDB/Proxy/Factory/LazyGhostProxyFactory.php

phpstan-baseline.neon

alcaeus

Changes LGTM. @malarzm do you want to take another look or should we merge this for 2.10?

GromNaN · 2025-01-22T13:46:25Z

I just found an incompatibility with DoctrineMongoDBODMBundle, the ProxyManager\Proxy\GhostObjectInterface interface is hardcoded. I think we have to keep lts-proxy-manager by default and add change the default configuration in a new version of DoctrineMongoDBODMBundle.

GromNaN mentioned this pull request Nov 23, 2024

Switch proxies back to Doctrine Persistence and LazyGhostTrait #2692

Closed

GromNaN force-pushed the proxy-var-exporter-2 branch 4 times, most recently from 9bdb39c to a81c08c Compare November 23, 2024 21:36

GromNaN changed the title ~~Switch proxies back to LazyGhostTrait~~ Switch proxies to LazyGhostTrait Nov 23, 2024

GromNaN commented Nov 23, 2024

View reviewed changes

GromNaN requested review from alcaeus and malarzm November 23, 2024 22:32

GromNaN commented Nov 26, 2024

View reviewed changes

lib/Doctrine/ODM/MongoDB/Proxy/Factory/LazyGhostProxyFactory.php Outdated Show resolved Hide resolved

GromNaN commented Nov 26, 2024

View reviewed changes

lib/Doctrine/ODM/MongoDB/Configuration.php Outdated Show resolved Hide resolved

GromNaN force-pushed the proxy-var-exporter-2 branch 2 times, most recently from 631e341 to cbab97c Compare December 31, 2024 11:24

GromNaN added this to the 2.10.0 milestone Jan 17, 2025

GromNaN added Feature Enhancement and removed Feature labels Jan 17, 2025

alcaeus reviewed Jan 21, 2025

View reviewed changes

GromNaN added 8 commits January 21, 2025 13:45

Generate proxy classes using symfony/var-exporter

d297a49

Leverage UOW::initializeObject in tests

6b404b5

Trigger lifecycleEventManager from lazy ghost object

e528f8c

Run the full test suite with proxy manager

4e34386

Fix assert not lazy object

041b12f

Fix deprecation version and refactor autoregenerate condition

1888aab

Fix testCreateProxyForDocumentWithUnmappedProperties

564f32d

Keep a single ProxyManagerConfiguration instance

1ce91ac

GromNaN force-pushed the proxy-var-exporter-2 branch from cbab97c to 0798779 Compare January 21, 2025 13:34

Type fixes

4d4c864

GromNaN force-pushed the proxy-var-exporter-2 branch from 0798779 to 4d4c864 Compare January 21, 2025 13:35

alcaeus approved these changes Jan 22, 2025

View reviewed changes

Merge branch '2.10.x' into proxy-var-exporter-2

3820d20

IonBazan approved these changes Jan 22, 2025

View reviewed changes

GromNaN merged commit 336a70e into doctrine:2.10.x Jan 22, 2025
19 checks passed

GromNaN deleted the proxy-var-exporter-2 branch January 22, 2025 12:18

GromNaN mentioned this pull request Jan 22, 2025

Incompatibility new proxy implementation changed the interface of proxy objects #2719

Closed

github-actions bot mentioned this pull request Jan 24, 2025

Merge release 2.10.0 into 3.0.x #2722

Merged

GromNaN mentioned this pull request Jan 29, 2025

Ignore issue for method_exists always true reported by phpstan doctrine/DoctrineMongoDBBundle#878

Merged

GromNaN mentioned this pull request Feb 19, 2025

Fix access to null $config variable #2732

Merged

Uh oh!

Conversation

GromNaN commented Nov 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GromNaN Nov 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SenseException commented Dec 30, 2024

Uh oh!

GromNaN commented Dec 31, 2024

Uh oh!

alcaeus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alcaeus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

GromNaN commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

GromNaN commented Nov 23, 2024 •

edited

Loading

GromNaN Nov 23, 2024 •

edited

Loading

GromNaN commented Jan 22, 2025 •

edited

Loading