Skip to content

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

Open
vidarl wants to merge 15 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 15 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

Warning

  • Update deprecation version with the next release one, before merging, depending on when it gets merged
🎫 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())

Code was initially based on ezsystems/ezplatform-kernel#408 which was not approved and merge in time before 3.3 went EO, but has been heavily modified since.

For QA:

Read ticket for info on how to reproduce

Documentation:

The IsBookmarked criterion existed before this PR, but it now also supports filtering.

@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

@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 4 times, most recently from fb6c2bd to 4ea28c1 Compare August 7, 2026 10:39
@vidarl

vidarl commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

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.

@alongosz : Added in 4ea28c1: Added regression tests for bookmarks of inaccessible and removed items

PR should not be ready again for your sharp eyes

@vidarl
vidarl requested a review from alongosz August 7, 2026 10:46
Comment thread src/contracts/Persistence/Bookmark/Handler.php
Comment thread tests/integration/Core/Repository/BookmarkServiceTest.php Outdated
@vidarl
vidarl requested a review from alongosz August 26, 2026 06:34
@vidarl
vidarl force-pushed the IBX-6773_Bookmarks_for_non-accessible_contents_cause_exception_4.6 branch from 8f23c67 to 7ce51b5 Compare September 3, 2026 07:59
vidarl and others added 15 commits September 11, 2026 13:38
Co-authored-by: Paweł Niedzielski <pawel.niedzielski@ibexa.co>
Co-authored-by: Andrew Longosz <alongosz@users.noreply.github.com>
@KamilSznajdrowicz
KamilSznajdrowicz force-pushed the IBX-6773_Bookmarks_for_non-accessible_contents_cause_exception_4.6 branch from 7ce51b5 to daec766 Compare September 11, 2026 11:38
@sonarqubecloud

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 QA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants