diff --git a/lib/Doctrine/ORM/ORMInvalidArgumentException.php b/lib/Doctrine/ORM/ORMInvalidArgumentException.php index 6aa270c78..796d2c5a5 100644 --- a/lib/Doctrine/ORM/ORMInvalidArgumentException.php +++ b/lib/Doctrine/ORM/ORMInvalidArgumentException.php @@ -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.' + ); + } } diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 4f8ce32db..0edd756f0 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -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 diff --git a/tests/Doctrine/Tests/ORM/ORMInvalidArgumentExceptionTest.php b/tests/Doctrine/Tests/ORM/ORMInvalidArgumentExceptionTest.php index b4d269078..dea8d4f77 100644 --- a/tests/Doctrine/Tests/ORM/ORMInvalidArgumentExceptionTest.php +++ b/tests/Doctrine/Tests/ORM/ORMInvalidArgumentExceptionTest.php @@ -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"}).', + ], + ]; + } } diff --git a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php index bfad30be8..12eadbdaa 100644 --- a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php +++ b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php @@ -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); + } +}