Skip to content

fix(bookmarks): honor shared-bookmark rules in getList and getByLabel - #20347

Open
cavalcante-willian wants to merge 1 commit into
phpmyadmin:masterfrom
cavalcante-willian:fix/bookmark-repository-query-issues
Open

fix(bookmarks): honor shared-bookmark rules in getList and getByLabel#20347
cavalcante-willian wants to merge 1 commit into
phpmyadmin:masterfrom
cavalcante-willian:fix/bookmark-repository-query-issues

Conversation

@cavalcante-willian

Copy link
Copy Markdown

BookmarkRepository::getList() filtered strictly by the requested database,
so bookmarks created without a database context (dbase = '', e.g. saved from
the Console, which does not require a database to be selected) never appeared
when browsing a specific database's bookmark list, even though the equivalent
shared-user fallback (user = '') already existed for the user filter.

BookmarkRepository::getByLabel() ignored the AllowSharedBookmarks setting
entirely, requiring an exact user match. This method backs
Sql::getDefaultSqlQueryForBrowse(), the per-table default browse query
feature described in FAQ 6.22, so a bookmark shared by another user never
applied to anyone browsing that table, unlike get()/getList() which
already honored the setting consistently.

Both methods now fall back to the shared value the same way get() already
did. The repeated "exact match, falling back to shared" SQL fragment was
extracted into a single userMatchCondition() helper reused by all three
methods, instead of a third copy of the same conditional.

Commits:

  • e39d997 fix(bookmarks): honor shared-bookmark rules in getList and getByLabel

Signed-off-by: Willian Cavalcante 248522986+cavalcante-willian@users.noreply.github.com

BookmarkRepository::getList() only matched bookmarks scoped to the exact
requested database, so bookmarks created without a database context
(dbase = '', e.g. via the Console, which does not require a database to
be selected) never showed up when browsing a specific database's
bookmark list. The dbase filter now also matches an empty dbase.

BookmarkRepository::getByLabel() ignored AllowSharedBookmarks entirely,
requiring an exact user match. This method backs
Sql::getDefaultSqlQueryForBrowse() (the per-table default browse query
feature documented in FAQ 6.22), so a bookmark shared by another user
never applied for anyone else, unlike get()/getList() which already
honored the setting. getByLabel() now falls back to an empty user the
same way the other two do.

Extracted the repeated "exact user match, falling back to shared" SQL
fragment into a single userMatchCondition() helper used by get(),
getList() and getByLabel(). Without it, fixing getByLabel() would have
copied a third instance of the same conditional that get()/getList()
already duplicated between them.

Updated docblocks on all three methods to state the actual contract
(shared-across-user and shared-across-database fallback behavior)
instead of paraphrasing the method names; getByLabel() now references
its real caller so the "why" of the shared-label lookup is not left
implicit.

Added 8 new tests covering both fixes and their AllowSharedBookmarks-off
counterparts, plus the previously untested positive-match path for
get(). Updated the two existing tests (BookmarkTest::testGetList and the
DbiDummy fixture used by ReplaceControllerTest::testReplace) that had
the old SQL text hardcoded, since DbiDummy matches queries by exact
string.

Signed-off-by: Willian Cavalcante <248522986+cavalcante-willian@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant