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

Problem/Motivation

There are several problems in shortcut.module:

  1. 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.)
  2. 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
  3. 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.

Proposed resolution

  1. Deprecate shortcut_set_edit_access() in favor of $shortcut_set->access('update')
  2. 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)
  3. Deprecate shortcut_current_displayed_set() in favor of a new ShortcutSetStorage::getCurrentSet() (API addition)
  4. Deprecate shortcut_default_set() in favor of ShortcutSetStorage::getDefaultSet(). The latter is also fixed to always return a shortcut set. (Bug fix)
  5. Deprecate shortcut_renderable_links() in favor of a new $shortcut_set->getRenderableShortcuts() (API addition)
  6. 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.
  7. Adjust all calling code in core to the new methods.

Remaining tasks

RTBC the patch.

User interface changes

None.

API changes

None.

<!--Uncomment the relevant rows for the issue. -->
πŸ“Œ Task
Status

Needs work

Version

11.0 πŸ”₯

Component
ShortcutΒ  β†’

Last updated about 1 month ago

  • Maintained by
  • πŸ‡΅πŸ‡°Pakistan @jibran
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.

Sign in to follow issues

Comments & Activities

Not all content is available!

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

Production build 0.69.0 2024