From c68d7d551f1b276c24f407562f4984711b425ae5 Mon Sep 17 00:00:00 2001 From: Jan Esser Date: Fri, 24 Jan 2020 14:24:02 +0100 Subject: [PATCH 1/4] add test to prove child node gets deleted on nodename change this only happens when: - there is a preUpdate listener - the parent collection has been iterated through this because the second time the changeset is proccessed it already has the new child nodenames as collection keys in which case it triggers a removal --- lib/Doctrine/ODM/PHPCR/UnitOfWork.php | 10 +++- .../References/ParentWithChildrenTestObj.php | 32 ++++++++++ .../ODM/PHPCR/Functional/UnitOfWorkTest.php | 58 ++++++++++++++++++- 3 files changed, 97 insertions(+), 3 deletions(-) create mode 100644 tests/Doctrine/Tests/Models/References/ParentWithChildrenTestObj.php diff --git a/lib/Doctrine/ODM/PHPCR/UnitOfWork.php b/lib/Doctrine/ODM/PHPCR/UnitOfWork.php index 3e84573c5..ffab2746c 100644 --- a/lib/Doctrine/ODM/PHPCR/UnitOfWork.php +++ b/lib/Doctrine/ODM/PHPCR/UnitOfWork.php @@ -54,6 +54,7 @@ use PHPCR\Util\NodeHelper; use PHPCR\Util\PathHelper; use PHPCR\Util\UUIDHelper; +use function spl_object_hash; /** * Unit of work class @@ -1437,8 +1438,13 @@ private function computeChildrenChanges($document, $class, $oid, $isNew, $change // check moved children to not accidentally remove a child that simply moved away. if (!(in_array($childName, $childNames) || in_array($childName, $movedChildNames))) { $child = $this->getDocumentById($id.'/'.$childName); - $this->scheduleRemove($child); - unset($originalNames[$key]); + // make sure that when the child move is already processed and another compute is triggered + // we don't remove that child + $childOid = spl_object_hash($child); + if (true || !isset($this->scheduledMoves[$childOid])) { + $this->scheduleRemove($child); + unset($originalNames[$key]); + } } } diff --git a/tests/Doctrine/Tests/Models/References/ParentWithChildrenTestObj.php b/tests/Doctrine/Tests/Models/References/ParentWithChildrenTestObj.php new file mode 100644 index 000000000..3b07a206c --- /dev/null +++ b/tests/Doctrine/Tests/Models/References/ParentWithChildrenTestObj.php @@ -0,0 +1,32 @@ +parent; + } + + public function setParentDocument($parent) + { + $this->parent = $parent; + } +} diff --git a/tests/Doctrine/Tests/ODM/PHPCR/Functional/UnitOfWorkTest.php b/tests/Doctrine/Tests/ODM/PHPCR/Functional/UnitOfWorkTest.php index 45b812782..ea3c6882d 100644 --- a/tests/Doctrine/Tests/ODM/PHPCR/Functional/UnitOfWorkTest.php +++ b/tests/Doctrine/Tests/ODM/PHPCR/Functional/UnitOfWorkTest.php @@ -3,7 +3,9 @@ namespace Doctrine\Tests\ODM\PHPCR\Functional; use Doctrine\Common\Collections\ArrayCollection; +use Doctrine\Common\EventSubscriber; use Doctrine\ODM\PHPCR\DocumentManager; +use Doctrine\ODM\PHPCR\Event; use Doctrine\ODM\PHPCR\Exception\OutOfBoundsException; use Doctrine\ODM\PHPCR\UnitOfWork; use Doctrine\Tests\Models\CMS\CmsAddress; @@ -14,8 +16,9 @@ use Doctrine\Tests\Models\CMS\CmsBlogPost; use Doctrine\Tests\Models\CMS\CmsGroup; use Doctrine\Tests\Models\CMS\CmsUser; -use Doctrine\Tests\Models\References\ParentNoNodeNameTestObj; +use Doctrine\Tests\Models\References\ParentNoNodenameTestObj; use Doctrine\Tests\Models\References\ParentTestObj; +use Doctrine\Tests\Models\References\ParentWithChildrenTestObj; use Doctrine\Tests\Models\Translation\Comment; use Doctrine\Tests\ODM\PHPCR\PHPCRFunctionalTestCase; @@ -124,6 +127,59 @@ public function testMoveParentNoNodeName() } } + public function testMoveChildThroughNodeNameChangeWithPreUpdateListener() + { + // preparing + $functional = $this->dm->find(null, 'functional'); + $root = new ParentWithChildrenTestObj(); + $root->nodename = "root"; + $root->name = "root"; + $root->setParentDocument($functional); + $this->dm->persist($root); + + $parent = new ParentWithChildrenTestObj(); + $parent->nodename = "parent"; + $parent->name = "parent"; + $parent->setParentDocument($root); + $this->dm->persist($parent); + + $child = new ParentTestObj(); + $child->setParentDocument($parent); + $child->nodename = $child->name = "child"; + $this->dm->persist($child); + + $child2 = new ParentTestObj(); + $child2->setParentDocument($parent); + $child2->nodename = $child2->name = "child2"; + $this->dm->persist($child2); + + $this->dm->flush(); + $this->dm->clear(); + + $parent = $this->dm->find(null, '/functional/root/parent'); + $parent->children->toArray(); // force container init + $child2 = $this->dm->find(null, '/functional/root/parent/child2'); + + // testing + $this->dm->getEventManager()->addEventSubscriber(new class implements EventSubscriber { + public function getSubscribedEvents() + { + return [Event::preUpdate]; + } + public function preUpdate() + { + } + }); + $child2->nodename = 'moved-child2'; + $this->dm->persist($child2); + + $this->dm->flush(); + + $movedChild = $this->dm->find(null, '/functional/root/parent/moved-child2'); + $this->assertInstanceOf(ParentTestObj::class, $movedChild); + + } + public function testGetScheduledReorders() { // TODO: do some real test From 4473bba07978c50cda4c74af470c43f8b13f35ea Mon Sep 17 00:00:00 2001 From: Jan Esser Date: Fri, 24 Jan 2020 14:25:36 +0100 Subject: [PATCH 2/4] ensure children don't get removed when another changeset check is triggered this only happens when: - there is a preUpdate listener - the parent collection has been iterated through this because the second time the changeset is proccessed it already has the new child nodenames as collection keys in which case a removal is requested --- lib/Doctrine/ODM/PHPCR/UnitOfWork.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Doctrine/ODM/PHPCR/UnitOfWork.php b/lib/Doctrine/ODM/PHPCR/UnitOfWork.php index ffab2746c..97d74573d 100644 --- a/lib/Doctrine/ODM/PHPCR/UnitOfWork.php +++ b/lib/Doctrine/ODM/PHPCR/UnitOfWork.php @@ -1441,7 +1441,7 @@ private function computeChildrenChanges($document, $class, $oid, $isNew, $change // make sure that when the child move is already processed and another compute is triggered // we don't remove that child $childOid = spl_object_hash($child); - if (true || !isset($this->scheduledMoves[$childOid])) { + if (!isset($this->scheduledMoves[$childOid])) { $this->scheduleRemove($child); unset($originalNames[$key]); } From c041cc85c695b13407d6b2f451d1a79dc5d7242f Mon Sep 17 00:00:00 2001 From: Jan Esser Date: Fri, 24 Jan 2020 14:38:20 +0100 Subject: [PATCH 3/4] fix for styleCI fix for styleCI fix for styleCI fix for styleCI --- .../ODM/PHPCR/Functional/UnitOfWorkTest.php | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/Doctrine/Tests/ODM/PHPCR/Functional/UnitOfWorkTest.php b/tests/Doctrine/Tests/ODM/PHPCR/Functional/UnitOfWorkTest.php index ea3c6882d..f833acfc7 100644 --- a/tests/Doctrine/Tests/ODM/PHPCR/Functional/UnitOfWorkTest.php +++ b/tests/Doctrine/Tests/ODM/PHPCR/Functional/UnitOfWorkTest.php @@ -132,25 +132,25 @@ public function testMoveChildThroughNodeNameChangeWithPreUpdateListener() // preparing $functional = $this->dm->find(null, 'functional'); $root = new ParentWithChildrenTestObj(); - $root->nodename = "root"; - $root->name = "root"; + $root->nodename = 'root'; + $root->name = 'root'; $root->setParentDocument($functional); $this->dm->persist($root); $parent = new ParentWithChildrenTestObj(); - $parent->nodename = "parent"; - $parent->name = "parent"; + $parent->nodename = 'parent'; + $parent->name = 'parent'; $parent->setParentDocument($root); $this->dm->persist($parent); $child = new ParentTestObj(); $child->setParentDocument($parent); - $child->nodename = $child->name = "child"; + $child->nodename = $child->name = 'child'; $this->dm->persist($child); $child2 = new ParentTestObj(); $child2->setParentDocument($parent); - $child2->nodename = $child2->name = "child2"; + $child2->nodename = $child2->name = 'child2'; $this->dm->persist($child2); $this->dm->flush(); @@ -161,11 +161,12 @@ public function testMoveChildThroughNodeNameChangeWithPreUpdateListener() $child2 = $this->dm->find(null, '/functional/root/parent/child2'); // testing - $this->dm->getEventManager()->addEventSubscriber(new class implements EventSubscriber { + $this->dm->getEventManager()->addEventSubscriber(new class() implements EventSubscriber { public function getSubscribedEvents() { return [Event::preUpdate]; } + public function preUpdate() { } @@ -177,7 +178,6 @@ public function preUpdate() $movedChild = $this->dm->find(null, '/functional/root/parent/moved-child2'); $this->assertInstanceOf(ParentTestObj::class, $movedChild); - } public function testGetScheduledReorders() From bbba2aa4a7d80a27695a7aab440574e87f7aab6b Mon Sep 17 00:00:00 2001 From: Jan Esser Date: Fri, 24 Jan 2020 15:11:23 +0100 Subject: [PATCH 4/4] revert accidental capitalization change after conflict resolution --- tests/Doctrine/Tests/ODM/PHPCR/Functional/UnitOfWorkTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Doctrine/Tests/ODM/PHPCR/Functional/UnitOfWorkTest.php b/tests/Doctrine/Tests/ODM/PHPCR/Functional/UnitOfWorkTest.php index f833acfc7..65f520232 100644 --- a/tests/Doctrine/Tests/ODM/PHPCR/Functional/UnitOfWorkTest.php +++ b/tests/Doctrine/Tests/ODM/PHPCR/Functional/UnitOfWorkTest.php @@ -16,7 +16,7 @@ use Doctrine\Tests\Models\CMS\CmsBlogPost; use Doctrine\Tests\Models\CMS\CmsGroup; use Doctrine\Tests\Models\CMS\CmsUser; -use Doctrine\Tests\Models\References\ParentNoNodenameTestObj; +use Doctrine\Tests\Models\References\ParentNoNodeNameTestObj; use Doctrine\Tests\Models\References\ParentTestObj; use Doctrine\Tests\Models\References\ParentWithChildrenTestObj; use Doctrine\Tests\Models\Translation\Comment;