Shortcut cleanup: Remove duplicated code and deprecate legacy functions

Created on 6 September 2013, almost 11 years ago

Updated 2 April 2024, 3 months ago

Updated: Comment #108

There are several problems in shortcut.module:

shortcut_current_displayed_set, shortcut_default_set(), and ShortcutSetStorage::getDefaultSet() can return NULL in edge cases, which is not documented and - worse - calling code does not account for. This is a bug. (It's not a super-easy-to-trigger bug, but it's a bug nonetheless.)
Shortcut module circumvents the standard patterns we have in core for checking access to entities. This is - strictly speaking - not a bug, because nothing is explicitly broken (i.e. there is no information disclosure, due to incorrect access checking), but it is very confusing and unnecessary limiting at best. Also note that Shortcut module has not been battle-tested in light of the new caching world order (e.g. #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!) → ) and fixing the problems that will inevitable crop up when doing so will be much easier with standard entity access logic. This is follow-up material, however
There is duplicated code between shortcut.module and ShortcutSetStorage/shortcut_default_set() <=> ShortcutSetStorage::getDefaultSet() and shortcut_set_edit_access() <=> ShortcutSetAccessControlHandler::checkAccess(). This is especially unnerving because in the latter case the code is not even directly copy-pasted but "looks" different, even though the actual logic is identical.

Deprecate shortcut_set_edit_access() in favor of $shortcut_set->access('update')
Deprecate shortcut_set_switch_access() in favor of $user->access('switch_shortcut_set') which calls out to a new hook_user_access() implementation in Shortcut module (API addition)
Deprecate shortcut_current_displayed_set() in favor of a new ShortcutSetStorage::getCurrentSet() (API addition)
Deprecate shortcut_default_set() in favor of ShortcutSetStorage::getDefaultSet(). The latter is also fixed to always return a shortcut set. (Bug fix)
Deprecate shortcut_renderable_links() in favor of a new $shortcut_set->getRenderableShortcuts() (API addition)
Add typehints in ShortcutSetStorageInterface to ::assignUser(), unassignUser(), getAssignedToUser(). This is not an API change, just a documenation clean-up: The methods already relied on the respective objects to be passed.
Adjust all calling code in core to the new methods.

RTBC the patch.

None.

None.

📌 Task

Status

Needs work

Version

11.0 🔥

Component

Shortcut →

Last updated about 2 months ago

Created by

🇵🇰Pakistan jibran Sydney, Australia

Live updates comments and jobs are added and updated live.

Needs reroll
The patch will have to be re-rolled with new suggestions/changes described in the comments in the issue.

Incomplete comments

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

Comment over 1 year ago →
needs-review-queue-bot
The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
Comment 3 months ago →
🇧🇪Belgium Wim Leers Ghent 🇧🇪🇪🇺
🐛 Shortcuts toolbar links are not updated automatically when default shortcut set is changed Needs review landed and AFAICT has solved some of the problems that this issue was aiming to solve 😊

Production build 0.69.0 2024