From ccdf2040c96085838e7044096b5c3ac4ceb91b82 Mon Sep 17 00:00:00 2001 From: Bartek Wajda Date: Thu, 17 Oct 2024 18:50:37 +0200 Subject: [PATCH 1/3] IBX-8418: Remove draft when trashing or deleting its parent or ancestor location --- .../Persistence/Content/Location/Handler.php | 5 + src/contracts/Test/IbexaKernelTestTrait.php | 6 + src/contracts/Test/IbexaTestKernel.php | 1 + src/lib/Persistence/Cache/LocationHandler.php | 14 +++ .../Persistence/Legacy/Content/Handler.php | 1 + .../Legacy/Content/Location/Gateway.php | 7 ++ .../Location/Gateway/DoctrineDatabase.php | 24 ++++ .../Location/Gateway/ExceptionConversion.php | 12 ++ .../Legacy/Content/Location/Handler.php | 5 + .../Legacy/Content/TreeHandler.php | 20 ++++ src/lib/Repository/TrashService.php | 1 + .../ContentService/DeleteContentTest.php | 110 ++++++++++++++++++ .../Persistence/Cache/LocationHandlerTest.php | 1 + .../Legacy/Content/LocationHandlerTest.php | 12 ++ .../Legacy/Content/TreeHandlerTest.php | 55 +++++++++ 15 files changed, 274 insertions(+) create mode 100644 tests/integration/Core/Repository/ContentService/DeleteContentTest.php diff --git a/src/contracts/Persistence/Content/Location/Handler.php b/src/contracts/Persistence/Content/Location/Handler.php index 0fbea26996..5eb76c4c78 100644 --- a/src/contracts/Persistence/Content/Location/Handler.php +++ b/src/contracts/Persistence/Content/Location/Handler.php @@ -215,6 +215,11 @@ public function create(CreateStruct $location); */ public function removeSubtree($locationId); + /** + * Removes all draft contents that have no location assigned to them under the given parent location. + */ + public function deleteChildrenDrafts(int $locationId): void; + /** * Set section on all content objects in the subtree. * Only main locations will be updated. diff --git a/src/contracts/Test/IbexaKernelTestTrait.php b/src/contracts/Test/IbexaKernelTestTrait.php index 6b75669022..c40329634d 100644 --- a/src/contracts/Test/IbexaKernelTestTrait.php +++ b/src/contracts/Test/IbexaKernelTestTrait.php @@ -19,6 +19,7 @@ use Ibexa\Contracts\Core\Repository\RoleService; use Ibexa\Contracts\Core\Repository\SearchService; use Ibexa\Contracts\Core\Repository\SectionService; +use Ibexa\Contracts\Core\Repository\TrashService; use Ibexa\Contracts\Core\Repository\URLAliasService; use Ibexa\Contracts\Core\Repository\UserService; use Ibexa\Contracts\Core\Test\Persistence\Fixture\FixtureImporter; @@ -169,6 +170,11 @@ protected static function getUrlAliasService(): URLAliasService return self::getServiceByClassName(URLAliasService::class); } + protected static function getTrashService(): TrashService + { + return self::getServiceByClassName(TrashService::class); + } + protected static function setAnonymousUser(): void { $anonymousUserId = 10; diff --git a/src/contracts/Test/IbexaTestKernel.php b/src/contracts/Test/IbexaTestKernel.php index ca18fc32a7..5c89768f84 100644 --- a/src/contracts/Test/IbexaTestKernel.php +++ b/src/contracts/Test/IbexaTestKernel.php @@ -92,6 +92,7 @@ class IbexaTestKernel extends Kernel implements IbexaTestKernelInterface Repository\TokenService::class, Repository\URLAliasService::class, Repository\BookmarkService::class, + Repository\TrashService::class, Handler::class, ]; diff --git a/src/lib/Persistence/Cache/LocationHandler.php b/src/lib/Persistence/Cache/LocationHandler.php index dc07fd9f93..74a50a2039 100644 --- a/src/lib/Persistence/Cache/LocationHandler.php +++ b/src/lib/Persistence/Cache/LocationHandler.php @@ -415,6 +415,20 @@ public function removeSubtree($locationId) return $return; } + public function deleteChildrenDrafts(int $locationId): void + { + $this->logger->logCall(__METHOD__, ['location' => $locationId]); + + $this->persistenceHandler->locationHandler()->deleteChildrenDrafts($locationId); + + $this->cache->invalidateTags([ + $this->cacheIdentifierGenerator->generateTag( + self::LOCATION_PATH_IDENTIFIER, + [$locationId], + ), + ]); + } + /** * {@inheritdoc} */ diff --git a/src/lib/Persistence/Legacy/Content/Handler.php b/src/lib/Persistence/Legacy/Content/Handler.php index 56871b0d27..0acf1ab1e0 100644 --- a/src/lib/Persistence/Legacy/Content/Handler.php +++ b/src/lib/Persistence/Legacy/Content/Handler.php @@ -650,6 +650,7 @@ public function deleteContent($contentId) $this->removeRawContent($contentId); } else { foreach ($contentLocations as $locationId) { + $this->treeHandler->deleteChildrenDrafts($locationId); $this->treeHandler->removeSubtree($locationId); } } diff --git a/src/lib/Persistence/Legacy/Content/Location/Gateway.php b/src/lib/Persistence/Legacy/Content/Location/Gateway.php index 57e7838eeb..5218cad951 100644 --- a/src/lib/Persistence/Legacy/Content/Location/Gateway.php +++ b/src/lib/Persistence/Legacy/Content/Location/Gateway.php @@ -111,6 +111,13 @@ abstract public function loadParentLocationsDataForDraftContent(int $contentId): */ abstract public function getSubtreeContent(int $sourceId, bool $onlyIds = false): array; + /** + * Finds draft contents created under the given parent location. + * + * @return array + */ + abstract public function getSubtreeChildrenDraftContentIds(int $sourceId): array; + abstract public function getSubtreeSize(string $path): int; /** diff --git a/src/lib/Persistence/Legacy/Content/Location/Gateway/DoctrineDatabase.php b/src/lib/Persistence/Legacy/Content/Location/Gateway/DoctrineDatabase.php index 8341e96196..e24366bf1c 100644 --- a/src/lib/Persistence/Legacy/Content/Location/Gateway/DoctrineDatabase.php +++ b/src/lib/Persistence/Legacy/Content/Location/Gateway/DoctrineDatabase.php @@ -10,6 +10,7 @@ use Doctrine\DBAL\FetchMode; use Doctrine\DBAL\ParameterType; use Doctrine\DBAL\Query\QueryBuilder; +use Doctrine\DBAL\Result; use Ibexa\Contracts\Core\Persistence\Content\ContentInfo; use Ibexa\Contracts\Core\Persistence\Content\Location; use Ibexa\Contracts\Core\Persistence\Content\Location\CreateStruct; @@ -237,6 +238,29 @@ public function getSubtreeContent(int $sourceId, bool $onlyIds = false): array : $results; } + /** + * @return array + * + * @throws \Doctrine\DBAL\Exception + * @throws \Doctrine\DBAL\Driver\Exception + */ + public function getSubtreeChildrenDraftContentIds(int $sourceId): array + { + $query = $this->connection->createQueryBuilder(); + $query + ->select('contentobject_id') + ->from('eznode_assignment', 'n') + ->innerJoin('n', 'ezcontentobject', 'c', 'n.contentobject_id = c.id') + ->andWhere('n.parent_node = :parentNode') + ->andWhere('c.status = :status') + ->setParameter(':parentNode', $sourceId, ParameterType::INTEGER) + ->setParameter(':status', ContentInfo::STATUS_DRAFT, ParameterType::INTEGER); + + $statement = $query->execute(); + + return $statement->fetchFirstColumn(); + } + public function getSubtreeSize(string $path): int { $query = $this->createNodeQueryBuilder([$this->dbPlatform->getCountExpression('node_id')]); diff --git a/src/lib/Persistence/Legacy/Content/Location/Gateway/ExceptionConversion.php b/src/lib/Persistence/Legacy/Content/Location/Gateway/ExceptionConversion.php index 70fe4b717c..e2d3c5db23 100644 --- a/src/lib/Persistence/Legacy/Content/Location/Gateway/ExceptionConversion.php +++ b/src/lib/Persistence/Legacy/Content/Location/Gateway/ExceptionConversion.php @@ -106,6 +106,18 @@ public function getSubtreeContent(int $sourceId, bool $onlyIds = false): array } } + /** + * @return array + */ + public function getSubtreeChildrenDraftContentIds(int $sourceId): array + { + try { + return $this->innerGateway->getSubtreeChildrenDraftContentIds($sourceId); + } catch (PDOException $e) { + throw DatabaseException::wrap($e); + } + } + public function getSubtreeSize(string $path): int { try { diff --git a/src/lib/Persistence/Legacy/Content/Location/Handler.php b/src/lib/Persistence/Legacy/Content/Location/Handler.php index 758e221c4a..8ce6a52493 100644 --- a/src/lib/Persistence/Legacy/Content/Location/Handler.php +++ b/src/lib/Persistence/Legacy/Content/Location/Handler.php @@ -546,6 +546,11 @@ public function removeSubtree($locationId) $this->treeHandler->removeSubtree($locationId); } + public function deleteChildrenDrafts(int $locationId): void + { + $this->treeHandler->deleteChildrenDrafts($locationId); + } + /** * Set section on all content objects in the subtree. * diff --git a/src/lib/Persistence/Legacy/Content/TreeHandler.php b/src/lib/Persistence/Legacy/Content/TreeHandler.php index 700b067e94..629326589b 100644 --- a/src/lib/Persistence/Legacy/Content/TreeHandler.php +++ b/src/lib/Persistence/Legacy/Content/TreeHandler.php @@ -215,6 +215,26 @@ public function removeSubtree($locationId) $this->locationGateway->deleteNodeAssignment($contentId); } + /** + * Removes draft contents assigned to the given parent location and its descendant locations. + * + * @throws \Ibexa\Contracts\Core\Repository\Exceptions\NotFoundException + */ + public function deleteChildrenDrafts(int $locationId): void + { + $subLocations = $this->locationGateway->getChildren($locationId); + foreach ($subLocations as $subLocation) { + $this->deleteChildrenDrafts($subLocation['node_id']); + } + + // Fetch child draft content ids + $subtreeChildrenDraftIds = $this->locationGateway->getSubtreeChildrenDraftContentIds($locationId); + + foreach ($subtreeChildrenDraftIds as $contentId) { + $this->removeRawContent($contentId); + } + } + /** * Set section on all content objects in the subtree. * diff --git a/src/lib/Repository/TrashService.php b/src/lib/Repository/TrashService.php index e32a0cea27..3ab569d18a 100644 --- a/src/lib/Repository/TrashService.php +++ b/src/lib/Repository/TrashService.php @@ -146,6 +146,7 @@ public function trash(Location $location): ?APITrashItem $this->repository->beginTransaction(); try { + $this->persistenceHandler->locationHandler()->deleteChildrenDrafts($location->id); $spiTrashItem = $this->persistenceHandler->trashHandler()->trashSubtree($location->id); $this->persistenceHandler->urlAliasHandler()->locationDeleted($location->id); $this->repository->commit(); diff --git a/tests/integration/Core/Repository/ContentService/DeleteContentTest.php b/tests/integration/Core/Repository/ContentService/DeleteContentTest.php new file mode 100644 index 0000000000..aaab37b91c --- /dev/null +++ b/tests/integration/Core/Repository/ContentService/DeleteContentTest.php @@ -0,0 +1,110 @@ +prepareContentStructure(); + + $contentService->deleteContent($folder->getContentInfo()); + + $contentInfos = $contentService->loadContentInfoList([ + $draft1->getId(), + $draft2->getId(), + $draft3->getId(), + $draftSecondDepth->getId(), + ]); + + self::assertEmpty($contentInfos); + } + + /** + * @throws \Ibexa\Contracts\Core\Repository\Exceptions\Exception + */ + public function testTrashLocationDeletesChildrenDrafts(): void + { + $trashService = self::getTrashService(); + $contentService = self::getContentService(); + + [$folder, $draft1, $draft2, $draft3, $draftSecondDepth] = $this->prepareContentStructure(); + + $folderMainLocationId = $folder->getVersionInfo()->getContentInfo()->getMainLocationId(); + Assert::assertIsNumeric($folderMainLocationId); + + $locationToTrash = self::getLocationService()->loadLocation($folderMainLocationId); + + $trashService->trash($locationToTrash); + + $contentInfos = $contentService->loadContentInfoList([ + $draft1->getId(), + $draft2->getId(), + $draft3->getId(), + $draftSecondDepth->getId(), + ]); + + self::assertEmpty($contentInfos); + } + + /** + * @return array + */ + private function prepareContentStructure(): array + { + $folder = $this->createFolder(['eng-GB' => 'Folder'], 2); + $folderMainLocationId = $folder->getVersionInfo()->getContentInfo()->getMainLocationId(); + Assert::assertIsNumeric($folderMainLocationId); + + $childFolder = $this->createFolder( + ['eng-GB' => 'Child folder'], + $folderMainLocationId, + ); + $childFolderMainLocationId = $childFolder->getVersionInfo()->getContentInfo()->getMainLocationId(); + Assert::assertIsNumeric($childFolderMainLocationId); + + $secondDepthChildFolder = $this->createFolder( + ['eng-GB' => 'Second depth folder'], + $childFolderMainLocationId, + ); + $secondDepthChildFolderLocationId = $secondDepthChildFolder + ->getVersionInfo() + ->getContentInfo() + ->getMainLocationId() + ; + Assert::assertIsNumeric($secondDepthChildFolderLocationId); + + $draft1 = $this->createFolderDraft(['eng-GB' => 'Folder draft 1'], $folderMainLocationId); + $draft2 = $this->createFolderDraft(['eng-GB' => 'Folder draft 2'], $childFolderMainLocationId); + $draft3 = $this->createFolderDraft(['eng-GB' => 'Folder draft 3'], $childFolderMainLocationId); + $draftSecondDepth = $this->createFolderDraft( + ['eng-GB' => 'Folder draft 4'], + $secondDepthChildFolderLocationId, + ); + + return [ + $folder, + $draft1, + $draft2, + $draft3, + $draftSecondDepth, + ]; + } +} diff --git a/tests/lib/Persistence/Cache/LocationHandlerTest.php b/tests/lib/Persistence/Cache/LocationHandlerTest.php index ed498aa495..c06f71b276 100644 --- a/tests/lib/Persistence/Cache/LocationHandlerTest.php +++ b/tests/lib/Persistence/Cache/LocationHandlerTest.php @@ -67,6 +67,7 @@ public function providerForUnCachedMethods(): array ['c-4', 'ragl-4'], ], ['removeSubtree', [12], [['location_path', [12], false]], null, ['lp-12']], + ['deleteChildrenDrafts', [12], [['location_path', [12], false]], null, ['lp-12']], ['setSectionForSubtree', [12, 2], [['location_path', [12], false]], null, ['lp-12']], ['changeMainLocation', [4, 12], [['content', [4], false]], null, ['c-4']], ['countLocationsByContent', [4]], diff --git a/tests/lib/Persistence/Legacy/Content/LocationHandlerTest.php b/tests/lib/Persistence/Legacy/Content/LocationHandlerTest.php index 6b971543cc..b22dd45d60 100644 --- a/tests/lib/Persistence/Legacy/Content/LocationHandlerTest.php +++ b/tests/lib/Persistence/Legacy/Content/LocationHandlerTest.php @@ -453,6 +453,18 @@ public function testRemoveSubtree() $handler->removeSubtree(42); } + public function testDeleteChildrenDrafts(): void + { + $handler = $this->getLocationHandler(); + + $this->treeHandler + ->expects(self::once()) + ->method('deleteChildrenDrafts') + ->with(42); + + $handler->deleteChildrenDrafts(42); + } + /** * Test for the copySubtree() method. */ diff --git a/tests/lib/Persistence/Legacy/Content/TreeHandlerTest.php b/tests/lib/Persistence/Legacy/Content/TreeHandlerTest.php index a3403d8fb8..9daf814d36 100644 --- a/tests/lib/Persistence/Legacy/Content/TreeHandlerTest.php +++ b/tests/lib/Persistence/Legacy/Content/TreeHandlerTest.php @@ -400,6 +400,61 @@ public function testLoadLocation() $this->assertTrue($location instanceof Location); } + public function testDeleteChildrenDraftsRecursive(): void + { + $locationGatewayMock = $this->getLocationGatewayMock(); + $contentGatewayMock = $this->getContentGatewayMock(); + $contentMapperMock = $this->getContentMapperMock(); + + $locationGatewayMock + ->expects(self::exactly(3)) + ->method('getChildren') + ->willReturnMap([ + [42, [ + ['node_id' => 201], + ['node_id' => 202], + ]], + [201, []], + [202, []], + ]); + + $locationGatewayMock + ->expects(self::exactly(3)) + ->method('getSubtreeChildrenDraftContentIds') + ->willReturnMap([ + [201, [101]], + [202, [102]], + [42, [99]], + ]); + + $contentGatewayMock + ->expects(self::exactly(3)) + ->method('loadContentInfo') + ->willReturnMap([ + [101, ['main_node_id' => 201]], + [102, ['main_node_id' => 202]], + [99, ['main_node_id' => 42]], + ]); + + $contentMapperMock + ->expects(self::exactly(3)) + ->method('extractContentInfoFromRow') + ->willReturnCallback(static function (array $row): ContentInfo { + return new ContentInfo(['mainLocationId' => $row['main_node_id']]); + }); + + $contentGatewayMock + ->expects(self::exactly(3)) + ->method('deleteContent') + ->willReturnCallback(static function (int $contentId): void { + self::assertContains($contentId, [99, 101, 102]); + }); + + $treeHandler = $this->getTreeHandler(); + + $treeHandler->deleteChildrenDrafts(42); + } + /** @var \PHPUnit\Framework\MockObject\MockObject|\Ibexa\Core\Persistence\Legacy\Content\Location\Gateway */ protected $locationGatewayMock; From e55cc6b4bff0cdd7b0bbe209fe0c6ae1fd80166f Mon Sep 17 00:00:00 2001 From: Bartek Wajda Date: Thu, 17 Oct 2024 18:58:55 +0200 Subject: [PATCH 2/3] IBX-8418: CS --- .../Legacy/Content/Location/Gateway/DoctrineDatabase.php | 1 - .../Core/Repository/ContentService/DeleteContentTest.php | 1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/Persistence/Legacy/Content/Location/Gateway/DoctrineDatabase.php b/src/lib/Persistence/Legacy/Content/Location/Gateway/DoctrineDatabase.php index e24366bf1c..bfd06ecb2a 100644 --- a/src/lib/Persistence/Legacy/Content/Location/Gateway/DoctrineDatabase.php +++ b/src/lib/Persistence/Legacy/Content/Location/Gateway/DoctrineDatabase.php @@ -10,7 +10,6 @@ use Doctrine\DBAL\FetchMode; use Doctrine\DBAL\ParameterType; use Doctrine\DBAL\Query\QueryBuilder; -use Doctrine\DBAL\Result; use Ibexa\Contracts\Core\Persistence\Content\ContentInfo; use Ibexa\Contracts\Core\Persistence\Content\Location; use Ibexa\Contracts\Core\Persistence\Content\Location\CreateStruct; diff --git a/tests/integration/Core/Repository/ContentService/DeleteContentTest.php b/tests/integration/Core/Repository/ContentService/DeleteContentTest.php index aaab37b91c..e1dd1faf7a 100644 --- a/tests/integration/Core/Repository/ContentService/DeleteContentTest.php +++ b/tests/integration/Core/Repository/ContentService/DeleteContentTest.php @@ -8,6 +8,7 @@ namespace Ibexa\Tests\Integration\Core\Repository\ContentService; +use Ibexa\Contracts\Core\Repository\Values\Content\Content; use Ibexa\Tests\Integration\Core\RepositoryTestCase; use PHPUnit\Framework\Assert; From 4db9078768f71a61577e5683ddc8f5d039e26eb7 Mon Sep 17 00:00:00 2001 From: Bartek Wajda Date: Fri, 8 Nov 2024 15:10:53 +0100 Subject: [PATCH 3/3] IBX-8418: PHPStan --- phpstan-baseline.neon | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 7bfaaed552..668d9e640a 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -19010,11 +19010,6 @@ parameters: count: 1 path: src/lib/Persistence/Legacy/User/Mapper.php - - - message: "#^array\\|string does not accept array\\\\.$#" - count: 1 - path: src/lib/Persistence/Legacy/User/Mapper.php - - message: "#^Method Ibexa\\\\Core\\\\Persistence\\\\Legacy\\\\User\\\\Role\\\\Gateway\\:\\:addPolicyLimitations\\(\\) has parameter \\$limitations with no value type specified in iterable type array\\.$#" count: 1 @@ -27090,23 +27085,18 @@ parameters: count: 5 path: tests/integration/Core/Repository/ContentServiceTest.php - - - message: "#^Cannot access offset 0 on iterable\\\\.$#" - count: 12 - path: tests/integration/Core/Repository/ContentServiceTest.php - - message: "#^Cannot access offset 0 on iterable\\\\.$#" count: 7 path: tests/integration/Core/Repository/ContentServiceTest.php - - message: "#^Cannot access offset 0 on iterable\\\\.$#" - count: 4 + message: "#^Cannot access offset 0 on iterable\\\\.$#" + count: 12 path: tests/integration/Core/Repository/ContentServiceTest.php - - message: "#^Cannot access offset 1 on iterable\\\\.$#" + message: "#^Cannot access offset 0 on iterable\\\\.$#" count: 4 path: tests/integration/Core/Repository/ContentServiceTest.php @@ -27115,6 +27105,11 @@ parameters: count: 2 path: tests/integration/Core/Repository/ContentServiceTest.php + - + message: "#^Cannot access offset 1 on iterable\\\\.$#" + count: 4 + path: tests/integration/Core/Repository/ContentServiceTest.php + - message: "#^Cannot access offset 1 on iterable\\\\.$#" count: 1 @@ -27141,12 +27136,12 @@ parameters: path: tests/integration/Core/Repository/ContentServiceTest.php - - message: "#^Cannot access offset mixed on iterable\\\\.$#" + message: "#^Cannot access offset int on iterable\\\\.$#" count: 1 path: tests/integration/Core/Repository/ContentServiceTest.php - - message: "#^Cannot access offset int on iterable\\\\.$#" + message: "#^Cannot access offset mixed on iterable\\\\.$#" count: 1 path: tests/integration/Core/Repository/ContentServiceTest.php