Add a safeguard against multiple objects competing for the same ident…

…ity map entry

While trying to understand #3037, I found that it may happen that we have more entries in `\Doctrine\ORM\UnitOfWork::$entityIdentifiers` than in `\Doctrine\ORM\UnitOfWork::$identityMap`.

The former is a mapping from `spl_object_id` values to ID hashes, the latter an array first of entity class names and then from ID hash to entity object instances.

(Basically, "ID hash" is a concatenation of all field values making up the `@Id` for a given entity.)

This means that at some point, we must have _different_ objects representing the same entity.

I don't think it makes sense to overwrite an entity in the identity map, since that may leave `\Doctrine\ORM\UnitOfWork::$entityIdentifiers` in an inconsistent state.

If it makes sense at all to _replace_ an entity, that should happen through dedicated management methods to first detach the old entity before persisting, merging or otherwise adding the new one. This way we could make sure the internal structures remain consistent.

lib/Doctrine/ORM/UnitOfWork.php

@@ -1569,6 +1569,15 @@ public function addToIdentityMap($entity)
         $className     = $classMetadata->rootEntityName;
 
         if (isset($this->identityMap[$className][$idHash])) {
+            if ($this->identityMap[$className][$idHash] !== $entity) {
+                throw new RuntimeException(sprintf(
+                    'While adding an entity of class %s with an ID hash of "%s" to the identity map, another object of class %s was already present for the same ID. This exception is a safeguard against an internal inconsistency - IDs should uniquely map to entity object instances. This problem may occur if you use application-provided IDs and reuse ID values. Or, if database-provided IDs are reassigned after truncating the database without clearing the EntityManager. Or, you might have been using EntityManager#getReference() to create a reference for a nonexistent ID that was subsequently (by the RDBMS) assigned to another entity. Otherwise, it might be an ORM-internal inconsistency, please report it.',
+                    get_class($entity),
+                    $idHash,
+                    get_class($this->identityMap[$className][$idHash])
+                ));
+            }
+
             return false;
         }
 
@@ -2788,25 +2797,20 @@ public function createEntity($className, array $data, &$hints = [])
             }
 
             $this->originalEntityData[$oid] = $data;
+
+            if ($entity instanceof NotifyPropertyChanged) {
+                $entity->addPropertyChangedListener($this);
+            }
         } else {
             $entity = $this->newInstance($class);
             $oid    = spl_object_id($entity);
-
-            $this->entityIdentifiers[$oid]  = $id;
-            $this->entityStates[$oid]       = self::STATE_MANAGED;
-            $this->originalEntityData[$oid] = $data;
-
-            $this->identityMap[$class->rootEntityName][$idHash] = $entity;
+            $this->registerManaged($entity, $id, $data);
 
             if (isset($hints[Query::HINT_READ_ONLY])) {
                 $this->readOnlyObjects[$oid] = true;
             }
         }
 
-        if ($entity instanceof NotifyPropertyChanged) {
-            $entity->addPropertyChangedListener($this);
-        }
-
         foreach ($data as $field => $value) {
             if (isset($class->fieldMappings[$field])) {
                 $class->reflFields[$field]->setValue($entity, $value);
@@ -2964,20 +2968,7 @@ public function createEntity($className, array $data, &$hints = [])
                                 break;
                             }
 
-                            // PERF: Inlined & optimized code from UnitOfWork#registerManaged()
-                            $newValueOid                                                     = spl_object_id($newValue);
-                            $this->entityIdentifiers[$newValueOid]                           = $associatedId;
-                            $this->identityMap[$targetClass->rootEntityName][$relatedIdHash] = $newValue;
-
-                            if (
-                                $newValue instanceof NotifyPropertyChanged &&
-                                ( ! $newValue instanceof Proxy || $newValue->__isInitialized())
-                            ) {
-                                $newValue->addPropertyChangedListener($this);
-                            }
-
-                            $this->entityStates[$newValueOid] = self::STATE_MANAGED;
-                            // make sure that when an proxy is then finally loaded, $this->originalEntityData is set also!
+                            $this->registerManaged($newValue, $associatedId, []);
                             break;
                     }
 

tests/Doctrine/Tests/ORM/Functional/ProxiesLikeEntitiesTest.php

@@ -95,7 +95,7 @@ public function testEntityWithIdentifier(): void
      */
     public function testProxyAsDqlParameterPersist(): void
     {
-        $proxy     = $this->_em->getProxyFactory()->getProxy(CmsUser::class, ['id' => $this->user->getId()]);
+        $proxy     = $this->_em->getReference(CmsUser::class, ['id' => $this->user->getId()]);
         $proxy->id = $this->user->getId();
         $result    = $this
             ->_em

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

@@ -56,7 +56,7 @@ public function testIssueProxyClear(): void
 
         $user2 = $this->_em->getReference(DDC1238User::class, $userId);
 
-        $user->__load();
+        //$user->__load();
 
         self::assertIsInt($user->getId(), 'Even if a proxy is detached, it should still have an identifier');
 

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

@@ -55,7 +55,7 @@ public function getClassMetadata($className): ClassMetadata
         $uow->clear();
         $uow->triggerEagerLoads();
 
-        self::assertSame(2, $em->getClassMetadataCalls);
+        self::assertSame(4, $em->getClassMetadataCalls);
     }
 }
 

tests/Doctrine/Tests/ORM/UnitOfWorkTest.php

@@ -108,6 +108,8 @@ protected function setUp(): void
         $driverConnection = $this->createMock(Driver\Connection::class);
         $driverConnection->method('prepare')
             ->willReturn($driverStatement);
+        $driverConnection->method('lastInsertId')
+            ->willReturnOnConsecutiveCalls(1, 2, 3, 4, 5, 6);
 
         $driver = $this->createMock(Driver::class);
         $driver->method('getDatabasePlatform')
@@ -923,6 +925,25 @@ public function testRemovedEntityIsRemovedFromOneToManyCollection(): void
         self::assertFalse($user->phonenumbers->isDirty());
         self::assertEmpty($user->phonenumbers->getSnapshot());
     }
+
+    public function testItThrowsWhenIdentityMapEntriesCollide(): void
+    {
+        // We're using application-provided IDs and assign the same ID twice
+        // Note this is about colliding IDs in the identity map in memory.
+        // Duplicate database-level IDs would be spotted when the EM is flushed.
+
+        $phone1 = new CmsPhonenumber();
+        $phone1->phonenumber = '1234';
+        $this->_unitOfWork->persist($phone1);
+
+        $phone2 = new CmsPhonenumber();
+        $phone2->phonenumber = '1234';
+
+        $this->expectException(\RuntimeException::class);
+        $this->expectExceptionMessageMatches('/another object was already present for the same ID/');
+
+        $this->_unitOfWork->persist($phone2);
+    }
 }
 
 /** @Entity */