From 0cc80910e8f5a0784e3227e50ee992895fe9e613 Mon Sep 17 00:00:00 2001 From: Vidar Langseid Date: Fri, 31 Jul 2026 11:59:52 +0200 Subject: [PATCH 01/15] IBX-6773: Bookmarks for non-accessible contents cause exception --- .../Persistence/Bookmark/Handler.php | 4 + .../Content/Query/Criterion/IsBookmarked.php | 39 +++++++ .../Content/Query/SortClause/BookmarkId.php | 24 ++++ .../Persistence/Legacy/Bookmark/Gateway.php | 4 + .../Location/BookmarkQueryBuilder.php | 76 +++++++++++++ .../Bookmark/IdSortClauseQueryBuilder.php | 38 +++++++ src/lib/Repository/BookmarkService.php | 45 +++++--- src/lib/Repository/Repository.php | 3 +- .../Core/Repository/BookmarkServiceTest.php | 20 +++- .../Filtering/Criterion/IsBookmarkedTest.php | 107 ++++++++++++++++++ .../Core/Repository/LocationServiceTest.php | 8 +- .../Location/BookmarkQueryBuilderTest.php | 53 +++++++++ .../Repository/Service/Mock/BookmarkTest.php | 53 ++------- 13 files changed, 408 insertions(+), 66 deletions(-) create mode 100644 src/contracts/Repository/Values/Content/Query/Criterion/IsBookmarked.php create mode 100644 src/contracts/Repository/Values/Content/Query/SortClause/BookmarkId.php create mode 100644 src/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/BookmarkQueryBuilder.php create mode 100644 src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Bookmark/IdSortClauseQueryBuilder.php create mode 100644 tests/integration/Core/Repository/Filtering/Criterion/IsBookmarkedTest.php create mode 100644 tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/BookmarkQueryBuilderTest.php diff --git a/src/contracts/Persistence/Bookmark/Handler.php b/src/contracts/Persistence/Bookmark/Handler.php index f9ef1a1cae..cf685b7cec 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.30 The "Handler::loadUserBookmarks()" method is deprecated, will be removed in 6.0.0. Use "LocationService::find()" and "Criterion\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.30 The "Handler::countUserBookmarks()" method is deprecated, will be removed in 6.0.0. Use "LocationService::count()" and "Criterion\IsBookmarked" instead. + * * @param int $userId * * @return int diff --git a/src/contracts/Repository/Values/Content/Query/Criterion/IsBookmarked.php b/src/contracts/Repository/Values/Content/Query/Criterion/IsBookmarked.php new file mode 100644 index 0000000000..4d3c659956 --- /dev/null +++ b/src/contracts/Repository/Values/Content/Query/Criterion/IsBookmarked.php @@ -0,0 +1,39 @@ +userId = $userId; + parent::__construct(null, null, $isBookmarked); + } + + public function getSpecifications(): array + { + return [ + new Specifications(Operator::EQ, Specifications::FORMAT_SINGLE, Specifications::TYPE_BOOLEAN), + ]; + } +} diff --git a/src/contracts/Repository/Values/Content/Query/SortClause/BookmarkId.php b/src/contracts/Repository/Values/Content/Query/SortClause/BookmarkId.php new file mode 100644 index 0000000000..a86e5f2858 --- /dev/null +++ b/src/contracts/Repository/Values/Content/Query/SortClause/BookmarkId.php @@ -0,0 +1,24 @@ +permissionResolver = $permissionResolver; + } + + public function accepts(FilteringCriterion $criterion): bool + { + return $criterion instanceof IsBookmarked; + } + + public function buildQueryConstraint( + FilteringQueryBuilder $queryBuilder, + FilteringCriterion $criterion + ): string { + /** @var \Ibexa\Contracts\Core\Repository\Values\Content\Query\Criterion\IsBookmarked $criterion */ + $isBookmarked = $criterion->value[0] ?? null; + if (!is_bool($isBookmarked)) { + throw new \InvalidArgumentException('IsBookmarked criterion value must be boolean at index 0.'); + } + $userId = $criterion->userId ?? $this->permissionResolver->getCurrentUserReference()->getUserId(); + + if ($isBookmarked) { + $queryBuilder + ->joinOnce( + 'location', + DoctrineDatabase::TABLE_BOOKMARKS, + 'bookmark', + 'location.node_id = bookmark.node_id' + ); + + return $queryBuilder->expr()->eq( + 'bookmark.user_id', + $queryBuilder->createNamedParameter( + $userId, + ParameterType::INTEGER + ) + ); + } else { + $queryBuilder + ->leftJoinOnce( + 'location', + DoctrineDatabase::TABLE_BOOKMARKS, + 'bookmark', + 'location.node_id = bookmark.node_id AND bookmark.user_id = :userId' + ) + ->setParameter('userId', $userId); + + return $queryBuilder->expr()->isNull('bookmark.id'); + } + } +} diff --git a/src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Bookmark/IdSortClauseQueryBuilder.php b/src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Bookmark/IdSortClauseQueryBuilder.php new file mode 100644 index 0000000000..1a0151f3dd --- /dev/null +++ b/src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Bookmark/IdSortClauseQueryBuilder.php @@ -0,0 +1,38 @@ +addSelect('bookmark.id'); + $queryBuilder->addOrderBy('bookmark.id', $sortClause->direction); + } +} diff --git a/src/lib/Repository/BookmarkService.php b/src/lib/Repository/BookmarkService.php index a74c79d5cb..7c04108e87 100644 --- a/src/lib/Repository/BookmarkService.php +++ b/src/lib/Repository/BookmarkService.php @@ -9,22 +9,28 @@ 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\BadStateException; 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; + + private LoggerInterface $logger; /** * BookmarkService constructor. @@ -32,10 +38,11 @@ class BookmarkService implements BookmarkServiceInterface * @param \Ibexa\Contracts\Core\Repository\Repository $repository * @param \Ibexa\Contracts\Core\Persistence\Bookmark\Handler $bookmarkHandler */ - public function __construct(RepositoryInterface $repository, BookmarkHandler $bookmarkHandler) + public function __construct(RepositoryInterface $repository, BookmarkHandler $bookmarkHandler, ?LoggerInterface $logger = null) { $this->repository = $repository; $this->bookmarkHandler = $bookmarkHandler; + $this->logger = $logger ?? new NullLogger(); } /** @@ -95,17 +102,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\IsBookmarked()) + ->withSortClause(new SortClause\BookmarkId(Query::SORT_DESC)) + ->sliceBy($limit, $offset); + + $result = $this->repository->getLocationService()->find($filter, []); + } catch (BadStateException $e) { + $this->logger->debug($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..8c21ab8247 100644 --- a/tests/integration/Core/Repository/BookmarkServiceTest.php +++ b/tests/integration/Core/Repository/BookmarkServiceTest.php @@ -9,7 +9,8 @@ namespace Ibexa\Tests\Integration\Core\Repository; use Ibexa\Contracts\Core\Repository\Exceptions\InvalidArgumentException; -use Ibexa\Contracts\Core\Repository\Values\Bookmark\BookmarkList; +use Ibexa\Contracts\Core\Repository\Values\Content\Query\Criterion; +use Ibexa\Contracts\Core\Repository\Values\Filter\Filter; /** * Test case for the BookmarkService. @@ -143,13 +144,24 @@ 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\IsBookmarked(true, 14)); + $count = $repository->getLocationService()->count($filter, []); + + self::assertEquals(5, $count); + } } class_alias(BookmarkServiceTest::class, 'eZ\Publish\API\Repository\Tests\BookmarkServiceTest'); diff --git a/tests/integration/Core/Repository/Filtering/Criterion/IsBookmarkedTest.php b/tests/integration/Core/Repository/Filtering/Criterion/IsBookmarkedTest.php new file mode 100644 index 0000000000..c7024c7919 --- /dev/null +++ b/tests/integration/Core/Repository/Filtering/Criterion/IsBookmarkedTest.php @@ -0,0 +1,107 @@ +getRepository(false); + $locationService = $repository->getLocationService(); + + $baseFilter = new Filter(); + $totalCount = $locationService->count($baseFilter); + + $bookmarkedFilter = clone $baseFilter; + $bookmarkedFilter->withCriterion(new Criterion\IsBookmarked(true)); + $bookmarkedCount = $locationService->count($bookmarkedFilter); + + $notBookmarkedFilter = clone $baseFilter; + $notBookmarkedFilter->withCriterion(new Criterion\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] + return [ + 'bookmarked=true' => [true, 0, 1, 0], + '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(); + + $filesLocation = $locationService->loadLocation(52); + + $filter = new Filter(); + $filter->withCriterion(new Criterion\IsBookmarked($isBookmarked)) + ->andWithCriterion(new Criterion\LocationId(52)); + + $locations = $locationService->find($filter); + self::assertCount( + $initialCount, + $locations, + 'Unexpected initial bookmark state for IsBookmarked(' . ($isBookmarked ? 'true' : 'false') . ')' + ); + + $bookmarkService->createBookmark($filesLocation); + + $filter = new Filter(); + $filter->withCriterion(new Criterion\IsBookmarked($isBookmarked)) + ->andWithCriterion(new Criterion\LocationId(52)); + + $locations = $locationService->find($filter); + self::assertCount( + $afterCreateCount, + $locations, + 'Unexpected state after creating bookmark for IsBookmarked(' . ($isBookmarked ? 'true' : 'false') . ')' + ); + + $bookmarkService->deleteBookmark($filesLocation); + + $filter = new Filter(); + $filter->withCriterion(new Criterion\IsBookmarked($isBookmarked)) + ->andWithCriterion(new Criterion\LocationId(52)); + + $locations = $locationService->find($filter); + self::assertCount( + $afterDeleteCount, + $locations, + 'Unexpected state after deleting bookmark for IsBookmarked(' . ($isBookmarked ? 'true' : 'false') . ')' + ); + } +} diff --git a/tests/integration/Core/Repository/LocationServiceTest.php b/tests/integration/Core/Repository/LocationServiceTest.php index 350347bda4..ef4e505c1f 100644 --- a/tests/integration/Core/Repository/LocationServiceTest.php +++ b/tests/integration/Core/Repository/LocationServiceTest.php @@ -1968,6 +1968,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 +1976,7 @@ public function testBookmarksAreSwappedAfterSwapLocation() $mediaLocation = $locationService->loadLocation($mediaLocationId); $demoDesignLocation = $locationService->loadLocation($demoDesignLocationId); + $contactUsLocation = $locationService->loadLocation($contactUsLocationId); // Bookmark locations $bookmarkService->createBookmark($mediaLocation); @@ -1983,13 +1985,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]->id); + self::assertEquals($beforeSwap->items[1]->id, $afterSwap->items[1]->id); } /** diff --git a/tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/BookmarkQueryBuilderTest.php b/tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/BookmarkQueryBuilderTest.php new file mode 100644 index 0000000000..348625cba3 --- /dev/null +++ b/tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/BookmarkQueryBuilderTest.php @@ -0,0 +1,53 @@ +}> + * + * @throws \Ibexa\Contracts\Core\Repository\Exceptions\InvalidCriterionArgumentException + */ + public function getFilteringCriteriaQueryData(): iterable + { + yield 'Bookmarks locations for user_id=14' => [ + new Criterion\IsBookmarked(true, 14), + 'bookmark.user_id = :dcValue1', + ['dcValue1' => 14], + ]; + + yield 'Bookmarks locations for user_id=14 OR user_id=7' => [ + new Criterion\LogicalOr( + [ + new Criterion\IsBookmarked(true, 14), + new Criterion\IsBookmarked(true, 7), + ] + ), + '(bookmark.user_id = :dcValue1) OR (bookmark.user_id = :dcValue2)', + ['dcValue1' => 14, 'dcValue2' => 7], + ]; + + yield 'Bookmarks locations for user_id=7' => [ + new Criterion\IsBookmarked(true, 7), + 'bookmark.user_id = :dcValue1', + ['dcValue1' => 7], + ]; + } + + protected function getCriterionQueryBuilders(): iterable + { + return [new BookmarkQueryBuilder($this->createMock(PermissionResolver::class))]; + } +} 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 8906ad592c5c424a27ef3482c3c2d2aacde1263e Mon Sep 17 00:00:00 2001 From: Vidar Langseid Date: Fri, 31 Jul 2026 11:59:52 +0200 Subject: [PATCH 02/15] Aligned PHPStan baseline with the bookmark changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Paweł Niedzielski Co-authored-by: Andrew Longosz --- phpstan-baseline.neon | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 9b8ac82db4..24c78f3e52 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -32220,12 +32220,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 @@ -69499,12 +69493,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 From 22773afd722fcf395eeed922087f3f3e4027c68a Mon Sep 17 00:00:00 2001 From: Vidar Langseid Date: Mon, 3 Aug 2026 11:09:46 +0200 Subject: [PATCH 03/15] Consolidate IsBookmarked criterion into Criterion\Location\IsBookmarked --- .../Persistence/Bookmark/Handler.php | 4 +- .../Content/Query/Criterion/IsBookmarked.php | 39 -------- .../Bookmark/Id.php} | 4 +- .../Persistence/Legacy/Bookmark/Gateway.php | 4 +- ...ilder.php => IsBookmarkedQueryBuilder.php} | 52 +++++----- .../Bookmark/IdSortClauseQueryBuilder.php | 38 -------- .../Bookmark/IdSortClauseQueryBuilder.php | 65 +++++++++++++ src/lib/Repository/BookmarkService.php | 4 +- .../Core/Repository/BookmarkServiceTest.php | 2 +- .../Filtering/Criterion/IsBookmarkedTest.php | 94 ++++++++++++++++--- .../Location/BookmarkQueryBuilderTest.php | 57 +++++++---- .../Bookmark/IdSortClauseQueryBuilderTest.php | 64 +++++++++++++ 12 files changed, 281 insertions(+), 146 deletions(-) delete mode 100644 src/contracts/Repository/Values/Content/Query/Criterion/IsBookmarked.php rename src/contracts/Repository/Values/Content/Query/SortClause/{BookmarkId.php => Location/Bookmark/Id.php} (88%) rename src/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/{BookmarkQueryBuilder.php => IsBookmarkedQueryBuilder.php} (54%) delete mode 100644 src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Bookmark/IdSortClauseQueryBuilder.php create mode 100644 src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/Bookmark/IdSortClauseQueryBuilder.php create mode 100644 tests/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/Bookmark/IdSortClauseQueryBuilderTest.php diff --git a/src/contracts/Persistence/Bookmark/Handler.php b/src/contracts/Persistence/Bookmark/Handler.php index cf685b7cec..25b62a8429 100644 --- a/src/contracts/Persistence/Bookmark/Handler.php +++ b/src/contracts/Persistence/Bookmark/Handler.php @@ -50,7 +50,7 @@ public function loadUserIdsByLocation(Location $location): array; /** * Loads bookmarks owned by user. * - * @deprecated 4.6.30 The "Handler::loadUserBookmarks()" method is deprecated, will be removed in 6.0.0. Use "LocationService::find()" and "Criterion\IsBookmarked" instead. + * @deprecated 4.6.30 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 @@ -63,7 +63,7 @@ public function loadUserBookmarks(int $userId, int $offset = 0, int $limit = -1) /** * Count bookmarks owned by user. * - * @deprecated 4.6.30 The "Handler::countUserBookmarks()" method is deprecated, will be removed in 6.0.0. Use "LocationService::count()" and "Criterion\IsBookmarked" instead. + * @deprecated 4.6.30 The "Handler::countUserBookmarks()" method is deprecated, will be removed in 6.0.0. Use "LocationService::count()" and "Criterion\Location\IsBookmarked" instead. * * @param int $userId * diff --git a/src/contracts/Repository/Values/Content/Query/Criterion/IsBookmarked.php b/src/contracts/Repository/Values/Content/Query/Criterion/IsBookmarked.php deleted file mode 100644 index 4d3c659956..0000000000 --- a/src/contracts/Repository/Values/Content/Query/Criterion/IsBookmarked.php +++ /dev/null @@ -1,39 +0,0 @@ -userId = $userId; - parent::__construct(null, null, $isBookmarked); - } - - public function getSpecifications(): array - { - return [ - new Specifications(Operator::EQ, Specifications::FORMAT_SINGLE, Specifications::TYPE_BOOLEAN), - ]; - } -} diff --git a/src/contracts/Repository/Values/Content/Query/SortClause/BookmarkId.php b/src/contracts/Repository/Values/Content/Query/SortClause/Location/Bookmark/Id.php similarity index 88% rename from src/contracts/Repository/Values/Content/Query/SortClause/BookmarkId.php rename to src/contracts/Repository/Values/Content/Query/SortClause/Location/Bookmark/Id.php index a86e5f2858..4cc4e8a4af 100644 --- a/src/contracts/Repository/Values/Content/Query/SortClause/BookmarkId.php +++ b/src/contracts/Repository/Values/Content/Query/SortClause/Location/Bookmark/Id.php @@ -6,7 +6,7 @@ */ declare(strict_types=1); -namespace Ibexa\Contracts\Core\Repository\Values\Content\Query\SortClause; +namespace Ibexa\Contracts\Core\Repository\Values\Content\Query\SortClause\Location\Bookmark; use Ibexa\Contracts\Core\Repository\Values\Content\Query; use Ibexa\Contracts\Core\Repository\Values\Content\Query\SortClause; @@ -15,7 +15,7 @@ /** * Sets sort direction on the bookmark id for a location query containing IsBookmarked criterion. */ -final class BookmarkId extends SortClause implements FilteringSortClause +final class Id extends SortClause implements FilteringSortClause { public function __construct(string $sortDirection = Query::SORT_ASC) { diff --git a/src/lib/Persistence/Legacy/Bookmark/Gateway.php b/src/lib/Persistence/Legacy/Bookmark/Gateway.php index 4905f253ab..ecdd13d476 100644 --- a/src/lib/Persistence/Legacy/Bookmark/Gateway.php +++ b/src/lib/Persistence/Legacy/Bookmark/Gateway.php @@ -52,7 +52,7 @@ abstract public function loadUserIdsByLocation(Location $location): array; /** * Load data for all bookmarks owned by given $userId. * - * @deprecated 4.6.30 Gateway::loadUserBookmarks()" method is deprecated, will be removed in 6.0.0. Use "LocationService::find()" and "Criterion\IsBookmarked" instead. + * @deprecated 4.6.30 Gateway::loadUserBookmarks()" method is deprecated, will be removed in 6.0.0. Use "LocationService::find()" and "Criterion\Location\IsBookmarked" instead. * * @param int $userId ID of user * @param int $offset Offset to start listing from, 0 by default @@ -65,7 +65,7 @@ abstract public function loadUserBookmarks(int $userId, int $offset = 0, int $li /** * Count bookmarks owned by given $userId. * - * @deprecated 4.6.30 The "Gateway::countUserBookmarks()" method is deprecated, will be removed in 6.0.0. Use "LocationService::count()" and "Criterion\IsBookmarked" instead. + * @deprecated 4.6.30 The "Gateway::countUserBookmarks()" method is deprecated, will be removed in 6.0.0. Use "LocationService::count()" and "Criterion\Location\IsBookmarked" instead. * * @param int $userId ID of user * diff --git a/src/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/BookmarkQueryBuilder.php b/src/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/IsBookmarkedQueryBuilder.php similarity index 54% rename from src/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/BookmarkQueryBuilder.php rename to src/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/IsBookmarkedQueryBuilder.php index d885c552ca..ae00011fef 100644 --- a/src/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/BookmarkQueryBuilder.php +++ b/src/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/IsBookmarkedQueryBuilder.php @@ -9,16 +9,17 @@ namespace Ibexa\Core\Persistence\Legacy\Filter\CriterionQueryBuilder\Location; use Doctrine\DBAL\ParameterType; +use Doctrine\DBAL\Query\QueryBuilder; use Ibexa\Contracts\Core\Persistence\Filter\Doctrine\FilteringQueryBuilder; -use Ibexa\Contracts\Core\Repository\Values\Content\Query\Criterion\IsBookmarked; +use Ibexa\Contracts\Core\Repository\PermissionResolver; +use Ibexa\Contracts\Core\Repository\Values\Content\Query\Criterion\Location\IsBookmarked; use Ibexa\Contracts\Core\Repository\Values\Filter\FilteringCriterion; use Ibexa\Core\Persistence\Legacy\Bookmark\Gateway\DoctrineDatabase; -use Ibexa\Core\Repository\Permission\PermissionResolver; /** * @internal for internal use by Repository Filtering */ -final class BookmarkQueryBuilder extends BaseLocationCriterionQueryBuilder +final class IsBookmarkedQueryBuilder extends BaseLocationCriterionQueryBuilder { private PermissionResolver $permissionResolver; @@ -37,40 +38,31 @@ public function buildQueryConstraint( FilteringQueryBuilder $queryBuilder, FilteringCriterion $criterion ): string { - /** @var \Ibexa\Contracts\Core\Repository\Values\Content\Query\Criterion\IsBookmarked $criterion */ + parent::buildQueryConstraint($queryBuilder, $criterion); + + /** @var \Ibexa\Contracts\Core\Repository\Values\Content\Query\Criterion\Location\IsBookmarked $criterion */ $isBookmarked = $criterion->value[0] ?? null; if (!is_bool($isBookmarked)) { throw new \InvalidArgumentException('IsBookmarked criterion value must be boolean at index 0.'); } - $userId = $criterion->userId ?? $this->permissionResolver->getCurrentUserReference()->getUserId(); - if ($isBookmarked) { - $queryBuilder - ->joinOnce( - 'location', - DoctrineDatabase::TABLE_BOOKMARKS, - 'bookmark', - 'location.node_id = bookmark.node_id' - ); + $userId = $this->permissionResolver->getCurrentUserReference()->getUserId(); - return $queryBuilder->expr()->eq( - 'bookmark.user_id', - $queryBuilder->createNamedParameter( - $userId, - ParameterType::INTEGER - ) + $subQueryBuilder = new QueryBuilder($queryBuilder->getConnection()); + $subQueryBuilder + ->select('1') + ->from(DoctrineDatabase::TABLE_BOOKMARKS, 'bookmark') + ->where( + $subQueryBuilder->expr()->eq( + 'bookmark.' . DoctrineDatabase::COLUMN_USER_ID, + $queryBuilder->createNamedParameter($userId, ParameterType::INTEGER) + ), + $subQueryBuilder->expr()->eq('bookmark.node_id', 'location.node_id') ); - } else { - $queryBuilder - ->leftJoinOnce( - 'location', - DoctrineDatabase::TABLE_BOOKMARKS, - 'bookmark', - 'location.node_id = bookmark.node_id AND bookmark.user_id = :userId' - ) - ->setParameter('userId', $userId); - return $queryBuilder->expr()->isNull('bookmark.id'); - } + return sprintf( + $isBookmarked ? 'EXISTS (%s)' : 'NOT EXISTS (%s)', + $subQueryBuilder->getSQL() + ); } } diff --git a/src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Bookmark/IdSortClauseQueryBuilder.php b/src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Bookmark/IdSortClauseQueryBuilder.php deleted file mode 100644 index 1a0151f3dd..0000000000 --- a/src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Bookmark/IdSortClauseQueryBuilder.php +++ /dev/null @@ -1,38 +0,0 @@ -addSelect('bookmark.id'); - $queryBuilder->addOrderBy('bookmark.id', $sortClause->direction); - } -} 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..b5ff52b110 --- /dev/null +++ b/src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/Bookmark/IdSortClauseQueryBuilder.php @@ -0,0 +1,65 @@ +permissionResolver = $permissionResolver; + } + + public function accepts(FilteringSortClause $sortClause): bool + { + return $sortClause instanceof Id; + } + + public function buildQuery( + FilteringQueryBuilder $queryBuilder, + FilteringSortClause $sortClause + ): void { + if (!$sortClause instanceof Id) { + throw new \InvalidArgumentException(sprintf( + 'Expected %s, got %s', + Id::class, + get_class($sortClause), + )); + } + + $userId = $this->permissionResolver->getCurrentUserReference()->getUserId(); + + $queryBuilder->leftJoinOnce( + 'location', + DoctrineDatabase::TABLE_BOOKMARKS, + self::ALIAS, + (string)$queryBuilder->expr()->andX( + sprintf('location.node_id = %s.node_id', self::ALIAS), + $queryBuilder->expr()->eq( + sprintf('%s.%s', self::ALIAS, DoctrineDatabase::COLUMN_USER_ID), + $queryBuilder->createNamedParameter($userId, ParameterType::INTEGER) + ) + ) + ); + + $queryBuilder->addSelect(self::ALIAS . '.id'); + $queryBuilder->addOrderBy(self::ALIAS . '.id', $sortClause->direction); + } +} diff --git a/src/lib/Repository/BookmarkService.php b/src/lib/Repository/BookmarkService.php index 7c04108e87..276b80e53c 100644 --- a/src/lib/Repository/BookmarkService.php +++ b/src/lib/Repository/BookmarkService.php @@ -105,8 +105,8 @@ public function loadBookmarks(int $offset = 0, int $limit = 25): BookmarkList $filter = new Filter(); try { $filter - ->withCriterion(new Criterion\IsBookmarked()) - ->withSortClause(new SortClause\BookmarkId(Query::SORT_DESC)) + ->withCriterion(new Criterion\Location\IsBookmarked()) + ->withSortClause(new SortClause\Location\Bookmark\Id(Query::SORT_DESC)) ->sliceBy($limit, $offset); $result = $this->repository->getLocationService()->find($filter, []); diff --git a/tests/integration/Core/Repository/BookmarkServiceTest.php b/tests/integration/Core/Repository/BookmarkServiceTest.php index 8c21ab8247..42c03d8686 100644 --- a/tests/integration/Core/Repository/BookmarkServiceTest.php +++ b/tests/integration/Core/Repository/BookmarkServiceTest.php @@ -157,7 +157,7 @@ public function testCountBookmarks(): void $filter = new Filter(); $filter - ->withCriterion(new Criterion\IsBookmarked(true, 14)); + ->withCriterion(new Criterion\Location\IsBookmarked(true)); $count = $repository->getLocationService()->count($filter, []); self::assertEquals(5, $count); diff --git a/tests/integration/Core/Repository/Filtering/Criterion/IsBookmarkedTest.php b/tests/integration/Core/Repository/Filtering/Criterion/IsBookmarkedTest.php index c7024c7919..e09095bb16 100644 --- a/tests/integration/Core/Repository/Filtering/Criterion/IsBookmarkedTest.php +++ b/tests/integration/Core/Repository/Filtering/Criterion/IsBookmarkedTest.php @@ -4,14 +4,21 @@ * @copyright Copyright (C) Ibexa AS. All rights reserved. * @license For full copyright and license information view LICENSE file distributed with this source code. */ -namespace Ibexa\Tests\Integration\Core\Repository\Filtering; +declare(strict_types=1); + +namespace Ibexa\Tests\Integration\Core\Repository\Filtering\Criterion; use Ibexa\Contracts\Core\Repository\Values\Content\Query\Criterion; use Ibexa\Contracts\Core\Repository\Values\Filter\Filter; use Ibexa\Tests\Integration\Core\Repository\BaseTest; -class IsBookmarkedTest extends BaseTest +/** + * @covers \Ibexa\Core\Persistence\Legacy\Filter\CriterionQueryBuilder\Location\IsBookmarkedQueryBuilder + */ +final class IsBookmarkedTest extends BaseTest { + private const BOOKMARKED_LOCATION_ID = 52; + public function testBookmarkedAndNotBookmarkedCountsMatchTotal(): void { $repository = $this->getRepository(false); @@ -21,11 +28,11 @@ public function testBookmarkedAndNotBookmarkedCountsMatchTotal(): void $totalCount = $locationService->count($baseFilter); $bookmarkedFilter = clone $baseFilter; - $bookmarkedFilter->withCriterion(new Criterion\IsBookmarked(true)); + $bookmarkedFilter->withCriterion(new Criterion\Location\IsBookmarked(true)); $bookmarkedCount = $locationService->count($bookmarkedFilter); $notBookmarkedFilter = clone $baseFilter; - $notBookmarkedFilter->withCriterion(new Criterion\IsBookmarked(false)); + $notBookmarkedFilter->withCriterion(new Criterion\Location\IsBookmarked(false)); $notBookmarkedCount = $locationService->count($notBookmarkedFilter); self::assertSame( @@ -65,11 +72,11 @@ public function testIsBookmarkedTrueAndFalse( $locationService = $repository->getLocationService(); $bookmarkService = $repository->getBookmarkService(); - $filesLocation = $locationService->loadLocation(52); + $filesLocation = $locationService->loadLocation(self::BOOKMARKED_LOCATION_ID); $filter = new Filter(); - $filter->withCriterion(new Criterion\IsBookmarked($isBookmarked)) - ->andWithCriterion(new Criterion\LocationId(52)); + $filter->withCriterion(new Criterion\Location\IsBookmarked($isBookmarked)) + ->andWithCriterion(new Criterion\LocationId(self::BOOKMARKED_LOCATION_ID)); $locations = $locationService->find($filter); self::assertCount( @@ -81,8 +88,8 @@ public function testIsBookmarkedTrueAndFalse( $bookmarkService->createBookmark($filesLocation); $filter = new Filter(); - $filter->withCriterion(new Criterion\IsBookmarked($isBookmarked)) - ->andWithCriterion(new Criterion\LocationId(52)); + $filter->withCriterion(new Criterion\Location\IsBookmarked($isBookmarked)) + ->andWithCriterion(new Criterion\LocationId(self::BOOKMARKED_LOCATION_ID)); $locations = $locationService->find($filter); self::assertCount( @@ -94,8 +101,8 @@ public function testIsBookmarkedTrueAndFalse( $bookmarkService->deleteBookmark($filesLocation); $filter = new Filter(); - $filter->withCriterion(new Criterion\IsBookmarked($isBookmarked)) - ->andWithCriterion(new Criterion\LocationId(52)); + $filter->withCriterion(new Criterion\Location\IsBookmarked($isBookmarked)) + ->andWithCriterion(new Criterion\LocationId(self::BOOKMARKED_LOCATION_ID)); $locations = $locationService->find($filter); self::assertCount( @@ -104,4 +111,69 @@ public function testIsBookmarkedTrueAndFalse( '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->id; + }, + 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); + } + } + + public function testIsBookmarkedFiltersContentAsWellAsLocations(): 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(true)) + ->andWithCriterion(new Criterion\LocationId(self::BOOKMARKED_LOCATION_ID)); + self::assertCount(0, $contentService->find($filter)); + + $bookmarkService->createBookmark($bookmarkedLocation); + + try { + $filter = new Filter(); + $filter->withCriterion(new Criterion\Location\IsBookmarked(true)) + ->andWithCriterion(new Criterion\LocationId(self::BOOKMARKED_LOCATION_ID)); + self::assertCount(1, $contentService->find($filter)); + } finally { + $bookmarkService->deleteBookmark($bookmarkedLocation); + } + } } diff --git a/tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/BookmarkQueryBuilderTest.php b/tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/BookmarkQueryBuilderTest.php index 348625cba3..0d608b025e 100644 --- a/tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/BookmarkQueryBuilderTest.php +++ b/tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/BookmarkQueryBuilderTest.php @@ -8,13 +8,22 @@ namespace Ibexa\Tests\Core\Persistence\Legacy\Filter\CriterionQueryBuilder\Location; +use Ibexa\Contracts\Core\Repository\PermissionResolver; use Ibexa\Contracts\Core\Repository\Values\Content\Query\Criterion as Criterion; -use Ibexa\Core\Persistence\Legacy\Filter\CriterionQueryBuilder\Location\BookmarkQueryBuilder; -use Ibexa\Core\Repository\Permission\PermissionResolver; +use Ibexa\Contracts\Core\Repository\Values\User\UserReference; +use Ibexa\Core\Persistence\Legacy\Filter\CriterionQueryBuilder\Location\IsBookmarkedQueryBuilder; use Ibexa\Tests\Core\Persistence\Legacy\Filter\BaseCriterionVisitorQueryBuilderTestCase; +/** + * @covers \Ibexa\Core\Persistence\Legacy\Filter\CriterionQueryBuilder\Location\IsBookmarkedQueryBuilder + */ final class BookmarkQueryBuilderTest extends BaseCriterionVisitorQueryBuilderTestCase { + private const CURRENT_USER_ID = 14; + + private const BOOKMARK_EXISTS_SUBQUERY = 'SELECT 1 FROM ezcontentbrowsebookmark bookmark WHERE ' + . '(bookmark.user_id = :dcValue%1$d) AND (bookmark.node_id = location.node_id)'; + /** * @return iterable}> * @@ -22,32 +31,42 @@ final class BookmarkQueryBuilderTest extends BaseCriterionVisitorQueryBuilderTes */ public function getFilteringCriteriaQueryData(): iterable { - yield 'Bookmarks locations for user_id=14' => [ - new Criterion\IsBookmarked(true, 14), - 'bookmark.user_id = :dcValue1', - ['dcValue1' => 14], + 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 'Bookmarks locations for user_id=14 OR user_id=7' => [ + yield 'IsBookmarked(true) OR IsBookmarked(false)' => [ new Criterion\LogicalOr( [ - new Criterion\IsBookmarked(true, 14), - new Criterion\IsBookmarked(true, 7), - ] + new Criterion\Location\IsBookmarked(true), + new Criterion\Location\IsBookmarked(false), + ] ), - '(bookmark.user_id = :dcValue1) OR (bookmark.user_id = :dcValue2)', - ['dcValue1' => 14, 'dcValue2' => 7], - ]; - - yield 'Bookmarks locations for user_id=7' => [ - new Criterion\IsBookmarked(true, 7), - 'bookmark.user_id = :dcValue1', - ['dcValue1' => 7], + 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 { - return [new BookmarkQueryBuilder($this->createMock(PermissionResolver::class))]; + $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..292e1eda1f --- /dev/null +++ b/tests/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/Bookmark/IdSortClauseQueryBuilderTest.php @@ -0,0 +1,64 @@ + 'sqlite:///:memory:']); + $queryBuilder = new FilteringQueryBuilder($connection); + $queryBuilder->select('location.node_id')->from('ezcontentobject_tree', 'location'); + + $userReference = $this->createMock(UserReference::class); + $userReference->method('getUserId')->willReturn(self::CURRENT_USER_ID); + + $permissionResolver = $this->createMock(PermissionResolver::class); + $permissionResolver->method('getCurrentUserReference')->willReturn($userReference); + + $builder = new IdSortClauseQueryBuilder($permissionResolver); + $sortClause = new Id(Query::SORT_DESC); + + self::assertTrue($builder->accepts($sortClause)); + + $builder->buildQuery($queryBuilder, $sortClause); + + self::assertContains( + 'ibexa_sort_bookmark.id', + $queryBuilder->getQueryPart('select') + ); + + $joins = $queryBuilder->getQueryPart('join'); + self::assertArrayHasKey('location', $joins); + self::assertSame(DoctrineDatabase::TABLE_BOOKMARKS, $joins['location'][0]['joinTable']); + self::assertSame('ibexa_sort_bookmark', $joins['location'][0]['joinAlias']); + self::assertSame( + '(location.node_id = ibexa_sort_bookmark.node_id) AND (ibexa_sort_bookmark.user_id = :dcValue1)', + (string)$joins['location'][0]['joinCondition'] + ); + self::assertSame(['dcValue1' => self::CURRENT_USER_ID], $queryBuilder->getParameters()); + + self::assertSame(['ibexa_sort_bookmark.id DESC'], $queryBuilder->getQueryPart('orderBy')); + } +} From 87679d7486394bebf38da18c567e948c8c0b0f58 Mon Sep 17 00:00:00 2001 From: Vidar Langseid Date: Mon, 3 Aug 2026 12:00:13 +0200 Subject: [PATCH 04/15] Tests: Extend IsBookmarked true/false coverage to Content filtering --- .../Filtering/Criterion/IsBookmarkedTest.php | 64 +++++++------------ 1 file changed, 23 insertions(+), 41 deletions(-) diff --git a/tests/integration/Core/Repository/Filtering/Criterion/IsBookmarkedTest.php b/tests/integration/Core/Repository/Filtering/Criterion/IsBookmarkedTest.php index e09095bb16..f905dbd582 100644 --- a/tests/integration/Core/Repository/Filtering/Criterion/IsBookmarkedTest.php +++ b/tests/integration/Core/Repository/Filtering/Criterion/IsBookmarkedTest.php @@ -19,6 +19,15 @@ final class IsBookmarkedTest extends BaseTest { private const BOOKMARKED_LOCATION_ID = 52; + /** + * @return iterable + */ + public function serviceProvider(): iterable + { + yield 'Location' => ['getLocationService']; + yield 'Content' => ['getContentService']; + } + public function testBookmarkedAndNotBookmarkedCountsMatchTotal(): void { $repository = $this->getRepository(false); @@ -48,21 +57,22 @@ public function testBookmarkedAndNotBookmarkedCountsMatchTotal(): void } /** - * @return iterable + * @return iterable */ public function isBookmarkedProvider(): iterable { - // [isBookmarkedCriterion, initialCount, afterCreateCount, afterDeleteCount] - return [ - 'bookmarked=true' => [true, 0, 1, 0], - 'bookmarked=false' => [false, 1, 0, 1], - ]; + // [initialCount, afterCreateCount, afterDeleteCount] per isBookmarkedCriterion value + foreach ($this->serviceProvider() as $serviceLabel => [$serviceGetter]) { + yield "$serviceLabel, bookmarked=true" => [$serviceGetter, true, 0, 1, 0]; + yield "$serviceLabel, bookmarked=false" => [$serviceGetter, false, 1, 0, 1]; + } } /** * @dataProvider isBookmarkedProvider */ public function testIsBookmarkedTrueAndFalse( + string $serviceGetter, bool $isBookmarked, int $initialCount, int $afterCreateCount, @@ -70,44 +80,42 @@ public function testIsBookmarkedTrueAndFalse( ): void { $repository = $this->getRepository(false); $locationService = $repository->getLocationService(); + $service = $repository->$serviceGetter(); $bookmarkService = $repository->getBookmarkService(); - $filesLocation = $locationService->loadLocation(self::BOOKMARKED_LOCATION_ID); + $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)); - $locations = $locationService->find($filter); self::assertCount( $initialCount, - $locations, + $service->find($filter), 'Unexpected initial bookmark state for IsBookmarked(' . ($isBookmarked ? 'true' : 'false') . ')' ); - $bookmarkService->createBookmark($filesLocation); + $bookmarkService->createBookmark($bookmarkedLocation); $filter = new Filter(); $filter->withCriterion(new Criterion\Location\IsBookmarked($isBookmarked)) ->andWithCriterion(new Criterion\LocationId(self::BOOKMARKED_LOCATION_ID)); - $locations = $locationService->find($filter); self::assertCount( $afterCreateCount, - $locations, + $service->find($filter), 'Unexpected state after creating bookmark for IsBookmarked(' . ($isBookmarked ? 'true' : 'false') . ')' ); - $bookmarkService->deleteBookmark($filesLocation); + $bookmarkService->deleteBookmark($bookmarkedLocation); $filter = new Filter(); $filter->withCriterion(new Criterion\Location\IsBookmarked($isBookmarked)) ->andWithCriterion(new Criterion\LocationId(self::BOOKMARKED_LOCATION_ID)); - $locations = $locationService->find($filter); self::assertCount( $afterDeleteCount, - $locations, + $service->find($filter), 'Unexpected state after deleting bookmark for IsBookmarked(' . ($isBookmarked ? 'true' : 'false') . ')' ); } @@ -150,30 +158,4 @@ static function ($location) { $bookmarkService->deleteBookmark($bookmarkedLocation); } } - - public function testIsBookmarkedFiltersContentAsWellAsLocations(): 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(true)) - ->andWithCriterion(new Criterion\LocationId(self::BOOKMARKED_LOCATION_ID)); - self::assertCount(0, $contentService->find($filter)); - - $bookmarkService->createBookmark($bookmarkedLocation); - - try { - $filter = new Filter(); - $filter->withCriterion(new Criterion\Location\IsBookmarked(true)) - ->andWithCriterion(new Criterion\LocationId(self::BOOKMARKED_LOCATION_ID)); - self::assertCount(1, $contentService->find($filter)); - } finally { - $bookmarkService->deleteBookmark($bookmarkedLocation); - } - } } From c0dddfda491ded64a846a42857c94fb63ae2b7e1 Mon Sep 17 00:00:00 2001 From: Vidar Langseid Date: Mon, 3 Aug 2026 12:23:08 +0200 Subject: [PATCH 05/15] Review comment: Throw Core InvalidArgumentException from IsBookmarkedQueryBuilder --- .../Location/IsBookmarkedQueryBuilder.php | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/IsBookmarkedQueryBuilder.php b/src/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/IsBookmarkedQueryBuilder.php index ae00011fef..92da3f095d 100644 --- a/src/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/IsBookmarkedQueryBuilder.php +++ b/src/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/IsBookmarkedQueryBuilder.php @@ -14,6 +14,7 @@ use Ibexa\Contracts\Core\Repository\PermissionResolver; use Ibexa\Contracts\Core\Repository\Values\Content\Query\Criterion\Location\IsBookmarked; use Ibexa\Contracts\Core\Repository\Values\Filter\FilteringCriterion; +use Ibexa\Core\Base\Exceptions\InvalidArgumentException; use Ibexa\Core\Persistence\Legacy\Bookmark\Gateway\DoctrineDatabase; /** @@ -34,6 +35,9 @@ public function accepts(FilteringCriterion $criterion): bool return $criterion instanceof IsBookmarked; } + /** + * @throws \Ibexa\Contracts\Core\Repository\Exceptions\InvalidArgumentException + */ public function buildQueryConstraint( FilteringQueryBuilder $queryBuilder, FilteringCriterion $criterion @@ -43,7 +47,10 @@ public function buildQueryConstraint( /** @var \Ibexa\Contracts\Core\Repository\Values\Content\Query\Criterion\Location\IsBookmarked $criterion */ $isBookmarked = $criterion->value[0] ?? null; if (!is_bool($isBookmarked)) { - throw new \InvalidArgumentException('IsBookmarked criterion value must be boolean at index 0.'); + throw new InvalidArgumentException( + '$criterion', + 'IsBookmarked criterion value must be boolean at index 0.' + ); } $userId = $this->permissionResolver->getCurrentUserReference()->getUserId(); From 8953c21a93bb2c3a1658bedf2722819b776437db Mon Sep 17 00:00:00 2001 From: Vidar Langseid Date: Mon, 3 Aug 2026 12:55:31 +0200 Subject: [PATCH 06/15] Move IsBookmarked integration tests into LocationFilteringTest/ContentFilteringTest and LocationFilteringTest --- .../Filtering/ContentFilteringTest.php | 63 +++++++ .../Filtering/Criterion/IsBookmarkedTest.php | 161 ------------------ .../Filtering/LocationFilteringTest.php | 128 ++++++++++++++ 3 files changed, 191 insertions(+), 161 deletions(-) delete mode 100644 tests/integration/Core/Repository/Filtering/Criterion/IsBookmarkedTest.php 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/Criterion/IsBookmarkedTest.php b/tests/integration/Core/Repository/Filtering/Criterion/IsBookmarkedTest.php deleted file mode 100644 index f905dbd582..0000000000 --- a/tests/integration/Core/Repository/Filtering/Criterion/IsBookmarkedTest.php +++ /dev/null @@ -1,161 +0,0 @@ - - */ - public function serviceProvider(): iterable - { - yield 'Location' => ['getLocationService']; - yield 'Content' => ['getContentService']; - } - - public function testBookmarkedAndNotBookmarkedCountsMatchTotal(): void - { - $repository = $this->getRepository(false); - $locationService = $repository->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 - { - // [initialCount, afterCreateCount, afterDeleteCount] per isBookmarkedCriterion value - foreach ($this->serviceProvider() as $serviceLabel => [$serviceGetter]) { - yield "$serviceLabel, bookmarked=true" => [$serviceGetter, true, 0, 1, 0]; - yield "$serviceLabel, bookmarked=false" => [$serviceGetter, false, 1, 0, 1]; - } - } - - /** - * @dataProvider isBookmarkedProvider - */ - public function testIsBookmarkedTrueAndFalse( - string $serviceGetter, - bool $isBookmarked, - int $initialCount, - int $afterCreateCount, - int $afterDeleteCount - ): void { - $repository = $this->getRepository(false); - $locationService = $repository->getLocationService(); - $service = $repository->$serviceGetter(); - $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, - $service->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, - $service->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, - $service->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->id; - }, - 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); - } - } -} diff --git a/tests/integration/Core/Repository/Filtering/LocationFilteringTest.php b/tests/integration/Core/Repository/Filtering/LocationFilteringTest.php index f5d165f34d..168e9c30ca 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->id; + }, + 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 */ From 0c995ac474317dbbffe8e7b219a2a69e49a86cc8 Mon Sep 17 00:00:00 2001 From: Vidar Langseid Date: Thu, 6 Aug 2026 13:08:34 +0200 Subject: [PATCH 07/15] Review comment: Use ALIAS/COLUMN_LOCATION_ID consts and @param docblock in IsBookmarkedQueryBuilder --- .../Location/IsBookmarkedQueryBuilder.php | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/IsBookmarkedQueryBuilder.php b/src/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/IsBookmarkedQueryBuilder.php index 92da3f095d..31721cf939 100644 --- a/src/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/IsBookmarkedQueryBuilder.php +++ b/src/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/IsBookmarkedQueryBuilder.php @@ -22,6 +22,8 @@ */ final class IsBookmarkedQueryBuilder extends BaseLocationCriterionQueryBuilder { + private const ALIAS = 'bookmark'; + private PermissionResolver $permissionResolver; public function __construct( @@ -36,6 +38,8 @@ public function accepts(FilteringCriterion $criterion): bool } /** + * @param \Ibexa\Contracts\Core\Repository\Values\Content\Query\Criterion\Location\IsBookmarked $criterion + * * @throws \Ibexa\Contracts\Core\Repository\Exceptions\InvalidArgumentException */ public function buildQueryConstraint( @@ -44,7 +48,6 @@ public function buildQueryConstraint( ): string { parent::buildQueryConstraint($queryBuilder, $criterion); - /** @var \Ibexa\Contracts\Core\Repository\Values\Content\Query\Criterion\Location\IsBookmarked $criterion */ $isBookmarked = $criterion->value[0] ?? null; if (!is_bool($isBookmarked)) { throw new InvalidArgumentException( @@ -58,13 +61,16 @@ public function buildQueryConstraint( $subQueryBuilder = new QueryBuilder($queryBuilder->getConnection()); $subQueryBuilder ->select('1') - ->from(DoctrineDatabase::TABLE_BOOKMARKS, 'bookmark') + ->from(DoctrineDatabase::TABLE_BOOKMARKS, self::ALIAS) ->where( $subQueryBuilder->expr()->eq( - 'bookmark.' . DoctrineDatabase::COLUMN_USER_ID, + self::ALIAS . '.' . DoctrineDatabase::COLUMN_USER_ID, $queryBuilder->createNamedParameter($userId, ParameterType::INTEGER) ), - $subQueryBuilder->expr()->eq('bookmark.node_id', 'location.node_id') + $subQueryBuilder->expr()->eq( + self::ALIAS . '.' . DoctrineDatabase::COLUMN_LOCATION_ID, + 'location.node_id' + ) ); return sprintf( From 4e223f8d8c561157e92c9f2b9bb54dd42c5f579d Mon Sep 17 00:00:00 2001 From: Vidar Langseid Date: Thu, 6 Aug 2026 13:08:45 +0200 Subject: [PATCH 08/15] Review comment: Document Content filtering matches main Locations only in IsBookmarked criterion --- .../Values/Content/Query/Criterion/Location/IsBookmarked.php | 2 ++ 1 file changed, 2 insertions(+) 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 { From 976d8016cf6da3341f30b609b1268fe7605f341e Mon Sep 17 00:00:00 2001 From: Vidar Langseid Date: Thu, 6 Aug 2026 13:08:57 +0200 Subject: [PATCH 09/15] Review comment: Use Core InvalidArgumentException, and(), and @internal in IdSortClauseQueryBuilder --- .../Bookmark/IdSortClauseQueryBuilder.php | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/Bookmark/IdSortClauseQueryBuilder.php b/src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/Bookmark/IdSortClauseQueryBuilder.php index b5ff52b110..128a2b1b8a 100644 --- a/src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/Bookmark/IdSortClauseQueryBuilder.php +++ b/src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/Bookmark/IdSortClauseQueryBuilder.php @@ -14,8 +14,12 @@ use Ibexa\Contracts\Core\Repository\Values\Content\Query\SortClause\Location\Bookmark\Id; use Ibexa\Contracts\Core\Repository\Values\Filter\FilteringSortClause; use Ibexa\Contracts\Core\Repository\Values\Filter\SortClauseQueryBuilder; +use Ibexa\Core\Base\Exceptions\InvalidArgumentException; use Ibexa\Core\Persistence\Legacy\Bookmark\Gateway\DoctrineDatabase; +/** + * @internal + */ final class IdSortClauseQueryBuilder implements SortClauseQueryBuilder { private const ALIAS = 'ibexa_sort_bookmark'; @@ -32,16 +36,18 @@ 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(sprintf( - 'Expected %s, got %s', - Id::class, - get_class($sortClause), - )); + throw new InvalidArgumentException( + '$sortClause', + sprintf('Expected %s, got %s', Id::class, get_class($sortClause)) + ); } $userId = $this->permissionResolver->getCurrentUserReference()->getUserId(); @@ -50,7 +56,7 @@ public function buildQuery( 'location', DoctrineDatabase::TABLE_BOOKMARKS, self::ALIAS, - (string)$queryBuilder->expr()->andX( + (string)$queryBuilder->expr()->and( sprintf('location.node_id = %s.node_id', self::ALIAS), $queryBuilder->expr()->eq( sprintf('%s.%s', self::ALIAS, DoctrineDatabase::COLUMN_USER_ID), From 7c4d80fd31fb64088baf2e05146178e07f5d8e11 Mon Sep 17 00:00:00 2001 From: Vidar Langseid Date: Thu, 6 Aug 2026 13:09:08 +0200 Subject: [PATCH 10/15] Review comment: Extend SortClause\Location instead of SortClause in Bookmark\Id --- .../Values/Content/Query/SortClause/Location/Bookmark/Id.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 index 4cc4e8a4af..29cb02d232 100644 --- a/src/contracts/Repository/Values/Content/Query/SortClause/Location/Bookmark/Id.php +++ b/src/contracts/Repository/Values/Content/Query/SortClause/Location/Bookmark/Id.php @@ -9,13 +9,13 @@ namespace Ibexa\Contracts\Core\Repository\Values\Content\Query\SortClause\Location\Bookmark; use Ibexa\Contracts\Core\Repository\Values\Content\Query; -use Ibexa\Contracts\Core\Repository\Values\Content\Query\SortClause; +use Ibexa\Contracts\Core\Repository\Values\Content\Query\SortClause\Location; use Ibexa\Contracts\Core\Repository\Values\Filter\FilteringSortClause; /** * Sets sort direction on the bookmark id for a location query containing IsBookmarked criterion. */ -final class Id extends SortClause implements FilteringSortClause +final class Id extends Location implements FilteringSortClause { public function __construct(string $sortDirection = Query::SORT_ASC) { From c035aa5126c9f430acacee314031dd8a86816cfb Mon Sep 17 00:00:00 2001 From: Vidar Langseid Date: Thu, 6 Aug 2026 13:09:23 +0200 Subject: [PATCH 11/15] Review comment: Simplify BookmarkService constructor doc, catch RepositoryException, log at error level --- src/lib/Repository/BookmarkService.php | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/lib/Repository/BookmarkService.php b/src/lib/Repository/BookmarkService.php index 276b80e53c..e838766509 100644 --- a/src/lib/Repository/BookmarkService.php +++ b/src/lib/Repository/BookmarkService.php @@ -12,7 +12,7 @@ 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\BadStateException; +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; @@ -32,12 +32,6 @@ class BookmarkService implements BookmarkServiceInterface private LoggerInterface $logger; - /** - * BookmarkService constructor. - * - * @param \Ibexa\Contracts\Core\Repository\Repository $repository - * @param \Ibexa\Contracts\Core\Persistence\Bookmark\Handler $bookmarkHandler - */ public function __construct(RepositoryInterface $repository, BookmarkHandler $bookmarkHandler, ?LoggerInterface $logger = null) { $this->repository = $repository; @@ -110,8 +104,8 @@ public function loadBookmarks(int $offset = 0, int $limit = 25): BookmarkList ->sliceBy($limit, $offset); $result = $this->repository->getLocationService()->find($filter, []); - } catch (BadStateException $e) { - $this->logger->debug($e->getMessage(), [ + } catch (RepositoryException $e) { + $this->logger->error($e->getMessage(), [ 'exception' => $e, ]); From cb6bddb5396adaa2966f2a559c91abbfe742bfcb Mon Sep 17 00:00:00 2001 From: Vidar Langseid Date: Thu, 6 Aug 2026 13:09:50 +0200 Subject: [PATCH 12/15] Review comment: Rename BookmarkQueryBuilderTest to IsBookmarkedQueryBuilderTest --- ...arkQueryBuilderTest.php => IsBookmarkedQueryBuilderTest.php} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/{BookmarkQueryBuilderTest.php => IsBookmarkedQueryBuilderTest.php} (96%) diff --git a/tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/BookmarkQueryBuilderTest.php b/tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/IsBookmarkedQueryBuilderTest.php similarity index 96% rename from tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/BookmarkQueryBuilderTest.php rename to tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/IsBookmarkedQueryBuilderTest.php index 0d608b025e..2ff31b3a3f 100644 --- a/tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/BookmarkQueryBuilderTest.php +++ b/tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Location/IsBookmarkedQueryBuilderTest.php @@ -17,7 +17,7 @@ /** * @covers \Ibexa\Core\Persistence\Legacy\Filter\CriterionQueryBuilder\Location\IsBookmarkedQueryBuilder */ -final class BookmarkQueryBuilderTest extends BaseCriterionVisitorQueryBuilderTestCase +final class IsBookmarkedQueryBuilderTest extends BaseCriterionVisitorQueryBuilderTestCase { private const CURRENT_USER_ID = 14; From 5a633268987d4b60766dd96055e44c22677dd5d2 Mon Sep 17 00:00:00 2001 From: Vidar Langseid Date: Thu, 6 Aug 2026 13:10:09 +0200 Subject: [PATCH 13/15] Review comment: Bump Bookmark deprecation tags to 4.6.32 --- src/contracts/Persistence/Bookmark/Handler.php | 4 ++-- src/lib/Persistence/Legacy/Bookmark/Gateway.php | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/contracts/Persistence/Bookmark/Handler.php b/src/contracts/Persistence/Bookmark/Handler.php index 25b62a8429..dd5ea7dcf5 100644 --- a/src/contracts/Persistence/Bookmark/Handler.php +++ b/src/contracts/Persistence/Bookmark/Handler.php @@ -50,7 +50,7 @@ public function loadUserIdsByLocation(Location $location): array; /** * Loads bookmarks owned by user. * - * @deprecated 4.6.30 The "Handler::loadUserBookmarks()" method is deprecated, will be removed in 6.0.0. Use "LocationService::find()" and "Criterion\Location\IsBookmarked" instead. + * @deprecated 4.6.32 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 @@ -63,7 +63,7 @@ public function loadUserBookmarks(int $userId, int $offset = 0, int $limit = -1) /** * Count bookmarks owned by user. * - * @deprecated 4.6.30 The "Handler::countUserBookmarks()" method is deprecated, will be removed in 6.0.0. Use "LocationService::count()" and "Criterion\Location\IsBookmarked" instead. + * @deprecated 4.6.32 The "Handler::countUserBookmarks()" method is deprecated, will be removed in 6.0.0. Use "LocationService::count()" and "Criterion\Location\IsBookmarked" instead. * * @param int $userId * diff --git a/src/lib/Persistence/Legacy/Bookmark/Gateway.php b/src/lib/Persistence/Legacy/Bookmark/Gateway.php index ecdd13d476..e3f1204e60 100644 --- a/src/lib/Persistence/Legacy/Bookmark/Gateway.php +++ b/src/lib/Persistence/Legacy/Bookmark/Gateway.php @@ -52,7 +52,7 @@ abstract public function loadUserIdsByLocation(Location $location): array; /** * Load data for all bookmarks owned by given $userId. * - * @deprecated 4.6.30 Gateway::loadUserBookmarks()" method is deprecated, will be removed in 6.0.0. Use "LocationService::find()" and "Criterion\Location\IsBookmarked" instead. + * @deprecated 4.6.32 "Gateway::loadUserBookmarks()" method is deprecated, will be removed in 6.0.0. Use "LocationService::find()" and "Criterion\Location\IsBookmarked" instead. * * @param int $userId ID of user * @param int $offset Offset to start listing from, 0 by default @@ -65,7 +65,7 @@ abstract public function loadUserBookmarks(int $userId, int $offset = 0, int $li /** * Count bookmarks owned by given $userId. * - * @deprecated 4.6.30 The "Gateway::countUserBookmarks()" method is deprecated, will be removed in 6.0.0. Use "LocationService::count()" and "Criterion\Location\IsBookmarked" instead. + * @deprecated 4.6.32 The "Gateway::countUserBookmarks()" method is deprecated, will be removed in 6.0.0. Use "LocationService::count()" and "Criterion\Location\IsBookmarked" instead. * * @param int $userId ID of user * From a0dcc03ec861ea783f71b26728f97bc64178f37c Mon Sep 17 00:00:00 2001 From: Vidar Langseid Date: Thu, 6 Aug 2026 14:23:54 +0200 Subject: [PATCH 14/15] Review comment: Reuse BaseLocationSortClauseQueryBuilder in bookmark Id sort clause --- .../BaseLocationSortClauseQueryBuilder.php | 19 +- .../Bookmark/IdSortClauseQueryBuilder.php | 36 +++- .../Bookmark/IdSortClauseQueryBuilderTest.php | 185 ++++++++++++++++-- 3 files changed, 213 insertions(+), 27 deletions(-) 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 index 128a2b1b8a..b3d2f57822 100644 --- a/src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/Bookmark/IdSortClauseQueryBuilder.php +++ b/src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/Bookmark/IdSortClauseQueryBuilder.php @@ -13,14 +13,14 @@ use Ibexa\Contracts\Core\Repository\PermissionResolver; use Ibexa\Contracts\Core\Repository\Values\Content\Query\SortClause\Location\Bookmark\Id; use Ibexa\Contracts\Core\Repository\Values\Filter\FilteringSortClause; -use Ibexa\Contracts\Core\Repository\Values\Filter\SortClauseQueryBuilder; use Ibexa\Core\Base\Exceptions\InvalidArgumentException; use Ibexa\Core\Persistence\Legacy\Bookmark\Gateway\DoctrineDatabase; +use Ibexa\Core\Persistence\Legacy\Filter\SortClauseQueryBuilder\Location\BaseLocationSortClauseQueryBuilder; /** * @internal */ -final class IdSortClauseQueryBuilder implements SortClauseQueryBuilder +final class IdSortClauseQueryBuilder extends BaseLocationSortClauseQueryBuilder { private const ALIAS = 'ibexa_sort_bookmark'; @@ -50,22 +50,46 @@ public function buildQuery( ); } + parent::buildQuery($queryBuilder, $sortClause); + } + + protected function joinAdditionalTables( + FilteringQueryBuilder $queryBuilder, + string $locationAlias + ): void { $userId = $this->permissionResolver->getCurrentUserReference()->getUserId(); $queryBuilder->leftJoinOnce( - 'location', + $locationAlias, DoctrineDatabase::TABLE_BOOKMARKS, self::ALIAS, (string)$queryBuilder->expr()->and( - sprintf('location.node_id = %s.node_id', self::ALIAS), + 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(); + } - $queryBuilder->addSelect(self::ALIAS . '.id'); - $queryBuilder->addOrderBy(self::ALIAS . '.id', $sortClause->direction); + protected function getSortFieldName(string $sortExpression): string + { + return 'bookmark_id'; } } diff --git a/tests/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/Bookmark/IdSortClauseQueryBuilderTest.php b/tests/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/Bookmark/IdSortClauseQueryBuilderTest.php index 292e1eda1f..f31817a6d2 100644 --- a/tests/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/Bookmark/IdSortClauseQueryBuilderTest.php +++ b/tests/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/Location/Bookmark/IdSortClauseQueryBuilderTest.php @@ -15,6 +15,7 @@ use Ibexa\Contracts\Core\Repository\Values\Content\Query\SortClause\Location\Bookmark\Id; use Ibexa\Contracts\Core\Repository\Values\User\UserReference; use Ibexa\Core\Persistence\Legacy\Bookmark\Gateway\DoctrineDatabase; +use Ibexa\Core\Persistence\Legacy\Content\Location\Gateway as LocationGateway; use Ibexa\Core\Persistence\Legacy\Filter\SortClauseQueryBuilder\Location\Bookmark\IdSortClauseQueryBuilder; use PHPUnit\Framework\TestCase; @@ -24,20 +25,20 @@ final class IdSortClauseQueryBuilderTest extends TestCase { private const CURRENT_USER_ID = 14; + private const BOOKMARK_ALIAS = 'ibexa_sort_bookmark'; + private const CONTENT_LOCATION_ALIAS = 'ibexa_sort_location'; + private const SORT_ALIAS = 'ibexa_filter_sort_bookmark_id'; + private const CONTENT_ITEM_TABLE = 'ezcontentobject'; - public function testBuildQueryJoinsBookmarksForCurrentUserAndOrdersByBookmarkId(): void + /** + * Location filtering: "location" is the FROM table, so the bookmarks table is joined + * directly against it. + */ + public function testBuildQueryInLocationFilteringContext(): void { - $connection = DriverManager::getConnection(['url' => 'sqlite:///:memory:']); - $queryBuilder = new FilteringQueryBuilder($connection); - $queryBuilder->select('location.node_id')->from('ezcontentobject_tree', 'location'); + $queryBuilder = $this->createLocationFilteringQueryBuilder(); - $userReference = $this->createMock(UserReference::class); - $userReference->method('getUserId')->willReturn(self::CURRENT_USER_ID); - - $permissionResolver = $this->createMock(PermissionResolver::class); - $permissionResolver->method('getCurrentUserReference')->willReturn($userReference); - - $builder = new IdSortClauseQueryBuilder($permissionResolver); + $builder = $this->createBuilder(); $sortClause = new Id(Query::SORT_DESC); self::assertTrue($builder->accepts($sortClause)); @@ -45,20 +46,170 @@ public function testBuildQueryJoinsBookmarksForCurrentUserAndOrdersByBookmarkId( $builder->buildQuery($queryBuilder, $sortClause); self::assertContains( - 'ibexa_sort_bookmark.id', + sprintf('%s.id AS %s', self::BOOKMARK_ALIAS, self::SORT_ALIAS), $queryBuilder->getQueryPart('select') ); $joins = $queryBuilder->getQueryPart('join'); self::assertArrayHasKey('location', $joins); - self::assertSame(DoctrineDatabase::TABLE_BOOKMARKS, $joins['location'][0]['joinTable']); - self::assertSame('ibexa_sort_bookmark', $joins['location'][0]['joinAlias']); + + $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( - '(location.node_id = ibexa_sort_bookmark.node_id) AND (ibexa_sort_bookmark.user_id = :dcValue1)', - (string)$joins['location'][0]['joinCondition'] + 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(['ibexa_sort_bookmark.id DESC'], $queryBuilder->getQueryPart('orderBy')); + 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:']); } } From daec766448eb4d2adb0b54f5343e5d3b920bc456 Mon Sep 17 00:00:00 2001 From: Vidar Langseid Date: Fri, 7 Aug 2026 10:51:43 +0200 Subject: [PATCH 15/15] Added regression tests for bookmarks of inaccessible and removed items --- .../Core/Repository/BookmarkServiceTest.php | 197 ++++++++++++++++-- .../Filtering/LocationFilteringTest.php | 2 +- .../Core/Repository/LocationServiceTest.php | 25 ++- 3 files changed, 204 insertions(+), 20 deletions(-) diff --git a/tests/integration/Core/Repository/BookmarkServiceTest.php b/tests/integration/Core/Repository/BookmarkServiceTest.php index 42c03d8686..cfb194935a 100644 --- a/tests/integration/Core/Repository/BookmarkServiceTest.php +++ b/tests/integration/Core/Repository/BookmarkServiceTest.php @@ -8,9 +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\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. @@ -22,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(); @@ -37,9 +40,6 @@ public function testIsBookmarked() $this->assertTrue($isBookmarked); } - /** - * @covers \Ibexa\Contracts\Core\Repository\BookmarkService::isBookmarked - */ public function testIsNotBookmarked() { $repository = $this->getRepository(); @@ -52,9 +52,6 @@ public function testIsNotBookmarked() $this->assertFalse($isBookmarked); } - /** - * @covers \Ibexa\Contracts\Core\Repository\BookmarkService::createBookmark - */ public function testCreateBookmark() { $repository = $this->getRepository(); @@ -74,7 +71,6 @@ public function testCreateBookmark() } /** - * @covers \Ibexa\Contracts\Core\Repository\BookmarkService::createBookmark * @depends testCreateBookmark */ public function testCreateBookmarkThrowsInvalidArgumentException() @@ -92,9 +88,6 @@ public function testCreateBookmarkThrowsInvalidArgumentException() /* END: Use Case */ } - /** - * @covers \Ibexa\Contracts\Core\Repository\BookmarkService::deleteBookmark - */ public function testDeleteBookmark() { $repository = $this->getRepository(); @@ -115,7 +108,6 @@ public function testDeleteBookmark() } /** - * @covers \Ibexa\Contracts\Core\Repository\BookmarkService::deleteBookmark * @depends testDeleteBookmark */ public function testDeleteBookmarkThrowsInvalidArgumentException() @@ -133,9 +125,6 @@ public function testDeleteBookmarkThrowsInvalidArgumentException() /* END: Use Case */ } - /** - * @covers \Ibexa\Contracts\Core\Repository\BookmarkService::loadBookmarks - */ public function testLoadBookmarks() { $repository = $this->getRepository(); @@ -162,6 +151,180 @@ public function testCountBookmarks(): void 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/LocationFilteringTest.php b/tests/integration/Core/Repository/Filtering/LocationFilteringTest.php index 168e9c30ca..f27807032b 100644 --- a/tests/integration/Core/Repository/Filtering/LocationFilteringTest.php +++ b/tests/integration/Core/Repository/Filtering/LocationFilteringTest.php @@ -135,7 +135,7 @@ public function testLogicalOrOfBookmarkedAndNotBookmarkedMatchesEveryLocationOnc $locationIds = array_map( static function ($location) { - return $location->id; + return $location->getId(); }, iterator_to_array($locationService->find($orFilter)) ); diff --git a/tests/integration/Core/Repository/LocationServiceTest.php b/tests/integration/Core/Repository/LocationServiceTest.php index ef4e505c1f..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; @@ -1990,8 +1991,8 @@ public function testBookmarksAreSwappedAfterSwapLocation() $afterSwap = $bookmarkService->loadBookmarks(); /* END: Use Case */ - self::assertEquals($contactUsLocationId, $afterSwap->items[0]->id); - self::assertEquals($beforeSwap->items[1]->id, $afterSwap->items[1]->id); + self::assertEquals($contactUsLocationId, $afterSwap->items[0]->getId()); + self::assertEquals($beforeSwap->items[1]->getId(), $afterSwap->items[1]->getId()); } /** @@ -2344,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' + ); } /**