Skip to content

IBX-6773: Fixed loading Bookmarks for non-accessible content items - #476

Open
vidarl wants to merge 42 commits into
4.6from
IBX-6773_Bookmarks_for_non-accessible_contents_cause_exception_4.6
Open

IBX-6773: Fixed loading Bookmarks for non-accessible content items#476
vidarl wants to merge 42 commits into
4.6from
IBX-6773_Bookmarks_for_non-accessible_contents_cause_exception_4.6

Conversation

@vidarl

@vidarl vidarl commented Feb 11, 2025

Copy link
Copy Markdown
Contributor
🎫 Issue IBX-6773

Related PRs:

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

Copy link
Copy Markdown

@vidarl
vidarl requested a review from a team February 12, 2025 12:28
@vidarl vidarl added the Doc needed The changes require some documentation label Feb 12, 2025
@adamwojs 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
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
Comment thread src/contracts/Repository/Values/Content/Query/Criterion/IsBookmarked.php Outdated
Comment thread src/lib/Repository/BookmarkService.php Outdated
Comment thread phpstan-baseline.neon Outdated
Comment thread phpstan-baseline.neon Outdated
Comment thread phpstan-baseline.neon Outdated
@konradoboza
konradoboza requested a review from a team February 24, 2026 08:56
Comment thread src/contracts/Persistence/Bookmark/Handler.php Outdated

@konradoboza konradoboza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@vidarl

vidarl commented Feb 24, 2026

Copy link
Copy Markdown
Contributor Author

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

Comment thread tests/integration/Core/Repository/LocationServiceTest.php Outdated
@vidarl
vidarl force-pushed the IBX-6773_Bookmarks_for_non-accessible_contents_cause_exception_4.6 branch from 0c025cd to fd7cc86 Compare August 3, 2026 09:49
@vidarl
vidarl requested a review from alongosz August 3, 2026 11:28

@alongosz alongosz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it's almost done, great work @vidarl 💪

I have one general remark to test coverage - it's extensive, but I don't see an integration test case that reproduces the scenario which created this bug report - remove permissions for an item user previously has bookmarked.

Other remarks:

Comment thread src/lib/Persistence/Legacy/Bookmark/Gateway.php Outdated
Comment thread src/contracts/Persistence/Bookmark/Handler.php Outdated
Comment thread src/lib/Repository/BookmarkService.php Outdated
Comment thread src/contracts/Repository/Values/Content/Query/SortClause/Location/Bookmark/Id.php Outdated
@vidarl
vidarl force-pushed the IBX-6773_Bookmarks_for_non-accessible_contents_cause_exception_4.6 branch from 46b69f8 to 493a1df Compare August 6, 2026 13:10
@vidarl
vidarl force-pushed the IBX-6773_Bookmarks_for_non-accessible_contents_cause_exception_4.6 branch from 493a1df to de482ee Compare August 6, 2026 13:42
@sonarqubecloud

sonarqubecloud Bot commented Aug 6, 2026

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Doc needed The changes require some documentation Ready for review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants