diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index 42a35b167e..204c3d6511 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -32256,12 +32256,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 @@ -69570,12 +69564,6 @@ parameters: count: 1 path: tests/lib/Repository/Service/Mock/BookmarkTest.php - - - message: '#^Method Ibexa\\Tests\\Core\\Repository\\Service\\Mock\\BookmarkTest\:\:testLoadBookmarksEmptyList\(\) has no return type specified\.$#' - identifier: missingType.return - count: 1 - path: tests/lib/Repository/Service/Mock/BookmarkTest.php - - message: '#^Method Ibexa\\Tests\\Core\\Repository\\Service\\Mock\\BookmarkTest\:\:testLocationShouldBeBookmarked\(\) has no return type specified\.$#' identifier: missingType.return diff --git a/src/contracts/Persistence/Bookmark/Handler.php b/src/contracts/Persistence/Bookmark/Handler.php index f9ef1a1cae..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