Avoid creating unmanaged proxy instances for referred-to entities dur…

…ing merge()

This PR tries to improve the situation/problem explained in #3037:

Under certain conditions – there may be multiple and not all are known/well-understood – we may get inconsistencies between the `\Doctrine\ORM\UnitOfWork::$entityIdentifiers` and `\Doctrine\ORM\UnitOfWork::$identityMap` arrays.

Since the `::$identityMap` is a plain array holding object references, objects contained in it cannot be garbage-collected.
`::$entityIdentifiers`, however, is indexed by `spl_object_id` values. When those objects are destructed and/or garbage-collected, the OID may be reused and reassigned to other objects later on.

When the OID re-assignment happens to be for another entity, the UoW may assume incorrect entity states and, for example, miss INSERT or UPDATE operations.

One cause for such inconsistencies is _replacing_ identity map entries with other object instances: This makes it possible that the old object becomes GC'd, while its OID is not cleaned up. Since that is not a use case we need to support (IMHO), #10785 is about adding a safeguard against it.

In this test shown here, the `merge()` operation is currently too eager in creating a proxy object for another referred-to entity. This proxy represents an entity already present in the identity map at that time, potentially leading to this problem later on.

lib/Doctrine/ORM/UnitOfWork.php

@@ -3640,14 +3640,18 @@ private function mergeEntityStateIntoManagedCopy($entity, $managedCopy): void
  $targetClass = $this->em->getClassMetadata($assoc2['targetEntity']);
  $relatedId = $targetClass->getIdentifierValues($other);
 
- if ($targetClass->subClasses) {
- $other = $this->em->find($targetClass->name, $relatedId);
- } else {
- $other = $this->em->getProxyFactory()->getProxy(
- $assoc2['targetEntity'],
- $relatedId
- );
- $this->registerManaged($other, $relatedId, []);
+ $other = $this->tryGetById($relatedId, $targetClass->name);
+
+ if (! $other) {
+ if ($targetClass->subClasses) {
+ $other = $this->em->find($targetClass->name, $relatedId);
+ } else {
+ $other = $this->em->getProxyFactory()->getProxy(
+ $assoc2['targetEntity'],
+ $relatedId
+ );
+ $this->registerManaged($other, $relatedId, []);
+ }
  }
  }
 

tests/Doctrine/Tests/ORM/Functional/Ticket/GH7407Test.php

@@ -0,0 +1,87 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\Tests\ORM\Functional\Ticket;
+
+use Doctrine\ORM\UnitOfWork;
+use Doctrine\Tests\Models\CMS\CmsArticle;
+use Doctrine\Tests\Models\CMS\CmsUser;
+use Doctrine\Tests\OrmFunctionalTestCase;
+use ReflectionClass;
+
+use function spl_object_id;
+
+class GH7407Test extends OrmFunctionalTestCase
+{
+ protected function setUp(): void
+ {
+ $this->useModelSet('cms');
+
+ parent::setUp();
+ }
+
+ public function testMergingEntitiesDoesNotCreateUnmanagedProxyReferences(): void
+ {
+ // 1. Create an article with a user; persist, flush and clear the entity manager
+ $user = new CmsUser();
+ $user->username = 'Test';
+ $user->name = 'Test';
+ $this->_em->persist($user);
+
+ $article = new CmsArticle();
+ $article->topic = 'Test';
+ $article->text = 'Test';
+ $article->setAuthor($user);
+ $this->_em->persist($article);
+
+ $this->_em->flush();
+ $this->_em->clear();
+
+ // 2. Merge the user object back in:
+ // We get a new (different) entity object that represents the user instance
+ // which is now (through this object instance) managed by the EM/UoW
+ $mergedUser = $this->_em->merge($user);
+ $mergedUserOid = spl_object_id($mergedUser);
+
+ // 3. Merge the article object back in,
+ // the returned entity object is the article instance as it is managed by the EM/UoW
+ $mergedArticle = $this->_em->merge($article);
+ $mergedArticleOid = spl_object_id($mergedArticle);
+
+ self::assertSame($mergedUser, $mergedArticle->user, 'The $mergedArticle\'s #user property should hold the $mergedUser we obtained previously, since that\'s the only legitimate object instance representing the user from the UoW\'s point of view.');
+
+ // Inspect internal UoW state
+ $uow = $this->_em->getUnitOfWork();
+ $entityIdentifiers = $this->grabProperty('entityIdentifiers', $uow);
+ $identityMap = $this->grabProperty('identityMap', $uow);
+ $entityStates = $this->grabProperty('entityStates', $uow);
+
+ self::assertCount(2, $entityIdentifiers, 'UoW#entityIdentifiers contains exactly two OID -> ID value mapping entries one for the article, one for the user object');
+ self::assertArrayHasKey($mergedArticleOid, $entityIdentifiers);
+ self::assertArrayHasKey($mergedUserOid, $entityIdentifiers);
+
+ self::assertSame([
+ $mergedUserOid => UnitOfWork::STATE_MANAGED,
+ $mergedArticleOid => UnitOfWork::STATE_MANAGED,
+ ], $entityStates, 'UoW#entityStates contains two OID -> state entries, one for the article, one for the user object');
+
+ self::assertCount(2, $entityIdentifiers);
+ self::assertArrayHasKey($mergedArticleOid, $entityIdentifiers);
+ self::assertArrayHasKey($mergedUserOid, $entityIdentifiers);
+
+ self::assertSame([
+ CmsUser::class => [$user->id => $mergedUser],
+ CmsArticle::class => [$article->id => $mergedArticle],
+ ], $identityMap, 'The identity map contains exactly two objects, the article and the user.');
+ }
+
+ private function grabProperty(string $name, UnitOfWork $uow)
+ {
+ $reflection = new ReflectionClass($uow);
+ $property = $reflection->getProperty($name);
+ $property->setAccessible(true);
+
+ return $property->getValue($uow);
+ }
+}