Merge branch 'fix/#1521-DDC-2922-defer-checking-non-persisted-entities-through-multiple-differently-mapped-cascading-associations'
Close #1521 Close DDC-2922
This commit is contained in:
Marco Pivetta
commit
8ad3dfd3bd
@ -82,21 +82,42 @@ class ORMInvalidArgumentException extends \InvalidArgumentException
|
||||
}
|
||||
|
||||
/**
|
||||
* @param array $assoc
|
||||
* @param array[][]|object[][] $newEntitiesWithAssociations non-empty an array
|
||||
* of [array $associationMapping, object $entity] pairs
|
||||
*
|
||||
* @return ORMInvalidArgumentException
|
||||
*/
|
||||
static public function newEntitiesFoundThroughRelationships($newEntitiesWithAssociations)
|
||||
{
|
||||
$errorMessages = array_map(
|
||||
function (array $newEntityWithAssociation) : string {
|
||||
[$associationMapping, $entity] = $newEntityWithAssociation;
|
||||
|
||||
return self::newEntityFoundThroughRelationshipMessage($associationMapping, $entity);
|
||||
},
|
||||
$newEntitiesWithAssociations
|
||||
);
|
||||
|
||||
if (1 === count($errorMessages)) {
|
||||
return new self(reset($errorMessages));
|
||||
}
|
||||
|
||||
return new self(
|
||||
'Multiple non-persisted new entities were found through the given association graph:'
|
||||
. "\n\n * "
|
||||
. implode("\n * ", $errorMessages)
|
||||
);
|
||||
}
|
||||
|
||||
/**
|
||||
* @param array $associationMapping
|
||||
* @param object $entry
|
||||
*
|
||||
* @return ORMInvalidArgumentException
|
||||
*/
|
||||
static public function newEntityFoundThroughRelationship(array $assoc, $entry)
|
||||
static public function newEntityFoundThroughRelationship(array $associationMapping, $entry)
|
||||
{
|
||||
return new self("A new entity was found through the relationship '"
|
||||
. $assoc['sourceEntity'] . "#" . $assoc['fieldName'] . "' that was not"
|
||||
. " configured to cascade persist operations for entity: " . self::objToStr($entry) . "."
|
||||
. " To solve this issue: Either explicitly call EntityManager#persist()"
|
||||
. " on this unknown entity or configure cascade persist"
|
||||
. " this association in the mapping for example @ManyToOne(..,cascade={\"persist\"})."
|
||||
. (method_exists($entry, '__toString') ? "": " If you cannot find out which entity causes the problem"
|
||||
. " implement '" . $assoc['targetEntity'] . "#__toString()' to get a clue."));
|
||||
return new self(self::newEntityFoundThroughRelationshipMessage($associationMapping, $entry));
|
||||
}
|
||||
|
||||
/**
|
||||
@ -229,8 +250,27 @@ class ORMInvalidArgumentException extends \InvalidArgumentException
|
||||
*
|
||||
* @return string
|
||||
*/
|
||||
private static function objToStr($obj)
|
||||
private static function objToStr($obj) : string
|
||||
{
|
||||
return method_exists($obj, '__toString') ? (string) $obj : get_class($obj).'@'.spl_object_hash($obj);
|
||||
}
|
||||
|
||||
/**
|
||||
* @param array $associationMapping
|
||||
* @param object $entity
|
||||
*/
|
||||
private static function newEntityFoundThroughRelationshipMessage(array $associationMapping, $entity) : string
|
||||
{
|
||||
return 'A new entity was found through the relationship \''
|
||||
. $associationMapping['sourceEntity'] . '#' . $associationMapping['fieldName'] . '\' that was not'
|
||||
. ' configured to cascade persist operations for entity: ' . self::objToStr($entity) . '.'
|
||||
. ' To solve this issue: Either explicitly call EntityManager#persist()'
|
||||
. ' on this unknown entity or configure cascade persist'
|
||||
. ' this association in the mapping for example @ManyToOne(..,cascade={"persist"}).'
|
||||
. (method_exists($entity, '__toString')
|
||||
? ''
|
||||
: ' If you cannot find out which entity causes the problem implement \''
|
||||
. $associationMapping['targetEntity'] . '#__toString()\' to get a clue.'
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@ -178,6 +178,19 @@ class UnitOfWork implements PropertyChangedListener
|
||||
*/
|
||||
private $entityDeletions = [];
|
||||
|
||||
/**
|
||||
* New entities that were discovered through relationships that were not
|
||||
* marked as cascade-persist. During flush, this array is populated and
|
||||
* then pruned of any entities that were discovered through a valid
|
||||
* cascade-persist path. (Leftovers cause an error.)
|
||||
*
|
||||
* Keys are OIDs, payload is a two-item array describing the association
|
||||
* and the entity.
|
||||
*
|
||||
* @var object[][]|array[][] indexed by respective object spl_object_hash()
|
||||
*/
|
||||
private $nonCascadedNewDetectedEntities = [];
|
||||
|
||||
/**
|
||||
* All pending collection deletions.
|
||||
*
|
||||
@ -346,6 +359,8 @@ class UnitOfWork implements PropertyChangedListener
|
||||
return; // Nothing to do.
|
||||
}
|
||||
|
||||
$this->assertThatThereAreNoUnintentionallyNonPersistedAssociations();
|
||||
|
||||
if ($this->orphanRemovals) {
|
||||
foreach ($this->orphanRemovals as $orphan) {
|
||||
$this->remove($orphan);
|
||||
@ -427,6 +442,7 @@ class UnitOfWork implements PropertyChangedListener
|
||||
$this->entityDeletions =
|
||||
$this->extraUpdates =
|
||||
$this->collectionUpdates =
|
||||
$this->nonCascadedNewDetectedEntities =
|
||||
$this->collectionDeletions =
|
||||
$this->visitedCollections =
|
||||
$this->orphanRemovals = [];
|
||||
@ -861,11 +877,20 @@ class UnitOfWork implements PropertyChangedListener
|
||||
switch ($state) {
|
||||
case self::STATE_NEW:
|
||||
if ( ! $assoc['isCascadePersist']) {
|
||||
throw ORMInvalidArgumentException::newEntityFoundThroughRelationship($assoc, $entry);
|
||||
/*
|
||||
* For now just record the details, because this may
|
||||
* not be an issue if we later discover another pathway
|
||||
* through the object-graph where cascade-persistence
|
||||
* is enabled for this object.
|
||||
*/
|
||||
$this->nonCascadedNewDetectedEntities[\spl_object_hash($entry)] = [$assoc, $entry];
|
||||
|
||||
break;
|
||||
}
|
||||
|
||||
$this->persistNew($targetClass, $entry);
|
||||
$this->computeChangeSet($targetClass, $entry);
|
||||
|
||||
break;
|
||||
|
||||
case self::STATE_REMOVED:
|
||||
@ -2411,6 +2436,7 @@ class UnitOfWork implements PropertyChangedListener
|
||||
$this->entityInsertions =
|
||||
$this->entityUpdates =
|
||||
$this->entityDeletions =
|
||||
$this->nonCascadedNewDetectedEntities =
|
||||
$this->collectionDeletions =
|
||||
$this->collectionUpdates =
|
||||
$this->extraUpdates =
|
||||
@ -3364,6 +3390,22 @@ class UnitOfWork implements PropertyChangedListener
|
||||
return $id1 === $id2 || implode(' ', $id1) === implode(' ', $id2);
|
||||
}
|
||||
|
||||
/**
|
||||
* @throws ORMInvalidArgumentException
|
||||
*/
|
||||
private function assertThatThereAreNoUnintentionallyNonPersistedAssociations() : void
|
||||
{
|
||||
$entitiesNeedingCascadePersist = \array_diff_key($this->nonCascadedNewDetectedEntities, $this->entityInsertions);
|
||||
|
||||
$this->nonCascadedNewDetectedEntities = [];
|
||||
|
||||
if ($entitiesNeedingCascadePersist) {
|
||||
throw ORMInvalidArgumentException::newEntitiesFoundThroughRelationships(
|
||||
\array_values($entitiesNeedingCascadePersist)
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* @param object $entity
|
||||
* @param object $managedCopy
|
||||
|
||||
@ -58,4 +58,95 @@ class ORMInvalidArgumentExceptionTest extends TestCase
|
||||
[new \stdClass(), 'Entity name must be a string, object given'],
|
||||
];
|
||||
}
|
||||
|
||||
/**
|
||||
* @dataProvider newEntitiesFoundThroughRelationshipsErrorMessages
|
||||
*/
|
||||
public function testNewEntitiesFoundThroughRelationships(array $newEntities, string $expectedMessage) : void
|
||||
{
|
||||
$exception = ORMInvalidArgumentException::newEntitiesFoundThroughRelationships($newEntities);
|
||||
|
||||
self::assertInstanceOf(ORMInvalidArgumentException::class, $exception);
|
||||
self::assertSame($expectedMessage, $exception->getMessage());
|
||||
}
|
||||
|
||||
public function newEntitiesFoundThroughRelationshipsErrorMessages() : array
|
||||
{
|
||||
$stringEntity3 = uniqid('entity3', true);
|
||||
$entity1 = new \stdClass();
|
||||
$entity2 = new \stdClass();
|
||||
$entity3 = $this->getMockBuilder(\stdClass::class)->setMethods(['__toString'])->getMock();
|
||||
$association1 = [
|
||||
'sourceEntity' => 'foo1',
|
||||
'fieldName' => 'bar1',
|
||||
'targetEntity' => 'baz1',
|
||||
];
|
||||
$association2 = [
|
||||
'sourceEntity' => 'foo2',
|
||||
'fieldName' => 'bar2',
|
||||
'targetEntity' => 'baz2',
|
||||
];
|
||||
$association3 = [
|
||||
'sourceEntity' => 'foo3',
|
||||
'fieldName' => 'bar3',
|
||||
'targetEntity' => 'baz3',
|
||||
];
|
||||
|
||||
$entity3->expects(self::any())->method('__toString')->willReturn($stringEntity3);
|
||||
|
||||
return [
|
||||
'one entity found' => [
|
||||
[
|
||||
[
|
||||
$association1,
|
||||
$entity1,
|
||||
],
|
||||
],
|
||||
'A new entity was found through the relationship \'foo1#bar1\' that was not configured to cascade '
|
||||
. 'persist operations for entity: stdClass@' . spl_object_hash($entity1)
|
||||
. '. To solve this issue: Either explicitly call EntityManager#persist() on this unknown entity '
|
||||
. 'or configure cascade persist this association in the mapping for example '
|
||||
. '@ManyToOne(..,cascade={"persist"}). If you cannot find out which entity causes the problem '
|
||||
. 'implement \'baz1#__toString()\' to get a clue.',
|
||||
],
|
||||
'two entities found' => [
|
||||
[
|
||||
[
|
||||
$association1,
|
||||
$entity1,
|
||||
],
|
||||
[
|
||||
$association2,
|
||||
$entity2,
|
||||
],
|
||||
],
|
||||
'Multiple non-persisted new entities were found through the given association graph:' . "\n\n"
|
||||
. ' * A new entity was found through the relationship \'foo1#bar1\' that was not configured to '
|
||||
. 'cascade persist operations for entity: stdClass@' . spl_object_hash($entity1) . '. '
|
||||
. 'To solve this issue: Either explicitly call EntityManager#persist() on this unknown entity '
|
||||
. 'or configure cascade persist this association in the mapping for example '
|
||||
. '@ManyToOne(..,cascade={"persist"}). If you cannot find out which entity causes the problem '
|
||||
. 'implement \'baz1#__toString()\' to get a clue.' . "\n"
|
||||
. ' * A new entity was found through the relationship \'foo2#bar2\' that was not configured to '
|
||||
. 'cascade persist operations for entity: stdClass@' . spl_object_hash($entity2) . '. To solve '
|
||||
. 'this issue: Either explicitly call EntityManager#persist() on this unknown entity or '
|
||||
. 'configure cascade persist this association in the mapping for example '
|
||||
. '@ManyToOne(..,cascade={"persist"}). If you cannot find out which entity causes the problem '
|
||||
. 'implement \'baz2#__toString()\' to get a clue.'
|
||||
],
|
||||
'two entities found, one is stringable' => [
|
||||
[
|
||||
[
|
||||
$association3,
|
||||
$entity3,
|
||||
],
|
||||
],
|
||||
'A new entity was found through the relationship \'foo3#bar3\' that was not configured to cascade '
|
||||
. 'persist operations for entity: ' . $stringEntity3
|
||||
. '. To solve this issue: Either explicitly call EntityManager#persist() on this unknown entity '
|
||||
. 'or configure cascade persist this association in the mapping for example '
|
||||
. '@ManyToOne(..,cascade={"persist"}).',
|
||||
],
|
||||
];
|
||||
}
|
||||
}
|
||||
|
||||
@ -625,6 +625,135 @@ class UnitOfWorkTest extends OrmTestCase
|
||||
self::assertSame($merged, $persistedEntity);
|
||||
self::assertSame($persistedEntity->generatedField, $mergedEntity->generatedField);
|
||||
}
|
||||
|
||||
/**
|
||||
* Unlike next test, this one demonstrates that the problem does
|
||||
* not necessarily reproduce if all the pieces are being flushed together.
|
||||
*
|
||||
* @group DDC-2922
|
||||
* @group #1521
|
||||
*/
|
||||
public function testNewAssociatedEntityPersistenceOfNewEntitiesThroughCascadedAssociationsFirst()
|
||||
{
|
||||
$persister1 = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata(CascadePersistedEntity::class));
|
||||
$persister2 = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata(EntityWithCascadingAssociation::class));
|
||||
$persister3 = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata(EntityWithNonCascadingAssociation::class));
|
||||
$this->_unitOfWork->setEntityPersister(CascadePersistedEntity::class, $persister1);
|
||||
$this->_unitOfWork->setEntityPersister(EntityWithCascadingAssociation::class, $persister2);
|
||||
$this->_unitOfWork->setEntityPersister(EntityWithNonCascadingAssociation::class, $persister3);
|
||||
|
||||
$cascadePersisted = new CascadePersistedEntity();
|
||||
$cascading = new EntityWithCascadingAssociation();
|
||||
$nonCascading = new EntityWithNonCascadingAssociation();
|
||||
|
||||
// First we persist and flush a EntityWithCascadingAssociation with
|
||||
// the cascading association not set. Having the "cascading path" involve
|
||||
// a non-new object is important to show that the ORM should be considering
|
||||
// cascades across entity changesets in subsequent flushes.
|
||||
$cascading->cascaded = $cascadePersisted;
|
||||
$nonCascading->cascaded = $cascadePersisted;
|
||||
|
||||
$this->_unitOfWork->persist($cascading);
|
||||
$this->_unitOfWork->persist($nonCascading);
|
||||
|
||||
$this->_unitOfWork->commit();
|
||||
|
||||
$this->assertCount(1, $persister1->getInserts());
|
||||
$this->assertCount(1, $persister2->getInserts());
|
||||
$this->assertCount(1, $persister3->getInserts());
|
||||
}
|
||||
|
||||
/**
|
||||
* This test exhibits the bug describe in the ticket, where an object that
|
||||
* ought to be reachable causes errors.
|
||||
*
|
||||
* @group DDC-2922
|
||||
* @group #1521
|
||||
*/
|
||||
public function testNewAssociatedEntityPersistenceOfNewEntitiesThroughNonCascadedAssociationsFirst()
|
||||
{
|
||||
$persister1 = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata(CascadePersistedEntity::class));
|
||||
$persister2 = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata(EntityWithCascadingAssociation::class));
|
||||
$persister3 = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata(EntityWithNonCascadingAssociation::class));
|
||||
$this->_unitOfWork->setEntityPersister(CascadePersistedEntity::class, $persister1);
|
||||
$this->_unitOfWork->setEntityPersister(EntityWithCascadingAssociation::class, $persister2);
|
||||
$this->_unitOfWork->setEntityPersister(EntityWithNonCascadingAssociation::class, $persister3);
|
||||
|
||||
$cascadePersisted = new CascadePersistedEntity();
|
||||
$cascading = new EntityWithCascadingAssociation();
|
||||
$nonCascading = new EntityWithNonCascadingAssociation();
|
||||
|
||||
// First we persist and flush a EntityWithCascadingAssociation with
|
||||
// the cascading association not set. Having the "cascading path" involve
|
||||
// a non-new object is important to show that the ORM should be considering
|
||||
// cascades across entity changesets in subsequent flushes.
|
||||
$cascading->cascaded = null;
|
||||
|
||||
$this->_unitOfWork->persist($cascading);
|
||||
$this->_unitOfWork->commit();
|
||||
|
||||
self::assertCount(0, $persister1->getInserts());
|
||||
self::assertCount(1, $persister2->getInserts());
|
||||
self::assertCount(0, $persister3->getInserts());
|
||||
|
||||
// Note that we have NOT directly persisted the CascadePersistedEntity,
|
||||
// and EntityWithNonCascadingAssociation does NOT have a configured
|
||||
// cascade-persist.
|
||||
$nonCascading->nonCascaded = $cascadePersisted;
|
||||
|
||||
// However, EntityWithCascadingAssociation *does* have a cascade-persist
|
||||
// association, which ought to allow us to save the CascadePersistedEntity
|
||||
// anyway through that connection.
|
||||
$cascading->cascaded = $cascadePersisted;
|
||||
|
||||
$this->_unitOfWork->persist($nonCascading);
|
||||
$this->_unitOfWork->commit();
|
||||
|
||||
self::assertCount(1, $persister1->getInserts());
|
||||
self::assertCount(1, $persister2->getInserts());
|
||||
self::assertCount(1, $persister3->getInserts());
|
||||
}
|
||||
|
||||
|
||||
/**
|
||||
* This test exhibits the bug describe in the ticket, where an object that
|
||||
* ought to be reachable causes errors.
|
||||
*
|
||||
* @group DDC-2922
|
||||
* @group #1521
|
||||
*/
|
||||
public function testPreviousDetectedIllegalNewNonCascadedEntitiesAreCleanedUpOnSubsequentCommits()
|
||||
{
|
||||
$persister1 = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata(CascadePersistedEntity::class));
|
||||
$persister2 = new EntityPersisterMock($this->_emMock, $this->_emMock->getClassMetadata(EntityWithNonCascadingAssociation::class));
|
||||
$this->_unitOfWork->setEntityPersister(CascadePersistedEntity::class, $persister1);
|
||||
$this->_unitOfWork->setEntityPersister(EntityWithNonCascadingAssociation::class, $persister2);
|
||||
|
||||
$cascadePersisted = new CascadePersistedEntity();
|
||||
$nonCascading = new EntityWithNonCascadingAssociation();
|
||||
|
||||
// We explicitly cause the ORM to detect a non-persisted new entity in the association graph:
|
||||
$nonCascading->nonCascaded = $cascadePersisted;
|
||||
|
||||
$this->_unitOfWork->persist($nonCascading);
|
||||
|
||||
try {
|
||||
$this->_unitOfWork->commit();
|
||||
|
||||
self::fail('An exception was supposed to be raised');
|
||||
} catch (ORMInvalidArgumentException $ignored) {
|
||||
self::assertEmpty($persister1->getInserts());
|
||||
self::assertEmpty($persister2->getInserts());
|
||||
}
|
||||
|
||||
$this->_unitOfWork->clear();
|
||||
$this->_unitOfWork->persist(new CascadePersistedEntity());
|
||||
$this->_unitOfWork->commit();
|
||||
|
||||
// Persistence operations should just recover normally:
|
||||
self::assertCount(1, $persister1->getInserts());
|
||||
self::assertCount(0, $persister2->getInserts());
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
@ -789,3 +918,45 @@ class EntityWithRandomlyGeneratedField
|
||||
$this->generatedField = mt_rand(0, 100000);
|
||||
}
|
||||
}
|
||||
|
||||
/** @Entity */
|
||||
class CascadePersistedEntity
|
||||
{
|
||||
/** @Id @Column(type="string") @GeneratedValue(strategy="NONE") */
|
||||
private $id;
|
||||
|
||||
public function __construct()
|
||||
{
|
||||
$this->id = uniqid(self::class, true);
|
||||
}
|
||||
}
|
||||
|
||||
/** @Entity */
|
||||
class EntityWithCascadingAssociation
|
||||
{
|
||||
/** @Id @Column(type="string") @GeneratedValue(strategy="NONE") */
|
||||
private $id;
|
||||
|
||||
/** @ManyToOne(targetEntity=CascadePersistedEntity::class, cascade={"persist"}) */
|
||||
public $cascaded;
|
||||
|
||||
public function __construct()
|
||||
{
|
||||
$this->id = uniqid(self::class, true);
|
||||
}
|
||||
}
|
||||
|
||||
/** @Entity */
|
||||
class EntityWithNonCascadingAssociation
|
||||
{
|
||||
/** @Id @Column(type="string") @GeneratedValue(strategy="NONE") */
|
||||
private $id;
|
||||
|
||||
/** @ManyToOne(targetEntity=CascadePersistedEntity::class) */
|
||||
public $nonCascaded;
|
||||
|
||||
public function __construct()
|
||||
{
|
||||
$this->id = uniqid(self::class, true);
|
||||
}
|
||||
}
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user