Problem/Motivation

While working on #3426575: Add tests for ShortcutsNavigationBlock found an inconsistency with the shortcut navigation block cache, which was not respecting the user selection.

Found that it was related to how the block is caching results.

This is also an issue in core's shortcut block, so also created #3426624: Shortcuts Block does not include the necessary cache tags

For navigation, we should refactor the way the block is built and take a look into how it is built in shortcut_toolbar() function.

Steps to reproduce

Scenario #1

  • Install default Standard Drupal installation
  • Create new Shortcut sets X, Y and add some links
  • Create 2 Users, A and B and assign them the same role, including "access shortcuts" & "access navigation" permission.
  • Login as User A and set the Y shortcut set as the preferred one
  • Login as User B and set the X shortcut set as the preferred one
  • Install navigation module
  • Visit the page containing the block as User A, so the Y shortcut set is shown in the navigation bar
  • Login as User B
  • Navigation bar shows the Y shortcut set instead of the expected X shortcut set

Scenarion #2

  • Install default Standard Drupal installation
  • Expand the shortcuts sectio in the navigation bar and check the links present there
  • Go to /admin/config/user-interface/shortcut/manage/default/add-link and add Taxonomy as the name and /admin/structure/taxonomy as the path
  • Expand the shortcuts group and check if the new shortcut is added

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Issue fork navigation-3426626

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.

rkoller’s picture

rkoller
he/him
Primary language German
Location Nürnberg, Germany
commented

I probably ran into the same yesterday #3426474: A potential caching issue, newly added shortcuts are not directly show in the shortcuts group. Saw your issue about creating a test. When i wanted to post a comment to the aforementioned issue i've created now i've noticed that it got postpone on this issue here.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Title: ShortcutsNavigationBlock does not take into account the user's default shortcut set » ShortcutsNavigationBlock is not being cached properly
Issue summary: View changes
Related issues: +#3426474: A potential caching issue, newly added shortcuts are not directly show in the shortcuts group

Thank you for the heads-up @rkoller!

Issue you mentioned is a secondary effect of the caching issues in general.

Let me rephrase this one to ensure both situations are solved here and we can close yours.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Assigned: Unassigned » plopesc
Priority: Major » Critical

Bump priority and assign to myself.

plopesc opened merge request !188

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented
Status: Active » Needs review
rkoller’s picture

rkoller
he/him
Primary language German
Location Nürnberg, Germany
commented
Status: Needs review » Needs work

I've applied the MR to the latest version of of the Navigation module (1.x-dev@dev) on a install of Drupal 11.x-dev as well as on an install of Drupal 10.2.4.

On both instances the changes from the MR are in place in code (i've cross checked to make sure) but functionally, when manually testing, the MR has no effect. On both instances i've tried to add a shortcut by clicking the star icon as well as manually adding a new shortcut via /admin/config/user-interface/shortcut/manage/default/customize. After adding the page isn't shown for each of the cases in the shortcuts group in the navigation sidebar, only /admin/config/user-interface/shortcut/manage/default/customize reflects the newly added shortcut. i've also tried by pressing the reload button with the pressed alt-key to clear caches on reload. The newly added shortcuts are still only showing after a drush cr is run. same behavior the other way around when you try to remove an existing shortcut (either via unchecking the star icon or deleting a shortcut on /admin/config/user-interface/shortcut/manage/default/customize) . i've tested in safari, firefox, and edge, all the same result. :/

plopesc’s picture

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

Thank you for your review @rkoller

Could you please take a look now? Added back one of the missing cache tags.

rkoller’s picture

rkoller
he/him
Primary language German
Location Nürnberg, Germany
commented

thank you for the quick followup @plopsec! at first something went wrong with the 11.x-dev instance, applying the recent changes, somehow those got not applied. noticed that after all the steps described in #7 were working properly with the 10.2.4 instance after applying the MR there as well. therefore i've completely removed the patch on the 11.x instance and then reapplied. after that the latest changes were in place as well there and also all the steps outlined in #7 working now properly. from a manual testing perspective i think things behave as expected now. i leave the status at needs review because i am not a developer and therefore not qualified for a code review.

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented

Wow, great find on this @plopesc!

The fix works for me, I just left a couple of notes to check my understanding. See my comment about keys. I'm not sure we need that, but I'm open to it, as I saw my knowledge on keys use in caching is a bit rusty.

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Status: Needs review » Needs work
m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Assigned: plopesc » Unassigned
Status: Needs work » Reviewed & tested by the community

On second thought, I see the core commit took the keys code, so lets not hold this up. It's an interesting nerd caching thought though.

Thanks again!

ckrina made their first commit to this issue’s fork.

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Status: Reviewed & tested by the community » Fixed

Thanks all, good catch!! Merged :)

  • ckrina committed f27440c0 on 1.x authored by plopesc 
    Issue #3426626: ShortcutsNavigationBlock is not being cached properly

Status: Fixed » Closed (fixed)

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