Shortcuts toolbar links are not updated automatically when default shortcut set is changed

MR created

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!

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

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.

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.

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?

Thank you for your comment.

I thought the approach would not affect he 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.

@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.

@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

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.

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.

Created 📌 [PP-1] Remove deprecated code from shortcut module Postponed 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.

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

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

Yay, one fewer bit of Shortcut architectural complexity! 👏

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