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

…ing merge()

This PR tries to improve the situation/problem explained in doctrine#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), doctrine#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,95 @@
+<?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);
+
+        // The $mergedArticle's #user property should hold the $mergedUser we obtained previously,
+        // since that's the only legetimate object instance representing the user from the UoW's
+        // point of view.
+        self::assertSame($mergedUser, $mergedArticle->user);
+
+        // Inspect internal UoW state
+        $uow               = $this->_em->getUnitOfWork();
+        $entityIdentifiers = $this->grabProperty('entityIdentifiers', $uow);
+        $identityMap       = $this->grabProperty('identityMap', $uow);
+        $entityStates      = $this->grabProperty('entityStates', $uow);
+
+        // UoW#entityIdentifiers contains two OID -> ID value mapping entries,
+        // one for the article, one for the user object
+        self::assertCount(2, $entityIdentifiers);
+        self::assertArrayHasKey($mergedArticleOid, $entityIdentifiers);
+        self::assertArrayHasKey($mergedUserOid, $entityIdentifiers);
+
+        // UoW#entityStates contains two OID -> state entries,
+        // one for the article, one for the user object
+        self::assertSame([
+            $mergedUserOid => UnitOfWork::STATE_MANAGED,
+            $mergedArticleOid => UnitOfWork::STATE_MANAGED,
+        ], $entityStates);
+
+        self::assertCount(2, $entityIdentifiers);
+        self::assertArrayHasKey($mergedArticleOid, $entityIdentifiers);
+        self::assertArrayHasKey($mergedUserOid, $entityIdentifiers);
+
+        // The identity map contains exactly two objects, the article and the user.
+        self::assertSame([
+            CmsUser::class => [$user->id => $mergedUser],
+            CmsArticle::class => [$article->id => $mergedArticle],
+        ], $identityMap);
+    }
+
+    private function grabProperty(string $name, UnitOfWork $uow)
+    {
+        $reflection = new ReflectionClass($uow);
+        $property   = $reflection->getProperty($name);
+        $property->setAccessible(true);
+
+        return $property->getValue($uow);
+    }
+}