Issue split from #2899392: user_hook_toolbar() makes all pages uncacheable → .
The shortcut implementation of hook_toolbar has a cache context per user in the global section that can not be placeholdered. This causes the Dynamic page cache to stop working, since the cardinality is too high.
Implement a lazy builder to add the shortcuts using placeholdering. Doing required some non-obvious (test) changes:
+++ b/core/modules/shortcut/shortcut.module
@@ -324,6 +325,11 @@ function shortcut_preprocess_page_title(&$variables) {
$shortcut_set = shortcut_current_displayed_set();
+ // Add a list cache tag for shortcuts.
+ $cacheability_metadata = CacheableMetadata::createFromRenderArray($variables);
+ $cacheability_metadata->addCacheTags(\Drupal::entityTypeManager()->getDefinition('shortcut')->getListCacheTags());
+ $cacheability_metadata->applyTo($variables);
+
Shortcut adds a link to the page title to add/remove a shortcut. That depends on the current list of shortcuts, adding or removing one could change this. So this now adds the shortcut_list cache tag. that means changing a shortcut will invalidate all those pages. But there are a few factors that IMHO make this a minor issue at best and doesn't need further optimizations:
* This only happens for users with administer shortcut permissions
* It only happens in themes that have this enabled, typically only admin themes
* Admin pages don't use dynamic page cache anyway, so it's only the page title block that's invalidated
* Changing shortcuts is a pretty rare operation on a live site.
+++ b/core/modules/shortcut/tests/src/Functional/ShortcutLinksTest.php
@@ -355,9 +355,9 @@ public function testAccessShortcutsPermission() {
// Verify that users without the 'administer site configuration' permission
- // can't see the cron shortcuts.
+ // can't see the cron shortcuts but can see shortcuts.
$this->drupalLogin($this->drupalCreateUser(['access toolbar', 'access shortcuts']));
- $this->assertNoLink('Shortcuts', 'Shortcut link not found on page.');
+ $this->assertLink('Shortcuts');
$this->assertNoLink('Cron', 'Cron shortcut link not found on page.');
This is a small regression that can't be avoided.
Until now, if a user has a shortcut set but no access to any of the shortcuts inside, we didn't display an empty "Shortcuts" item. Now we do, because we don't know yet if there will be links or not.. they will be added only later on.
This is similar to #2135445: Toolbar displays Manage tab even if the user is not permitted to see it → , which has been a known issue for a long time.
+++ b/core/modules/toolbar/tests/src/Functional/ToolbarCacheContextsTest.php
@@ -98,11 +98,6 @@ public function testToolbarCacheContextsCaller() {
$this->assertToolbarCacheContexts(['user.permissions'], 'Expected cache contexts found with tour module enabled.');
\Drupal::service('module_installer')->uninstall(['tour']);
-
- // Test with shortcut module enabled.
- $this->installExtraModules(['shortcut']);
- $this->adminUser2 = $this->drupalCreateUser(array_merge($this->perms, ['access shortcuts', 'administer shortcuts']));
- $this->assertToolbarCacheContexts(['user'], 'Expected cache contexts found with shortcut module enabled.');
And finally, we remove this existing, pretty useless test coverage. Even though it is now a lazy builder, the final cache contexts still include user, as it includes the output of the lazy builder. So testing this doesn't help but it becomes complicated as the two users now have different contexts. Earlier patches did a lot of refactoring in this test instead.
The new test coverage covers this and much more.
See also #43 and #46 for more details.
None.
No API changes. There's a *behaviour* change as pages for users with access to the shortcuts now become cacheable in DPC. That means it is possible that custom/contrib code that incorrectly adds cache contexts suddenly becomes cached incorrectly. However, we already did such a change to the username that's displayed in the toolbar, so this already became an issue for sites without shortcuts in an earlier core version. Also, this usually only applies to things that aren't getting render-cached on their own, so logic outside of blocks/entities/views/...
None.
The shortcut module disabled the cache for Dynamic Page Cache for users with access to shortcuts. These pages will now be cached which should result in a performance improvement. In rare cases, cache coherency bugs which were previously hidden by the disabled cache may now be surfaced.
Fixed
8.9 ⚰️
It affects performance. It is often combined with the Needs profiling tag.
Not all content is available!
It's likely this issue predates Contrib.social: some issue and comment data are missing.