user_hook_toolbar() makes all pages uncacheable

On my local environment front page loads two times slower when Toolbar module is enabled.

From #2854131: dynamic cache never ever worked out Probably this can help to investigate

I do not think that is the toolbar module is se that cause this issue... I think that some implementations of the hook_toolbar like user_toolbar() and shortcut_toolbar() add uncacheable cache contexts or cache contexts with with an high cardinality and this cause the problem..
A proof of that? try to disable shortcut module and enable the testing module toolbar_disable_user_toolbar on a clean installation and you will see that the issue is gone

This is the expected behaviour: by default the toolbar includes the user name, which is an element with a "high-frequency" auto-placeholdered context, as you can see in sites/default/default.services.yml under parameters.renderer.config.auto_placeholder_conditions.contexts.

The "UNCACHEABLE" here does not mean the page is not cacheable at all, just that it cannot be cached as a whole page for the Page Cache module, but will need to be assembled using the Dynamic Page Cache, assuming it is enabled.

>The "UNCACHEABLE" here does not mean the page is not cacheable at all, just that it cannot be cached as a whole page for the Page Cache module, but will need to be assembled using the Dynamic Page Cache, assuming it is enabled.

That's not correct. UNCACHEABLE *is* the dynamic page cache module, it means dynamic page cache modules does *not* work on this page.

So the pages *are* not cacheable, but there is no easy fix. We could try to make the username in toolbar a placeholder, but as soon as you have a single menu link with per-user access, you're back to uncacheable.

Oh, my bad, then: I automatically assumed it was a placeholder already. Should have checked.

Resetting to active as per @berdir's comment.

Can we simply replace user name with something more cacheable. Say "Account".

Re #5:

So the pages *are* not cacheable, but there is no easy fix. We could try to make the username in toolbar a placeholder, but as soon as you have a single menu link with per-user access, you're back to uncacheable.

I would like to suggest Drupal Core should be compatible with its own caching mechanisms. Currently both User and Shortcut implement a hook_toolbar with a cache context per user. I have split the Shortcut issue to #2914110: Shortcut hook_toolbar implementation makes all pages uncacheable

Re #8:

Can we simply replace user name with something more cacheable. Say "Account".

I tried to add the tab link to the lazy builder, but the Toolbar modules adds an #id and #attributes to several parts of the renderable array. This causes an exception in the Dynamic page cache: DomainException: "When a #lazy_builder callback is specified, no properties can exist; all properties must be generated by the #lazy_builder callback. You specified the following properties: #attributes.. As a result, the tab is now called 'My account' for all authenticated users.

Attached patch implements a lazy builder so the user profile links are added through a placeholder.

Review:

+++ b/core/modules/user/user.module
@@ -1320,72 +1319,24 @@ function user_cookie_delete($cookie_name) {
-      '#title' => $user->getDisplayName(),

We should try to use a lazybuilder for the username too, that way the current functionality is maintained. Can we move the creation of attributes to the lazybuilder from where ever toolbar generated them now?

Maybe look at what the devel module does: devel/src/ToolbarHandler.php

Re: #9
Code looks good and patch works.
NB: when testing:
- When I apply the patch the page becomes cacheable.
- When I then enable admin_toolbar_tools the page becomes uncacheable again. (see https://www.drupal.org/node/2897309)

Does this need more tests?

#10.1 See 'Re #8:' in #9

#12 Added a functional test to the Toolbar module

Note to self: include new files when creating patches.

When I uninstall the core toolbar modules on Drupal 8.4.2 the "X-Drupal-Dynamic-Cache" header says "MISS" or "HIT". However, when I use patch #9 or #16, it still says "UNCACHEABLE", even after clearing the cache.

Does this mean that all responses that use a "user" context in the cache config are uncachable?

relating older issue that was stagnant.

1. +++ b/core/modules/toolbar/tests/src/Functional/ToolbarCacheContextsTest.php
   --- /dev/null
   +++ b/core/modules/user/src/ToolbarHandler.php
   

   '\Drupal\user\ToolbarHandler' seems pretty vague.

2. +++ b/core/modules/user/src/ToolbarHandler.php
   @@ -0,0 +1,85 @@
   +  public function lazyBuilder() {
   

   Seems to be a psuedo standard of naming these renderX which seems more descriptive.

3. +++ b/core/modules/user/user.module
   @@ -1328,72 +1327,24 @@ function user_cookie_delete($cookie_name) {
   -      '#title' => $user->getDisplayName(),
   +      '#title' => $user->isAnonymous() ? $user->getDisplayName() : t('My account'),
   

   Technically changes the display for users. Probably OK but might need to be make it a placeholder too if we want to be 100% consistent with previous releases.

4. +++ b/core/modules/user/user.module
   @@ -1328,72 +1327,24 @@ function user_cookie_delete($cookie_name) {
   +        // Cacheable per "anonymous or not", because the links to
   +        // display depend on that.
   +        'contexts' => ['user.roles:anonymous'],
   

   Reads kinda awkwardly but having trouble with better wording atm.

5. +++ b/core/modules/user/user.module
   @@ -1328,72 +1327,24 @@ function user_cookie_delete($cookie_name) {
   -        '#cache' => [
   -          // Cacheable per "authenticated or not", because the links to
   -          // display depend on that.
   -          'contexts' => Cache::mergeContexts(['user.roles:authenticated'], $links_cache_contexts),
   -        ],
   

   Looks like we lost the cache context on this element.

When I uninstall the core toolbar modules on Drupal 8.4.2 the "X-Drupal-Dynamic-Cache" header says "MISS" or "HIT". However, when I use patch #9 or #16, it still says "UNCACHEABLE", even after clearing the cache.

That's because shortcut module's toolbar tab also makes the toolbar uncacheable. See #2914110: Shortcut hook_toolbar implementation makes all pages uncacheable for that. If you uninstall the Shortcut module, then test this, you'll see that it does become cacheable.

#21++ in general, and especially #21.3.

Thank you for your review, neclimdul. I have updated the patch per your comments:

#21.1 I chose this name since it matches the only similar Toolbar implementation I could find: the Devel module. The Devel toolbar integration was added in #2839502: Add toolbar integration - make devel menu items more accessible . I'm open for suggestions though.

#21.2 Updated the lazy_builder callback methods to renderToolbarLinks() and renderDisplayName() in line with other lazy_builder callbacks such as NodeViewBuilder::renderLinks and comment.lazy_builders:renderForm.

#21.3 Added a placeholder for the username for authenticated users.

#21.4 Replaced the comment with a similar wording from the CommentForm class.

#21.5 Since the display name and the toolbar links are now added through a lazy builder, there is no longer a need for this cache context. The cache context currently duplicates the context by also adding a user.roles:anonymous, so this context was unnecessary in the first place.

I've merged this into mass.gov and it has finally enabled our dyn page cache for authenticated viewers. I did a code review and it looks worthy IMO.

Sorry I agree ToolbarHandler() is too vague. Could we change it to ToolbarLinkBuilder or similar - and a more descriptive class summary?

#27 Renamed ToolbarHandler to ToolbarLinkBuilder, and added class description

Awesome! Think we're good to go back to RTBC. Thanks for sticking it out through all the nits idebr :-D

Committed/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!

YAY! This means Drupal 8 just become quite a bit faster for many authenticated users in practice :) And it'll ship with Drupal 8.5! Yay!

Except most sites also have the Shortcut module installed, which also prevents Dynamic Page Cache from working. So now we just need to do the same for shortcut_toolbar(), in #2914110: Shortcut hook_toolbar implementation makes all pages uncacheable, and then we'll have a significant performance boost :) Especially now that 8.5 is also shipping with BigPipe!

Thank you everyone for helping make this happen. I had done so many cacheability fixes like this one before D8 shipped, but I was asked to work on things more important than this, and then lost track. It's wonderful to see this finally reaching completion :)

This may be tracked somewhere already, but is anyone worried that once all the "caching for authenticated users" issues are fixed in core that lots of contrib modules are going to have information disclosure issues (ie. caching things less specifically than they should, and showing one user's data to another user) because they gave incorrect cache contexts but the problem was hidden by toolbar and shortcuts disabling caching for authenticated users?

I'm personally a little nervous about this going into 8.5 for that reason. Going into 8.6 is great, because it gives contrib 6 months to realize that they've been doing caching for authenticated users wrong and fix it. But there's not that much time for contrib to test before 8.5 is released.

This may be tracked somewhere already, but is anyone worried that once all the "caching for authenticated users" issues are fixed in core that lots of contrib modules are going to have information disclosure issues (ie. caching things less specifically than they should, and showing one user's data to another user) because they gave incorrect cache contexts but the problem was hidden by toolbar and shortcuts disabling caching for authenticated users?

No, because 99% of modules do permission-based access control, and for that reason user.permissions is a required cache context. See #2493033: Make 'user.permissions' a required cache context, where we had that exact discussion.

And for again that same reason, Dynamic Page Cache shipped with \Drupal\dynamic_page_cache\PageCache\ResponsePolicy\DenyAdminRoutes since day 1: no admin route is ever going to be cached by Dynamic Page Cache!

Note that integration (functional) tests already don't have toolbar and shortcut enabled. Any contrib module doing any integration tests will therefore already have caught any cache context-related problems if they vary by anything else than user.permissions, for example if they vary by user or session.

Ok, I'll trust you! :-)

FYI, the use case I'm imagining is blocks that are only shown for authenticated users, which could potentially have some private data specific to that user, that could end up getting shown to the wrong user. So, maybe they had the cache context set for permissions, not per user, and everything was working fine because the toolbar was adding the per user context, but now that that's gone... information disclosure.

Again, I'm trusting you and accepting that that's a non-issue -- I'm not super great at all the caching stuff, it's something I personally struggle with constantly as a Drupal 8 developer -- but I just wanted to make sure the case I had in mind was clear. :-)

Its not like caching is a hard problem or anything. :) Thanks for all the work guys and I look forward to seeing this in action!

I think that's a valid concern, it is something that I was thinking about as well.

But yes, there are a number of valid reasons why it is not such a huge deal, for example that this is only about the users that have access to the toolbar. And I think on many sites that have registered users, only a small subset does have that. The others already had their cached pages and any issues would have been noticed already there.

Additionally, this only changes parts of the page that are *not* already explicitly cacheable on their own, which includes basically any rendered entity and any block. So this is mostly about the main content region when it is not an entity (like a view, page_manager page or custom controllers) and custom stuff added in preprocess/templates outside of blocks. For parts that already have been cached, this would have already happened, even for users with access to the toolbar.

#37: in the specific example of a per-user block: that block would have already been cached on its own (by render cache). So any bug in there would've already been noticeable. So: what Berdir said :)

Thinking about cacheability while generating output is indeed important in Drupal 8 (and it was important in previous Drupal versions too, the abstractions just weren't really there yet, and it allowed sites to easily break completely). To provide a better introduction into the topic I did https://wimleers.com/talk/rendering-caching-journey-layers recently.

This now makes it impossible for contrib modules to use hook_toolbar_alter() to add links to $items['user']['tray']['user_links']. I had code that did this which now throws the "When a #lazy_builder callback is specified, no properties can exist; all properties must be generated by the #lazy_builder callback." error.

How should modules add or alter user links now that this change has been made?

#41: great question. Note that what your module was doing was not an official/supported API — it was merely altering a render array.

There are a few options:

1. alter the lazy builder callback in $items['user']['tray']['user_links'] to your own, and in your own, call the existing one first. IOW: decorating the existing lazy builder callback
2. similar, but a more reliable approach: decorate the user.toolbar_link_builder service (see big_pipe.services.yml for the only example of service decoration in core)
3. add another set of links to $items['user']['tray'], without modifying the 'user_links' subtree: add your own subtree. Not sure how good this will work.
4. fix the limitations in '#theme' => 'links', so that each link (in the '#links' property) can be a placeholder. Because for example the "log out" link does not actually need a placeholder — it's the same for everyone. Then each individual link can be a placeholder.

Option 4 would be nice, but will require fixing existing flaws/limitations in core. Option 2 is also very nice, and is by far the most reliable option today.

Thanks @wim-leers - I'll think I'll need to try option 3 because I actually have two modules that need to add user links, and neither module is dependent on (or aware of) the other. A service decorator would work if there was only one alter, but I'm not that lucky. :-)

Followup #2951268: Improve rendering account link in the toolbar.

@wim-leers the problem with #2 is that before 8.5.x, user.toolbar_link_builder doesn't exist.

See #2952042: Toolbar still shows "Edit profile" link for a slightly different approach, trying to remove a link if the userprotect module disables it.

Another followup #2956319: Toolbar user links do not work with Big pipe. Also #2952447: Initial page load for a BigPipe-loaded toolbar menu tray results in incorrect toolbar height might be related.