diff --git a/.doctrine-project.json b/.doctrine-project.json index c25d00e4443..e1f84c7b85d 100644 --- 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 index ef703efa09d..326d17de079 100644 --- 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 index fdd42aeb6c5..6761ac1b974 100644 --- 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``: 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 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/ORMSetup.php b/lib/Doctrine/ORM/ORMSetup.php index 4b667628610..7210209727d 100644 --- 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 index 5bf8a6b6085..64ca810e78b 100644 --- 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 new file mode 100644 index 00000000000..0919d6d33b4 --- /dev/null +++ b/tests/Doctrine/Tests/Models/Issue9300/Issue9300Child.php @@ -0,0 +1,44 @@ + + * @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 new file mode 100644 index 00000000000..f9b50b36c95 --- /dev/null +++ b/tests/Doctrine/Tests/Models/Issue9300/Issue9300Parent.php @@ -0,0 +1,30 @@ +_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 new file mode 100644 index 00000000000..5a6309e433c --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/Issue9300Test.php @@ -0,0 +1,80 @@ +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 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. diff --git a/tests/Doctrine/Tests/OrmFunctionalTestCase.php b/tests/Doctrine/Tests/OrmFunctionalTestCase.php index 07bf94eb654..4a64136f184 100644 --- 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 */