Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Turn identity map collisions from exception to deprecation notice #10878

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions UPGRADE.md
Original file line number Diff line number Diff line change
@@ -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
Expand Down
10 changes: 10 additions & 0 deletions lib/Doctrine/ORM/Configuration.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
47 changes: 40 additions & 7 deletions lib/Doctrine/ORM/UnitOfWork.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
Expand Down
2 changes: 2 additions & 0 deletions tests/Doctrine/Tests/ORM/Functional/BasicFunctionalTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down
20 changes: 20 additions & 0 deletions tests/Doctrine/Tests/ORM/UnitOfWorkTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down