Skip to content

Commit

Permalink
Reject ID collisions in identity map unconditionally
Browse files Browse the repository at this point in the history
  • Loading branch information
greg0ire committed Oct 7, 2023
1 parent 46ef989 commit 795e773
Show file tree
Hide file tree
Showing 5 changed files with 20 additions and 56 deletions.
6 changes: 6 additions & 0 deletions UPGRADE.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# Upgrade to 3.0

## BC BREAK: collisions in identity map are unconditionally rejected

`Doctrine\ORM\Configuration::setRejectIdCollisionInIdentityMap()` and
`Doctrine\ORM\Configuration::isRejectIdCollisionInIdentityMapEnabled()` are now
no-ops and will be deprecated in 3.1.0.

## BC BREAK: Lifecycle callback mapping on embedded classes is now explicitly forbidden

Lifecycle callback mapping on embedded classes produced no effect, and is now
Expand Down
15 changes: 13 additions & 2 deletions lib/Doctrine/ORM/Configuration.php
Original file line number Diff line number Diff line change
Expand Up @@ -605,13 +605,24 @@ public function setLazyGhostObjectEnabled(bool $flag): void
$this->attributes['isLazyGhostObjectEnabled'] = $flag;
}

/** To be deprecated in 3.1.0 */
public function setRejectIdCollisionInIdentityMap(bool $flag): void
{
$this->attributes['rejectIdCollisionInIdentityMap'] = $flag;
if (! $flag) {
throw new LogicException(<<<'EXCEPTION'

Check warning on line 612 in lib/Doctrine/ORM/Configuration.php

View check run for this annotation

Codecov / codecov/patch

lib/Doctrine/ORM/Configuration.php#L611-L612

Added lines #L611 - L612 were not covered by tests
Rejecting ID collisions in the identity map cannot be disabled anymore.
Please remove the call to setRejectIdCollisionInIdentityMap(false).
EXCEPTION);

Check warning on line 615 in lib/Doctrine/ORM/Configuration.php

View check run for this annotation

Codecov / codecov/patch

lib/Doctrine/ORM/Configuration.php#L615

Added line #L615 was not covered by tests
}
}

/**
* To be deprecated in 3.1.0
*
* @return true
*/
public function isRejectIdCollisionInIdentityMapEnabled(): bool
{
return $this->attributes['rejectIdCollisionInIdentityMap'] ?? false;
return true;

Check warning on line 626 in lib/Doctrine/ORM/Configuration.php

View check run for this annotation

Codecov / codecov/patch

lib/Doctrine/ORM/Configuration.php#L626

Added line #L626 was not covered by tests
}
}
33 changes: 1 addition & 32 deletions lib/Doctrine/ORM/UnitOfWork.php
Original file line number Diff line number Diff line change
Expand Up @@ -1485,38 +1485,7 @@ public function addToIdentityMap(object $entity): bool

if (isset($this->identityMap[$className][$idHash])) {
if ($this->identityMap[$className][$idHash] !== $entity) {
if ($this->em->getConfiguration()->isRejectIdCollisionInIdentityMapEnabled()) {
throw EntityIdentityCollisionException::create($this->identityMap[$className][$idHash], $entity, $idHash);
}

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]),
);
throw EntityIdentityCollisionException::create($this->identityMap[$className][$idHash], $entity, $idHash);
}

return false;
Expand Down
2 changes: 0 additions & 2 deletions tests/Doctrine/Tests/ORM/Functional/BasicFunctionalTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -1079,8 +1079,6 @@ 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: 0 additions & 20 deletions tests/Doctrine/Tests/ORM/UnitOfWorkTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -628,28 +628,8 @@ 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

0 comments on commit 795e773

Please sign in to comment.