Shortcut hook_toolbar implementation makes all pages uncacheable

#2899392: user_hook_toolbar() makes all pages uncacheable landed, now let's do this one!

I think the only reason this depends on the user is because users have a setting for the shortcut set?

What about adding a cache context for the shortcut set (user.shortcut_set) and allow to cache shortcuts by that instead not caching it at all?

The thing is that shortcuts are actually quite slow slow to render, due to how we are accessing and rendering them (we create urls and go through the whole access/validation process them, which involves loading routes, executing access callbacks and so on). And unless other parts like breadcrumbs and local tasks, they do *not* vary by url and most pages only have a very small set of shortcut sets, so the cache hit ratio should be pretty good?

Right — quoting from shortcut_toolbar():

        // Cacheable per user, because each user can have their own shortcut
        // set, even if they cannot create or select a shortcut set, because
        // an administrator may have assigned a non-default shortcut set.

I think such a cache context is a good idea for the reasons you provide. The root cause though is #2339903: Shortcut module's access checking is insane. But fixing that is going to be VERY hard, simply because we absolutely cannot change existing behavior. The cache context you propose is the pragmatic way forward.

This is effectively a big performance blocker. With this, D8 becomes significantly faster for many use cases.

Yeah, thinking about it a bit more, the problem is that with stickyness of cache contexts, the result would be that we'd have this cache context the on every page for all users once one admin user visited a given page. Maybe it could be a combination of forced lazy loader + separate cache, so we don't add the cache context to the page. But it's probably also not a big deal to have it, should be relatively fast to get that info.

Yeah, thinking about it a bit more, the problem is that with stickyness of cache contexts, the result would be that we'd have this cache context the on every page for all users once one admin user visited a given page. Maybe it could be a combination of forced lazy loader + separate cache, so we don't add the cache context to the page. But it's probably also not a big deal to have it, should be relatively fast to get that info.

I thought so, too, but it turns out that we had been more clever in the past ;) ...

Because we have user.permissions as required cache context and a normal anonymous user does not have permission to 'access shortcut links', this would only affect admin users, which actually see the toolbar.

The $pre_bubbling_cid actually acts like a cache splitter after which we then do the cache_redirect logic - if needed.

And for those it is perfectly fine to have the cache context always in there.

Overall lazy building + caching (even if per user) is optimal.

The biggest problem is the empty condition as we need a cache context to determine whether to show the block in the first place or not.

WOW! @Fabianx is in the house! 🐦 Would be lovely to have this in, then Dynamic Page Cache and BigPipe can finally live up to their full potential, at least in Vanilla Drupal :) ✋

Just want to say nice to see these updates! Also to note, the POC patch in #8 fixes the issue from here - allowing dynamic page cache with the shortcut module enabled. Getting great performance with it. What else needs doing for this one?

This should fix one of the tests.

Thanks for the updates in this area.

I just want to note, I found with the lazyloaded shortcuts toolbar, sometimes the toolbar would overlap and obscure the page content header.
It seemed an issue with calculated the body offset or height in JS there.

I had to add some JS like this to ensure the shortcut toolbar height:

  $(document).ready(function ensureToolbarHeight() {
    if (typeof Drupal.toolbar !== 'undefined') {
      Drupal.toolbar.views.toolbarVisualView.updateToolbarHeight();
      // Check it again after 2 secs
      setTimeout(function refreshHeight() {
        Drupal.toolbar.views.toolbarVisualView.updateToolbarHeight();
      }, 2000);
    }
  });

Perhaps there's some way to trigger the toolbar offset calc, attach the behaviour or similar in the patch here.

Otherwise is good! with thanks.

#13 is working for us on d8.5.

#15:

I just want to note, I found with the lazyloaded shortcuts toolbar, sometimes the toolbar would overlap and obscure the page content header.
It seemed an issue with calculated the body offset or height in JS there.

This is exactly what I warned for in #2542050: Toolbar implementation creates super annoying re-rendering.. It's still good that that landed to improve the situation for most, but we'll now have to come up with a better solution. That's not in scope for this issue though! Fortunately!

Drupal\Tests\shortcut\Functional\ShortcutLinksTest::testAccessShortcutsPermission
Behat\Mink\Exception\ExpectationException: Link with label Shortcuts not found.

The problem here was that, since the test user has the permission 'access shortcuts', the shortcuts link is visible. So I removed the part which asserts that the link doesn't exist.

Second fail:

Drupal\Tests\toolbar\Functional\ToolbarCacheContextsTest::testToolbarCacheContextsCaller
Unwanted cache contexts in response: user

The cache contexts for the shortcuts was changed to user.permissions, but we have another cache context for the tray, which I changed to 'user.permissions' also. We had a 'user.permissions' and a 'user' and as drupal optimizes cache contexts, it would use just the 'user' for the page since it stands higher in the hierarchy.
We could maybe even omit the cache context for the tray, since it adds the same, but I'm not sure about that.

Forgot to upload the interdiff.

1. +++ b/core/modules/shortcut/shortcut.module
   @@ -433,7 +433,7 @@ function shortcut_toolbar() {
                '#cache' => [
                  'keys' => ['shortcut_set_toolbar_links'],
   -              'contexts' => ['user'],
   +              'contexts' => ['user.permissions'],
                ],
              ],
            ],
   

   this is not correct.

   shortcut links *are* per-user and they must have this cache context.

   I would have to investigate why it affects the test the way it does, but this change is not the correct fix.

2. +++ b/core/modules/shortcut/tests/src/Functional/ShortcutLinksTest.php
   @@ -353,7 +353,6 @@ public function testAccessShortcutsPermission() {
        $this->drupalLogin($this->drupalCreateUser(['access toolbar', 'access shortcuts']));
   -    $this->assertNoLink('Shortcuts', 'Shortcut link not found on page.');
        $this->assertNoLink('Cron', 'Cron shortcut link not found on page.');
    
   

   kinda similar her, this seems to be a shortcut-specific access check, each shortcut needs a dedicated access check.

   If this is broken then we might have a bug in the implementation here.

So debugging this it seems that one test failure was caused by the fact that even if there are no links to render in a set, the Shortcut set link is still displayed.

In this case, the Shortcuts link is still displayed even if there are no shortcuts inside. Is this an acceptable case?

@Berdir in Slack 12 hours ago:

berdir   [Yesterday at 9:41 PM]
@wimleers (he/him) so, we have a small behavior change in https://www.drupal.org/project/drupal/issues/2914110 which causes at least some of the test fails. before, if a user had access to shortcuts but couldn't access any of the links, the Shortcut iterms wasn't displayed. now it is. I think that's a pretty edge case scenario, but I think it's pretty much by design with a lazybuilder approach. thoughts?

It is indeed a common consequence of lazy building: we don't know ahead of time whether the user will have any shortcuts to see. I think for the Shortcut case this is super exceptional though, since the 99% case is going to have site-provided shortcuts per user role.

Plus, the same is already true for the "Manage" toolbar tab: a authenticated user in the Standard install profile who gets the access toolbar permission also sees a "Manage" toolbar tab without a tray. Same problem.

Which matches what @yongt9412 wrote in #23:

In this case, the Shortcuts link is still displayed even if there are no shortcuts inside. Is this an acceptable case?

I think the answer is "yes".

Notes:

• In principle every user should be able to see shortcuts, that's what the default shortcut set is for.
• If a user does not have access to access any of those links, then that's a poorly built site.
• The logic that determines which shortcut set a user can access is ridiculously complicated, and is part of the reason this issue is tricky. I opened #2339903: Shortcut module's access checking is insane for that many years ago. If we'd have either ONLY a per-role default shortcut set OR start from that and then allow the user to customize it for their needs, it'd be impossible to run into this "cannot access any shortcut link" edge case. In other words: that's a long pre-existing architectural problem. We shouldn't hold up improving the experience for the >90% case just for that.

+++ b/core/modules/shortcut/shortcut.module
@@ -380,28 +404,17 @@ function shortcut_toolbar() {
+    // @todo Use a cache context to vary this for users that have links and
+    // those who don't.
+    if (TRUE || !empty($links) || !empty($configure_link)) {

Per #25, I think we can remove this condition.

For

Plus, the same is already true for the "Manage" toolbar tab: a authenticated user in the Standard install profile who gets the access toolbar permission also sees a "Manage" toolbar tab without a tray. Same problem.

@Berdir pointed me to #2135445: Toolbar displays Manage tab even if the user is not permitted to see it, which I'd forgotten about.

I might be mistaken but this should fix one fail. As we check for the permission administer shortcuts permission to add the user context, both users, where one has that permission and the other doesn't will have different contexts.

And thank you @Wim Leers for the previous patch! :)

Had a look at the last test fail and it confused me for a while but turns out, it is *beautiful*. What's was happening is that the page got cached in dynamic page cache, as we are testing on a node and not in the backend. So it worked at first, but adding a shortcut didn't invalidate the render cache and then it didn't show the link to remove it.

I added the shortcut list cache tag and then it worked.
Next steps:

* Document shortcut_toolbar_lazybuilder_links()
* Explicit test coverage for the dynamic page cache, I'd suggest we extend \Drupal\Tests\toolbar\Functional\ToolbarCacheContextsTest::testCacheIntegration(), enable the shortcut module and users with the necessary permission and make sure it still is a cache hit, at least for users without administer shortcuts. That said, the extra test method is a bit strange as dynamic_page_cache is enabled by default, this must be an old test that was created before that was done. Maybe we can speed it up a little bit by creating just one single test method for those two things?
* Also, the changes to that test confused me a bit. The problem is that what the test is doing is asserting cache contexts of the whole page, in a non-JS scenario. That means dynamic page cache runs, caches it, but then runs the lazy builders and the final result correctly contains their cacheability metadata, which means the returned page headers do still contain the user cache context. That simply isn't that useful. At minimum we need some better comments in the test and assertion method to explain what we are testing and then as mentioned before, we need to test for the X-Drupal-Dynamic-Cache responses too, to make sure the page is cacheable.

+++ b/core/modules/shortcut/shortcut.module

@@ -324,6 +325,11 @@ function shortcut_preprocess_page_title(&$variables) {
+    // Add a list cache tag for shortcuts.

This seems wired adding the cacheable metadata in preprocess.

I took a stab at the next steps from #31.
1) Documented lazy builder and I converted it to a trusted callback service because we shouldn't be adding code using deprecated interfaces :)
2) Rewrote the tests to explicitly check cache-ability. Stole the test cases from the ShortcutLinksTest and the verification logic from CommentCacheTagsTest and the base page cache class used for EntityCacheTagTestBase tests.
3) Not sure about this one but I think the new test covers things and is more clear?

re #34, yeah it kinda feels ugly but the preprocess adds a link that varies based on a cache tag so its correct right? I don't know but I think its fine.

Finally... this is going to fail. Same failure as #31. I dug in to this and its kinda frustrating and complicated and related to that page_title preprocess cache data. I'll follow up with another comment breaking it down and see if someone has a good solution but tldr it looks like a false positive and everything is working correctly.

I started writing everything up but it became apparent this is a pretty notable shortcoming in EntityCacheTagsTestBase. Basically, if you modify any page cache tags, you can't test entity cache tags. Really limiting for modules and unneeded complexity for the test case.

Opened #3090949: EntityCacheTagsTestBase doesn't allow a module to interact with page tags and test entity cache tags at the same time with all the details... I guess this is probably blocked on coming up with a solution to that?

Oh man, i really didn't read that toolbar test right. Sort of half understood Berdir's review and saw what I expected I guess. I added the toolbar test back the way it was for now and I changed it so there's less code reuse though.

As toolbar displays only one set of shortcuts then there should not be "_list" cache tag there, so test expectation is ok

+++ b/core/modules/shortcut/shortcut.module
@@ -324,6 +325,11 @@ function shortcut_preprocess_page_title(&$variables) {
+    $cacheablity_metadata->addCacheTags(\Drupal::entityTypeManager()->getDefinition('shortcut')->getListCacheTags());

so this is getting the wrong tag? It seems right, but maybe not. @berdir added it in #31 "I added the shortcut list cache tag and then it worked."

I still think EntityCacheTagsTestBase is kinda unworkable because any tag we add could break it. For example if we decide"config:shortcut.set.default" was the correct tag to add for the title link it would "fix" the test as its written now but only by accident because if we ran the same test referencing a different shortcut the tags wouldn't match again.

> 1) Documented lazy builder and I converted it to a trusted callback service because we shouldn't be adding code using deprecated interfaces :)

This code was written long before certain security issues happened that resulted in callback hardening being added to core ;) Sure, now we have to do that, but it wasn't exactly possible back then (would have been possible to do it as a service..)

> As toolbar displays only one set of shortcuts then there should not be "_list" cache tag there, so test expectation is ok

This cache tag isn't about the toolbar, it's about the title block where shortcut adds the "Add to shortcuts link". And that link needs to check if a shortcut already exists or not. And the problem is that the shortcut set then again varies on the user, so that would also result in making it either a lazy builder itself or vary by user, which would be worse than the list cache tag. This is only added in admin themes unless you enable that feature for your frontend theme, so I can live with that.

Honestly, \Drupal\Tests\shortcut\Functional\ShortcutCacheTagsTest is just weird. Wim likes doing generic tests and applying them equally to each entity type and we didn't really have something better back then. But not all entity types are the same. Referencing shortcuts and rendering them is just not a thing. Why should we test that. We add specific cache coverage here for actually relevant use cases, so what I did now is just make it extend from PageCacheTagsTestBase and instead extend the test coverage to also cover additional new shortcuts and changes. And to make it up to Wim for removing test coverage he added, I added some Llamas :-)

Update: I suppose we could in theory also test multiple shortcut sets, but since it now *doesn't actually invalidate* the dynamic page cache, we also can't really verify that adding a shortcut to another set does not result in an invalidation.

That drupalGet() was just for debugging something.

> This is only added in admin themes unless you enable that feature for your frontend theme, so I can live with that.

Also, it's only added for users that have permission to change shortcuts.

This looks great to me! I just found a few nitpicks:

1. +++ b/core/modules/shortcut/src/ShortcutLazyBuilders.php
   @@ -0,0 +1,64 @@
   +class ShortcutLazyBuilders implements TrustedCallbackInterface {
   +  /**
   

   Missing class doxygen and an empty line after the class declaration.

2. +++ b/core/modules/shortcut/src/ShortcutLazyBuilders.php
   @@ -0,0 +1,64 @@
   +   * {@inheritDoc}
   

   inheritdoc :)

3. +++ b/core/modules/toolbar/tests/src/Functional/ToolbarCacheContextsTest.php
   @@ -145,6 +144,22 @@ protected function assertToolbarCacheContexts(array $cache_contexts, $message =
   +   * @return
   +   *   TRUE if the assertion succeeded, FALSE otherwise.
   

   This method doesn't return anything.

Thanks for the review.

I did a bit more cleanup too.

* Testing cache contexts on the page when working with lazy builders is pretty meaningless, as contexts from lazy-built content is still included, at least without big_pipe. But ToolbarCacheContextsTest was doing just that. But that suddenly doesn't make much sense anymore and required a a lot of changes to that test. So instead I just extended the test coverage in ShortcutCacheTagsTest further and removed the shortcut tests from the toolbar test.
* I added some cache context asserts to ShortCacheTagsTest but the main thing is that I'm now additionally testing with two different users with the same set of permissions and making sure that the second user gets a cache hit on the first request. That was already kind of implied, because the whole point of this issue is that the page doesn't get cached at all when there is a non-placeholdered user cache context, but this makes it more explicit.
* @neclimdul added \Drupal\Tests\system\Functional\Cache\PageCacheTagsTestBase::verifyDynamicPageCache(), which is useful, but the support for $tags was never used by this new patch and wouldn't have worked. DPC is much more complicated in how it builds its cache key, which includes cache contexts, and there are cache redirects involved and what not. So I dropped support for that, which means we don't need the new helper method.

Edit: I included the interdiff but it's probably easier to just look at the actual patch as it's almost as big and confusing.

Yay!!! green patch. Nice work!

1. +++ b/core/modules/shortcut/shortcut.module
   @@ -324,6 +325,11 @@ function shortcut_preprocess_page_title(&$variables) {
   +    // Add a list cache tag for shortcuts.
   +    $cacheablity_metadata = CacheableMetadata::createFromRenderArray($variables);
   +    $cacheablity_metadata->addCacheTags(\Drupal::entityTypeManager()->getDefinition('shortcut')->getListCacheTags());
   +    $cacheablity_metadata->applyTo($variables);
   

   First of all hacking the page title in preprocess is not ideal and this problem is not generated here so leaving #34 alone. It seems fine to have this here given we are 'title_suffix' in this function.

2. +++ b/core/modules/shortcut/shortcut.module
   @@ -380,52 +386,43 @@ function shortcut_toolbar() {
   -        'user',
   +        'user.permissions',
   ...
   +      'tray' => [
   ...
   +            'contexts' => ['user'],
   

   This change makes total sense that overall shortcuts toolbar item showing depends on user permission and actual tray contents changes based on the actual user so +1 for the change. Let's update the IS to explain the UI change.

This is RTBC for me. Let's wait for @Wim Leers's review.

+++ b/core/modules/shortcut/shortcut.module
@@ -324,6 +325,11 @@ function shortcut_preprocess_page_title(&$variables) {
+    $cacheablity_metadata = CacheableMetadata::createFromRenderArray($variables);
+    $cacheablity_metadata->addCacheTags(\Drupal::entityTypeManager()->getDefinition('shortcut')->getListCacheTags());
+    $cacheablity_metadata->applyTo($variables);

Typo in "cacheability"

Fixes #48

Looks perfect now :)

I think we can to a little more perfecter ;) No longer need the setUp() in the test and createEntity doesn't work as an @inheritdoc anymore, so I've inlined it into the one place that still used it.

I've also updated the issue summary and commented on the "interesting" changes there again.

So something about my comments in the issue summary still didn't feel quite right. So I checked again and yes, my arguments on why the new list cache tag in page_title is acceptable was based on assumptions, not facts. So as it turns out, shortcut.module *only* checks edit access, then it tries to find a matching shortcut, loads the whole set and only *then* figures out that it actually doesn't have to do anything. That's why we've seen the existing entity cache tags fail, as it has been added there already.

So, I've fixed that too, added explicit test coverage for when the cache tag should be present (and when not!), and even though I still think it doesn't make sense to test shortcut cashing as a regular entity, I've restored that test coverage so we don't remove anything from HEAD anymore.

This wasn't a big performance hit at least so far, as we still display these shortcuts in the toolbar, so we have to load them anyway. But now we only need to load them in the lazy builder, so combined with big_pipe, it only needs to happen after the initial response has been sent. And the loadByProperties() call itself is uncached, so it should save at least one query I think.

The interdiff might not be very useful as it reverts a good chunk of the test changes.

+++ b/core/modules/shortcut/shortcut.module
@@ -324,6 +325,11 @@ function shortcut_preprocess_page_title(&$variables) {
+    // Add a list cache tag for shortcuts.

I think this deserves a bit more of a description about why we're adding this list tag considering the discussion above ;)

Other than that, this looks like it should be good to go back to RTBC!

Here's a patch for #55 with a bit of rewording.

Reviewing this now @acbramley @DS

Have tested this manually on a fresh install with Shortcuts enabled and confirmed:

• Without the patch, logged in as user 1, I'm getting X-Drupal-Dynamic-Cache: UNCACHEABLE
• With the patch, logged in as user 1, I'm getting X-Drupal-Dynamic-Cache: MISS / X-Drupal-Dynamic-Cache: HIT

Will wait for tests before marking RTBC.

Reviewed the code at Drupal South 2019, looks good to me RTBC +1

I queued the patch for retest and it failed again on D8.7, not sure why
But I can apply the patch manually, no issue on my fresh install local env.
All looks good
RTBC+1

I added 8.7 tests by mistake, this patch does not need to work on 8.7 so all good.

Can we get a test-only patch for this to prove the coverage?

Sure thing. Also re-uploading the complete patch from #56, so it doesn't get set back to needs work. Note that it will fail early on the cache tag test coverage that was added for the related fix/improvement to the page title, not on the main thing that we're fixing here, but that would definitely fail as well.

Great that this is RTBC, I was hoping that Wim finds time to look at it, but he's drowned in migrate issues and other things at the moment.

The main thing where I would have liked his feedback has been improved since then, so I think it's not that important anymore and this has been reviewed and tested a lot.

Edit: For the record, I think backporting this to 8.8 is somewhat problematic, for the same reason that #2985253: content_moderation_entity_access() wrongly adds uncacheable dependencies wasn't, improving cacheabilty in core could expose missing cacheability in custom/contrib code and result in things being cached incorrectly. But I guess if anything then before 8.8.0 than in a patch release.

#43:

And the problem is that the shortcut set then again varies on the user, so that would also result in making it either a lazy builder itself or vary by user, which would be worse than the list cache tag. This is only added in admin themes unless you enable that feature for your frontend theme, so I can live with that.

I agree with this conclusion. And I have to point out that even varying by user would be inadequate — see #2339903: Shortcut module's access checking is insane.

EDIT: oh hah, in #53 you also noticed this!

Honestly, \Drupal\Tests\shortcut\Functional\ShortcutCacheTagsTest is just weird. Wim likes doing generic tests and applying them equally to each entity type and we didn't really have something better back then. But not all entity types are the same. Referencing shortcuts and rendering them is just not a thing.

I agree with you :)

#46:

the main thing is that I'm now additionally testing with two different users with the same set of permissions and making sure that the second user gets a cache hit on the first request

Perfect! 🥳

#63:

The main thing where I would have liked his feedback has been improved since then, so I think it's not that important anymore and this has been reviewed and tested a lot.

I agree — #53 made this patch much better :)

1. +++ b/core/modules/shortcut/shortcut.module
   @@ -306,10 +307,10 @@ function shortcut_preprocess_block(&$variables) {
    function shortcut_preprocess_page_title(&$variables) {
   

   There are really only two changes in here:

   1. the theme_get_settings(…) if-condition is pulled up to the top-level if-statement — this causes the majority of the change hunks here, but most of them are just indentation changes
   2. now the appropriate cacheability is added to $variables

   👍

2. +++ b/core/modules/shortcut/shortcut.module
   @@ -380,52 +385,43 @@ function shortcut_toolbar() {
      $items['shortcuts'] = [
        '#cache' => [
          'contexts' => [
   -        // Cacheable per user, because each user can have their own shortcut
   -        // set, even if they cannot create or select a shortcut set, because
   -        // an administrator may have assigned a non-default shortcut set.
   -        'user',
   +        'user.permissions',
          ],
        ],
      ];
   

   👍 The "shortcuts" toolbar tab now appears for all users that have the appropriate permission.

   This is what ensures Dynamic Page Cache can now actually cache the partial HTML response (with placeholders not yet rendered/lazy builders not yet executed). This is the performance problem this issue/patch fixes! 🥳

3. +++ b/core/modules/shortcut/shortcut.module
   @@ -380,52 +385,43 @@ function shortcut_toolbar() {
   +        'children' => [
   +          '#lazy_builder' => ['shortcut.lazy_builders:lazyLinks', []],
   +          '#create_placeholder' => TRUE,
   

   👍 Instead, the user-varying rendered results are now guaranteed to be placeholdered and hence lazily built. Which is what allows Dynamic Page Cache to be effective even when the Shortcut module is installed.

4. +++ b/core/modules/shortcut/shortcut.module
   @@ -380,52 +385,43 @@ function shortcut_toolbar() {
   +          '#cache' => [
   +            'keys' => ['shortcut_set_toolbar_links'],
   +            'contexts' => ['user'],
              ],
   

   👍 But, thanks to the awesome work in this issue, we avoid the need for running the lazy builder on every request: it is cached per user.

5. 🤓 I see some comment nits but it's really not worth delaying this patch over those. Shortcut module has lots of legacy in need of cleaning up — but we already have #2083123: Shortcut cleanup: Remove duplicated code and deprecate legacy functions for that :)

Confirming RTBC 🥳

Committed and pushed b941c7c4c3 to 9.0.x and 00ba9683c4 to 8.9.x. Thanks!

Going to ask release managers about committing this to 8.8.x. Tagged with 8.9.0 release notes as a notable bugfix if we choose to not release it in an 8.8.x bugfix release.

Thanks @alexpott!!

🥳🥳🥳🥳

Thanks to everyone who helped make this happen — finally we'll be able to see the full potential of Dynamic Page Cache & BigPipe, finally users with both toolbar and shortcut enabled will also see the benefits!

Discussed with @catch - we decided to leave this as 8.9.x only as the patch adds a new service - which requires a container rebuild.

Next up #2949457: Enhance Toolbar's subtree caching so that menu links with CSRF token do not need one subtree cache item per session which is waiting for review.

"Noteworthy bug fix" does not describe important update information for a potentially disruptive change.

That said, we typically list the minor-only resolved criticals in the release notes, so I've added it to that list since the 8.9 release notes don't include much else aside from the fixed criticals. In general we should not do this though because for anything other than 8.9 there's too much copy and too many major issues to list even a subset of them.

@xjm: This one might be worth mentioning. Fixing this bug has an "interesting" side effect. It can result in caching pages now with dynamic page cache that previously weren't cacheable for users that had had access to shortcuts. As a result, it is possible that suddenly, content might be cached that does not have correct cacheability metadata. Unikely to be a serious problem, as everything still varies by user role at least, but if sites have custom routes with per-user or query-argument content, then it could still result in problems for them.

I think we need to improve the release note here with a bit more information. :)

Ah yes, a release note based on #76 would be good. What it was before -- what changed -- who's affected -- what they might need to do as a result.

Tried updating the release note.

We are still facing this issue even after trying all the patch from this thread, please suggest

@PratikshaD most probably the cause is admin toolbar contrib module

thanks for the quick reply @andypost, we will check this by disabling that module

#3131589: [meta] Investigate performance regression in Drupal 8.9.x and 9.x and 10.x