Problem/Motivation

When users change their default shortcut set, those changes are not being reflected in the toolbar shortcuts section until cache is cleared.
These changes should be reflected automatically.

Steps to reproduce

Install a default Drupal standard installation
Login as user 1
Click on the toolbar "Shortcuts" icon and check the links included there
Visit /admin/config/user-interface/shortcut and create a new shortcut set
Change the list of links for the new set, adding, removing or editing the links
Visit /user/1/shortcuts and change the default shortcut set to the new one
Once the form is saved, click again on the "Shortcuts" toolbar icon
Links are the ones in the default shortcut set and not the ones in the new one

Proposed resolution

Add logic to invalidate user's selected shortcut set to recalculate it next time the toolbar is generated.

Remaining tasks

Review

User interface changes

None

API changes

Deprecate shortcut_current_displayed_set() and shortcut_default_set(). Add new method to ShortcutSetStorageInterface to replace the former.

Data model changes

None

Release notes snippet

Issue fork drupal-3427046

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

plopesc created an issue. See original summary.

plopesc opened merge request !6992

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Active » Needs review

MR created

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Ran the test-only feature and got failing tests as expected https://git.drupalcode.org/issue/drupal-3427046/-/jobs/1047844

Change record is present and reads fine.
Link in MR matches

Issue summary is complete (thank you for that!)
Following the steps believe I am seeing the issue.

Adding the cacheTags appears to be inline with how it's done in other places.

LGTM!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

I've added some comments to the MR. This looks like a nice thing to get fixed.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs work » Needs review

Thank you for your review @alexpott.

I think all the suggested improvements have been implemented properly and the R is ready for a new round of reviews.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Re-ran test-only feature https://git.drupalcode.org/issue/drupal-3427046/-/jobs/1150296

Seems coverage is still there. Appears feedback from @alexpott has been addressed.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Discussed with @catch because I'm not sure about the affect of having a cache tag per user and whether it makes much sense. I think this might end up creating lots and lots of cache entries.

I think we should be able to invalidate cache by the user's current default shortcut set... ie. by doing something like

    $default = $this->getDefaultSet($account);
    if ($default instanceof ShortcutSet) {
      Cache::invalidateTags($default->getCacheTags());
    }

in assignUser and unassignUser... this is working for assignUser but is not working for unassignUser - yes we will ending clearing more cache when assign and unassign happen - but how often does this occur?

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs work » Needs review

Thank you for your comment.

I thought the approach would not affect the number of cache entries, given that we already had the user context, but forgot to take into account the cachetags table, where a new row per tag is added.

Based on your suggestion, this new approach invalidate the previous set cache tag when assigning a new set or unassigning the current one. This is a bit more aggressive because it invalidates some entries unnecessarily, but maintains the size of cachetags table under control.

Also , we are not introducing a BC change, so the CR created would not be necessary anymore.

Regarding the implementation, instead of calling shortcut_current_displayed_set(), we might need to add a new method in the storage class getDisplayedToUser() that internally invokes getAssignedToUser() and getDefaultSet() if the former does not bring any result.

Not sure if that might need a CR given that we're adding a new method, leaving the existing ones untouched.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Needs work

@plopesc thanks for sticking with this. Cache invalidation of stuff like this is never simple. I agree with your proposal about moving stuff to ShortcutSetStorage and have commented on the MR with some thoughts.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs work » Needs review

@alexpott New role for review including the new method ShortcutSetStorage::getDisplayedToUser() and deprecation of shortcut_current_displayed_set() & shortcut_default_set() functions.

Some followup questions.

  • We are adding new parameters to ShortcutLazyBuilders service and deprecating 2 functions. Can we have both in a single CR? Or we might need 2?
  • shortcut_current_displayed_set() implements the drupal_static() pattern. Do we need to add something special for this? It's being invoked only from ShortcutSetStorage to be refreshed once shortcut sets are being assigned/unassigned or removed.

Thank you

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Needs work

All in one CR is fine because it all goes together in my mind. Also the constructor changes are secondary and should only form a small part of the CR. Added some comments to the MR too.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Needs work » Needs review

Feedback has been addressed and CR updated.

See comment on MR about the need to add static cache to getDisplayedToUser().

While we're here, would you consider appropriate a follow up issue to provide a shortcut related cache context to avoid having a cache entry per user?. In an scenario where most users have selected the same shortcut_set, most of those cache entries would be redundant.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Issue summary: View changes
Related issues: +#3436892: Remove deprecated code from shortcut module

Created #3436892: Remove deprecated code from shortcut module to keep track of deprecation code that might need to be removed before 11.0.0.

Also updated IS to reflect the final approach taken.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Added some simple typehints to the new function. Rest of the changes look good though

  • catch committed 26f98e16 on 10.3.x 
    Issue #3427046 by plopesc, smustgrave, alexpott: Shortcuts toolbar links...

  • catch committed 30a1adec on 11.x 
    Issue #3427046 by plopesc, smustgrave, alexpott: Shortcuts toolbar links...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

The new cache invalidation approach looks good to me. I did briefly think about just using the user entity cache tag here, but then we'd potentially be invalidating entity caches for the user when we update their shortcut set which is much more likely to affect things (especially when the user is rendered as part of entities they've created like node/comment author).

Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!

quietone closed merge request !6992

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2083123: Shortcut cleanup: Remove duplicated code and deprecate legacy functions

Yay, one fewer bit of Shortcut architectural complexity! 👏

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.