From 08c15f998d6b30d5693f9590da25ac20ae5f83fd Mon Sep 17 00:00:00 2001 From: Vidar Langseid Date: Wed, 16 Sep 2026 09:56:08 +0200 Subject: [PATCH 1/3] IBX-6773: Fixed loading Bookmarks for non-accessible content items (#476) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For more details see https://ibexa.atlassian.net/browse/IBX-6773 and https://github.com/ibexa/core/pull/476 Key changes: * Fixed `BookmarkService::loadBookmarks()` throwing an exception when the bookmark list contained locations the current user no longer has access to, by resolving bookmarks through filtering instead of `LocationService::loadLocation()`. * Extended `Criterion\Location\IsBookmarked` to support filtering via both `LocationService::find()`/`count()` and `ContentService::find()` (matching main Locations only for Content filtering). * Added `SortClause\Location\Bookmark\Id` and its query builders to allow sorting filtered bookmark results by recency. * Deprecated `Handler::loadUserBookmarks()`, `Handler::countUserBookmarks()` and their Gateway counterparts in favor of `LocationService::find()`/`count()` with `Criterion\Location\IsBookmarked`. * Made `BookmarkService` log and return an empty list instead of failing when the underlying filter query throws a repository exception. --------- Co-Authored-By: Paweł Niedzielski <3183926+Steveb-p@users.noreply.github.com> Co-Authored-By: Andrew Longosz <7099219+alongosz@users.noreply.github.com> --- phpstan-baseline.neon | 12 - .../Persistence/Bookmark/Handler.php | 4 + .../Query/Criterion/Location/IsBookmarked.php | 2 + .../Query/SortClause/Location/Bookmark/Id.php | 24 ++ .../Persistence/Legacy/Bookmark/Gateway.php | 4 + .../Location/IsBookmarkedQueryBuilder.php | 81 +++++++ .../BaseLocationSortClauseQueryBuilder.php | 19 +- .../Bookmark/IdSortClauseQueryBuilder.php | 95 ++++++++ src/lib/Repository/BookmarkService.php | 51 ++-- src/lib/Repository/Repository.php | 3 +- .../Core/Repository/BookmarkServiceTest.php | 217 ++++++++++++++++-- .../Filtering/ContentFilteringTest.php | 63 +++++ .../Filtering/LocationFilteringTest.php | 128 +++++++++++ .../Core/Repository/LocationServiceTest.php | 29 ++- .../Location/IsBookmarkedQueryBuilderTest.php | 72 ++++++ .../Bookmark/IdSortClauseQueryBuilderTest.php | 215 +++++++++++++++++ .../Repository/Service/Mock/BookmarkTest.php | 53 +---- 17 files changed, 967 insertions(+), 105 deletions(-) create mode 100644 src/contracts/Repository/Values/Content/Query/SortClause/Location/Bookmark/Id.php create mode 100644 src/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/IsBookmarkedQueryBuilder.php create mode 100644 src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/Bookmark/IdSortClauseQueryBuilder.php create mode 100644 tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/IsBookmarkedQueryBuilderTest.php create mode 100644 tests/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/Bookmark/IdSortClauseQueryBuilderTest.php diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 974a28354b..646f9a81c2 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -32190,12 +32190,6 @@ parameters: count: 1 path: tests/integration/Core/Repository/BaseURLServiceTest.php - - - message: '#^Call to method PHPUnit\\Framework\\Assert\:\:assertInstanceOf\(\) with ''Ibexa\\\\Contracts\\\\Core\\\\Repository\\\\Values\\\\Bookmark\\\\BookmarkList'' and Ibexa\\Contracts\\Core\\Repository\\Values\\Bookmark\\BookmarkList will always evaluate to true\.$#' - identifier: method.alreadyNarrowedType - count: 1 - path: tests/integration/Core/Repository/BookmarkServiceTest.php - - message: '#^Method Ibexa\\Tests\\Integration\\Core\\Repository\\BookmarkServiceTest\:\:testCreateBookmark\(\) has no return type specified\.$#' identifier: missingType.return @@ -69469,12 +69463,6 @@ parameters: count: 1 path: tests/lib/Repository/Service/Mock/BookmarkTest.php - - - message: '#^Method Ibexa\\Tests\\Core\\Repository\\Service\\Mock\\BookmarkTest\:\:testLoadBookmarksEmptyList\(\) has no return type specified\.$#' - identifier: missingType.return - count: 1 - path: tests/lib/Repository/Service/Mock/BookmarkTest.php - - message: '#^Method Ibexa\\Tests\\Core\\Repository\\Service\\Mock\\BookmarkTest\:\:testLocationShouldBeBookmarked\(\) has no return type specified\.$#' identifier: missingType.return diff --git a/src/contracts/Persistence/Bookmark/Handler.php b/src/contracts/Persistence/Bookmark/Handler.php index f9ef1a1cae..389de6069d 100644 --- a/src/contracts/Persistence/Bookmark/Handler.php +++ b/src/contracts/Persistence/Bookmark/Handler.php @@ -50,6 +50,8 @@ public function loadUserIdsByLocation(Location $location): array; /** * Loads bookmarks owned by user. * + * @deprecated 4.6.33 The "Handler::loadUserBookmarks()" method is deprecated, will be removed in 6.0.0. Use "LocationService::find()" and "Criterion\Location\IsBookmarked" instead. + * * @param int $userId * @param int $offset the start offset for paging * @param int $limit the number of bookmarked locations returned @@ -61,6 +63,8 @@ public function loadUserBookmarks(int $userId, int $offset = 0, int $limit = -1) /** * Count bookmarks owned by user. * + * @deprecated 4.6.33 The "Handler::countUserBookmarks()" method is deprecated, will be removed in 6.0.0. Use "LocationService::count()" and "Criterion\Location\IsBookmarked" instead. + * * @param int $userId * * @return int diff --git a/src/contracts/Repository/Values/Content/Query/Criterion/Location/IsBookmarked.php b/src/contracts/Repository/Values/Content/Query/Criterion/Location/IsBookmarked.php index 36d7972270..af06a4bbc8 100644 --- a/src/contracts/Repository/Values/Content/Query/Criterion/Location/IsBookmarked.php +++ b/src/contracts/Repository/Values/Content/Query/Criterion/Location/IsBookmarked.php @@ -15,6 +15,8 @@ /** * This criterion only works for current user reference. + * + * When used with Content filtering, it matches bookmarks placed on main Locations only. */ final class IsBookmarked extends Location implements FilteringCriterion { diff --git a/src/contracts/Repository/Values/Content/Query/SortClause/Location/Bookmark/Id.php b/src/contracts/Repository/Values/Content/Query/SortClause/Location/Bookmark/Id.php new file mode 100644 index 0000000000..29cb02d232 --- /dev/null +++ b/src/contracts/Repository/Values/Content/Query/SortClause/Location/Bookmark/Id.php @@ -0,0 +1,24 @@ +permissionResolver = $permissionResolver; + } + + public function accepts(FilteringCriterion $criterion): bool + { + return $criterion instanceof IsBookmarked; + } + + /** + * @param \Ibexa\Contracts\Core\Repository\Values\Content\Query\Criterion\Location\IsBookmarked $criterion + * + * @throws \Ibexa\Contracts\Core\Repository\Exceptions\InvalidArgumentException + */ + public function buildQueryConstraint( + FilteringQueryBuilder $queryBuilder, + FilteringCriterion $criterion + ): string { + parent::buildQueryConstraint($queryBuilder, $criterion); + + $isBookmarked = $criterion->value[0] ?? null; + if (!is_bool($isBookmarked)) { + throw new InvalidArgumentException( + '$criterion', + 'IsBookmarked criterion value must be boolean at index 0.' + ); + } + + $userId = $this->permissionResolver->getCurrentUserReference()->getUserId(); + + $subQueryBuilder = new QueryBuilder($queryBuilder->getConnection()); + $subQueryBuilder + ->select('1') + ->from(DoctrineDatabase::TABLE_BOOKMARKS, self::ALIAS) + ->where( + $subQueryBuilder->expr()->eq( + self::ALIAS . '.' . DoctrineDatabase::COLUMN_USER_ID, + $queryBuilder->createNamedParameter($userId, ParameterType::INTEGER) + ), + $subQueryBuilder->expr()->eq( + self::ALIAS . '.' . DoctrineDatabase::COLUMN_LOCATION_ID, + 'location.node_id' + ) + ); + + return sprintf( + $isBookmarked ? 'EXISTS (%s)' : 'NOT EXISTS (%s)', + $subQueryBuilder->getSQL() + ); + } +} diff --git a/src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/BaseLocationSortClauseQueryBuilder.php b/src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/BaseLocationSortClauseQueryBuilder.php index 2932835436..f9572a8e8d 100644 --- a/src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/BaseLocationSortClauseQueryBuilder.php +++ b/src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/BaseLocationSortClauseQueryBuilder.php @@ -28,18 +28,29 @@ public function buildQuery( $locationContext = $this->prepareLocationContext($queryBuilder); $locationAlias = $locationContext['alias']; - $sort = $this->getSortingExpressionForAlias($locationAlias); - $sortAlias = $this->getSortFieldAlias($sort); - $queryBuilder->addSelect(sprintf('%s AS %s', $sort, $sortAlias)); - if ($locationContext['needsMainLocationJoin']) { $this->joinMainLocationOnly($queryBuilder, $locationAlias); } + $this->joinAdditionalTables($queryBuilder, $locationAlias); + + $sort = $this->getSortingExpressionForAlias($locationAlias); + $sortAlias = $this->getSortFieldAlias($sort); + $queryBuilder->addSelect(sprintf('%s AS %s', $sort, $sortAlias)); + /** @var \Ibexa\Contracts\Core\Repository\Values\Content\Query\SortClause $sortClause */ $queryBuilder->addOrderBy($sortAlias, $sortClause->direction); } + /** + * Hook for sort clauses which need to join further tables against the resolved Location alias. + */ + protected function joinAdditionalTables( + FilteringQueryBuilder $queryBuilder, + string $locationAlias + ): void { + } + /** * @return array{alias: string, needsMainLocationJoin: bool} */ diff --git a/src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/Bookmark/IdSortClauseQueryBuilder.php b/src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/Bookmark/IdSortClauseQueryBuilder.php new file mode 100644 index 0000000000..b3d2f57822 --- /dev/null +++ b/src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/Bookmark/IdSortClauseQueryBuilder.php @@ -0,0 +1,95 @@ +permissionResolver = $permissionResolver; + } + + public function accepts(FilteringSortClause $sortClause): bool + { + return $sortClause instanceof Id; + } + + /** + * @throws \Ibexa\Contracts\Core\Repository\Exceptions\InvalidArgumentException + */ + public function buildQuery( + FilteringQueryBuilder $queryBuilder, + FilteringSortClause $sortClause + ): void { + if (!$sortClause instanceof Id) { + throw new InvalidArgumentException( + '$sortClause', + sprintf('Expected %s, got %s', Id::class, get_class($sortClause)) + ); + } + + parent::buildQuery($queryBuilder, $sortClause); + } + + protected function joinAdditionalTables( + FilteringQueryBuilder $queryBuilder, + string $locationAlias + ): void { + $userId = $this->permissionResolver->getCurrentUserReference()->getUserId(); + + $queryBuilder->leftJoinOnce( + $locationAlias, + DoctrineDatabase::TABLE_BOOKMARKS, + self::ALIAS, + (string)$queryBuilder->expr()->and( + sprintf( + '%s.node_id = %s.%s', + $locationAlias, + self::ALIAS, + DoctrineDatabase::COLUMN_LOCATION_ID + ), + $queryBuilder->expr()->eq( + sprintf('%s.%s', self::ALIAS, DoctrineDatabase::COLUMN_USER_ID), + $queryBuilder->createNamedParameter($userId, ParameterType::INTEGER) + ) + ) + ); + } + + protected function getSortingExpression(): string + { + return self::ALIAS . '.' . DoctrineDatabase::COLUMN_ID; + } + + protected function getSortingExpressionForAlias(string $locationAlias): string + { + return $this->getSortingExpression(); + } + + protected function getSortFieldName(string $sortExpression): string + { + return 'bookmark_id'; + } +} diff --git a/src/lib/Repository/BookmarkService.php b/src/lib/Repository/BookmarkService.php index a74c79d5cb..e838766509 100644 --- a/src/lib/Repository/BookmarkService.php +++ b/src/lib/Repository/BookmarkService.php @@ -9,33 +9,34 @@ namespace Ibexa\Core\Repository; use Exception; -use Ibexa\Contracts\Core\Persistence\Bookmark\Bookmark; use Ibexa\Contracts\Core\Persistence\Bookmark\CreateStruct; use Ibexa\Contracts\Core\Persistence\Bookmark\Handler as BookmarkHandler; use Ibexa\Contracts\Core\Repository\BookmarkService as BookmarkServiceInterface; +use Ibexa\Contracts\Core\Repository\Exceptions\Exception as RepositoryException; use Ibexa\Contracts\Core\Repository\Repository as RepositoryInterface; use Ibexa\Contracts\Core\Repository\Values\Bookmark\BookmarkList; use Ibexa\Contracts\Core\Repository\Values\Content\Location; +use Ibexa\Contracts\Core\Repository\Values\Content\Query; +use Ibexa\Contracts\Core\Repository\Values\Content\Query\Criterion; +use Ibexa\Contracts\Core\Repository\Values\Content\Query\SortClause; +use Ibexa\Contracts\Core\Repository\Values\Filter\Filter; use Ibexa\Core\Base\Exceptions\InvalidArgumentException; +use Psr\Log\LoggerInterface; +use Psr\Log\NullLogger; class BookmarkService implements BookmarkServiceInterface { - /** @var \Ibexa\Contracts\Core\Repository\Repository */ - protected $repository; + protected RepositoryInterface $repository; - /** @var \Ibexa\Contracts\Core\Persistence\Bookmark\Handler */ - protected $bookmarkHandler; + protected BookmarkHandler $bookmarkHandler; - /** - * BookmarkService constructor. - * - * @param \Ibexa\Contracts\Core\Repository\Repository $repository - * @param \Ibexa\Contracts\Core\Persistence\Bookmark\Handler $bookmarkHandler - */ - public function __construct(RepositoryInterface $repository, BookmarkHandler $bookmarkHandler) + private LoggerInterface $logger; + + public function __construct(RepositoryInterface $repository, BookmarkHandler $bookmarkHandler, ?LoggerInterface $logger = null) { $this->repository = $repository; $this->bookmarkHandler = $bookmarkHandler; + $this->logger = $logger ?? new NullLogger(); } /** @@ -95,17 +96,25 @@ public function deleteBookmark(Location $location): void */ public function loadBookmarks(int $offset = 0, int $limit = 25): BookmarkList { - $currentUserId = $this->getCurrentUserId(); + $filter = new Filter(); + try { + $filter + ->withCriterion(new Criterion\Location\IsBookmarked()) + ->withSortClause(new SortClause\Location\Bookmark\Id(Query::SORT_DESC)) + ->sliceBy($limit, $offset); + + $result = $this->repository->getLocationService()->find($filter, []); + } catch (RepositoryException $e) { + $this->logger->error($e->getMessage(), [ + 'exception' => $e, + ]); + + return new BookmarkList(); + } $list = new BookmarkList(); - $list->totalCount = $this->bookmarkHandler->countUserBookmarks($currentUserId); - if ($list->totalCount > 0) { - $bookmarks = $this->bookmarkHandler->loadUserBookmarks($currentUserId, $offset, $limit); - - $list->items = array_map(function (Bookmark $bookmark) { - return $this->repository->getLocationService()->loadLocation($bookmark->locationId); - }, $bookmarks); - } + $list->totalCount = $result->totalCount; + $list->items = iterator_to_array($result->getIterator()); return $list; } diff --git a/src/lib/Repository/Repository.php b/src/lib/Repository/Repository.php index fa165c2222..81fccd268d 100644 --- a/src/lib/Repository/Repository.php +++ b/src/lib/Repository/Repository.php @@ -608,7 +608,8 @@ public function getBookmarkService(): BookmarkServiceInterface if ($this->bookmarkService === null) { $this->bookmarkService = new BookmarkService( $this, - $this->persistenceHandler->bookmarkHandler() + $this->persistenceHandler->bookmarkHandler(), + $this->logger ); } diff --git a/tests/integration/Core/Repository/BookmarkServiceTest.php b/tests/integration/Core/Repository/BookmarkServiceTest.php index b4d1542d59..cfb194935a 100644 --- a/tests/integration/Core/Repository/BookmarkServiceTest.php +++ b/tests/integration/Core/Repository/BookmarkServiceTest.php @@ -8,8 +8,15 @@ namespace Ibexa\Tests\Integration\Core\Repository; +use Doctrine\DBAL\Connection; +use Doctrine\DBAL\ParameterType; use Ibexa\Contracts\Core\Repository\Exceptions\InvalidArgumentException; -use Ibexa\Contracts\Core\Repository\Values\Bookmark\BookmarkList; +use Ibexa\Contracts\Core\Repository\Values\Content\Content; +use Ibexa\Contracts\Core\Repository\Values\Content\Location; +use Ibexa\Contracts\Core\Repository\Values\Content\Query\Criterion; +use Ibexa\Contracts\Core\Repository\Values\Filter\Filter; +use Ibexa\Contracts\Core\Repository\Values\User\Limitation\SectionLimitation; +use Ibexa\Core\Persistence\Legacy\Bookmark\Gateway\DoctrineDatabase; /** * Test case for the BookmarkService. @@ -21,9 +28,6 @@ class BookmarkServiceTest extends BaseTest public const LOCATION_ID_BOOKMARKED = 5; public const LOCATION_ID_NOT_BOOKMARKED = 44; - /** - * @covers \Ibexa\Contracts\Core\Repository\BookmarkService::isBookmarked - */ public function testIsBookmarked() { $repository = $this->getRepository(); @@ -36,9 +40,6 @@ public function testIsBookmarked() $this->assertTrue($isBookmarked); } - /** - * @covers \Ibexa\Contracts\Core\Repository\BookmarkService::isBookmarked - */ public function testIsNotBookmarked() { $repository = $this->getRepository(); @@ -51,9 +52,6 @@ public function testIsNotBookmarked() $this->assertFalse($isBookmarked); } - /** - * @covers \Ibexa\Contracts\Core\Repository\BookmarkService::createBookmark - */ public function testCreateBookmark() { $repository = $this->getRepository(); @@ -73,7 +71,6 @@ public function testCreateBookmark() } /** - * @covers \Ibexa\Contracts\Core\Repository\BookmarkService::createBookmark * @depends testCreateBookmark */ public function testCreateBookmarkThrowsInvalidArgumentException() @@ -91,9 +88,6 @@ public function testCreateBookmarkThrowsInvalidArgumentException() /* END: Use Case */ } - /** - * @covers \Ibexa\Contracts\Core\Repository\BookmarkService::deleteBookmark - */ public function testDeleteBookmark() { $repository = $this->getRepository(); @@ -114,7 +108,6 @@ public function testDeleteBookmark() } /** - * @covers \Ibexa\Contracts\Core\Repository\BookmarkService::deleteBookmark * @depends testDeleteBookmark */ public function testDeleteBookmarkThrowsInvalidArgumentException() @@ -132,9 +125,6 @@ public function testDeleteBookmarkThrowsInvalidArgumentException() /* END: Use Case */ } - /** - * @covers \Ibexa\Contracts\Core\Repository\BookmarkService::loadBookmarks - */ public function testLoadBookmarks() { $repository = $this->getRepository(); @@ -143,13 +133,198 @@ public function testLoadBookmarks() $bookmarks = $repository->getBookmarkService()->loadBookmarks(1, 3); /* END: Use Case */ - $this->assertInstanceOf(BookmarkList::class, $bookmarks); - $this->assertEquals($bookmarks->totalCount, 5); + self::assertEquals(5, $bookmarks->totalCount); // Assert bookmarks order: recently added should be first - $this->assertEquals([15, 13, 12], array_map(static function ($location) { + self::assertEquals([15, 13, 12], array_map(static function ($location) { return $location->id; }, $bookmarks->items)); } + + public function testCountBookmarks(): void + { + $repository = $this->getRepository(); + + $filter = new Filter(); + $filter + ->withCriterion(new Criterion\Location\IsBookmarked(true)); + $count = $repository->getLocationService()->count($filter, []); + + self::assertEquals(5, $count); + } + + /** + * Regression test for IBX-6773: bookmarking an item and then losing read access to it used to + * make the whole bookmark list explode with an UnauthorizedException, because every bookmark + * was resolved through LocationService::loadLocation(). The item must simply be filtered out + * instead. + */ + public function testLoadBookmarksSkipsBookmarksUserLostAccessTo(): void + { + $repository = $this->getRepository(); + $sectionService = $repository->getSectionService(); + $permissionResolver = $repository->getPermissionResolver(); + $bookmarkService = $repository->getBookmarkService(); + + $administratorUser = $permissionResolver->getCurrentUserReference(); + + // A section the restricted user will *not* be allowed to read + $sectionCreateStruct = $sectionService->newSectionCreateStruct(); + $sectionCreateStruct->name = 'Restricted'; + $sectionCreateStruct->identifier = 'restricted_bookmarks'; + $restrictedSection = $sectionService->createSection($sectionCreateStruct); + + // Created as administrator, so it lands in the Standard section (ID 1) + $folder = $this->createFolder(['eng-GB' => 'Bookmarked folder'], 2); + $folderLocationId = $folder->getContentInfo()->getMainLocationId(); + self::assertNotNull($folderLocationId); + + // User may only read content in the Standard section + $user = $this->createUserWithPolicies( + 'bookmark_section_limited', + [ + [ + 'module' => 'content', + 'function' => 'read', + 'limitations' => [new SectionLimitation(['limitationValues' => [1]])], + ], + ] + ); + + $permissionResolver->setCurrentUserReference($user); + + $bookmarkService->createBookmark( + $repository->getLocationService()->loadLocation($folderLocationId) + ); + + // Sanity check: while readable, the bookmark shows up + $bookmarks = $bookmarkService->loadBookmarks(); + self::assertSame(1, $bookmarks->totalCount); + self::assertSame( + [$folderLocationId], + array_map( + static function (Location $location): int { + return $location->getId(); + }, + $bookmarks->items + ) + ); + + // Move the bookmarked item out of reach of the user + $permissionResolver->setCurrentUserReference($administratorUser); + $sectionService->assignSection($folder->getContentInfo(), $restrictedSection); + + $permissionResolver->setCurrentUserReference($user); + + // Used to throw UnauthorizedException + $bookmarksAfterLosingAccess = $bookmarkService->loadBookmarks(); + + self::assertSame( + 0, + $bookmarksAfterLosingAccess->totalCount, + 'Bookmark of a no longer readable item should not be counted' + ); + self::assertSame( + [], + $bookmarksAfterLosingAccess->items, + 'Bookmark of a no longer readable item should not be listed' + ); + } + + public function testLoadBookmarksAfterTrashingBookmarkedLocation(): void + { + $repository = $this->getRepository(); + $bookmarkService = $repository->getBookmarkService(); + + $folder = $this->createFolder(['eng-GB' => 'Folder to be trashed'], 2); + $location = $this->loadMainLocation($folder); + + $bookmarkService->createBookmark($location); + self::assertBookmarkRowCount(1, $location->getId(), $this->getRawDatabaseConnection()); + + $repository->getTrashService()->trash($location); + + $this->assertBookmarkGone($location->getId()); + } + + public function testLoadBookmarksAfterDeletingBookmarkedContent(): void + { + $repository = $this->getRepository(); + $bookmarkService = $repository->getBookmarkService(); + + $folder = $this->createFolder(['eng-GB' => 'Folder to be deleted'], 2); + $location = $this->loadMainLocation($folder); + + $bookmarkService->createBookmark($location); + self::assertBookmarkRowCount(1, $location->getId(), $this->getRawDatabaseConnection()); + + $repository->getContentService()->deleteContent($folder->getContentInfo()); + + $this->assertBookmarkGone($location->getId()); + } + + /** + * @throws \Ibexa\Contracts\Core\Repository\Exceptions\NotFoundException + * @throws \Ibexa\Contracts\Core\Repository\Exceptions\UnauthorizedException + */ + private function loadMainLocation(Content $content): Location + { + $mainLocationId = $content->getContentInfo()->getMainLocationId(); + self::assertNotNull($mainLocationId); + + return $this->getRepository()->getLocationService()->loadLocation($mainLocationId); + } + + /** + * Asserts both that the bookmark is no longer listed and that its row is actually gone. + * + * @throws \Doctrine\DBAL\DBALException + * @throws \ErrorException + */ + private function assertBookmarkGone(int $locationId): void + { + $bookmarks = $this->getRepository()->getBookmarkService()->loadBookmarks(0, 9999); + + foreach ($bookmarks as $bookmarkedLocation) { + self::assertNotEquals( + $locationId, + $bookmarkedLocation->getId(), + 'Bookmark of a removed Location should not be listed' + ); + } + + self::assertBookmarkRowCount(0, $locationId, $this->getRawDatabaseConnection()); + } + + /** + * @throws \Doctrine\DBAL\DBALException + */ + private static function assertBookmarkRowCount( + int $expectedCount, + int $locationId, + Connection $connection + ): void { + $query = $connection->createQueryBuilder(); + $query + ->select('COUNT(' . DoctrineDatabase::COLUMN_ID . ')') + ->from(DoctrineDatabase::TABLE_BOOKMARKS) + ->where( + $query->expr()->eq( + DoctrineDatabase::COLUMN_LOCATION_ID, + $query->createNamedParameter($locationId, ParameterType::INTEGER) + ) + ); + + self::assertSame( + $expectedCount, + (int)$query->execute()->fetchColumn(), + sprintf( + 'Expected %d "%s" row(s) for Location %d', + $expectedCount, + DoctrineDatabase::TABLE_BOOKMARKS, + $locationId + ) + ); + } } class_alias(BookmarkServiceTest::class, 'eZ\Publish\API\Repository\Tests\BookmarkServiceTest'); diff --git a/tests/integration/Core/Repository/Filtering/ContentFilteringTest.php b/tests/integration/Core/Repository/Filtering/ContentFilteringTest.php index a5a490520c..cc16341f21 100644 --- a/tests/integration/Core/Repository/Filtering/ContentFilteringTest.php +++ b/tests/integration/Core/Repository/Filtering/ContentFilteringTest.php @@ -33,6 +33,69 @@ */ final class ContentFilteringTest extends BaseRepositoryFilteringTestCase { + private const BOOKMARKED_LOCATION_ID = 52; + + /** + * @return iterable + */ + public function isBookmarkedProvider(): iterable + { + // [isBookmarkedCriterion, initialCount, afterCreateCount, afterDeleteCount] + yield 'bookmarked=true' => [true, 0, 1, 0]; + yield 'bookmarked=false' => [false, 1, 0, 1]; + } + + /** + * @dataProvider isBookmarkedProvider + */ + public function testIsBookmarkedTrueAndFalse( + bool $isBookmarked, + int $initialCount, + int $afterCreateCount, + int $afterDeleteCount + ): void { + $repository = $this->getRepository(false); + $locationService = $repository->getLocationService(); + $contentService = $repository->getContentService(); + $bookmarkService = $repository->getBookmarkService(); + + $bookmarkedLocation = $locationService->loadLocation(self::BOOKMARKED_LOCATION_ID); + + $filter = new Filter(); + $filter->withCriterion(new Criterion\Location\IsBookmarked($isBookmarked)) + ->andWithCriterion(new Criterion\LocationId(self::BOOKMARKED_LOCATION_ID)); + + self::assertCount( + $initialCount, + $contentService->find($filter), + 'Unexpected initial bookmark state for IsBookmarked(' . ($isBookmarked ? 'true' : 'false') . ')' + ); + + $bookmarkService->createBookmark($bookmarkedLocation); + + $filter = new Filter(); + $filter->withCriterion(new Criterion\Location\IsBookmarked($isBookmarked)) + ->andWithCriterion(new Criterion\LocationId(self::BOOKMARKED_LOCATION_ID)); + + self::assertCount( + $afterCreateCount, + $contentService->find($filter), + 'Unexpected state after creating bookmark for IsBookmarked(' . ($isBookmarked ? 'true' : 'false') . ')' + ); + + $bookmarkService->deleteBookmark($bookmarkedLocation); + + $filter = new Filter(); + $filter->withCriterion(new Criterion\Location\IsBookmarked($isBookmarked)) + ->andWithCriterion(new Criterion\LocationId(self::BOOKMARKED_LOCATION_ID)); + + self::assertCount( + $afterDeleteCount, + $contentService->find($filter), + 'Unexpected state after deleting bookmark for IsBookmarked(' . ($isBookmarked ? 'true' : 'false') . ')' + ); + } + /** * Test that special cases of Location Sort Clauses are working correctly. * diff --git a/tests/integration/Core/Repository/Filtering/LocationFilteringTest.php b/tests/integration/Core/Repository/Filtering/LocationFilteringTest.php index f5d165f34d..f27807032b 100644 --- a/tests/integration/Core/Repository/Filtering/LocationFilteringTest.php +++ b/tests/integration/Core/Repository/Filtering/LocationFilteringTest.php @@ -22,6 +22,134 @@ */ final class LocationFilteringTest extends BaseRepositoryFilteringTestCase { + private const BOOKMARKED_LOCATION_ID = 52; + + public function testBookmarkedAndNotBookmarkedCountsMatchTotal(): void + { + $locationService = $this->getRepository(false)->getLocationService(); + + $baseFilter = new Filter(); + $totalCount = $locationService->count($baseFilter); + + $bookmarkedFilter = clone $baseFilter; + $bookmarkedFilter->withCriterion(new Criterion\Location\IsBookmarked(true)); + $bookmarkedCount = $locationService->count($bookmarkedFilter); + + $notBookmarkedFilter = clone $baseFilter; + $notBookmarkedFilter->withCriterion(new Criterion\Location\IsBookmarked(false)); + $notBookmarkedCount = $locationService->count($notBookmarkedFilter); + + self::assertSame( + $totalCount, + $bookmarkedCount + $notBookmarkedCount, + sprintf( + 'Mismatch: total=%d, bookmarked=%d, notBookmarked=%d', + $totalCount, + $bookmarkedCount, + $notBookmarkedCount + ) + ); + } + + /** + * @return iterable + */ + public function isBookmarkedProvider(): iterable + { + // [isBookmarkedCriterion, initialCount, afterCreateCount, afterDeleteCount] + yield 'bookmarked=true' => [true, 0, 1, 0]; + yield 'bookmarked=false' => [false, 1, 0, 1]; + } + + /** + * @dataProvider isBookmarkedProvider + */ + public function testIsBookmarkedTrueAndFalse( + bool $isBookmarked, + int $initialCount, + int $afterCreateCount, + int $afterDeleteCount + ): void { + $repository = $this->getRepository(false); + $locationService = $repository->getLocationService(); + $bookmarkService = $repository->getBookmarkService(); + + $bookmarkedLocation = $locationService->loadLocation(self::BOOKMARKED_LOCATION_ID); + + $filter = new Filter(); + $filter->withCriterion(new Criterion\Location\IsBookmarked($isBookmarked)) + ->andWithCriterion(new Criterion\LocationId(self::BOOKMARKED_LOCATION_ID)); + + self::assertCount( + $initialCount, + $locationService->find($filter), + 'Unexpected initial bookmark state for IsBookmarked(' . ($isBookmarked ? 'true' : 'false') . ')' + ); + + $bookmarkService->createBookmark($bookmarkedLocation); + + $filter = new Filter(); + $filter->withCriterion(new Criterion\Location\IsBookmarked($isBookmarked)) + ->andWithCriterion(new Criterion\LocationId(self::BOOKMARKED_LOCATION_ID)); + + self::assertCount( + $afterCreateCount, + $locationService->find($filter), + 'Unexpected state after creating bookmark for IsBookmarked(' . ($isBookmarked ? 'true' : 'false') . ')' + ); + + $bookmarkService->deleteBookmark($bookmarkedLocation); + + $filter = new Filter(); + $filter->withCriterion(new Criterion\Location\IsBookmarked($isBookmarked)) + ->andWithCriterion(new Criterion\LocationId(self::BOOKMARKED_LOCATION_ID)); + + self::assertCount( + $afterDeleteCount, + $locationService->find($filter), + 'Unexpected state after deleting bookmark for IsBookmarked(' . ($isBookmarked ? 'true' : 'false') . ')' + ); + } + + public function testLogicalOrOfBookmarkedAndNotBookmarkedMatchesEveryLocationOnce(): void + { + $repository = $this->getRepository(false); + $locationService = $repository->getLocationService(); + $bookmarkService = $repository->getBookmarkService(); + + $bookmarkedLocation = $locationService->loadLocation(self::BOOKMARKED_LOCATION_ID); + $bookmarkService->createBookmark($bookmarkedLocation); + + try { + $totalCount = $locationService->count(new Filter()); + + $orFilter = new Filter(); + $orFilter->withCriterion( + new Criterion\LogicalOr([ + new Criterion\Location\IsBookmarked(true), + new Criterion\Location\IsBookmarked(false), + ]) + ); + + self::assertSame($totalCount, $locationService->count($orFilter)); + + $locationIds = array_map( + static function ($location) { + return $location->getId(); + }, + iterator_to_array($locationService->find($orFilter)) + ); + + self::assertSame( + $totalCount, + count(array_unique($locationIds)), + 'LogicalOr(IsBookmarked(true), IsBookmarked(false)) returned duplicate locations' + ); + } finally { + $bookmarkService->deleteBookmark($bookmarkedLocation); + } + } + /** * @throws \Ibexa\Contracts\Core\Repository\Exceptions\InvalidArgumentException */ diff --git a/tests/integration/Core/Repository/LocationServiceTest.php b/tests/integration/Core/Repository/LocationServiceTest.php index 350347bda4..3c91328d55 100644 --- a/tests/integration/Core/Repository/LocationServiceTest.php +++ b/tests/integration/Core/Repository/LocationServiceTest.php @@ -6,6 +6,7 @@ */ namespace Ibexa\Tests\Integration\Core\Repository; +use Doctrine\DBAL\ParameterType; use Exception; use Ibexa\Contracts\Core\Repository\Exceptions\BadStateException; use Ibexa\Contracts\Core\Repository\Exceptions\InvalidArgumentException; @@ -1968,6 +1969,7 @@ public function testBookmarksAreSwappedAfterSwapLocation() $mediaLocationId = $this->generateId('location', 43); $demoDesignLocationId = $this->generateId('location', 56); + $contactUsLocationId = $this->generateId('location', 60); /* BEGIN: Use Case */ $locationService = $repository->getLocationService(); @@ -1975,6 +1977,7 @@ public function testBookmarksAreSwappedAfterSwapLocation() $mediaLocation = $locationService->loadLocation($mediaLocationId); $demoDesignLocation = $locationService->loadLocation($demoDesignLocationId); + $contactUsLocation = $locationService->loadLocation($contactUsLocationId); // Bookmark locations $bookmarkService->createBookmark($mediaLocation); @@ -1983,13 +1986,13 @@ public function testBookmarksAreSwappedAfterSwapLocation() $beforeSwap = $bookmarkService->loadBookmarks(); // Swaps the content referred to by the locations - $locationService->swapLocation($mediaLocation, $demoDesignLocation); + $locationService->swapLocation($demoDesignLocation, $contactUsLocation); $afterSwap = $bookmarkService->loadBookmarks(); /* END: Use Case */ - $this->assertEquals($beforeSwap->items[0]->id, $afterSwap->items[1]->id); - $this->assertEquals($beforeSwap->items[1]->id, $afterSwap->items[0]->id); + self::assertEquals($contactUsLocationId, $afterSwap->items[0]->getId()); + self::assertEquals($beforeSwap->items[1]->getId(), $afterSwap->items[1]->getId()); } /** @@ -2342,6 +2345,26 @@ public function testDeleteLocationDeletesRelatedBookmarks() foreach ($bookmarkService->loadBookmarks(0, 9999) as $bookmarkedLocation) { $this->assertNotEquals($childLocation->id, $bookmarkedLocation->id); } + + // The assertion above only proves the bookmark is not *listed* but loadBookmarks() + // Check the row itself to actually cover the cleanup. + $connection = $this->getRawDatabaseConnection(); + $query = $connection->createQueryBuilder(); + $query + ->select('COUNT(id)') + ->from('ezcontentbrowsebookmark') + ->where( + $query->expr()->eq( + 'node_id', + $query->createNamedParameter($childLocation->getId(), ParameterType::INTEGER) + ) + ); + + self::assertSame( + 0, + (int)$query->execute()->fetchColumn(), + 'Bookmark row of a deleted Location should have been removed' + ); } /** diff --git a/tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/IsBookmarkedQueryBuilderTest.php b/tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/IsBookmarkedQueryBuilderTest.php new file mode 100644 index 0000000000..2ff31b3a3f --- /dev/null +++ b/tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/IsBookmarkedQueryBuilderTest.php @@ -0,0 +1,72 @@ +}> + * + * @throws \Ibexa\Contracts\Core\Repository\Exceptions\InvalidCriterionArgumentException + */ + public function getFilteringCriteriaQueryData(): iterable + { + yield 'IsBookmarked(true)' => [ + new Criterion\Location\IsBookmarked(true), + sprintf('EXISTS (%s)', sprintf(self::BOOKMARK_EXISTS_SUBQUERY, 1)), + ['dcValue1' => self::CURRENT_USER_ID], + ]; + + yield 'IsBookmarked(false)' => [ + new Criterion\Location\IsBookmarked(false), + sprintf('NOT EXISTS (%s)', sprintf(self::BOOKMARK_EXISTS_SUBQUERY, 1)), + ['dcValue1' => self::CURRENT_USER_ID], + ]; + + yield 'IsBookmarked(true) OR IsBookmarked(false)' => [ + new Criterion\LogicalOr( + [ + new Criterion\Location\IsBookmarked(true), + new Criterion\Location\IsBookmarked(false), + ] + ), + sprintf( + '(EXISTS (%s)) OR (NOT EXISTS (%s))', + sprintf(self::BOOKMARK_EXISTS_SUBQUERY, 1), + sprintf(self::BOOKMARK_EXISTS_SUBQUERY, 2) + ), + ['dcValue1' => self::CURRENT_USER_ID, 'dcValue2' => self::CURRENT_USER_ID], + ]; + } + + protected function getCriterionQueryBuilders(): iterable + { + $userReference = $this->createMock(UserReference::class); + $userReference->method('getUserId')->willReturn(self::CURRENT_USER_ID); + + $permissionResolver = $this->createMock(PermissionResolver::class); + $permissionResolver->method('getCurrentUserReference')->willReturn($userReference); + + return [new IsBookmarkedQueryBuilder($permissionResolver)]; + } +} diff --git a/tests/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/Bookmark/IdSortClauseQueryBuilderTest.php b/tests/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/Bookmark/IdSortClauseQueryBuilderTest.php new file mode 100644 index 0000000000..f31817a6d2 --- /dev/null +++ b/tests/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/Bookmark/IdSortClauseQueryBuilderTest.php @@ -0,0 +1,215 @@ +createLocationFilteringQueryBuilder(); + + $builder = $this->createBuilder(); + $sortClause = new Id(Query::SORT_DESC); + + self::assertTrue($builder->accepts($sortClause)); + + $builder->buildQuery($queryBuilder, $sortClause); + + self::assertContains( + sprintf('%s.id AS %s', self::BOOKMARK_ALIAS, self::SORT_ALIAS), + $queryBuilder->getQueryPart('select') + ); + + $joins = $queryBuilder->getQueryPart('join'); + self::assertArrayHasKey('location', $joins); + + $bookmarkJoin = $this->findJoinByAlias($joins['location'], self::BOOKMARK_ALIAS); + self::assertNotNull($bookmarkJoin, 'Bookmarks table was not joined against "location"'); + self::assertSame(DoctrineDatabase::TABLE_BOOKMARKS, $bookmarkJoin['joinTable']); + self::assertSame( + sprintf( + '(location.node_id = %1$s.node_id) AND (%1$s.user_id = :dcValue1)', + self::BOOKMARK_ALIAS + ), + (string)$bookmarkJoin['joinCondition'] + ); + self::assertSame(['dcValue1' => self::CURRENT_USER_ID], $queryBuilder->getParameters()); + + self::assertSame( + [self::SORT_ALIAS . ' DESC'], + $queryBuilder->getQueryPart('orderBy') + ); + + // the whole join graph has to resolve + self::assertStringContainsString(self::BOOKMARK_ALIAS, $queryBuilder->getSQL()); + } + + /** + * Content filtering: there is no "location" FROM table, so the Content item's main Location + * has to be joined first and the bookmarks table joined against *that* alias. + */ + public function testBuildQueryInContentFilteringContext(): void + { + $queryBuilder = $this->createContentFilteringQueryBuilder(); + + $this->createBuilder()->buildQuery($queryBuilder, new Id(Query::SORT_DESC)); + + $joins = $queryBuilder->getQueryPart('join'); + + // main Location joined off the "content" FROM table... + self::assertArrayHasKey('content', $joins); + self::assertSame(LocationGateway::CONTENT_TREE_TABLE, $joins['content'][0]['joinTable']); + self::assertSame(self::CONTENT_LOCATION_ALIAS, $joins['content'][0]['joinAlias']); + + // ...and bookmarks joined off that alias, not off a hardcoded "location" + self::assertArrayHasKey(self::CONTENT_LOCATION_ALIAS, $joins); + self::assertSame( + DoctrineDatabase::TABLE_BOOKMARKS, + $joins[self::CONTENT_LOCATION_ALIAS][0]['joinTable'] + ); + self::assertSame( + self::BOOKMARK_ALIAS, + $joins[self::CONTENT_LOCATION_ALIAS][0]['joinAlias'] + ); + self::assertSame( + sprintf( + '(%1$s.node_id = %2$s.node_id) AND (%2$s.user_id = :dcValue1)', + self::CONTENT_LOCATION_ALIAS, + self::BOOKMARK_ALIAS + ), + (string)$joins[self::CONTENT_LOCATION_ALIAS][0]['joinCondition'] + ); + + self::assertArrayNotHasKey('location', $joins); + + self::assertSame( + [self::SORT_ALIAS . ' DESC'], + $queryBuilder->getQueryPart('orderBy') + ); + + self::assertStringContainsString(self::BOOKMARK_ALIAS, $queryBuilder->getSQL()); + } + + /** + * @return iterable + */ + public function standaloneContextProvider(): iterable + { + yield 'Location filtering' => [$this->createLocationFilteringQueryBuilder()]; + yield 'Content filtering' => [$this->createContentFilteringQueryBuilder()]; + } + + /** + * Test that sort clause works without an IsBookmarked criterion having joined anything first. + * + * @dataProvider standaloneContextProvider + */ + public function testBuildQueryStandaloneProducesResolvableSql( + FilteringQueryBuilder $queryBuilder + ): void { + $this->createBuilder()->buildQuery($queryBuilder, new Id(Query::SORT_ASC)); + + $sql = $queryBuilder->getSQL(); + + self::assertStringContainsString(DoctrineDatabase::TABLE_BOOKMARKS, $sql); + self::assertStringContainsString('ORDER BY ' . self::SORT_ALIAS . ' ASC', $sql); + } + + /** + * @param array> $joins + * + * @return array|null + */ + private function findJoinByAlias(array $joins, string $joinAlias): ?array + { + foreach ($joins as $join) { + if ($join['joinAlias'] === $joinAlias) { + return $join; + } + } + + return null; + } + + private function createBuilder(): IdSortClauseQueryBuilder + { + $userReference = $this->createMock(UserReference::class); + $userReference->method('getUserId')->willReturn(self::CURRENT_USER_ID); + + $permissionResolver = $this->createMock(PermissionResolver::class); + $permissionResolver->method('getCurrentUserReference')->willReturn($userReference); + + return new IdSortClauseQueryBuilder($permissionResolver); + } + + /** + * Mirrors the baseline query built by + * {@see \Ibexa\Core\Persistence\Legacy\Filter\Gateway\Location\Doctrine\DoctrineGateway}: + * "location" is the FROM table and "content" is joined off it. + */ + private function createLocationFilteringQueryBuilder(): FilteringQueryBuilder + { + $queryBuilder = new FilteringQueryBuilder($this->createInMemoryConnection()); + $queryBuilder + ->select('location.node_id') + ->from(LocationGateway::CONTENT_TREE_TABLE, 'location') + ->join( + 'location', + self::CONTENT_ITEM_TABLE, + 'content', + 'content.id = location.contentobject_id' + ); + + return $queryBuilder; + } + + /** + * Mirrors the baseline query built by + * {@see \Ibexa\Core\Persistence\Legacy\Filter\Gateway\Content\Doctrine\DoctrineGateway}: + * "content" is the FROM table and there is no "location" alias at all. + */ + private function createContentFilteringQueryBuilder(): FilteringQueryBuilder + { + $queryBuilder = new FilteringQueryBuilder($this->createInMemoryConnection()); + $queryBuilder + ->select('content.id') + ->from(self::CONTENT_ITEM_TABLE, 'content'); + + return $queryBuilder; + } + + private function createInMemoryConnection(): \Doctrine\DBAL\Connection + { + return DriverManager::getConnection(['url' => 'sqlite:///:memory:']); + } +} diff --git a/tests/lib/Repository/Service/Mock/BookmarkTest.php b/tests/lib/Repository/Service/Mock/BookmarkTest.php index 536a0c2390..5bed42d23a 100644 --- a/tests/lib/Repository/Service/Mock/BookmarkTest.php +++ b/tests/lib/Repository/Service/Mock/BookmarkTest.php @@ -15,6 +15,7 @@ use Ibexa\Contracts\Core\Repository\LocationService; use Ibexa\Contracts\Core\Repository\PermissionResolver; use Ibexa\Contracts\Core\Repository\Values\Content\ContentInfo; +use Ibexa\Contracts\Core\Repository\Values\Content\LocationList; use Ibexa\Core\Repository\BookmarkService; use Ibexa\Core\Repository\Values\Content\Location; use Ibexa\Core\Repository\Values\User\UserReference; @@ -220,28 +221,13 @@ public function testLoadBookmarks() $expectedItems = array_map(function ($locationId) { return $this->createLocation($locationId); }, range(1, $expectedTotalCount)); - - $this->bookmarkHandler - ->expects($this->once()) - ->method('countUserBookmarks') - ->with(self::CURRENT_USER_ID) - ->willReturn($expectedTotalCount); - - $this->bookmarkHandler - ->expects($this->once()) - ->method('loadUserBookmarks') - ->with(self::CURRENT_USER_ID, $offset, $limit) - ->willReturn(array_map(static function ($locationId) { - return new Bookmark(['locationId' => $locationId]); - }, range(1, $expectedTotalCount))); + $locationList = new LocationList(['totalCount' => $expectedTotalCount, 'locations' => $expectedItems]); $locationServiceMock = $this->createMock(LocationService::class); $locationServiceMock - ->expects($this->exactly($expectedTotalCount)) - ->method('loadLocation') - ->willReturnCallback(function ($locationId) { - return $this->createLocation($locationId); - }); + ->expects(self::once()) + ->method('find') + ->willReturn($locationList); $repository = $this->getRepositoryMock(); $repository @@ -249,33 +235,17 @@ public function testLoadBookmarks() ->method('getLocationService') ->willReturn($locationServiceMock); + // All tests in this class expect a call to getPermissionResolver()->getCurrentUserReference(), except this very test + // This is because PermissionResolver is called from BookmarkQueryBuilder, not from BookmarkService when loading bookmarks + // As it is defined in setup() that getCurrentUserReference needs to be called at least once, we'll force a call here + $repository->getPermissionResolver()->getCurrentUserReference(); + $bookmarks = $this->createBookmarkService()->loadBookmarks($offset, $limit); $this->assertEquals($expectedTotalCount, $bookmarks->totalCount); $this->assertEquals($expectedItems, $bookmarks->items); } - /** - * @covers \Ibexa\Contracts\Core\Repository\BookmarkService::loadBookmarks - */ - public function testLoadBookmarksEmptyList() - { - $this->bookmarkHandler - ->expects($this->once()) - ->method('countUserBookmarks') - ->with(self::CURRENT_USER_ID) - ->willReturn(0); - - $this->bookmarkHandler - ->expects($this->never()) - ->method('loadUserBookmarks'); - - $bookmarks = $this->createBookmarkService()->loadBookmarks(0, 10); - - $this->assertEquals(0, $bookmarks->totalCount); - $this->assertEmpty($bookmarks->items); - } - /** * @covers \Ibexa\Contracts\Core\Repository\BookmarkService::isBookmarked */ @@ -290,9 +260,6 @@ public function testLocationShouldNotBeBookmarked() $this->assertFalse($this->createBookmarkService()->isBookmarked($this->createLocation(self::LOCATION_ID))); } - /** - * @covers \Ibexa\Contracts\Core\Repository\BookmarkService::isBookmarked - */ public function testLocationShouldBeBookmarked() { $this->bookmarkHandler From cc704c0ef2eb9fc20c38e0f5736c71de8722d5e4 Mon Sep 17 00:00:00 2001 From: Vidar Langseid Date: Wed, 16 Sep 2026 13:03:05 +0200 Subject: [PATCH 2/3] Adapted merged bookmark tests to DBAL 3 and the 5.0 Criterion hierarchy --- tests/integration/Core/Repository/BookmarkServiceTest.php | 6 +++--- tests/integration/Core/Repository/LocationServiceTest.php | 2 +- .../Location/IsBookmarkedQueryBuilderTest.php | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/integration/Core/Repository/BookmarkServiceTest.php b/tests/integration/Core/Repository/BookmarkServiceTest.php index 9d979113e2..694ea6709d 100644 --- a/tests/integration/Core/Repository/BookmarkServiceTest.php +++ b/tests/integration/Core/Repository/BookmarkServiceTest.php @@ -277,7 +277,7 @@ private function loadMainLocation(Content $content): Location /** * Asserts both that the bookmark is no longer listed and that its row is actually gone. * - * @throws \Doctrine\DBAL\DBALException + * @throws \Doctrine\DBAL\Exception * @throws \ErrorException */ private function assertBookmarkGone(int $locationId): void @@ -296,7 +296,7 @@ private function assertBookmarkGone(int $locationId): void } /** - * @throws \Doctrine\DBAL\DBALException + * @throws \Doctrine\DBAL\Exception */ private static function assertBookmarkRowCount( int $expectedCount, @@ -316,7 +316,7 @@ private static function assertBookmarkRowCount( self::assertSame( $expectedCount, - (int)$query->execute()->fetchColumn(), + (int)$query->executeQuery()->fetchOne(), sprintf( 'Expected %d "%s" row(s) for Location %d', $expectedCount, diff --git a/tests/integration/Core/Repository/LocationServiceTest.php b/tests/integration/Core/Repository/LocationServiceTest.php index 3c25fa0b44..2050cbde92 100644 --- a/tests/integration/Core/Repository/LocationServiceTest.php +++ b/tests/integration/Core/Repository/LocationServiceTest.php @@ -2423,7 +2423,7 @@ public function testDeleteLocationDeletesRelatedBookmarks() self::assertSame( 0, - (int)$query->execute()->fetchColumn(), + (int)$query->executeQuery()->fetchOne(), 'Bookmark row of a deleted Location should have been removed' ); } diff --git a/tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/IsBookmarkedQueryBuilderTest.php b/tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/IsBookmarkedQueryBuilderTest.php index 2ff31b3a3f..0d8a619fe8 100644 --- a/tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/IsBookmarkedQueryBuilderTest.php +++ b/tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/IsBookmarkedQueryBuilderTest.php @@ -25,7 +25,7 @@ final class IsBookmarkedQueryBuilderTest extends BaseCriterionVisitorQueryBuilde . '(bookmark.user_id = :dcValue%1$d) AND (bookmark.node_id = location.node_id)'; /** - * @return iterable}> + * @return iterable}> * * @throws \Ibexa\Contracts\Core\Repository\Exceptions\InvalidCriterionArgumentException */ From 6d2d9ea0c0915851c99cda1de12c44274e69fb64 Mon Sep 17 00:00:00 2001 From: Vidar Langseid Date: Wed, 16 Sep 2026 13:12:55 +0200 Subject: [PATCH 3/3] Used bookmark table constant in tests and stubbed platform for sub-query SQL --- tests/integration/Core/Repository/LocationServiceTest.php | 7 ++++--- .../Filter/BaseCriterionVisitorQueryBuilderTestCase.php | 5 +++++ .../Location/IsBookmarkedQueryBuilderTest.php | 3 ++- 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/tests/integration/Core/Repository/LocationServiceTest.php b/tests/integration/Core/Repository/LocationServiceTest.php index 2050cbde92..3c5a0710d5 100644 --- a/tests/integration/Core/Repository/LocationServiceTest.php +++ b/tests/integration/Core/Repository/LocationServiceTest.php @@ -25,6 +25,7 @@ use Ibexa\Contracts\Core\Repository\Values\Content\Search\SearchHit; use Ibexa\Contracts\Core\Repository\Values\Content\URLAlias; use Ibexa\Contracts\Core\Repository\Values\User\Limitation\SubtreeLimitation; +use Ibexa\Core\Persistence\Legacy\Bookmark\Gateway\DoctrineDatabase; use Ibexa\Core\Persistence\Legacy\Content\Location\Gateway; use Ibexa\Core\Repository\Values\Content\ContentUpdateStruct; @@ -2412,11 +2413,11 @@ public function testDeleteLocationDeletesRelatedBookmarks() $connection = $this->getRawDatabaseConnection(); $query = $connection->createQueryBuilder(); $query - ->select('COUNT(id)') - ->from('ezcontentbrowsebookmark') + ->select('COUNT(' . DoctrineDatabase::COLUMN_ID . ')') + ->from(DoctrineDatabase::TABLE_BOOKMARKS) ->where( $query->expr()->eq( - 'node_id', + DoctrineDatabase::COLUMN_LOCATION_ID, $query->createNamedParameter($childLocation->getId(), ParameterType::INTEGER) ) ); diff --git a/tests/lib/Persistence/Legacy/Filter/BaseCriterionVisitorQueryBuilderTestCase.php b/tests/lib/Persistence/Legacy/Filter/BaseCriterionVisitorQueryBuilderTestCase.php index 0372364174..d0ea870aac 100644 --- a/tests/lib/Persistence/Legacy/Filter/BaseCriterionVisitorQueryBuilderTestCase.php +++ b/tests/lib/Persistence/Legacy/Filter/BaseCriterionVisitorQueryBuilderTestCase.php @@ -9,6 +9,7 @@ namespace Ibexa\Tests\Core\Persistence\Legacy\Filter; use Doctrine\DBAL\Connection; +use Doctrine\DBAL\Platforms\SqlitePlatform; use Doctrine\DBAL\Query\Expression\ExpressionBuilder; use Ibexa\Contracts\Core\Persistence\Filter\Doctrine\FilteringQueryBuilder; use Ibexa\Contracts\Core\Repository\Values\Filter\FilteringCriterion; @@ -85,6 +86,10 @@ private function getQueryBuilder(): FilteringQueryBuilder ->willReturn( new ExpressionBuilder($connectionMock) ); + // Criterion Query Builders which render a sub-query need a platform to build its SQL + $connectionMock + ->method('getDatabasePlatform') + ->willReturn(new SqlitePlatform()); return new FilteringQueryBuilder($connectionMock); } diff --git a/tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/IsBookmarkedQueryBuilderTest.php b/tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/IsBookmarkedQueryBuilderTest.php index 0d8a619fe8..bb92b5d761 100644 --- a/tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/IsBookmarkedQueryBuilderTest.php +++ b/tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/IsBookmarkedQueryBuilderTest.php @@ -11,6 +11,7 @@ use Ibexa\Contracts\Core\Repository\PermissionResolver; use Ibexa\Contracts\Core\Repository\Values\Content\Query\Criterion as Criterion; use Ibexa\Contracts\Core\Repository\Values\User\UserReference; +use Ibexa\Core\Persistence\Legacy\Bookmark\Gateway\DoctrineDatabase; use Ibexa\Core\Persistence\Legacy\Filter\CriterionQueryBuilder\Location\IsBookmarkedQueryBuilder; use Ibexa\Tests\Core\Persistence\Legacy\Filter\BaseCriterionVisitorQueryBuilderTestCase; @@ -21,7 +22,7 @@ final class IsBookmarkedQueryBuilderTest extends BaseCriterionVisitorQueryBuilde { private const CURRENT_USER_ID = 14; - private const BOOKMARK_EXISTS_SUBQUERY = 'SELECT 1 FROM ezcontentbrowsebookmark bookmark WHERE ' + private const BOOKMARK_EXISTS_SUBQUERY = 'SELECT 1 FROM ' . DoctrineDatabase::TABLE_BOOKMARKS . ' bookmark WHERE ' . '(bookmark.user_id = :dcValue%1$d) AND (bookmark.node_id = location.node_id)'; /**