From c4465abaa0fc968d2668a08069d041c30b2811f9 Mon Sep 17 00:00:00 2001 From: Darien Hager Date: Tue, 29 Sep 2015 16:36:48 -0700 Subject: [PATCH 01/16] Initial failing test-case to demonstrate cascade-persist problem. --- tests/Doctrine/Tests/Models/CMS/CmsEmail.php | 2 +- .../ORM/Functional/BasicFunctionalTest.php | 45 +++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/tests/Doctrine/Tests/Models/CMS/CmsEmail.php b/tests/Doctrine/Tests/Models/CMS/CmsEmail.php index c79c30036..0faf4b876 100644 --- a/tests/Doctrine/Tests/Models/CMS/CmsEmail.php +++ b/tests/Doctrine/Tests/Models/CMS/CmsEmail.php @@ -22,7 +22,7 @@ class CmsEmail public $email; /** - * @OneToOne(targetEntity="CmsUser", mappedBy="email") + * @OneToOne(targetEntity="CmsUser", mappedBy="email", cascade={"persist"}) */ public $user; diff --git a/tests/Doctrine/Tests/ORM/Functional/BasicFunctionalTest.php b/tests/Doctrine/Tests/ORM/Functional/BasicFunctionalTest.php index 7bdb84653..a61595af6 100644 --- a/tests/Doctrine/Tests/ORM/Functional/BasicFunctionalTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/BasicFunctionalTest.php @@ -9,6 +9,7 @@ use Doctrine\ORM\ORMInvalidArgumentException; use Doctrine\ORM\PersistentCollection; use Doctrine\ORM\Proxy\Proxy; use Doctrine\ORM\Query; +use Doctrine\Tests\Models\CMS\CmsEmail; use Doctrine\ORM\UnitOfWork; use Doctrine\Tests\Models\CMS\CmsAddress; use Doctrine\Tests\Models\CMS\CmsArticle; @@ -801,6 +802,50 @@ class BasicFunctionalTest extends OrmFunctionalTestCase $this->_em->flush(); } + /** + * @group DDC-2922 + */ + public function testNewAssociatedEntityWorksWithJustOnePath() + { + + /** + * First we persist and flush an e-mail with no user. This seems + * Save an un-owned email with no user. This seems to + * matter for reproducing the bug + */ + $mail = new CmsEmail(); + $mail->email = "nobody@example.com"; + $mail->user = null; + + $this->_em->persist($mail); + $this->_em->flush(); + + $user = new CmsUser(); + $user->username = "beberlei"; + $user->name = "Benjamin E."; + $user->status = 'active'; + + $mail->user = $user; + + /** + * Note that we have NOT directly persisted the CmsUser, and CmsAddress + * does NOT have cascade-persist. + * + * However, CmsEmail *does* have a cascade-persist, which ought to + * allow us to save the CmsUser anyway through that connection. + */ + $address = new CmsAddress(); + $address->city = "Bonn"; + $address->zip = "12354"; + $address->country = "Germany"; + $address->street = "somestreet"; + $address->user = $user; + + $this->_em->persist($address); + $this->_em->flush(); + + } + public function testOneToOneOrphanRemoval() { $user = new CmsUser(); From 6f8a80be79f8f026f381c33b6fd2db2cd9467962 Mon Sep 17 00:00:00 2001 From: Darien Hager Date: Tue, 29 Sep 2015 16:45:01 -0700 Subject: [PATCH 02/16] Move failing unit test into ticket-specific case --- .../ORM/Functional/BasicFunctionalTest.php | 45 ------------ .../ORM/Functional/Ticket/DDC2922Test.php | 72 +++++++++++++++++++ 2 files changed, 72 insertions(+), 45 deletions(-) create mode 100644 tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2922Test.php diff --git a/tests/Doctrine/Tests/ORM/Functional/BasicFunctionalTest.php b/tests/Doctrine/Tests/ORM/Functional/BasicFunctionalTest.php index a61595af6..7bdb84653 100644 --- a/tests/Doctrine/Tests/ORM/Functional/BasicFunctionalTest.php +++ b/tests/Doctrine/Tests/ORM/Functional/BasicFunctionalTest.php @@ -9,7 +9,6 @@ use Doctrine\ORM\ORMInvalidArgumentException; use Doctrine\ORM\PersistentCollection; use Doctrine\ORM\Proxy\Proxy; use Doctrine\ORM\Query; -use Doctrine\Tests\Models\CMS\CmsEmail; use Doctrine\ORM\UnitOfWork; use Doctrine\Tests\Models\CMS\CmsAddress; use Doctrine\Tests\Models\CMS\CmsArticle; @@ -802,50 +801,6 @@ class BasicFunctionalTest extends OrmFunctionalTestCase $this->_em->flush(); } - /** - * @group DDC-2922 - */ - public function testNewAssociatedEntityWorksWithJustOnePath() - { - - /** - * First we persist and flush an e-mail with no user. This seems - * Save an un-owned email with no user. This seems to - * matter for reproducing the bug - */ - $mail = new CmsEmail(); - $mail->email = "nobody@example.com"; - $mail->user = null; - - $this->_em->persist($mail); - $this->_em->flush(); - - $user = new CmsUser(); - $user->username = "beberlei"; - $user->name = "Benjamin E."; - $user->status = 'active'; - - $mail->user = $user; - - /** - * Note that we have NOT directly persisted the CmsUser, and CmsAddress - * does NOT have cascade-persist. - * - * However, CmsEmail *does* have a cascade-persist, which ought to - * allow us to save the CmsUser anyway through that connection. - */ - $address = new CmsAddress(); - $address->city = "Bonn"; - $address->zip = "12354"; - $address->country = "Germany"; - $address->street = "somestreet"; - $address->user = $user; - - $this->_em->persist($address); - $this->_em->flush(); - - } - public function testOneToOneOrphanRemoval() { $user = new CmsUser(); diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2922Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2922Test.php new file mode 100644 index 000000000..265f61a86 --- /dev/null +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2922Test.php @@ -0,0 +1,72 @@ +useModelSet('cms'); + parent::setUp(); + } + + /** + * @group DDC-2922 + */ + public function testNewAssociatedEntityWorksWithJustOnePath() + { + + /** + * First we persist and flush an e-mail with no user. This seems + * Save an un-owned email with no user. This seems to + * matter for reproducing the bug + */ + $mail = new CmsEmail(); + $mail->email = "nobody@example.com"; + $mail->user = null; + + $this->_em->persist($mail); + $this->_em->flush(); + + $user = new CmsUser(); + $user->username = "beberlei"; + $user->name = "Benjamin E."; + $user->status = 'active'; + + $mail->user = $user; + + /** + * Note that we have NOT directly persisted the CmsUser, and CmsAddress + * does NOT have cascade-persist. + * + * However, CmsEmail *does* have a cascade-persist, which ought to + * allow us to save the CmsUser anyway through that connection. + */ + $address = new CmsAddress(); + $address->city = "Bonn"; + $address->zip = "12354"; + $address->country = "Germany"; + $address->street = "somestreet"; + $address->user = $user; + + $this->_em->persist($address); + try{ + $this->_em->flush(); + }catch(ORMInvalidArgumentException $e){ + if(strpos($e->getMessage(),'not configured to cascade persist operations') !== FALSE) { + $this->fail($e); + } + throw $e; + } + + + } +} \ No newline at end of file From 997000352af4587fd8f98a901db6563322f1a5bd Mon Sep 17 00:00:00 2001 From: Darien Hager Date: Tue, 29 Sep 2015 16:59:08 -0700 Subject: [PATCH 03/16] Add another test to demonstrate the bug doesn't appear if everything is flushed at once. --- .../ORM/Functional/Ticket/DDC2922Test.php | 71 ++++++++++++++----- 1 file changed, 52 insertions(+), 19 deletions(-) diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2922Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2922Test.php index 265f61a86..ce7788740 100644 --- a/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2922Test.php +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2922Test.php @@ -19,29 +19,64 @@ class DDC2922Test extends \Doctrine\Tests\OrmFunctionalTestCase } /** + * Unlike next test, this one demonstrates that the problem does + * not necessarily reproduce if all the pieces are being flushed together. + * * @group DDC-2922 */ - public function testNewAssociatedEntityWorksWithJustOnePath() + public function testNewAssociatedEntityWorksWithJustOnePathIfAllPartsNew() { - /** - * First we persist and flush an e-mail with no user. This seems - * Save an un-owned email with no user. This seems to - * matter for reproducing the bug - */ - $mail = new CmsEmail(); - $mail->email = "nobody@example.com"; - $mail->user = null; - - $this->_em->persist($mail); - $this->_em->flush(); - $user = new CmsUser(); $user->username = "beberlei"; $user->name = "Benjamin E."; $user->status = 'active'; - $mail->user = $user; + $email = new CmsEmail(); + $email->email = "nobody@example.com"; + $email->user = $user; + + $address = new CmsAddress(); + $address->city = "Bonn"; + $address->zip = "12354"; + $address->country = "Germany"; + $address->street = "somestreet"; + $address->user = $user; + + $this->_em->persist($email); + $this->_em->persist($address); + + $this->_em->flush(); + + } + + /** + * This test exhibits the bug describe in the ticket, where an object that + * ought to be reachable causes errors. + * + * @group DDC-2922 + */ + public function testNewAssociatedEntityWorksWithJustOnePath() + { + + /** + * First we persist and flush an e-mail with no user. Having the + * "cascading path" involve a non-new object seems to be important to + * reproducing the bug. + */ + $email = new CmsEmail(); + $email->email = "nobody@example.com"; + $email->user = null; + + $this->_em->persist($email); + $this->_em->flush(); // Flush before introducing CmsUser + + $user = new CmsUser(); + $user->username = "beberlei"; + $user->name = "Benjamin E."; + $user->status = 'active'; + + $email->user = $user; /** * Note that we have NOT directly persisted the CmsUser, and CmsAddress @@ -58,15 +93,13 @@ class DDC2922Test extends \Doctrine\Tests\OrmFunctionalTestCase $address->user = $user; $this->_em->persist($address); - try{ + try { $this->_em->flush(); - }catch(ORMInvalidArgumentException $e){ - if(strpos($e->getMessage(),'not configured to cascade persist operations') !== FALSE) { + } catch (ORMInvalidArgumentException $e) { + if (strpos($e->getMessage(), 'not configured to cascade persist operations') !== FALSE) { $this->fail($e); } throw $e; } - - } } \ No newline at end of file From 92dc39bfb9bf739d5a74ba7b5c79a4f31dd9d1c6 Mon Sep 17 00:00:00 2001 From: Darien Hager Date: Tue, 29 Sep 2015 18:35:57 -0700 Subject: [PATCH 04/16] Add extra assertions to test to ensure that flush actually succeeded in saving all items --- .../Tests/ORM/Functional/Ticket/DDC2922Test.php | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2922Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2922Test.php index ce7788740..188d68fee 100644 --- a/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2922Test.php +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2922Test.php @@ -48,6 +48,11 @@ class DDC2922Test extends \Doctrine\Tests\OrmFunctionalTestCase $this->_em->flush(); + // Verify the flush succeeded + $this->assertEquals($email, $this->_em->find(get_class($email),$email->id)); + $this->assertEquals($address, $this->_em->find(get_class($address),$address->id)); + $this->assertEquals($user, $this->_em->find(get_class($user),$user->id)); + } /** @@ -101,5 +106,11 @@ class DDC2922Test extends \Doctrine\Tests\OrmFunctionalTestCase } throw $e; } + + // Verify the flushes succeeded + $this->assertEquals($email, $this->_em->find(get_class($email),$email->id)); + $this->assertEquals($address, $this->_em->find(get_class($address),$address->id)); + $this->assertEquals($user, $this->_em->find(get_class($user),$user->id)); + } } \ No newline at end of file From 17b996da8cf7bfd75e956e44a5bc80eb9faa32b8 Mon Sep 17 00:00:00 2001 From: Darien Hager Date: Tue, 29 Sep 2015 18:44:47 -0700 Subject: [PATCH 05/16] Speculative fix: Defer any errors for missing cascade-persist until object graph has been better-explored --- lib/Doctrine/ORM/UnitOfWork.php | 39 +++++++++++++++++++++++++++++---- 1 file changed, 35 insertions(+), 4 deletions(-) diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 4f8ce32db..7d4d8d80a 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 array + */ + private $newEntitiesWithoutCascade = array(); + /** * All pending collection deletions. * @@ -427,6 +440,7 @@ class UnitOfWork implements PropertyChangedListener $this->entityDeletions = $this->extraUpdates = $this->collectionUpdates = + $this->newEntitiesWithoutCascade = $this->collectionDeletions = $this->visitedCollections = $this->orphanRemovals = []; @@ -815,6 +829,16 @@ class UnitOfWork implements PropertyChangedListener } } } + + /** + * Filter out any entities that we (successfully) managed to schedule + * for insertion. + */ + $entitiesNeedingCascadePersist = array_diff_key($this->newEntitiesWithoutCascade, $this->entityInsertions); + if(count($entitiesNeedingCascadePersist) > 0){ + list($assoc,$entity) = array_values($entitiesNeedingCascadePersist)[0]; + throw ORMInvalidArgumentException::newEntityFoundThroughRelationship($assoc, $entity); + } } /** @@ -861,11 +885,17 @@ 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->newEntitiesWithoutCascade[spl_object_hash($entry)] = array($assoc,$entry); + }else { + $this->persistNew($targetClass, $entry); + $this->computeChangeSet($targetClass, $entry); } - - $this->persistNew($targetClass, $entry); - $this->computeChangeSet($targetClass, $entry); break; case self::STATE_REMOVED: @@ -2411,6 +2441,7 @@ class UnitOfWork implements PropertyChangedListener $this->entityInsertions = $this->entityUpdates = $this->entityDeletions = + $this->newEntitiesWithoutCascade = $this->collectionDeletions = $this->collectionUpdates = $this->extraUpdates = From b456cffa2d2340fb078ba7aebba92985578db333 Mon Sep 17 00:00:00 2001 From: Darien Hager Date: Wed, 30 Sep 2015 21:04:22 -0700 Subject: [PATCH 06/16] Move final cascade-persist-checking so that it covers not just normal flushes, but also flushes where specific entities are singled out. --- lib/Doctrine/ORM/UnitOfWork.php | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index 7d4d8d80a..b3f3fb23d 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -347,6 +347,8 @@ class UnitOfWork implements PropertyChangedListener } } + $this->assertNoCascadingGaps(); + if ( ! ($this->entityInsertions || $this->entityDeletions || $this->entityUpdates || @@ -829,16 +831,6 @@ class UnitOfWork implements PropertyChangedListener } } } - - /** - * Filter out any entities that we (successfully) managed to schedule - * for insertion. - */ - $entitiesNeedingCascadePersist = array_diff_key($this->newEntitiesWithoutCascade, $this->entityInsertions); - if(count($entitiesNeedingCascadePersist) > 0){ - list($assoc,$entity) = array_values($entitiesNeedingCascadePersist)[0]; - throw ORMInvalidArgumentException::newEntityFoundThroughRelationship($assoc, $entity); - } } /** @@ -3395,6 +3387,26 @@ class UnitOfWork implements PropertyChangedListener return $id1 === $id2 || implode(' ', $id1) === implode(' ', $id2); } + /** + * Checks that there are no new entities found through non-cascade-persist + * paths which are not also scheduled for insertion through valid paths. + * + * @return void + * @throws ORMInvalidArgumentException + */ + private function assertNoCascadingGaps() + { + /** + * Filter out any entities that we (successfully) managed to schedule + * for insertion. + */ + $entitiesNeedingCascadePersist = array_diff_key($this->newEntitiesWithoutCascade, $this->entityInsertions); + if(count($entitiesNeedingCascadePersist) > 0){ + list($assoc,$entity) = array_values($entitiesNeedingCascadePersist)[0]; + throw ORMInvalidArgumentException::newEntityFoundThroughRelationship($assoc, $entity); + } + } + /** * @param object $entity * @param object $managedCopy From e21b29c26410837b516af43d2bc751e1dbba5e4c Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Mon, 21 Aug 2017 19:44:38 +0200 Subject: [PATCH 07/16] #1521 DDC-2922 simplified test case to a minimum reproducible unit --- tests/Doctrine/Tests/Models/CMS/CmsEmail.php | 2 +- .../ORM/Functional/Ticket/DDC2922Test.php | 129 +++++++++++++++++- 2 files changed, 125 insertions(+), 6 deletions(-) diff --git a/tests/Doctrine/Tests/Models/CMS/CmsEmail.php b/tests/Doctrine/Tests/Models/CMS/CmsEmail.php index 0faf4b876..c79c30036 100644 --- a/tests/Doctrine/Tests/Models/CMS/CmsEmail.php +++ b/tests/Doctrine/Tests/Models/CMS/CmsEmail.php @@ -22,7 +22,7 @@ class CmsEmail public $email; /** - * @OneToOne(targetEntity="CmsUser", mappedBy="email", cascade={"persist"}) + * @OneToOne(targetEntity="CmsUser", mappedBy="email") */ public $user; diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2922Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2922Test.php index 188d68fee..6598be42e 100644 --- a/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2922Test.php +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2922Test.php @@ -1,21 +1,34 @@ useModelSet('cms'); parent::setUp(); + + try { + $this->_schemaTool->createSchema(array_map( + function (string $className) : ClassMetadata { + return $this->_em->getClassMetadata($className); + }, + [ + DDC2922CascadePersistedEntity::class, + DDC2922EntityWithCascadingAssociation::class, + DDC2922EntityWithNonCascadingAssociation::class, + ] + )); + } catch (ToolsException $ignored) {} } /** @@ -63,7 +76,7 @@ class DDC2922Test extends \Doctrine\Tests\OrmFunctionalTestCase */ public function testNewAssociatedEntityWorksWithJustOnePath() { - + self::markTestSkipped(); /** * First we persist and flush an e-mail with no user. Having the * "cascading path" involve a non-new object seems to be important to @@ -113,4 +126,110 @@ class DDC2922Test extends \Doctrine\Tests\OrmFunctionalTestCase $this->assertEquals($user, $this->_em->find(get_class($user),$user->id)); } -} \ No newline at end of file + + /** + * Unlike next test, this one demonstrates that the problem does + * not necessarily reproduce if all the pieces are being flushed together. + * + * @group DDC-2922 + */ + public function testNewAssociatedEntityWorksWithJustOnePath__() + { + $cascadePersisted = new DDC2922CascadePersistedEntity(); + $cascading = new DDC2922EntityWithCascadingAssociation(); + $nonCascading = new DDC2922EntityWithNonCascadingAssociation(); + + // First we persist and flush a DDC2922EntityWithCascadingAssociation 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->_em->persist($cascading); + $this->_em->persist($nonCascading); + $this->_em->flush(); + + // @TODO assert persistence on both associations + } + + + /** + * This test exhibits the bug describe in the ticket, where an object that + * ought to be reachable causes errors. + * + * @group DDC-2922 + */ + public function testNewAssociatedEntityWorksWithJustOnePath_() + { + $cascadePersisted = new DDC2922CascadePersistedEntity(); + $cascading = new DDC2922EntityWithCascadingAssociation(); + $nonCascading = new DDC2922EntityWithNonCascadingAssociation(); + + // First we persist and flush a DDC2922EntityWithCascadingAssociation 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->_em->persist($cascading); + $this->_em->flush(); + + // Note that we have NOT directly persisted the DDC2922CascadePersistedEntity, + // and DDC2922EntityWithNonCascadingAssociation does NOT have a configured + // cascade-persist. + $nonCascading->nonCascaded = $cascadePersisted; + + // However, DDC2922EntityWithCascadingAssociation *does* have a cascade-persist + // association, which ought to allow us to save the DDC2922CascadePersistedEntity + // anyway through that connection. + $cascading->cascaded = $cascadePersisted; + + $this->_em->persist($nonCascading); + $this->_em->flush(); + + // @TODO assert persistence on both associations + } +} + +/** @Entity */ +class DDC2922CascadePersistedEntity +{ + /** @Id @Column(type="string") @GeneratedValue(strategy="NONE") */ + private $id; + + public function __construct() + { + $this->id = uniqid(self::class, true); + } +} + +/** @Entity */ +class DDC2922EntityWithCascadingAssociation +{ + /** @Id @Column(type="string") @GeneratedValue(strategy="NONE") */ + private $id; + + /** @ManyToOne(targetEntity=DDC2922CascadePersistedEntity::class, cascade={"persist"}) */ + public $cascaded; + + public function __construct() + { + $this->id = uniqid(self::class, true); + } +} + +/** @Entity */ +class DDC2922EntityWithNonCascadingAssociation +{ + /** @Id @Column(type="string") @GeneratedValue(strategy="NONE") */ + private $id; + + /** @ManyToOne(targetEntity=DDC2922CascadePersistedEntity::class) */ + public $nonCascaded; + + public function __construct() + { + $this->id = uniqid(self::class, true); + } +} From a3208f8d086d37fc9c6f2de8b10a8b4bc9c1a929 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Mon, 21 Aug 2017 19:47:16 +0200 Subject: [PATCH 08/16] #1521 DDC-2922 removed unrelated model usage from test - using minimal models only --- .../ORM/Functional/Ticket/DDC2922Test.php | 107 +----------------- 1 file changed, 4 insertions(+), 103 deletions(-) diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2922Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2922Test.php index 6598be42e..b1aa6c0da 100644 --- a/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2922Test.php +++ b/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2922Test.php @@ -4,17 +4,13 @@ declare(strict_types=1); namespace Doctrine\Tests\ORM\Functional\Ticket; use Doctrine\ORM\Mapping\ClassMetadata; -use Doctrine\ORM\ORMInvalidArgumentException; use Doctrine\ORM\Tools\ToolsException; -use Doctrine\Tests\Models\CMS\CmsAddress; -use Doctrine\Tests\Models\CMS\CmsEmail; -use Doctrine\Tests\Models\CMS\CmsUser; +use Doctrine\Tests\OrmFunctionalTestCase; -class DDC2922Test extends \Doctrine\Tests\OrmFunctionalTestCase +class DDC2922Test extends OrmFunctionalTestCase { - protected function setUp() + protected function setUp() : void { - $this->useModelSet('cms'); parent::setUp(); try { @@ -28,103 +24,8 @@ class DDC2922Test extends \Doctrine\Tests\OrmFunctionalTestCase DDC2922EntityWithNonCascadingAssociation::class, ] )); - } catch (ToolsException $ignored) {} - } - - /** - * Unlike next test, this one demonstrates that the problem does - * not necessarily reproduce if all the pieces are being flushed together. - * - * @group DDC-2922 - */ - public function testNewAssociatedEntityWorksWithJustOnePathIfAllPartsNew() - { - - $user = new CmsUser(); - $user->username = "beberlei"; - $user->name = "Benjamin E."; - $user->status = 'active'; - - $email = new CmsEmail(); - $email->email = "nobody@example.com"; - $email->user = $user; - - $address = new CmsAddress(); - $address->city = "Bonn"; - $address->zip = "12354"; - $address->country = "Germany"; - $address->street = "somestreet"; - $address->user = $user; - - $this->_em->persist($email); - $this->_em->persist($address); - - $this->_em->flush(); - - // Verify the flush succeeded - $this->assertEquals($email, $this->_em->find(get_class($email),$email->id)); - $this->assertEquals($address, $this->_em->find(get_class($address),$address->id)); - $this->assertEquals($user, $this->_em->find(get_class($user),$user->id)); - - } - - /** - * This test exhibits the bug describe in the ticket, where an object that - * ought to be reachable causes errors. - * - * @group DDC-2922 - */ - public function testNewAssociatedEntityWorksWithJustOnePath() - { - self::markTestSkipped(); - /** - * First we persist and flush an e-mail with no user. Having the - * "cascading path" involve a non-new object seems to be important to - * reproducing the bug. - */ - $email = new CmsEmail(); - $email->email = "nobody@example.com"; - $email->user = null; - - $this->_em->persist($email); - $this->_em->flush(); // Flush before introducing CmsUser - - $user = new CmsUser(); - $user->username = "beberlei"; - $user->name = "Benjamin E."; - $user->status = 'active'; - - $email->user = $user; - - /** - * Note that we have NOT directly persisted the CmsUser, and CmsAddress - * does NOT have cascade-persist. - * - * However, CmsEmail *does* have a cascade-persist, which ought to - * allow us to save the CmsUser anyway through that connection. - */ - $address = new CmsAddress(); - $address->city = "Bonn"; - $address->zip = "12354"; - $address->country = "Germany"; - $address->street = "somestreet"; - $address->user = $user; - - $this->_em->persist($address); - try { - $this->_em->flush(); - } catch (ORMInvalidArgumentException $e) { - if (strpos($e->getMessage(), 'not configured to cascade persist operations') !== FALSE) { - $this->fail($e); - } - throw $e; + } catch (ToolsException $ignored) { } - - // Verify the flushes succeeded - $this->assertEquals($email, $this->_em->find(get_class($email),$email->id)); - $this->assertEquals($address, $this->_em->find(get_class($address),$address->id)); - $this->assertEquals($user, $this->_em->find(get_class($user),$user->id)); - } /** From f39614136ffe9272ff3bfad5ac6a04e943d55870 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Mon, 21 Aug 2017 19:58:16 +0200 Subject: [PATCH 09/16] #1521 DDC-2922 reproducing test scenarios within the `UnitOfWorkTest` --- tests/Doctrine/Tests/ORM/UnitOfWorkTest.php | 131 ++++++++++++++++++++ 1 file changed, 131 insertions(+) diff --git a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php index bfad30be8..47ea39b47 100644 --- a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php +++ b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php @@ -625,6 +625,95 @@ 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()); + } } /** @@ -789,3 +878,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); + } +} From 87e8bccb11c32e35d02a47ae2a6116b9baced5f5 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Mon, 21 Aug 2017 19:58:52 +0200 Subject: [PATCH 10/16] #1521 DDC-2922 removed redundant integration test that was replaced by a unit test --- .../ORM/Functional/Ticket/DDC2922Test.php | 136 ------------------ 1 file changed, 136 deletions(-) delete mode 100644 tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2922Test.php diff --git a/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2922Test.php b/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2922Test.php deleted file mode 100644 index b1aa6c0da..000000000 --- a/tests/Doctrine/Tests/ORM/Functional/Ticket/DDC2922Test.php +++ /dev/null @@ -1,136 +0,0 @@ -_schemaTool->createSchema(array_map( - function (string $className) : ClassMetadata { - return $this->_em->getClassMetadata($className); - }, - [ - DDC2922CascadePersistedEntity::class, - DDC2922EntityWithCascadingAssociation::class, - DDC2922EntityWithNonCascadingAssociation::class, - ] - )); - } catch (ToolsException $ignored) { - } - } - - /** - * Unlike next test, this one demonstrates that the problem does - * not necessarily reproduce if all the pieces are being flushed together. - * - * @group DDC-2922 - */ - public function testNewAssociatedEntityWorksWithJustOnePath__() - { - $cascadePersisted = new DDC2922CascadePersistedEntity(); - $cascading = new DDC2922EntityWithCascadingAssociation(); - $nonCascading = new DDC2922EntityWithNonCascadingAssociation(); - - // First we persist and flush a DDC2922EntityWithCascadingAssociation 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->_em->persist($cascading); - $this->_em->persist($nonCascading); - $this->_em->flush(); - - // @TODO assert persistence on both associations - } - - - /** - * This test exhibits the bug describe in the ticket, where an object that - * ought to be reachable causes errors. - * - * @group DDC-2922 - */ - public function testNewAssociatedEntityWorksWithJustOnePath_() - { - $cascadePersisted = new DDC2922CascadePersistedEntity(); - $cascading = new DDC2922EntityWithCascadingAssociation(); - $nonCascading = new DDC2922EntityWithNonCascadingAssociation(); - - // First we persist and flush a DDC2922EntityWithCascadingAssociation 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->_em->persist($cascading); - $this->_em->flush(); - - // Note that we have NOT directly persisted the DDC2922CascadePersistedEntity, - // and DDC2922EntityWithNonCascadingAssociation does NOT have a configured - // cascade-persist. - $nonCascading->nonCascaded = $cascadePersisted; - - // However, DDC2922EntityWithCascadingAssociation *does* have a cascade-persist - // association, which ought to allow us to save the DDC2922CascadePersistedEntity - // anyway through that connection. - $cascading->cascaded = $cascadePersisted; - - $this->_em->persist($nonCascading); - $this->_em->flush(); - - // @TODO assert persistence on both associations - } -} - -/** @Entity */ -class DDC2922CascadePersistedEntity -{ - /** @Id @Column(type="string") @GeneratedValue(strategy="NONE") */ - private $id; - - public function __construct() - { - $this->id = uniqid(self::class, true); - } -} - -/** @Entity */ -class DDC2922EntityWithCascadingAssociation -{ - /** @Id @Column(type="string") @GeneratedValue(strategy="NONE") */ - private $id; - - /** @ManyToOne(targetEntity=DDC2922CascadePersistedEntity::class, cascade={"persist"}) */ - public $cascaded; - - public function __construct() - { - $this->id = uniqid(self::class, true); - } -} - -/** @Entity */ -class DDC2922EntityWithNonCascadingAssociation -{ - /** @Id @Column(type="string") @GeneratedValue(strategy="NONE") */ - private $id; - - /** @ManyToOne(targetEntity=DDC2922CascadePersistedEntity::class) */ - public $nonCascaded; - - public function __construct() - { - $this->id = uniqid(self::class, true); - } -} From 2751c0fff2a52f651354236f66b5a257dc17502e Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Mon, 21 Aug 2017 20:08:20 +0200 Subject: [PATCH 11/16] #1521 DDC-2922 minor code cleanup - renaming internal private methods, variables, removing redundant if/else --- lib/Doctrine/ORM/UnitOfWork.php | 41 ++++++++++++++++----------------- 1 file changed, 20 insertions(+), 21 deletions(-) diff --git a/lib/Doctrine/ORM/UnitOfWork.php b/lib/Doctrine/ORM/UnitOfWork.php index b3f3fb23d..5efb01b3b 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -187,9 +187,9 @@ class UnitOfWork implements PropertyChangedListener * Keys are OIDs, payload is a two-item array describing the association * and the entity. * - * @var array + * @var object[][]|array[][] indexed by respective object spl_object_hash() */ - private $newEntitiesWithoutCascade = array(); + private $nonCascadedNewDetectedEntities = []; /** * All pending collection deletions. @@ -347,7 +347,8 @@ class UnitOfWork implements PropertyChangedListener } } - $this->assertNoCascadingGaps(); + // @TODO move this further down + $this->assertThatThereAreNoUnintentionallyNonPersistedAssociations(); if ( ! ($this->entityInsertions || $this->entityDeletions || @@ -442,7 +443,7 @@ class UnitOfWork implements PropertyChangedListener $this->entityDeletions = $this->extraUpdates = $this->collectionUpdates = - $this->newEntitiesWithoutCascade = + $this->nonCascadedNewDetectedEntities = $this->collectionDeletions = $this->visitedCollections = $this->orphanRemovals = []; @@ -883,11 +884,14 @@ class UnitOfWork implements PropertyChangedListener * through the object-graph where cascade-persistence * is enabled for this object. */ - $this->newEntitiesWithoutCascade[spl_object_hash($entry)] = array($assoc,$entry); - }else { - $this->persistNew($targetClass, $entry); - $this->computeChangeSet($targetClass, $entry); + $this->nonCascadedNewDetectedEntities[\spl_object_hash($entry)] = [$assoc, $entry]; + + break; } + + $this->persistNew($targetClass, $entry); + $this->computeChangeSet($targetClass, $entry); + break; case self::STATE_REMOVED: @@ -2433,7 +2437,7 @@ class UnitOfWork implements PropertyChangedListener $this->entityInsertions = $this->entityUpdates = $this->entityDeletions = - $this->newEntitiesWithoutCascade = + $this->nonCascadedNewDetectedEntities = $this->collectionDeletions = $this->collectionUpdates = $this->extraUpdates = @@ -3388,21 +3392,16 @@ class UnitOfWork implements PropertyChangedListener } /** - * Checks that there are no new entities found through non-cascade-persist - * paths which are not also scheduled for insertion through valid paths. - * - * @return void * @throws ORMInvalidArgumentException */ - private function assertNoCascadingGaps() + private function assertThatThereAreNoUnintentionallyNonPersistedAssociations() : void { - /** - * Filter out any entities that we (successfully) managed to schedule - * for insertion. - */ - $entitiesNeedingCascadePersist = array_diff_key($this->newEntitiesWithoutCascade, $this->entityInsertions); - if(count($entitiesNeedingCascadePersist) > 0){ - list($assoc,$entity) = array_values($entitiesNeedingCascadePersist)[0]; + $entitiesNeedingCascadePersist = \array_diff_key($this->nonCascadedNewDetectedEntities, $this->entityInsertions); + + if($entitiesNeedingCascadePersist){ + [$assoc, $entity] = \array_values($entitiesNeedingCascadePersist)[0]; + + // @TODO internal clean up here throw ORMInvalidArgumentException::newEntityFoundThroughRelationship($assoc, $entity); } } From 89fbb6a0606c61af5b190b6e0e9d61834e071db3 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 22 Aug 2017 08:49:42 +0200 Subject: [PATCH 12/16] #1521 DDC-2922 verifying that persistence operations will resume normally after a crash due to invalid new values detected on associations --- tests/Doctrine/Tests/ORM/UnitOfWorkTest.php | 42 ++++++++++++++++++++- 1 file changed, 41 insertions(+), 1 deletion(-) diff --git a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php index 47ea39b47..2a06c28f3 100644 --- a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php +++ b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php @@ -663,7 +663,6 @@ class UnitOfWorkTest extends OrmTestCase $this->assertCount(1, $persister3->getInserts()); } - /** * This test exhibits the bug describe in the ticket, where an object that * ought to be reachable causes errors. @@ -714,6 +713,47 @@ class UnitOfWorkTest extends OrmTestCase 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->persist($cascadePersisted); + $this->_unitOfWork->persist($nonCascading); + $this->_unitOfWork->commit(); + + // Persistence operations should just recover normally: + self::assertCount(1, $persister1->getInserts()); + self::assertCount(1, $persister2->getInserts()); + } } /** From a754eae0f0637988688e70fab2c51b8c9029f045 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 22 Aug 2017 08:51:27 +0200 Subject: [PATCH 13/16] #1521 DDC-2922 verifying that persistence operations will resume normally after a crash due to invalid new values detected on associations - tweaked test to make it fail --- tests/Doctrine/Tests/ORM/UnitOfWorkTest.php | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php index 2a06c28f3..ab6bd9c2c 100644 --- a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php +++ b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php @@ -746,13 +746,12 @@ class UnitOfWorkTest extends OrmTestCase self::assertEmpty($persister2->getInserts()); } - $this->_unitOfWork->persist($cascadePersisted); - $this->_unitOfWork->persist($nonCascading); + $this->_unitOfWork->persist(new CascadePersistedEntity()); $this->_unitOfWork->commit(); // Persistence operations should just recover normally: self::assertCount(1, $persister1->getInserts()); - self::assertCount(1, $persister2->getInserts()); + self::assertCount(0, $persister2->getInserts()); } } From 4a007c76f534df1cceb6aaae7f4bb74f4a9c0254 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 22 Aug 2017 09:56:43 +0200 Subject: [PATCH 14/16] #1521 DDC-2922 spec for a new exception endpoint that produces a multi-new-non-cascaded-entities error message --- .../ORM/ORMInvalidArgumentExceptionTest.php | 91 +++++++++++++++++++ 1 file changed, 91 insertions(+) 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"}).', + ], + ]; + } } From 2be32f249c4e7df7ee9b9046c8950f217408c04e Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 22 Aug 2017 09:57:57 +0200 Subject: [PATCH 15/16] #1521 DDC-2922 adapting UoW and exception implementation to the new specification --- .../ORM/ORMInvalidArgumentException.php | 62 +++++++++++++++---- lib/Doctrine/ORM/UnitOfWork.php | 14 ++--- 2 files changed, 58 insertions(+), 18 deletions(-) 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 5efb01b3b..0edd756f0 100644 --- a/lib/Doctrine/ORM/UnitOfWork.php +++ b/lib/Doctrine/ORM/UnitOfWork.php @@ -347,9 +347,6 @@ class UnitOfWork implements PropertyChangedListener } } - // @TODO move this further down - $this->assertThatThereAreNoUnintentionallyNonPersistedAssociations(); - if ( ! ($this->entityInsertions || $this->entityDeletions || $this->entityUpdates || @@ -362,6 +359,8 @@ class UnitOfWork implements PropertyChangedListener return; // Nothing to do. } + $this->assertThatThereAreNoUnintentionallyNonPersistedAssociations(); + if ($this->orphanRemovals) { foreach ($this->orphanRemovals as $orphan) { $this->remove($orphan); @@ -3398,11 +3397,12 @@ class UnitOfWork implements PropertyChangedListener { $entitiesNeedingCascadePersist = \array_diff_key($this->nonCascadedNewDetectedEntities, $this->entityInsertions); - if($entitiesNeedingCascadePersist){ - [$assoc, $entity] = \array_values($entitiesNeedingCascadePersist)[0]; + $this->nonCascadedNewDetectedEntities = []; - // @TODO internal clean up here - throw ORMInvalidArgumentException::newEntityFoundThroughRelationship($assoc, $entity); + if ($entitiesNeedingCascadePersist) { + throw ORMInvalidArgumentException::newEntitiesFoundThroughRelationships( + \array_values($entitiesNeedingCascadePersist) + ); } } From 645cccf2db16209d4192400d90d6256138375db7 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Tue, 22 Aug 2017 10:24:46 +0200 Subject: [PATCH 16/16] #1521 DDC-2922 adapting test so the `UnitOfWork` stops bragging about previous inconsistent states --- tests/Doctrine/Tests/ORM/UnitOfWorkTest.php | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php index ab6bd9c2c..12eadbdaa 100644 --- a/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php +++ b/tests/Doctrine/Tests/ORM/UnitOfWorkTest.php @@ -746,6 +746,7 @@ class UnitOfWorkTest extends OrmTestCase self::assertEmpty($persister2->getInserts()); } + $this->_unitOfWork->clear(); $this->_unitOfWork->persist(new CascadePersistedEntity()); $this->_unitOfWork->commit();