IBX-6773: Fixed loading Bookmarks for non-accessible content items by vidarl · Pull Request #476 · ibexa/core

vidarl · 2025-02-11T14:17:47Z

Question	Answer
JIRA issue	IBX-6773
Type	bug
Target Ibexa version	`v4.6`
BC breaks	yes

Related PRs:

IBX-6773: Aligned Bookmarks loadBookmarks call with Repository filtering requirements admin-ui#1835

Description:

If user bookmark some location which he later looses access too, then the bookmark list in admin-ui fails with an exception.
Simply fixing BookmarkService::loadBookmarks() would be easy. The problem is to implement countUserBookmarks() in persistence layer and having it taking into account user permissions so that BC would be kept.

Talked with Adam on how to solve this without breaking BC and he suggested implementing it using filtering

The Bookmark filter will only work with location filtering (LocationService::find()), not with content (ContentService::find())

This is a port of ezsystems/ezplatform-kernel#408 which was not approved and merge in time before 3.3 went EOL.

For QA:

Read ticket for info on how to reproduce

Documentation:

Documentation for the new filter needs to be made, indeed

sonarqubecloud · 2025-02-12T12:26:48Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

konradoboza

Did you make sure none of the deprecated classes are still in use?

vidarl · 2026-02-24T11:44:12Z

@konradoboza : FYI : Had to add related PR : ibexa/admin-ui#1835

konradoboza · 2026-02-25T14:43:33Z

+        $this->assertEquals($contactUsLocationId, $afterSwap->items[0]->id);
+        $this->assertEquals($beforeSwap->items[1]->id, $afterSwap->items[1]->id);


Why this change? It's not clear at first glance.

@konradoboza
The testcase initially tests swapping two locations that are both bookmarked.
I modified the test to check behavior when 1 location is bookmarked and the other is not

…tion/BookmarkQueryBuilderTest.php Co-authored-by: Konrad Oboza <konrad.oboza@ibexa.co>

Co-authored-by: Konrad Oboza <konrad.oboza@ibexa.co>

…ervice::count()

…cationService::count()

… and LocationService::count()

Co-authored-by: Konrad Oboza <konrad.oboza@ibexa.co>

Adjusted tests accoringly to latest commits

…nd userId

This reverts commit 2e5d7c7.

…nd userId

…tLoadBookmarks()

…arked and userId

vidarl · 2026-04-23T09:38:12Z

+        /** @var \Ibexa\Contracts\Core\Repository\Values\Content\Query\Criterion\IsBookmarked $criterion */
+        $isBookmarked = $criterion->value;
+        $userId = $criterion->userId ?? $this->permissionResolver->getCurrentUserReference()->getUserId();
+


@alongosz
There is one problem with this queryBuilder.. In Bookmark table, we store node_id, so we need to join with ezcontentobject_tree. That is no problem if LocationService::find() is used.
However, ATM it won't work with Content filtering (ContentService::find() as that table is already join in that case

I don't see a safe way to detect in the queryBuilder if we are either in Content or Location context..
I have a proposal below using $queryBuilder->getExistingTableAliasJoinCondition but not sure if it is really safe. It checks if alias "location" is joined at that time or not. But not sure if order is deterministic and if this good enough, but not sure if there is any other alternative ?

The other option is somehow to state that this filter can only use with Location ?

Suggested change

$isContentQuery = $queryBuilder->getExistingTableAliasJoinCondition('location') === null;

if ($isContentQuery) {

$connection = $queryBuilder->getConnection();

$subQb = $connection->createQueryBuilder();

$paramName = $queryBuilder->createNamedParameter(

$userId,

ParameterType::INTEGER

);

$subQuerySql = $subQb

->select('1')

->from(Gateway::CONTENT_TREE_TABLE, 'l')

->innerJoin('l', DoctrineDatabase::TABLE_BOOKMARKS, 'b', 'l.node_id = b.node_id')

->where('l.contentobject_id = content.id')

->andWhere("b.user_id = $paramName")

->getSQL();

return $isBookmarked

? "EXISTS ($subQuerySql)"

: "NOT EXISTS ($subQuerySql)";

}

Well... You could join main location for content filtering, but there's a group of filtering criteria that was not meant for content search. AFAICS you don't need it for your use case, so you should just move it to Criterion\Location\IsBookmarked and change extends to \Ibexa\Contracts\Core\Repository\Values\Content\Query\Criterion\Location. This - maybe not in a straightforward way - designates a criterion to work with Location filtering only.

Warning
Which brings me back to my main question from this current review...

BTW.

EXISTS (...) NOT EXISTS (...)

was exactly what I'd expect in such query builder anyway - it's way faster than joining.

vidarl · 2026-04-23T09:46:52Z

+ * @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;


@alongosz : I created a integration test for IsBookmarked. Didn't actually find any other similar integration tests for other criterieons, so added it here. Not sure if there is a better place to put it ?

I created a integration test for IsBookmarked. Didn't actually find any other similar integration tests for other criterieons, so added it here. Not sure if there is a better place to put it ?

There's \Ibexa\Tests\Integration\Core\Repository\Filtering\LocationFilteringTest. It rather should be added there through data providers.

…arked and userId

sonarqubecloud · 2026-04-23T10:50:40Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

alongosz

Hey @vidarl. Again apologies for another long wait...

I've now allocated more time to look into this carefully. One question that I have when looking at the whole codebase is - have you tried to use existing \Ibexa\Contracts\Core\Repository\Values\Content\Query\Criterion\Location\IsBookmarked introduced via #417?

Honestly, this is where I'd move that criterion, given it's for Locations only, but... it already exists.

The issue that needs to be fixed is that it was declared as a filtering criterion, it's documented as a filtering criterion, but it doesn't have the query building logic for filtering, only for search. I think I've missed this when reviewing #417... The good news is that your PR already provides that.

I should've given this feedback back in February, I just completely forgot that we've been there before, I'm sorry :(

The query builder and integration tests remarks:

alongosz · 2026-05-15T12:08:15Z

+        /** @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.');


You should throw \Ibexa\Core\Base\Exceptions\InvalidArgumentException instead and report

/** * @throws \Ibexa\Contracts\Core\Repository\Exceptions\InvalidArgumentException */

on the method

alongosz · 2026-05-15T12:16:15Z

+ * @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;


I created a integration test for IsBookmarked. Didn't actually find any other similar integration tests for other criterieons, so added it here. Not sure if there is a better place to put it ?

There's \Ibexa\Tests\Integration\Core\Repository\Filtering\LocationFilteringTest. It rather should be added there through data providers.

alongosz · 2026-05-15T13:02:59Z

+        /** @var \Ibexa\Contracts\Core\Repository\Values\Content\Query\Criterion\IsBookmarked $criterion */
+        $isBookmarked = $criterion->value;
+        $userId = $criterion->userId ?? $this->permissionResolver->getCurrentUserReference()->getUserId();
+


Well... You could join main location for content filtering, but there's a group of filtering criteria that was not meant for content search. AFAICS you don't need it for your use case, so you should just move it to Criterion\Location\IsBookmarked and change extends to \Ibexa\Contracts\Core\Repository\Values\Content\Query\Criterion\Location. This - maybe not in a straightforward way - designates a criterion to work with Location filtering only.

Warning
Which brings me back to my main question from this current review...

BTW.

EXISTS (...) NOT EXISTS (...)

was exactly what I'd expect in such query builder anyway - it's way faster than joining.

github-actions Bot assigned mikadamczyk, alongosz and tischsoic Feb 11, 2025

vidarl force-pushed the IBX-6773_Bookmarks_for_non-accessible_contents_cause_exception_4.6 branch from bdac332 to 9164daa Compare February 12, 2025 12:23

vidarl mentioned this pull request Feb 12, 2025

IBX-6773: Bookmarks for non accessible contents cause exception ezsystems/ezplatform-kernel#408

Closed

6 tasks

vidarl added the Ready for review label Feb 12, 2025

vidarl requested a review from a team February 12, 2025 12:28

vidarl added the Doc needed The changes require some documentation label Feb 12, 2025

adamwojs changed the title ~~IBX:6773 bookmarks for non accessible contents cause exception~~ IBX:6773: Bookmarks for non accessible contents cause exception Feb 23, 2025

vidarl force-pushed the IBX-6773_Bookmarks_for_non-accessible_contents_cause_exception_4.6 branch from 9164daa to ec96060 Compare February 23, 2026 15:43

konradoboza reviewed Feb 24, 2026

View reviewed changes

vidarl commented Feb 24, 2026

View reviewed changes

Comment thread phpstan-baseline.neon Outdated

konradoboza requested a review from a team February 24, 2026 08:56

vidarl commented Feb 24, 2026

View reviewed changes

Comment thread src/contracts/Persistence/Bookmark/Handler.php Outdated

ibexa-workflow-automation-1 Bot requested review from Steveb-p, ViniTou, alongosz, barw4, ciastektk, konradoboza, mikadamczyk, tbialcz and wiewiurdp and removed request for a team February 24, 2026 08:56

konradoboza reviewed Feb 24, 2026

View reviewed changes

vidarl mentioned this pull request Feb 24, 2026

IBX-6773: Aligned Bookmarks loadBookmarks call with Repository filtering requirements ibexa/admin-ui#1835

Open

vidarl requested a review from konradoboza February 24, 2026 11:42

konradoboza reviewed Feb 25, 2026

View reviewed changes

vidarl and others added 8 commits April 21, 2026 09:36

Update tests/lib/Persistence/Legacy/Filter/CriterionQueryBuilder/Loca…

9635474

…tion/BookmarkQueryBuilderTest.php Co-authored-by: Konrad Oboza <konrad.oboza@ibexa.co>

[PHPStan] Aligned baseline with the upstream changes

6cc6903

Apply suggestions from code review

0d293e5

Co-authored-by: Konrad Oboza <konrad.oboza@ibexa.co>

Added integration test for using Criterion\IsBookmarked and LocationS…

8e63f7e

…ervice::count()

fixup! Added integration test for using Criterion\IsBookmarked and Lo…

310d457

…cationService::count()

fixup! Added integration test for using Criterion\IsBookmarked and Lo…

3594b4f

…cationService::count()

fixup! fixup! Added integration test for using Criterion\IsBookmarked…

5fb1a53

… and LocationService::count()

Apply suggestion from @konradoboza

6c862de

Co-authored-by: Konrad Oboza <konrad.oboza@ibexa.co>

vidarl force-pushed the IBX-6773_Bookmarks_for_non-accessible_contents_cause_exception_4.6 branch from 5343730 to 6c862de Compare April 21, 2026 07:36

vidarl added 5 commits April 21, 2026 10:43

Fixed deprecation notices

ca42c8d

Removed dependency to Criterion

2e5d7c7

Fixed so that Bookmark criterion has parameters isBookmarked and userId

c700f40

tests/integration/Core/Repository/BookmarkServiceTest.php

7368c0d

Adjusted tests accoringly to latest commits

Removed use of magic property

abf0fff

vidarl requested a review from alongosz April 21, 2026 10:45

vidarl added 8 commits April 21, 2026 15:53

fixup! Fixed so that Bookmark criterion has parameters isBookmarked a…

fb61a41

…nd userId

Revert "Removed dependency to Criterion"

690322f

This reverts commit 2e5d7c7.

fixup! tests/integration/Core/Repository/BookmarkServiceTest.php

4d3f4b7

fixup! Revert "Removed dependency to Criterion"

80cb93e

fixup! Fixed so that Bookmark criterion has parameters isBookmarked a…

89d995e

…nd userId

Fixed test Ibexa\Tests\Core\Repository\Service\Mock\BookmarkTest::tes…

6a32843

…tLoadBookmarks()

Fixed PHPstan error

2f69957

fixup! fixup! Fixed so that Bookmark criterion has parameters isBookm…

83deac2

…arked and userId

vidarl commented Apr 23, 2026

View reviewed changes

Added integration test for Criterion\IsBookmarked

ba5cf53

vidarl commented Apr 23, 2026

View reviewed changes

vidarl added 2 commits April 23, 2026 12:20

fixup! Added integration test for Criterion\IsBookmarked

4d5dc1e

fixup! fixup! Fixed so that Bookmark criterion has parameters isBookm…

0c025cd

…arked and userId

alongosz requested changes May 18, 2026

View reviewed changes

		$this->assertEquals($contactUsLocationId, $afterSwap->items[0]->id);
		$this->assertEquals($beforeSwap->items[1]->id, $afterSwap->items[1]->id);

+        $isContentQuery = $queryBuilder->getExistingTableAliasJoinCondition('location') === null;
+        if ($isContentQuery) {
+            $connection = $queryBuilder->getConnection();
+            $subQb = $connection->createQueryBuilder();
+            $paramName = $queryBuilder->createNamedParameter(
+                $userId,
+                ParameterType::INTEGER
+            );
+            $subQuerySql = $subQb
+                ->select('1')
+                ->from(Gateway::CONTENT_TREE_TABLE, 'l')
+                ->innerJoin('l', DoctrineDatabase::TABLE_BOOKMARKS, 'b', 'l.node_id = b.node_id')
+                ->where('l.contentobject_id = content.id')
+                ->andWhere("b.user_id = $paramName")
+                ->getSQL();
+            return $isBookmarked
+                ? "EXISTS ($subQuerySql)"
+                : "NOT EXISTS ($subQuerySql)";
+        }

Conversation

vidarl commented Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related PRs:

Description:

For QA:

Documentation:

Uh oh!

sonarqubecloud Bot commented Feb 12, 2025

Quality Gate passed

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

konradoboza left a comment

Choose a reason for hiding this comment

Uh oh!

vidarl commented Feb 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vidarl Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Apr 23, 2026

Quality Gate passed

Uh oh!

alongosz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

vidarl commented Feb 11, 2025 •

edited

Loading

vidarl Apr 23, 2026 •

edited

Loading