Fix proxy interface for ProxyManager or Symfony LazyGhost #947
Conversation
GromNaN
requested review from
jmikola and
paulinevos
and removed request for
jmikola
GromNaN
force-pushed
the
fix-proxy-interface
branch
from
95dbccc to
ca1909c
Compare
|
|
||
| $container->getDefinition('doctrine_mongodb') | ||
| ->setArgument(5, $config['enable_lazy_ghost_objects'] ? LazyLoadingInterface::class : Proxy::class); | ||
| ->setArgument(5, $config['enable_lazy_ghost_objects'] ? Proxy::class : LazyLoadingInterface::class); |
There was a problem hiding this comment.
Reading #940 (comment) I'm still confused. These aren't the best class names. Is it worth adding a block comment above this to explain?
I went so far as to look at https://github.com/FriendsOfPHP/proxy-manager-lts/blob/1.x/src/ProxyManager/Proxy/LazyLoadingInterface.php and it's not even clear from that file.
There was a problem hiding this comment.
This interfaces are used to get the parent class when looking for a class metadata from a proxy class name.
https://github.com/doctrine/persistence/blob/00bd9ccc1f9c6a74fc2dc557313869acd1998cba/src/AbstractManagerRegistry.php#L135
| $loader->load([$config], $container); | ||
| } | ||
|
|
||
| #[DataProvider('provideLazyObjectConfigurations')] |
There was a problem hiding this comment.
Does PHPStan want you to annotate array $config as an iterable of array shapes? I don't think that's necessary.
There was a problem hiding this comment.
I added the @phpstan-type
|
|
||
| if (PHP_VERSION_ID >= 80400) { | ||
| yield 'Native Lazy Objects' => [ | ||
| ['enable_lazy_ghost_objects' => false, 'enable_native_lazy_objects' => true], |
There was a problem hiding this comment.
Should you test specifying true for both options? I assume enable_native_lazy_objects might take precedence, but would understand if they are mutually exclusive and should throw.
There was a problem hiding this comment.
enable_native_lazy_objects has priority over enable_lazy_ghost_objects. I don't test which lazy object implementation is used here, only overriding both default values since they are dynamically set depending of the PHP and ODM versions.
| self::assertInstanceOf(ManagerRegistry::class, $registry); | ||
| self::assertInstanceOf(DocumentManager::class, $dm); | ||
|
|
||
| // Create a lazy object |
There was a problem hiding this comment.
Consider a bigger comment that explains the last assertion on the DM registry is actually pertinent to testing the 5th argument (and your bug fix).
Assertion messages also work fine.
GromNaN
force-pushed
the
fix-proxy-interface
branch
from
07663ae to
fd608c2
Compare
2 participants
I was wrong in #940 (comment).
Fix the interface that identifies the lazy objects.
Note that
Doctrine\Persistence\Mapping\ProxyClassNameResolvershould be used byDoctrine\Persistence\AbstractManagerRegistryinstead of repeating the same logic.