From 8f7c5529e99d039c957b601e2ff9d632f587b9a6 Mon Sep 17 00:00:00 2001 From: Sebastian Helzle Date: Sun, 2 Apr 2023 00:15:28 +0700 Subject: [PATCH 1/7] TASK: Enable hierarchical asset collections This allows assigning a parent to collections to enable the hierachy feature in the Flowpack.Media.UI. --- .../Classes/Domain/Model/AssetCollection.php | 73 +++++++ .../Repository/AssetCollectionRepository.php | 3 + .../Mysql/Version20230401055019.php | 34 +++ .../Postgresql/Version20230401171612.php | 34 +++ .../Domain/Model/AssetCollectionTest.php | 205 ++++++++++++++++++ .../AssetCollectionRepositoryTest.php | 89 ++++++++ 6 files changed, 438 insertions(+) create mode 100644 Neos.Media/Migrations/Mysql/Version20230401055019.php create mode 100644 Neos.Media/Migrations/Postgresql/Version20230401171612.php create mode 100644 Neos.Media/Tests/Functional/Domain/Model/AssetCollectionTest.php create mode 100644 Neos.Media/Tests/Functional/Domain/Repository/AssetCollectionRepositoryTest.php diff --git a/Neos.Media/Classes/Domain/Model/AssetCollection.php b/Neos.Media/Classes/Domain/Model/AssetCollection.php index 641e53c8194..59d09729760 100644 --- a/Neos.Media/Classes/Domain/Model/AssetCollection.php +++ b/Neos.Media/Classes/Domain/Model/AssetCollection.php @@ -44,6 +44,22 @@ class AssetCollection */ protected $tags; + /** + * @var AssetCollection + * @ORM\ManyToOne(inversedBy="children", cascade={"persist"}) + * @ORM\JoinColumn(onDelete="SET NULL") + * @Flow\Lazy + */ + protected $parent; + + /** + * @var Collection + * @ORM\OneToMany(mappedBy="parent", orphanRemoval=true, cascade={"persist"}) + * @ORM\OrderBy({"title"="ASC"}) + * @Flow\Lazy + */ + protected $children; + /** * @param string $title */ @@ -52,6 +68,7 @@ public function __construct($title) $this->title = $title; $this->assets = new ArrayCollection(); $this->tags = new ArrayCollection(); + $this->children = new ArrayCollection(); } /** @@ -177,4 +194,60 @@ public function removeTag(Tag $tag) } return false; } + + public function getParent(): ?self + { + return $this->parent; + } + + public function setParent(?self $parent): void + { + $this->parent = $parent; + + if ($parent) { + // Throw an error if a circular dependency has been detected + $parents = [$parent]; + while ($parent !== null) { + $parent = $parent->getParent(); + if (in_array($parent, $parents, true)) { + throw new \InvalidArgumentException('Circular reference detected', 1680328041); + } + } + } + } + + public function addChild(self $child): void + { + if (!$this->children->contains($child)) { + $this->children->add($child); + } + } + + public function removeChild(self $child): void + { + if ($this->children->contains($child)) { + $this->children->removeElement($child); + } + } + + /** + * @return Collection + */ + public function getChildren(): Collection + { + return $this->children ?? new ArrayCollection(); + } + + /** + * @param Collection $children + */ + public function setChildren(Collection $children): void + { + foreach ($this->children as $child) { + $child->setParent(null); + } + foreach ($children as $child) { + $child->setParent($this); + } + } } diff --git a/Neos.Media/Classes/Domain/Repository/AssetCollectionRepository.php b/Neos.Media/Classes/Domain/Repository/AssetCollectionRepository.php index 6883c50359c..d789d314288 100644 --- a/Neos.Media/Classes/Domain/Repository/AssetCollectionRepository.php +++ b/Neos.Media/Classes/Domain/Repository/AssetCollectionRepository.php @@ -14,10 +14,13 @@ use Neos\Flow\Annotations as Flow; use Neos\Flow\Persistence\QueryInterface; use Neos\Flow\Persistence\Repository; +use Neos\Media\Domain\Model\AssetCollection; /** * A repository for AssetCollections * + * @method AssetCollection findOneByTitle(string $title) + * * @Flow\Scope("singleton") */ class AssetCollectionRepository extends Repository diff --git a/Neos.Media/Migrations/Mysql/Version20230401055019.php b/Neos.Media/Migrations/Mysql/Version20230401055019.php new file mode 100644 index 00000000000..37cd042863b --- /dev/null +++ b/Neos.Media/Migrations/Mysql/Version20230401055019.php @@ -0,0 +1,34 @@ +abortIf(!$this->connection->getDatabasePlatform()->getName() !== 'mysql', 'Migration can only be executed safely on "mysql".'); + + $this->addSql('ALTER TABLE neos_media_domain_model_assetcollection ADD parent VARCHAR(40) DEFAULT NULL'); + $this->addSql('ALTER TABLE neos_media_domain_model_assetcollection ADD CONSTRAINT FK_74C770A3D8E604F FOREIGN KEY (parent) REFERENCES neos_media_domain_model_assetcollection (persistence_object_identifier) ON DELETE SET NULL'); + $this->addSql('CREATE INDEX IDX_74C770A3D8E604F ON neos_media_domain_model_assetcollection (parent)'); + } + + public function down(Schema $schema): void + { + $this->abortIf(!$this->connection->getDatabasePlatform()->getName() !== 'mysql', 'Migration can only be executed safely on "mysql".'); + + $this->addSql('ALTER TABLE neos_media_domain_model_assetcollection DROP FOREIGN KEY FK_74C770A3D8E604F'); + $this->addSql('DROP INDEX IDX_74C770A3D8E604F ON neos_media_domain_model_assetcollection'); + $this->addSql('ALTER TABLE neos_media_domain_model_assetcollection DROP parent'); + } +} diff --git a/Neos.Media/Migrations/Postgresql/Version20230401171612.php b/Neos.Media/Migrations/Postgresql/Version20230401171612.php new file mode 100644 index 00000000000..2384b5db68b --- /dev/null +++ b/Neos.Media/Migrations/Postgresql/Version20230401171612.php @@ -0,0 +1,34 @@ +abortIf($this->connection->getDatabasePlatform()->getName() !== 'postgresql', 'Migration can only be executed safely on "postgresql".'); + + $this->addSql('ALTER TABLE neos_media_domain_model_assetcollection ADD parent VARCHAR(40) DEFAULT NULL'); + $this->addSql('ALTER TABLE neos_media_domain_model_assetcollection ADD CONSTRAINT FK_74C770A3D8E604F FOREIGN KEY (parent) REFERENCES neos_media_domain_model_assetcollection (persistence_object_identifier) ON DELETE SET NULL NOT DEFERRABLE INITIALLY IMMEDIATE'); + $this->addSql('CREATE INDEX IDX_74C770A3D8E604F ON neos_media_domain_model_assetcollection (parent)'); + } + + public function down(Schema $schema): void + { + $this->abortIf($this->connection->getDatabasePlatform()->getName() !== 'postgresql', 'Migration can only be executed safely on "postgresql".'); + + $this->addSql('ALTER TABLE neos_media_domain_model_assetcollection DROP CONSTRAINT FK_74C770A3D8E604F'); + $this->addSql('DROP INDEX IDX_74C770A3D8E604F'); + $this->addSql('ALTER TABLE neos_media_domain_model_assetcollection DROP parent'); + } +} diff --git a/Neos.Media/Tests/Functional/Domain/Model/AssetCollectionTest.php b/Neos.Media/Tests/Functional/Domain/Model/AssetCollectionTest.php new file mode 100644 index 00000000000..857b06c1745 --- /dev/null +++ b/Neos.Media/Tests/Functional/Domain/Model/AssetCollectionTest.php @@ -0,0 +1,205 @@ +persistenceManager instanceof PersistenceManager) { + static::markTestSkipped('Doctrine persistence is not enabled'); + } + + $this->assetCollectionRepository = $this->objectManager->get(AssetCollectionRepository::class); + } + + /** + * @test + */ + public function parentChildrenRelation(): void + { + $child1 = new AssetCollection('child1'); + $child2 = new AssetCollection('child2'); + $parent = new AssetCollection('parent'); + + $child1->setParent($parent); + $child2->setParent($parent); + + $this->assetCollectionRepository->add($parent); + $this->assetCollectionRepository->add($child1); + $this->assetCollectionRepository->add($child2); + + $this->persistenceManager->persistAll(); + $this->persistenceManager->clearState(); + + $persistedChild = $this->assetCollectionRepository->findOneByTitle('child1'); + $persistedParent = $persistedChild->getParent(); + + self::assertEquals('parent', $persistedParent->getTitle()); + self::assertNull($persistedParent->getParent()); + + $children = $persistedParent->getChildren(); + self::assertEquals(2, $children->count()); + self::assertEquals('child1', $children->offsetGet(0)->getTitle()); + self::assertEquals('child2', $children->offsetGet(1)->getTitle()); + } + + /** + * @test + */ + public function circularParentChildrenRelationThrowsError(): void + { + $child = new AssetCollection('child'); + $childOfChild = new AssetCollection('childOfChild'); + $parent = new AssetCollection('parent'); + + $child->setParent($parent); + $childOfChild->setParent($child); + + $this->expectException(\InvalidArgumentException::class); + $parent->setParent($childOfChild); + } + + /** + * @test + */ + public function removingTheChildDeletesIt(): void + { + $child = new AssetCollection('child'); + $parent = new AssetCollection('parent'); + + $child->setParent($parent); + + $this->assetCollectionRepository->add($parent); + $this->assetCollectionRepository->add($child); + + $this->persistenceManager->persistAll(); + $this->persistenceManager->clearState(); + + $persistedChild = $this->assetCollectionRepository->findOneByTitle('child'); + $persistedParent = $this->assetCollectionRepository->findOneByTitle('parent'); + + $persistedParent->removeChild($persistedChild); + + $this->persistenceManager->persistAll(); + $this->persistenceManager->clearState(); + + $persistedChild = $this->assetCollectionRepository->findOneByTitle('child'); + $persistedParent = $this->assetCollectionRepository->findOneByTitle('parent'); + + self::assertNull($persistedChild); + self::assertEquals(0, $persistedParent->getChildren()->count()); + } + + /** + * @test + */ + public function unsettingTheParentRemovesChildFromParent(): void + { + $child = new AssetCollection('child'); + $parent = new AssetCollection('parent'); + + $child->setParent($parent); + + $this->assetCollectionRepository->add($parent); + $this->assetCollectionRepository->add($child); + + $this->persistenceManager->persistAll(); + $this->persistenceManager->clearState(); + + $persistedChild = $this->assetCollectionRepository->findOneByTitle('child'); + $persistedChild->setParent(null); + + $this->assetCollectionRepository->update($persistedChild); + $this->persistenceManager->persistAll(); + $this->persistenceManager->clearState(); + + $persistedChild = $this->assetCollectionRepository->findOneByTitle('child'); + $persistedParent = $this->assetCollectionRepository->findOneByTitle('parent'); + + self::assertNull($persistedChild->getParent()); + self::assertEquals(0, $persistedParent->getChildren()->count()); + } + + /** + * @test + */ + public function deletingTheParentDeletesTheChild(): void + { + $child = new AssetCollection('child'); + $parent = new AssetCollection('parent'); + + $child->setParent($parent); + + $this->assetCollectionRepository->add($parent); + $this->assetCollectionRepository->add($child); + + $this->persistenceManager->persistAll(); + $this->persistenceManager->clearState(); + + $persistedParent = $this->assetCollectionRepository->findOneByTitle('parent'); + $this->assetCollectionRepository->remove($persistedParent); + + $this->persistenceManager->persistAll(); + $this->persistenceManager->clearState(); + + $persistedChild = $this->assetCollectionRepository->findOneByTitle('child'); + $persistedParent = $this->assetCollectionRepository->findOneByTitle('parent'); + + self::assertNull($persistedChild); + self::assertNull($persistedParent); + } + + /** + * @test + */ + public function settingChildrenSetsTheirParent(): void + { + $child1 = new AssetCollection('child1'); + $child2 = new AssetCollection('child2'); + $parent = new AssetCollection('parent'); + + $parent->setChildren(new ArrayCollection([$child1, $child2])); + + $this->assetCollectionRepository->add($parent); + $this->assetCollectionRepository->add($child1); + $this->assetCollectionRepository->add($child2); + + $this->persistenceManager->persistAll(); + $this->persistenceManager->clearState(); + + $persistedChild1 = $this->assetCollectionRepository->findOneByTitle('child1'); + $persistedChild2 = $this->assetCollectionRepository->findOneByTitle('child2'); + + self::assertEquals('parent', $persistedChild1->getParent()->getTitle()); + self::assertEquals('parent', $persistedChild2->getParent()->getTitle()); + } +} diff --git a/Neos.Media/Tests/Functional/Domain/Repository/AssetCollectionRepositoryTest.php b/Neos.Media/Tests/Functional/Domain/Repository/AssetCollectionRepositoryTest.php new file mode 100644 index 00000000000..4c2537879ec --- /dev/null +++ b/Neos.Media/Tests/Functional/Domain/Repository/AssetCollectionRepositoryTest.php @@ -0,0 +1,89 @@ +persistenceManager instanceof PersistenceManager) { + static::markTestSkipped('Doctrine persistence is not enabled'); + } + + $this->assetCollectionRepository = $this->objectManager->get(AssetCollectionRepository::class); + } + + /** + * @test + */ + public function assetCollectionsCanBePersisted(): void + { + $assetCollection = new AssetCollection('foobar'); + + $this->assetCollectionRepository->add($assetCollection); + $this->persistenceManager->persistAll(); + $this->persistenceManager->clearState(); + self::assertCount(1, $this->assetCollectionRepository->findAll()); + self::assertInstanceOf(AssetCollection::class, $this->assetCollectionRepository->findAll()->getFirst()); + } + + /** + * @test + */ + public function parentRemoveRemovesCompleteHierarchy(): void + { + $grandchild = new AssetCollection('grandChild'); + $child = new AssetCollection('child'); + $parent = new AssetCollection('parent'); + $child->setParent($parent); + $grandchild->setParent($child); + + $this->assetCollectionRepository->add($parent); + $this->assetCollectionRepository->add($child); + $this->assetCollectionRepository->add($grandchild); + + $this->persistenceManager->persistAll(); + $this->persistenceManager->clearState(); + + $persistedParent = $this->assetCollectionRepository->findOneByTitle('parent'); + $this->assetCollectionRepository->remove($persistedParent); + $this->persistenceManager->persistAll(); + $this->persistenceManager->clearState(); + + static::assertNull($this->assetCollectionRepository->findOneByTitle('child')); + static::assertNull($this->assetCollectionRepository->findOneByTitle('grandChild')); + static::assertNull($this->assetCollectionRepository->findOneByTitle('parent')); + } +} From 65cccfe0c53a0dd944e3e41606dcaae975949c9d Mon Sep 17 00:00:00 2001 From: Sebastian Helzle Date: Sat, 8 Apr 2023 15:41:07 +0700 Subject: [PATCH 2/7] TASK: Check circular dependency when modifying asset collection parent or children --- .../Classes/Domain/Model/AssetCollection.php | 30 +++++++++------ .../Domain/Model/AssetCollectionTest.php | 37 +++++++++++++++++-- 2 files changed, 53 insertions(+), 14 deletions(-) diff --git a/Neos.Media/Classes/Domain/Model/AssetCollection.php b/Neos.Media/Classes/Domain/Model/AssetCollection.php index 59d09729760..34c783a8166 100644 --- a/Neos.Media/Classes/Domain/Model/AssetCollection.php +++ b/Neos.Media/Classes/Domain/Model/AssetCollection.php @@ -200,26 +200,33 @@ public function getParent(): ?self return $this->parent; } - public function setParent(?self $parent): void + /** + * Throws an error if a circular dependency has been detected + * @throws \InvalidArgumentException + */ + private function validateCircularHierarchy(): void { - $this->parent = $parent; - - if ($parent) { - // Throw an error if a circular dependency has been detected - $parents = [$parent]; - while ($parent !== null) { - $parent = $parent->getParent(); - if (in_array($parent, $parents, true)) { - throw new \InvalidArgumentException('Circular reference detected', 1680328041); - } + $parent = $this->parent; + $parents = [$parent]; + while ($parent !== null) { + $parent = $parent->getParent(); + if (in_array($parent, $parents, true)) { + throw new \InvalidArgumentException('Circular reference detected', 1680328041); } } } + public function setParent(?self $parent): void + { + $this->parent = $parent; + $this->validateCircularHierarchy(); + } + public function addChild(self $child): void { if (!$this->children->contains($child)) { $this->children->add($child); + $child->setParent($this); } } @@ -249,5 +256,6 @@ public function setChildren(Collection $children): void foreach ($children as $child) { $child->setParent($this); } + $this->validateCircularHierarchy(); } } diff --git a/Neos.Media/Tests/Functional/Domain/Model/AssetCollectionTest.php b/Neos.Media/Tests/Functional/Domain/Model/AssetCollectionTest.php index 857b06c1745..5e3ad884acc 100644 --- a/Neos.Media/Tests/Functional/Domain/Model/AssetCollectionTest.php +++ b/Neos.Media/Tests/Functional/Domain/Model/AssetCollectionTest.php @@ -51,11 +51,10 @@ public function parentChildrenRelation(): void $parent = new AssetCollection('parent'); $child1->setParent($parent); - $child2->setParent($parent); + $parent->addChild($child2); $this->assetCollectionRepository->add($parent); $this->assetCollectionRepository->add($child1); - $this->assetCollectionRepository->add($child2); $this->persistenceManager->persistAll(); $this->persistenceManager->clearState(); @@ -75,7 +74,7 @@ public function parentChildrenRelation(): void /** * @test */ - public function circularParentChildrenRelationThrowsError(): void + public function circularParentChildrenRelationThrowsErrorWhenSettingParent(): void { $child = new AssetCollection('child'); $childOfChild = new AssetCollection('childOfChild'); @@ -88,6 +87,38 @@ public function circularParentChildrenRelationThrowsError(): void $parent->setParent($childOfChild); } + /** + * @test + */ + public function circularParentChildrenRelationThrowsErrorWhenAddingChild(): void + { + $child = new AssetCollection('child'); + $childOfChild = new AssetCollection('childOfChild'); + $parent = new AssetCollection('parent'); + + $parent->addChild($child); + $child->addChild($childOfChild); + + $this->expectException(\InvalidArgumentException::class); + $childOfChild->addChild($parent); + } + + /** + * @test + */ + public function circularParentChildrenRelationThrowsErrorWhenSettingChildren(): void + { + $child = new AssetCollection('child'); + $childOfChild = new AssetCollection('childOfChild'); + $parent = new AssetCollection('parent'); + + $parent->setChildren(new ArrayCollection([$child])); + $child->setChildren(new ArrayCollection([$childOfChild])); + + $this->expectException(\InvalidArgumentException::class); + $childOfChild->setChildren(new ArrayCollection([$parent])); + } + /** * @test */ From e2c7825f501c52a2975397b90f06ec15ce66f9f1 Mon Sep 17 00:00:00 2001 From: Sebastian Helzle Date: Fri, 21 Apr 2023 10:07:34 +0200 Subject: [PATCH 3/7] TASK: Only use parent property for manipulating and traversing asset collection hierarchy --- .../Classes/Domain/Model/AssetCollection.php | 38 -------- .../Repository/AssetCollectionRepository.php | 2 + .../Domain/Model/AssetCollectionTest.php | 97 ++----------------- 3 files changed, 8 insertions(+), 129 deletions(-) diff --git a/Neos.Media/Classes/Domain/Model/AssetCollection.php b/Neos.Media/Classes/Domain/Model/AssetCollection.php index 34c783a8166..7247f2b0d4c 100644 --- a/Neos.Media/Classes/Domain/Model/AssetCollection.php +++ b/Neos.Media/Classes/Domain/Model/AssetCollection.php @@ -68,7 +68,6 @@ public function __construct($title) $this->title = $title; $this->assets = new ArrayCollection(); $this->tags = new ArrayCollection(); - $this->children = new ArrayCollection(); } /** @@ -221,41 +220,4 @@ public function setParent(?self $parent): void $this->parent = $parent; $this->validateCircularHierarchy(); } - - public function addChild(self $child): void - { - if (!$this->children->contains($child)) { - $this->children->add($child); - $child->setParent($this); - } - } - - public function removeChild(self $child): void - { - if ($this->children->contains($child)) { - $this->children->removeElement($child); - } - } - - /** - * @return Collection - */ - public function getChildren(): Collection - { - return $this->children ?? new ArrayCollection(); - } - - /** - * @param Collection $children - */ - public function setChildren(Collection $children): void - { - foreach ($this->children as $child) { - $child->setParent(null); - } - foreach ($children as $child) { - $child->setParent($this); - } - $this->validateCircularHierarchy(); - } } diff --git a/Neos.Media/Classes/Domain/Repository/AssetCollectionRepository.php b/Neos.Media/Classes/Domain/Repository/AssetCollectionRepository.php index d789d314288..f82c3e6b623 100644 --- a/Neos.Media/Classes/Domain/Repository/AssetCollectionRepository.php +++ b/Neos.Media/Classes/Domain/Repository/AssetCollectionRepository.php @@ -13,6 +13,7 @@ use Neos\Flow\Annotations as Flow; use Neos\Flow\Persistence\QueryInterface; +use Neos\Flow\Persistence\QueryResultInterface; use Neos\Flow\Persistence\Repository; use Neos\Media\Domain\Model\AssetCollection; @@ -20,6 +21,7 @@ * A repository for AssetCollections * * @method AssetCollection findOneByTitle(string $title) + * @method QueryResultInterface findByParent(AssetCollection $parent) * * @Flow\Scope("singleton") */ diff --git a/Neos.Media/Tests/Functional/Domain/Model/AssetCollectionTest.php b/Neos.Media/Tests/Functional/Domain/Model/AssetCollectionTest.php index 5e3ad884acc..64da25da4cb 100644 --- a/Neos.Media/Tests/Functional/Domain/Model/AssetCollectionTest.php +++ b/Neos.Media/Tests/Functional/Domain/Model/AssetCollectionTest.php @@ -51,10 +51,11 @@ public function parentChildrenRelation(): void $parent = new AssetCollection('parent'); $child1->setParent($parent); - $parent->addChild($child2); + $child2->setParent($parent); $this->assetCollectionRepository->add($parent); $this->assetCollectionRepository->add($child1); + $this->assetCollectionRepository->add($child2); $this->persistenceManager->persistAll(); $this->persistenceManager->clearState(); @@ -65,7 +66,7 @@ public function parentChildrenRelation(): void self::assertEquals('parent', $persistedParent->getTitle()); self::assertNull($persistedParent->getParent()); - $children = $persistedParent->getChildren(); + $children = $this->assetCollectionRepository->findByParent($parent); self::assertEquals(2, $children->count()); self::assertEquals('child1', $children->offsetGet(0)->getTitle()); self::assertEquals('child2', $children->offsetGet(1)->getTitle()); @@ -87,69 +88,6 @@ public function circularParentChildrenRelationThrowsErrorWhenSettingParent(): vo $parent->setParent($childOfChild); } - /** - * @test - */ - public function circularParentChildrenRelationThrowsErrorWhenAddingChild(): void - { - $child = new AssetCollection('child'); - $childOfChild = new AssetCollection('childOfChild'); - $parent = new AssetCollection('parent'); - - $parent->addChild($child); - $child->addChild($childOfChild); - - $this->expectException(\InvalidArgumentException::class); - $childOfChild->addChild($parent); - } - - /** - * @test - */ - public function circularParentChildrenRelationThrowsErrorWhenSettingChildren(): void - { - $child = new AssetCollection('child'); - $childOfChild = new AssetCollection('childOfChild'); - $parent = new AssetCollection('parent'); - - $parent->setChildren(new ArrayCollection([$child])); - $child->setChildren(new ArrayCollection([$childOfChild])); - - $this->expectException(\InvalidArgumentException::class); - $childOfChild->setChildren(new ArrayCollection([$parent])); - } - - /** - * @test - */ - public function removingTheChildDeletesIt(): void - { - $child = new AssetCollection('child'); - $parent = new AssetCollection('parent'); - - $child->setParent($parent); - - $this->assetCollectionRepository->add($parent); - $this->assetCollectionRepository->add($child); - - $this->persistenceManager->persistAll(); - $this->persistenceManager->clearState(); - - $persistedChild = $this->assetCollectionRepository->findOneByTitle('child'); - $persistedParent = $this->assetCollectionRepository->findOneByTitle('parent'); - - $persistedParent->removeChild($persistedChild); - - $this->persistenceManager->persistAll(); - $this->persistenceManager->clearState(); - - $persistedChild = $this->assetCollectionRepository->findOneByTitle('child'); - $persistedParent = $this->assetCollectionRepository->findOneByTitle('parent'); - - self::assertNull($persistedChild); - self::assertEquals(0, $persistedParent->getChildren()->count()); - } - /** * @test */ @@ -176,8 +114,10 @@ public function unsettingTheParentRemovesChildFromParent(): void $persistedChild = $this->assetCollectionRepository->findOneByTitle('child'); $persistedParent = $this->assetCollectionRepository->findOneByTitle('parent'); + $children = $this->assetCollectionRepository->findByParent($persistedParent); + self::assertNull($persistedChild->getParent()); - self::assertEquals(0, $persistedParent->getChildren()->count()); + self::assertEquals(0, count($children)); } /** @@ -208,29 +148,4 @@ public function deletingTheParentDeletesTheChild(): void self::assertNull($persistedChild); self::assertNull($persistedParent); } - - /** - * @test - */ - public function settingChildrenSetsTheirParent(): void - { - $child1 = new AssetCollection('child1'); - $child2 = new AssetCollection('child2'); - $parent = new AssetCollection('parent'); - - $parent->setChildren(new ArrayCollection([$child1, $child2])); - - $this->assetCollectionRepository->add($parent); - $this->assetCollectionRepository->add($child1); - $this->assetCollectionRepository->add($child2); - - $this->persistenceManager->persistAll(); - $this->persistenceManager->clearState(); - - $persistedChild1 = $this->assetCollectionRepository->findOneByTitle('child1'); - $persistedChild2 = $this->assetCollectionRepository->findOneByTitle('child2'); - - self::assertEquals('parent', $persistedChild1->getParent()->getTitle()); - self::assertEquals('parent', $persistedChild2->getParent()->getTitle()); - } } From 50585d34b2d842f6e62365c25b1a8a72d42489e2 Mon Sep 17 00:00:00 2001 From: Sebastian Helzle Date: Wed, 10 May 2023 14:18:47 +0200 Subject: [PATCH 4/7] TASK: Explicitely remove child asset collections when their parent is deleted --- .../Classes/Domain/Model/AssetCollection.php | 8 -------- .../Repository/AssetCollectionRepository.php | 15 +++++++++++++++ 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/Neos.Media/Classes/Domain/Model/AssetCollection.php b/Neos.Media/Classes/Domain/Model/AssetCollection.php index 7247f2b0d4c..516f64c9c3d 100644 --- a/Neos.Media/Classes/Domain/Model/AssetCollection.php +++ b/Neos.Media/Classes/Domain/Model/AssetCollection.php @@ -52,14 +52,6 @@ class AssetCollection */ protected $parent; - /** - * @var Collection - * @ORM\OneToMany(mappedBy="parent", orphanRemoval=true, cascade={"persist"}) - * @ORM\OrderBy({"title"="ASC"}) - * @Flow\Lazy - */ - protected $children; - /** * @param string $title */ diff --git a/Neos.Media/Classes/Domain/Repository/AssetCollectionRepository.php b/Neos.Media/Classes/Domain/Repository/AssetCollectionRepository.php index f82c3e6b623..2ae40fb2a76 100644 --- a/Neos.Media/Classes/Domain/Repository/AssetCollectionRepository.php +++ b/Neos.Media/Classes/Domain/Repository/AssetCollectionRepository.php @@ -31,4 +31,19 @@ class AssetCollectionRepository extends Repository * @var array */ protected $defaultOrderings = ['title' => QueryInterface::ORDER_ASCENDING]; + + /** + * Remove all child collections recursively to prevent orphaned collections + */ + public function remove($object): void + { + /** @var AssetCollection $object */ + $childCollections = $this->findByParent($object); + foreach ($childCollections as $childCollection) { + $this->remove($childCollection); + } + parent::remove($object); + } + + } From 339512685a080b4f94bca113b53ec7f1522b36ee Mon Sep 17 00:00:00 2001 From: Sebastian Helzle Date: Thu, 1 Jun 2023 13:11:39 +0200 Subject: [PATCH 5/7] TASK: Adjust migration for parent collections Co-authored-by: Bastian Waidelich --- Neos.Media/Migrations/Mysql/Version20230401055019.php | 5 +++-- Neos.Media/Migrations/Postgresql/Version20230401171612.php | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/Neos.Media/Migrations/Mysql/Version20230401055019.php b/Neos.Media/Migrations/Mysql/Version20230401055019.php index 37cd042863b..6707b39e09d 100644 --- a/Neos.Media/Migrations/Mysql/Version20230401055019.php +++ b/Neos.Media/Migrations/Mysql/Version20230401055019.php @@ -4,6 +4,7 @@ namespace Neos\Flow\Persistence\Doctrine\Migrations; +use Doctrine\DBAL\Platforms\MySqlPlatform; use Doctrine\DBAL\Schema\Schema; use Doctrine\Migrations\AbstractMigration; @@ -16,7 +17,7 @@ public function getDescription(): string public function up(Schema $schema): void { - $this->abortIf(!$this->connection->getDatabasePlatform()->getName() !== 'mysql', 'Migration can only be executed safely on "mysql".'); + $this->abortIf(!$this->connection->getDatabasePlatform() instanceof MySqlPlatform, 'Migration can only be executed safely on "mysql".'); $this->addSql('ALTER TABLE neos_media_domain_model_assetcollection ADD parent VARCHAR(40) DEFAULT NULL'); $this->addSql('ALTER TABLE neos_media_domain_model_assetcollection ADD CONSTRAINT FK_74C770A3D8E604F FOREIGN KEY (parent) REFERENCES neos_media_domain_model_assetcollection (persistence_object_identifier) ON DELETE SET NULL'); @@ -25,7 +26,7 @@ public function up(Schema $schema): void public function down(Schema $schema): void { - $this->abortIf(!$this->connection->getDatabasePlatform()->getName() !== 'mysql', 'Migration can only be executed safely on "mysql".'); + $this->abortIf(!$this->connection->getDatabasePlatform() instanceof MySqlPlatform, 'Migration can only be executed safely on "mysql".'); $this->addSql('ALTER TABLE neos_media_domain_model_assetcollection DROP FOREIGN KEY FK_74C770A3D8E604F'); $this->addSql('DROP INDEX IDX_74C770A3D8E604F ON neos_media_domain_model_assetcollection'); diff --git a/Neos.Media/Migrations/Postgresql/Version20230401171612.php b/Neos.Media/Migrations/Postgresql/Version20230401171612.php index 2384b5db68b..94584aa31a7 100644 --- a/Neos.Media/Migrations/Postgresql/Version20230401171612.php +++ b/Neos.Media/Migrations/Postgresql/Version20230401171612.php @@ -4,6 +4,7 @@ namespace Neos\Flow\Persistence\Doctrine\Migrations; +use Doctrine\DBAL\Platforms\PostgreSQL100Platform; use Doctrine\DBAL\Schema\Schema; use Doctrine\Migrations\AbstractMigration; @@ -16,7 +17,7 @@ public function getDescription(): string public function up(Schema $schema): void { - $this->abortIf($this->connection->getDatabasePlatform()->getName() !== 'postgresql', 'Migration can only be executed safely on "postgresql".'); + $this->abortIf(!$this->connection->getDatabasePlatform() instanceof PostgreSQL100Platform, 'Migration can only be executed safely on "postgresql".'); $this->addSql('ALTER TABLE neos_media_domain_model_assetcollection ADD parent VARCHAR(40) DEFAULT NULL'); $this->addSql('ALTER TABLE neos_media_domain_model_assetcollection ADD CONSTRAINT FK_74C770A3D8E604F FOREIGN KEY (parent) REFERENCES neos_media_domain_model_assetcollection (persistence_object_identifier) ON DELETE SET NULL NOT DEFERRABLE INITIALLY IMMEDIATE'); @@ -25,7 +26,7 @@ public function up(Schema $schema): void public function down(Schema $schema): void { - $this->abortIf($this->connection->getDatabasePlatform()->getName() !== 'postgresql', 'Migration can only be executed safely on "postgresql".'); + $this->abortIf(!$this->connection->getDatabasePlatform()->getName() instanceof PostgreSQL100Platform, 'Migration can only be executed safely on "postgresql".'); $this->addSql('ALTER TABLE neos_media_domain_model_assetcollection DROP CONSTRAINT FK_74C770A3D8E604F'); $this->addSql('DROP INDEX IDX_74C770A3D8E604F'); From 3551a54069eb415a97f53b29c6d08dc8672eae66 Mon Sep 17 00:00:00 2001 From: Sebastian Helzle Date: Thu, 1 Jun 2023 13:09:32 +0200 Subject: [PATCH 6/7] TASK: Simplify circularHierarchy check and improve errormessage and test --- .../Classes/Domain/Model/AssetCollection.php | 8 +++++--- .../Domain/Model/AssetCollectionTest.php | 15 +++++++++------ 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/Neos.Media/Classes/Domain/Model/AssetCollection.php b/Neos.Media/Classes/Domain/Model/AssetCollection.php index 516f64c9c3d..db36919ae87 100644 --- a/Neos.Media/Classes/Domain/Model/AssetCollection.php +++ b/Neos.Media/Classes/Domain/Model/AssetCollection.php @@ -198,11 +198,13 @@ public function getParent(): ?self private function validateCircularHierarchy(): void { $parent = $this->parent; - $parents = [$parent]; while ($parent !== null) { $parent = $parent->getParent(); - if (in_array($parent, $parents, true)) { - throw new \InvalidArgumentException('Circular reference detected', 1680328041); + if ($parent === $this->parent) { + throw new \InvalidArgumentException(sprintf( + 'Circular reference detected, parent AssetCollection "%s" appeared twice in the hierarchy', + $parent->getTitle() + ), 1680328041); } } } diff --git a/Neos.Media/Tests/Functional/Domain/Model/AssetCollectionTest.php b/Neos.Media/Tests/Functional/Domain/Model/AssetCollectionTest.php index 64da25da4cb..b75cfc167cf 100644 --- a/Neos.Media/Tests/Functional/Domain/Model/AssetCollectionTest.php +++ b/Neos.Media/Tests/Functional/Domain/Model/AssetCollectionTest.php @@ -73,19 +73,22 @@ public function parentChildrenRelation(): void } /** + * Verifies the following hierarchie throws an error: + * first -> second -> third -> first + * * @test */ public function circularParentChildrenRelationThrowsErrorWhenSettingParent(): void { - $child = new AssetCollection('child'); - $childOfChild = new AssetCollection('childOfChild'); - $parent = new AssetCollection('parent'); + $firstCollection = new AssetCollection('first'); + $secondCollection = new AssetCollection('second'); + $thirdCollection = new AssetCollection('third'); - $child->setParent($parent); - $childOfChild->setParent($child); + $secondCollection->setParent($firstCollection); + $thirdCollection->setParent($secondCollection); $this->expectException(\InvalidArgumentException::class); - $parent->setParent($childOfChild); + $firstCollection->setParent($thirdCollection); } /** From 9c3737b013e8334ae0fe6bf0d2a971469014c9a8 Mon Sep 17 00:00:00 2001 From: Sebastian Helzle Date: Thu, 1 Jun 2023 15:54:57 +0200 Subject: [PATCH 7/7] TASK: Add hasParent and unsetParent methods --- .../Classes/Domain/Model/AssetCollection.php | 14 ++++++++-- .../Domain/Model/AssetCollectionTest.php | 27 +++++++++++++++++-- 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/Neos.Media/Classes/Domain/Model/AssetCollection.php b/Neos.Media/Classes/Domain/Model/AssetCollection.php index db36919ae87..4e44c51fa7a 100644 --- a/Neos.Media/Classes/Domain/Model/AssetCollection.php +++ b/Neos.Media/Classes/Domain/Model/AssetCollection.php @@ -200,7 +200,7 @@ private function validateCircularHierarchy(): void $parent = $this->parent; while ($parent !== null) { $parent = $parent->getParent(); - if ($parent === $this->parent) { + if ($parent && $parent === $this->parent) { throw new \InvalidArgumentException(sprintf( 'Circular reference detected, parent AssetCollection "%s" appeared twice in the hierarchy', $parent->getTitle() @@ -209,9 +209,19 @@ private function validateCircularHierarchy(): void } } - public function setParent(?self $parent): void + public function setParent(self $parent): void { $this->parent = $parent; $this->validateCircularHierarchy(); } + + public function unsetParent(): void + { + $this->parent = null; + } + + public function hasParent(): bool + { + return $this->parent !== null; + } } diff --git a/Neos.Media/Tests/Functional/Domain/Model/AssetCollectionTest.php b/Neos.Media/Tests/Functional/Domain/Model/AssetCollectionTest.php index b75cfc167cf..d647c5b6b27 100644 --- a/Neos.Media/Tests/Functional/Domain/Model/AssetCollectionTest.php +++ b/Neos.Media/Tests/Functional/Domain/Model/AssetCollectionTest.php @@ -108,7 +108,7 @@ public function unsettingTheParentRemovesChildFromParent(): void $this->persistenceManager->clearState(); $persistedChild = $this->assetCollectionRepository->findOneByTitle('child'); - $persistedChild->setParent(null); + $persistedChild->unsetParent(null); $this->assetCollectionRepository->update($persistedChild); $this->persistenceManager->persistAll(); @@ -120,7 +120,7 @@ public function unsettingTheParentRemovesChildFromParent(): void $children = $this->assetCollectionRepository->findByParent($persistedParent); self::assertNull($persistedChild->getParent()); - self::assertEquals(0, count($children)); + self::assertCount(0, $children); } /** @@ -151,4 +151,27 @@ public function deletingTheParentDeletesTheChild(): void self::assertNull($persistedChild); self::assertNull($persistedParent); } + + /** + * @test + */ + public function hasParentReturnsTrueIfParentIsSet(): void + { + $child = new AssetCollection('child'); + $parent = new AssetCollection('parent'); + + $child->setParent($parent); + + $this->assetCollectionRepository->add($parent); + $this->assetCollectionRepository->add($child); + + $this->persistenceManager->persistAll(); + $this->persistenceManager->clearState(); + + $persistedChild = $this->assetCollectionRepository->findOneByTitle('child'); + $persistedParent = $this->assetCollectionRepository->findOneByTitle('parent'); + + self::assertTrue($persistedChild->hasParent()); + self::assertFalse($persistedParent->hasParent()); + } }