ShortcutsNavigationBlock is not being cached properly

I probably ran into the same yesterday 🐛 A potential caching issue, newly added shortcuts are not directly show in the shortcuts group Active . 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.

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.

Bump priority and assign to myself.

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. :/

Thank you for your review @rkoller

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

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.

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.

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!

Thanks all, good catch!! Merged :)

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