From a616914887ea160db4158d2c67752e99624f7c8a Mon Sep 17 00:00:00 2001
From: Matthias Pigulla <mp@webfactory.de>
Date: Fri, 4 Aug 2023 14:06:02 +0200
Subject: [PATCH] Turn identity map collisions from exception to deprecation
 notice (#10878)

In #10785, a check was added that prevents entity instances from getting into the identity map when another object for the same ID is already being tracked.

This caused regressions for users that work with application-provided IDs and expect this condition to fail with `UniqueConstraintViolationExceptions` when flushing to the database.

Thus, this PR turns the exception into a deprecation notice. Users can opt-in to the new behavior. In 3.0, the exception will be used.

Implements #10871.
---
 UPGRADE.md                                    | 14 ++++++
 lib/Doctrine/ORM/Configuration.php            | 10 ++++
 lib/Doctrine/ORM/UnitOfWork.php               | 47 ++++++++++++++++---
 .../ORM/Functional/BasicFunctionalTest.php    |  2 +
 tests/Doctrine/Tests/ORM/UnitOfWorkTest.php   | 20 ++++++++
 5 files changed, 86 insertions(+), 7 deletions(-)

diff --git a/UPGRADE.md b/UPGRADE.md
index b7d099fe9ba..326d17de079 100644
--- a/UPGRADE.md
+++ b/UPGRADE.md
@@ -1,5 +1,19 @@
 # Upgrade to 2.16
 
+## Deprecated accepting duplicate IDs in the identity map
+
+For any given entity class and ID value, there should be only one object instance
+representing the entity. 
+
+In https://github.com/doctrine/orm/pull/10785, a check was added that will guard this
+in the identity map. The most probable cause for violations of this rule are collisions
+of application-provided IDs.
+
+In ORM 2.16.0, the check was added by throwing an exception. In ORM 2.16.1, this will be
+changed to a deprecation notice. ORM 3.0 will make it an exception again. Use
+`\Doctrine\ORM\Configuration::setRejectIdCollisionInIdentityMap()` if you want to opt-in
+to the new mode.
+
 ## Potential changes to the order in which `INSERT`s are executed
 
 In https://github.com/doctrine/orm/pull/10547, the commit order computation was improved
diff --git a/lib/Doctrine/ORM/Configuration.php b/lib/Doctrine/ORM/Configuration.php
index fcde0c190fa..ca5063667c2 100644
--- a/lib/Doctrine/ORM/Configuration.php
+++ b/lib/Doctrine/ORM/Configuration.php
@@ -1117,4 +1117,14 @@ public function setLazyGhostObjectEnabled(bool $flag): void
 
         $this->_attributes['isLazyGhostObjectEnabled'] = $flag;
     }
+
+    public function setRejectIdCollisionInIdentityMap(bool $flag): void
+    {
+        $this->_attributes['rejectIdCollisionInIdentityMap'] = $flag;
+    }
+
+    public function isRejectIdCollisionInIdentityMapEnabled(): bool
+    {
+        return $this->_attributes['rejectIdCollisionInIdentityMap'] ?? false;
+    }
 }
diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php
index f9b8cf2b892..64ca810e78b 100644
--- a/lib/Doctrine/ORM/UnitOfWork.php
+++ b/lib/Doctrine/ORM/UnitOfWork.php
@@ -1635,8 +1635,10 @@ public function addToIdentityMap($entity)
 
         if (isset($this->identityMap[$className][$idHash])) {
             if ($this->identityMap[$className][$idHash] !== $entity) {
-                throw new RuntimeException(sprintf(
-                    <<<'EXCEPTION'
+                if ($this->em->getConfiguration()->isRejectIdCollisionInIdentityMapEnabled()) {
+                    throw new RuntimeException(
+                        sprintf(
+                            <<<'EXCEPTION'
 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
@@ -1651,11 +1653,42 @@ public function addToIdentityMap($entity)
 
 Otherwise, it might be an ORM-internal inconsistency, please report it. 
 EXCEPTION
-                    ,
-                    get_class($entity),
-                    $idHash,
-                    get_class($this->identityMap[$className][$idHash])
-                ));
+                            ,
+                            get_class($entity),
+                            $idHash,
+                            get_class($this->identityMap[$className][$idHash])
+                        )
+                    );
+                } else {
+                    Deprecation::trigger(
+                        'doctrine/orm',
+                        'https://github.com/doctrine/orm/pull/10785',
+                        <<<'EXCEPTION'
+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 will trigger
+an exception in ORM 3.0.
+
+IDs should uniquely map to entity object instances. This problem may occur if:
+
+- you use application-provided IDs and reuse ID values;
+- database-provided IDs are reassigned after truncating the database without
+clearing the EntityManager;
+- 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.
+
+To opt-in to the new exception, call 
+\Doctrine\ORM\Configuration::setRejectIdCollisionInIdentityMap on the entity
+manager's configuration.
+EXCEPTION
+                        ,
+                        get_class($entity),
+                        $idHash,
+                        get_class($this->identityMap[$className][$idHash])
+                    );
+                }
             }
 
             return false;
diff --git a/tests/Doctrine/Tests/ORM/Functional/BasicFunctionalTest.php b/tests/Doctrine/Tests/ORM/Functional/BasicFunctionalTest.php
index 7356a1bfe0b..2be3feba4ed 100644
--- a/tests/Doctrine/Tests/ORM/Functional/BasicFunctionalTest.php
+++ b/tests/Doctrine/Tests/ORM/Functional/BasicFunctionalTest.php
@@ -1329,6 +1329,8 @@ public function testWrongAssociationInstance(): void
 
     public function testItThrowsWhenReferenceUsesIdAssignedByDatabase(): void
     {
+        $this->_em->getConfiguration()->setRejectIdCollisionInIdentityMap(true);
+
         $user           = new CmsUser();
         $user->name     = 'test';
         $user->username = 'test';
diff --git a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php
index e47af9d57df..31fa5ea8f64 100644
--- a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php
+++ b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php
@@ -927,8 +927,28 @@ public function testRemovedEntityIsRemovedFromOneToManyCollection(): void
         self::assertEmpty($user->phonenumbers->getSnapshot());
     }
 
+    public function testItTriggersADeprecationNoticeWhenApplicationProvidedIdsCollide(): 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->expectDeprecationWithIdentifier('https://github.com/doctrine/orm/pull/10785');
+
+        $this->_unitOfWork->persist($phone2);
+    }
+
     public function testItThrowsWhenApplicationProvidedIdsCollide(): void
     {
+        $this->_emMock->getConfiguration()->setRejectIdCollisionInIdentityMap(true);
+
         // 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.