IBX-6773: Bookmarks for non accessible contents cause exception

eZ/Publish/API/Repository/Tests/LocationServiceTest.php

@@ -1965,13 +1965,15 @@ public function testBookmarksAreSwappedAfterSwapLocation()
 
         $mediaLocationId = $this->generateId('location', 43);
         $demoDesignLocationId = $this->generateId('location', 56);
+        $contactUsLocationId = $this->generateId('location', 60);
 
         /* BEGIN: Use Case */
         $locationService = $repository->getLocationService();
         $bookmarkService = $repository->getBookmarkService();
 
         $mediaLocation = $locationService->loadLocation($mediaLocationId);
         $demoDesignLocation = $locationService->loadLocation($demoDesignLocationId);
+        $contactUsLocation = $locationService->loadLocation($contactUsLocationId);
 
         // Bookmark locations
         $bookmarkService->createBookmark($mediaLocation);
@@ -1980,13 +1982,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);
+        $this->assertEquals($contactUsLocationId, $afterSwap->items[0]->id);
+        $this->assertEquals($beforeSwap->items[1]->id, $afterSwap->items[1]->id);
     }
 
     /**

eZ/Publish/Core/Persistence/Cache/BookmarkHandler.php

@@ -85,6 +85,8 @@ function (Bookmark $bookmark) {
     }
 
     /**
+     * @deprecated The "BookmarkHandler::loadUserBookmarks()" method is deprecated, will be removed in 5.0.0. Use "LocationService::find()" and "Criterion\IsBookmarked" instead.
+     *
      * {@inheritdoc}
      */
     public function loadUserBookmarks(int $userId, int $offset = 0, int $limit = -1): array
@@ -99,6 +101,8 @@ public function loadUserBookmarks(int $userId, int $offset = 0, int $limit = -1)
     }
 
     /**
+     * @deprecated The "BookmarkHandler::countUserBookmarks()" method is deprecated, will be removed in 5.0.0. Use "LocationService::count()" and "Criterion\IsBookmarked" instead.
+     *
      * {@inheritdoc}
      */
     public function countUserBookmarks(int $userId): int

eZ/Publish/Core/Persistence/Legacy/Bookmark/Gateway.php

@@ -44,6 +44,8 @@ abstract public function loadBookmarkDataByUserIdAndLocationId(int $userId, arra
     /**
      * Load data for all bookmarks owned by given $userId.
      *
+     * @deprecated Gateway::loadUserBookmarks()" method is deprecated, will be removed in 5.0.0. Use "LocationService::find()" and "Criterion\IsBookmarked" instead.
+     *
      * @param int $userId ID of user
      * @param int $offset Offset to start listing from, 0 by default
      * @param int $limit Limit for the listing. -1 by default (no limit)
@@ -55,6 +57,8 @@ abstract public function loadUserBookmarks(int $userId, int $offset = 0, int $li
     /**
      * Count bookmarks owned by given $userId.
      *
+     * @deprecated The "Gateway::countUserBookmarks()" method is deprecated, will be removed in 5.0.0. Use "LocationService::count()" and "Criterion\IsBookmarked" instead.
+     *
      * @param int $userId ID of user
      *
      * @return int

eZ/Publish/Core/Persistence/Legacy/Bookmark/Gateway/DoctrineDatabase.php

@@ -101,6 +101,8 @@ public function loadBookmarkDataByUserIdAndLocationId(int $userId, array $locati
     }
 
     /**
+     * @deprecated The "DoctrineDatabase::loadUserBookmarks()" method is deprecated, will be removed in 5.0.0. Use "LocationService::find()" and "Criterion\IsBookmarked" instead.
+     *
      * {@inheritdoc}
      */
     public function loadUserBookmarks(int $userId, int $offset = 0, int $limit = -1): array
@@ -123,6 +125,8 @@ public function loadUserBookmarks(int $userId, int $offset = 0, int $limit = -1)
     }
 
     /**
+     * @deprecated The "DoctrineDatabase::countUserBookmarks()" method is deprecated, will be removed in 5.0.0. Use "LocationService::count()" and "Criterion\IsBookmarked" instead.
+     *
      * {@inheritdoc}
      */
     public function countUserBookmarks(int $userId): int

eZ/Publish/Core/Persistence/Legacy/Bookmark/Gateway/ExceptionConversion.php

@@ -56,6 +56,11 @@ public function loadBookmarkDataByUserIdAndLocationId(int $userId, array $locati
         }
     }
 
+    /**
+     * @deprecated The "ExceptionConversion::loadUserBookmarks()" method is deprecated, will be removed in 5.0.0. Use "LocationService::find()" and "Criterion\IsBookmarked" instead.
+     *
+     * @return array
+     */
     public function loadUserBookmarks(int $userId, int $offset = 0, int $limit = -1): array
     {
         try {
@@ -65,6 +70,9 @@ public function loadUserBookmarks(int $userId, int $offset = 0, int $limit = -1)
         }
     }
 
+    /**
+     * @deprecated The "ExceptionConversion::countUserBookmarks()" method is deprecated, will be removed in 5.0.0. Use "LocationService::count()" and "Criterion\IsBookmarked" instead.
+     */
     public function countUserBookmarks(int $userId): int
     {
         try {

eZ/Publish/Core/Persistence/Legacy/Bookmark/Handler.php

@@ -74,6 +74,8 @@ public function loadByUserIdAndLocationId(int $userId, array $locationIds): arra
     }
 
     /**
+     * @deprecated The "Handler::loadUserBookmarks()" method is deprecated, will be removed in 5.0.0. Use "LocationService::find()" and "Criterion\IsBookmarked" instead.
+     *
      * {@inheritdoc}
      */
     public function loadUserBookmarks(int $userId, int $offset = 0, int $limit = -1): array
@@ -84,6 +86,8 @@ public function loadUserBookmarks(int $userId, int $offset = 0, int $limit = -1)
     }
 
     /**
+     * @deprecated The "Handler::countUserBookmarks()" method is deprecated, will be removed in 5.0.0. Use "LocationService::count()" and "Criterion\IsBookmarked" instead.
+     *
      * {@inheritdoc}
      */
     public function countUserBookmarks(int $userId): int

eZ/Publish/Core/Repository/BookmarkService.php

@@ -10,13 +10,19 @@
 
 use Exception;
 use eZ\Publish\API\Repository\BookmarkService as BookmarkServiceInterface;
+use eZ\Publish\API\Repository\Exceptions\BadStateException;
 use eZ\Publish\API\Repository\Repository as RepositoryInterface;
 use eZ\Publish\API\Repository\Values\Bookmark\BookmarkList;
 use eZ\Publish\API\Repository\Values\Content\Location;
+use eZ\Publish\API\Repository\Values\Content\Query;
+use eZ\Publish\API\Repository\Values\Filter\Filter;
 use eZ\Publish\Core\Base\Exceptions\InvalidArgumentException;
-use eZ\Publish\SPI\Persistence\Bookmark\Bookmark;
 use eZ\Publish\SPI\Persistence\Bookmark\CreateStruct;
 use eZ\Publish\SPI\Persistence\Bookmark\Handler as BookmarkHandler;
+use Ibexa\Contracts\Core\Repository\Values\Content\Query\Criterion;
+use Ibexa\Contracts\Core\Repository\Values\Content\Query\SortClause;
+use Psr\Log\LoggerInterface;
+use Psr\Log\NullLogger;
 
 class BookmarkService implements BookmarkServiceInterface
 {
@@ -26,16 +32,17 @@ class BookmarkService implements BookmarkServiceInterface
     /** @var \eZ\Publish\SPI\Persistence\Bookmark\Handler */
     protected $bookmarkHandler;
 
+    /** @var \Psr\Log\LoggerInterface */
+    private $logger;
+
     /**
      * BookmarkService constructor.
-     *
-     * @param \eZ\Publish\API\Repository\Repository $repository
-     * @param \eZ\Publish\SPI\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();
     }
 
     /**
@@ -97,16 +104,24 @@ public function loadBookmarks(int $offset = 0, int $limit = 25): BookmarkList
     {
         $currentUserId = $this->getCurrentUserId();
 
-        $list = new BookmarkList();
-        $list->totalCount = $this->bookmarkHandler->countUserBookmarks($currentUserId);
-        if ($list->totalCount > 0) {
-            $bookmarks = $this->bookmarkHandler->loadUserBookmarks($currentUserId, $offset, $limit);
+        $filter = new Filter();
+        try {
+            $filter
+                ->withCriterion(new Criterion\IsBookmarked($currentUserId))
+                ->withSortClause(new SortClause\BookmarkId(Query::SORT_DESC))
+                ->sliceBy($limit, $offset);
 
-            $list->items = array_map(function (Bookmark $bookmark) {
-                return $this->repository->getLocationService()->loadLocation($bookmark->locationId);
-            }, $bookmarks);
+            $result = $this->repository->getlocationService()->find($filter, []);
+        } catch (BadStateException $e) {
+            $this->logger->debug($e);
+
+            return new BookmarkList();
         }
 
+        $list = new BookmarkList();
+        $list->totalCount = $result->totalCount;
+        $list->items = $result->locations;
+
         return $list;
     }
 

eZ/Publish/Core/Repository/Repository.php

@@ -599,7 +599,8 @@ public function getBookmarkService(): BookmarkServiceInterface
         if ($this->bookmarkService === null) {
             $this->bookmarkService = new BookmarkService(
                 $this,
-                $this->persistenceHandler->bookmarkHandler()
+                $this->persistenceHandler->bookmarkHandler(),
+                $this->logger
             );
         }
 

eZ/Publish/Core/Repository/Tests/Service/Mock/BookmarkTest.php

@@ -12,6 +12,7 @@
 use eZ\Publish\API\Repository\LocationService;
 use eZ\Publish\API\Repository\PermissionResolver;
 use eZ\Publish\API\Repository\Values\Content\ContentInfo;
+use eZ\Publish\API\Repository\Values\Content\LocationList;
 use eZ\Publish\Core\Repository\BookmarkService;
 use eZ\Publish\Core\Repository\Tests\Service\Mock\Base as BaseServiceMockTest;
 use eZ\Publish\Core\Repository\Values\Content\Location;
@@ -219,28 +220,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($this->once())
+            ->method('find')
+            ->willReturn($locationList);
 
         $repository = $this->getRepositoryMock();
         $repository
@@ -254,27 +240,6 @@ public function testLoadBookmarks()
         $this->assertEquals($expectedItems, $bookmarks->items);
     }
 
-    /**
-     * @covers \eZ\Publish\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 \eZ\Publish\Core\Repository\BookmarkService::isBookmarked
      */
@@ -289,9 +254,6 @@ public function testLocationShouldNotBeBookmarked()
         $this->assertFalse($this->createBookmarkService()->isBookmarked($this->createLocation(self::LOCATION_ID)));
     }
 
-    /**
-     * @covers \eZ\Publish\Core\Repository\BookmarkService::isBookmarked
-     */
     public function testLocationShouldBeBookmarked()
     {
         $this->bookmarkHandler

eZ/Publish/Core/settings/storage_engines/legacy/filter/query_builders.yaml

@@ -9,6 +9,11 @@ services:
     eZ\Publish\Core\Persistence\Legacy\Filter\CriterionQueryBuilder\:
         resource: '../../../../Persistence/Legacy/Filter/CriterionQueryBuilder/*'
 
+    Ibexa\Core\Persistence\Legacy\Filter\CriterionQueryBuilder\:
+        resource: '../../../../../../../src/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/*'
+
     eZ\Publish\Core\Persistence\Legacy\Filter\SortClauseQueryBuilder\:
         resource: '../../../../Persistence/Legacy/Filter/SortClauseQueryBuilder/*'
 
+    Ibexa\Core\Persistence\Legacy\Filter\SortClauseQueryBuilder\:
+        resource: '../../../../../../../src/lib/Persistence/Legacy/Filter/SortClauseQueryBuilder/*'

eZ/Publish/SPI/Persistence/Bookmark/Handler.php

@@ -41,6 +41,8 @@ public function loadByUserIdAndLocationId(int $userId, array $locationIds): arra
     /**
      * Loads bookmarks owned by user.
      *
+     * @deprecated The "Handler::loadUserBookmarks()" method is deprecated, will be removed in 5.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
@@ -52,6 +54,8 @@ public function loadUserBookmarks(int $userId, int $offset = 0, int $limit = -1)
     /**
      * Count bookmarks owned by user.
      *
+     * @deprecated The "Handler::countUserBookmarks()" method is deprecated, will be removed in 5.0.0. Use "LocationService::count()" and "Criterion\IsBookmarked" instead.
+     *
      * @param int $userId
      *
      * @return int

src/contracts/Repository/Values/Content/Query/Criterion/IsBookmarked.php

@@ -0,0 +1,44 @@
+<?php
+
+/**
+ * @copyright Copyright (C) Ibexa AS. All rights reserved.
+ * @license For full copyright and license information view LICENSE file distributed with this source code.
+ */
+declare(strict_types=1);
+
+namespace Ibexa\Contracts\Core\Repository\Values\Content\Query\Criterion;
+
+use eZ\Publish\API\Repository\Values\Content\Query\Criterion;
+use eZ\Publish\API\Repository\Values\Content\Query\Criterion\Operator;
+use eZ\Publish\API\Repository\Values\Content\Query\Criterion\Operator\Specifications;
+use eZ\Publish\SPI\Repository\Values\Filter\FilteringCriterion;
+
+/**
+ * A criterion that matches locations of bookmarks for a given user id.
+ *
+ *
+ * Supported operators:
+ * - EQ: matches against a unique user id
+ */
+class IsBookmarked extends Criterion implements FilteringCriterion
+{
+    /**
+     * Creates a new IsBookmarked criterion.
+     *
+     * @param int $value UserID for which bookmarked locations must be matched against
+     *
+     * @throws \InvalidArgumentException if a non numeric id is given
+     * @throws \InvalidArgumentException if the value type doesn't match the operator
+     */
+    public function __construct($value)
+    {
+        parent::__construct(null, null, $value);
+    }
+
+    public function getSpecifications(): array
+    {
+        return [
+            new Specifications(Operator::EQ, Specifications::FORMAT_SINGLE, Specifications::TYPE_INTEGER),
+        ];
+    }
+}

src/contracts/Repository/Values/Content/Query/SortClause/BookmarkId.php

@@ -0,0 +1,24 @@
+<?php
+
+/**
+ * @copyright Copyright (C) Ibexa AS. All rights reserved.
+ * @license For full copyright and license information view LICENSE file distributed with this source code.
+ */
+declare(strict_types=1);
+
+namespace Ibexa\Contracts\Core\Repository\Values\Content\Query\SortClause;
+
+use eZ\Publish\API\Repository\Values\Content\Query;
+use eZ\Publish\API\Repository\Values\Content\Query\SortClause;
+use eZ\Publish\SPI\Repository\Values\Filter\FilteringSortClause;
+
+/**
+ * Sets sort direction on the Bookmark ID for a location query containing a IsBookmarked criterion.
+ */
+class BookmarkId extends SortClause implements FilteringSortClause
+{
+    public function __construct(string $sortDirection = Query::SORT_ASC)
+    {
+        parent::__construct('id', $sortDirection);
+    }
+}