From 710dde83aa7e3661d5814abb65f6bc401af90152 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gr=C3=A9goire=20Paris?= Date: Tue, 1 Aug 2023 14:56:34 +0200 Subject: [PATCH 1/5] Update branch metadata (#10862) --- .doctrine-project.json | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) 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", From eeefc6bc0fad9cc0bae2687e3dfe5d08a3b64375 Mon Sep 17 00:00:00 2001 From: Matthias Pigulla Date: Wed, 2 Aug 2023 13:42:49 +0200 Subject: [PATCH 2/5] Add an UPGRADE notice about the potential changes in commit order (#10866) --- UPGRADE.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/UPGRADE.md b/UPGRADE.md index ef703efa09d..b7d099fe9ba 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -1,5 +1,17 @@ # Upgrade to 2.16 +## 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 From f50803ccb91c5c31aede16df73fa7560a42f02ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Ol=C5=A1avsk=C3=BD?= Date: Wed, 2 Aug 2023 13:44:15 +0200 Subject: [PATCH 3/5] Fix UnitOfWork->originalEntityData is missing not-modified collections after computeChangeSet (#9301) * Fix original data incomplete after flush * Apply suggestions from code review Co-authored-by: Alexander M. Turek --------- Co-authored-by: Alexander M. Turek --- lib/Doctrine/ORM/UnitOfWork.php | 1 + .../Tests/Models/Issue9300/Issue9300Child.php | 44 ++++++++++ .../Models/Issue9300/Issue9300Parent.php | 30 +++++++ .../ORM/Functional/Ticket/Issue9300Test.php | 80 +++++++++++++++++++ .../Doctrine/Tests/OrmFunctionalTestCase.php | 4 + 5 files changed, 159 insertions(+) create mode 100644 tests/Doctrine/Tests/Models/Issue9300/Issue9300Child.php create mode 100644 tests/Doctrine/Tests/Models/Issue9300/Issue9300Parent.php create mode 100644 tests/Doctrine/Tests/ORM/Functional/Ticket/Issue9300Test.php diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 5bf8a6b6085..f9b8cf2b892 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; } 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 @@ +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/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 */ From fd0bdc69b0a25b9de99de814be60dbc163ac84a2 Mon Sep 17 00:00:00 2001 From: Dieter Beck Date: Wed, 2 Aug 2023 14:34:13 +0200 Subject: [PATCH 4/5] Add possibility to set reportFieldsWhereDeclared to true in ORMSetup (#10865) Otherwise it is impossible to avoid a deprecation warning when using ORMSetup::createAttributeMetadataConfiguration() --- docs/en/reference/advanced-configuration.rst | 4 ++-- lib/Doctrine/ORM/ORMSetup.php | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) 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/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; } From a616914887ea160db4158d2c67752e99624f7c8a Mon Sep 17 00:00:00 2001 From: Matthias Pigulla Date: Fri, 4 Aug 2023 14:06:02 +0200 Subject: [PATCH 5/5] 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.