Merge branch '2.16.x' into 2.17.x

* 2.16.x: Turn identity map collisions from exception to deprecation notice (#10878) Add possibility to set reportFieldsWhereDeclared to true in ORMSetup (#10865) Fix UnitOfWork->originalEntityData is missing not-modified collections after computeChangeSet (#9301) Add an UPGRADE notice about the potential changes in commit order (#10866) Update branch metadata (#10862)
doctrine · Aug 7, 2023 · 58df407 · 58df407
2 parents eda1909 + a616914
commit 58df407
Show file tree

Hide file tree

Showing 12 changed files with 274 additions and 17 deletions.
diff --git a/.doctrine-project.json b/.doctrine-project.json
@@ -12,21 +12,27 @@
             "upcoming": true
         },
         {
-            "name": "2.16",
-            "branchName": "2.16.x",
-            "slug": "2.16",
+            "name": "2.17",
+            "branchName": "2.17.x",
+            "slug": "2.17",
             "upcoming": true
         },
         {
-            "name": "2.15",
-            "branchName": "2.15.x",
-            "slug": "2.15",
+            "name": "2.16",
+            "branchName": "2.16.x",
+            "slug": "2.16",
             "current": true,
             "aliases": [
                 "current",
                 "stable"
             ]
         },
+        {
+            "name": "2.15",
+            "branchName": "2.15.x",
+            "slug": "2.15",
+            "maintained": false
+        },
         {
             "name": "2.14",
             "branchName": "2.14.x",

diff --git a/UPGRADE.md b/UPGRADE.md
@@ -1,5 +1,31 @@
 # 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
+to fix a series of bugs where a correct (working) commit order was previously not found.
+Also, the new computation may get away with fewer queries being executed: By inserting
+referred-to entities first and using their ID values for foreign key fields in subsequent
+`INSERT` statements, additional `UPDATE` statements that were previously necessary can be
+avoided.
+
+When using database-provided, auto-incrementing IDs, this may lead to IDs being assigned
+to entities in a different order than it was previously the case.
+
 ## Deprecated `\Doctrine\ORM\Internal\CommitOrderCalculator` and related classes
 
 With changes made to the commit order computation, the internal classes

diff --git a/docs/en/reference/advanced-configuration.rst b/docs/en/reference/advanced-configuration.rst
@@ -29,7 +29,7 @@ steps of configuration.
 
     $config = new Configuration;
     $config->setMetadataCache($metadataCache);
-    $driverImpl = new AttributeDriver(['/path/to/lib/MyProject/Entities']);
+    $driverImpl = new AttributeDriver(['/path/to/lib/MyProject/Entities'], true);
     $config->setMetadataDriverImpl($driverImpl);
     $config->setQueryCache($queryCache);
     $config->setProxyDir('/path/to/myproject/lib/MyProject/Proxies');
@@ -134,7 +134,7 @@ The attribute driver can be injected in the ``Doctrine\ORM\Configuration``:
     <?php
     use Doctrine\ORM\Mapping\Driver\AttributeDriver;
 
-    $driverImpl = new AttributeDriver(['/path/to/lib/MyProject/Entities']);
+    $driverImpl = new AttributeDriver(['/path/to/lib/MyProject/Entities'], true);
     $config->setMetadataDriverImpl($driverImpl);
 
 The path information to the entities is required for the attribute

diff --git 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/ORMSetup.php b/lib/Doctrine/ORM/ORMSetup.php
@@ -101,10 +101,11 @@ public static function createAttributeMetadataConfiguration(
         array $paths,
         bool $isDevMode = false,
         ?string $proxyDir = null,
-        ?CacheItemPoolInterface $cache = null
+        ?CacheItemPoolInterface $cache = null,
+        bool $reportFieldsWhereDeclared = false
     ): Configuration {
         $config = self::createConfiguration($isDevMode, $proxyDir, $cache);
-        $config->setMetadataDriverImpl(new AttributeDriver($paths));
+        $config->setMetadataDriverImpl(new AttributeDriver($paths, $reportFieldsWhereDeclared));
 
         return $config;
     }

diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php
@@ -692,6 +692,7 @@ public function computeChangeSet(ClassMetadata $class, $entity)
             if ($class->isCollectionValuedAssociation($name) && $value !== null) {
                 if ($value instanceof PersistentCollection) {
                     if ($value->getOwner() === $entity) {
+                        $actualData[$name] = $value;
                         continue;
                     }
 
@@ -1634,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
@@ -1650,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/Models/Issue9300/Issue9300Child.php b/tests/Doctrine/Tests/Models/Issue9300/Issue9300Child.php
@@ -0,0 +1,44 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\Tests\Models\Issue9300;
+
+use Doctrine\Common\Collections\ArrayCollection;
+use Doctrine\Common\Collections\Collection;
+use Doctrine\ORM\Mapping\Column;
+use Doctrine\ORM\Mapping\Entity;
+use Doctrine\ORM\Mapping\GeneratedValue;
+use Doctrine\ORM\Mapping\Id;
+use Doctrine\ORM\Mapping\ManyToMany;
+
+/**
+ * @Entity
+ */
+class Issue9300Child
+{
+    /**
+     * @var int
+     * @Id
+     * @Column(type="integer")
+     * @GeneratedValue
+     */
+    public $id;
+
+    /**
+     * @var Collection<int, Issue9300Parent>
+     * @ManyToMany(targetEntity="Issue9300Parent")
+     */
+    public $parents;
+
+    /**
+     * @var string
+     * @Column(type="string")
+     */
+    public $name;
+
+    public function __construct()
+    {
+        $this->parents = new ArrayCollection();
+    }
+}
diff --git a/tests/Doctrine/Tests/Models/Issue9300/Issue9300Parent.php b/tests/Doctrine/Tests/Models/Issue9300/Issue9300Parent.php
@@ -0,0 +1,30 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\Tests\Models\Issue9300;
+
+use Doctrine\ORM\Mapping\Column;
+use Doctrine\ORM\Mapping\Entity;
+use Doctrine\ORM\Mapping\GeneratedValue;
+use Doctrine\ORM\Mapping\Id;
+
+/**
+ * @Entity
+ */
+class Issue9300Parent
+{
+    /**
+     * @var int
+     * @Id
+     * @Column(type="integer")
+     * @GeneratedValue
+     */
+    public $id;
+
+    /**
+     * @var string
+     * @Column(type="string")
+     */
+    public $name;
+}
diff --git 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/Functional/Ticket/Issue9300Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/Issue9300Test.php
@@ -0,0 +1,80 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Doctrine\Tests\ORM\Functional\Ticket;
+
+use Doctrine\Common\Collections\ArrayCollection;
+use Doctrine\Tests\Models\Issue9300\Issue9300Child;
+use Doctrine\Tests\Models\Issue9300\Issue9300Parent;
+use Doctrine\Tests\OrmFunctionalTestCase;
+
+/**
+ * @group GH-9300
+ */
+class Issue9300Test extends OrmFunctionalTestCase
+{
+    protected function setUp(): void
+    {
+        $this->useModelSet('issue9300');
+
+        parent::setUp();
+    }
+
+    /**
+     * @group GH-9300
+     */
+    public function testPersistedCollectionIsPresentInOriginalDataAfterFlush(): void
+    {
+        $parent = new Issue9300Parent();
+        $child  = new Issue9300Child();
+        $child->parents->add($parent);
+
+        $parent->name = 'abc';
+        $child->name  = 'abc';
+
+        $this->_em->persist($parent);
+        $this->_em->persist($child);
+        $this->_em->flush();
+
+        $parent->name = 'abcd';
+        $child->name  = 'abcd';
+
+        $this->_em->flush();
+
+        self::assertArrayHasKey('parents', $this->_em->getUnitOfWork()->getOriginalEntityData($child));
+    }
+
+    /**
+     * @group GH-9300
+     */
+    public function testPersistingCollectionAfterFlushWorksAsExpected(): void
+    {
+        $parentOne = new Issue9300Parent();
+        $parentTwo = new Issue9300Parent();
+        $childOne  = new Issue9300Child();
+
+        $parentOne->name   = 'abc';
+        $parentTwo->name   = 'abc';
+        $childOne->name    = 'abc';
+        $childOne->parents = new ArrayCollection([$parentOne]);
+
+        $this->_em->persist($parentOne);
+        $this->_em->persist($parentTwo);
+        $this->_em->persist($childOne);
+        $this->_em->flush();
+
+        // Recalculate change-set -> new original data
+        $childOne->name = 'abcd';
+        $this->_em->flush();
+
+        $childOne->parents = new ArrayCollection([$parentTwo]);
+
+        $this->_em->flush();
+        $this->_em->clear();
+
+        $childOneFresh = $this->_em->find(Issue9300Child::class, $childOne->id);
+        self::assertCount(1, $childOneFresh->parents);
+        self::assertEquals($parentTwo->id, $childOneFresh->parents[0]->id);
+    }
+}
diff --git 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.

diff --git a/tests/Doctrine/Tests/OrmFunctionalTestCase.php b/tests/Doctrine/Tests/OrmFunctionalTestCase.php
@@ -338,6 +338,10 @@ abstract class OrmFunctionalTestCase extends OrmTestCase
             Models\Issue5989\Issue5989Employee::class,
             Models\Issue5989\Issue5989Manager::class,
         ],
+        'issue9300' => [
+            Models\Issue9300\Issue9300Child::class,
+            Models\Issue9300\Issue9300Parent::class,
+        ],
     ];
 
     /** @param class-string ...$models */