As soon as I install the toolbar module, it seems that all pages get the "X-Drupal-Dynamic-Cache: UNCACHEABLE" response header (can be verified e.g with Chrome's web inspector tool). This means that all logged in Drupal users (web editors etc.) who see a toolbar never see any advantage of the dynamic page cache which I find a bit disappointing, because they'll never get any performance speed-up :-(

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

rgpublic created an issue. See original summary.

Chi’s picture

Title: toolbar module makes all pages uncacheable » Toolbar module makes all pages uncacheable
Version: 8.3.6 » 8.4.x-dev
Priority: Normal » Major
Issue tags: +D8 cacheability

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

willzyx’s picture

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

fgm’s picture

Status: Active » Closed (works as designed)
Issue tags: +Vienna2017

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.

Berdir’s picture

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

fgm’s picture

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

fgm’s picture

Status: Closed (works as designed) » Active

Resetting to active as per @berdir's comment.

Chi’s picture

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

idebr’s picture

Title: Toolbar module makes all pages uncacheable » User hook_toolbar implementation makes all pages uncacheable
Version: 8.4.x-dev » 8.5.x-dev
Component: toolbar.module » user.module
Status: Active » Needs review
Related issues: +#2914110: Shortcut hook_toolbar implementation makes all pages uncacheable
FileSize
6.12 KB

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.

finne’s picture

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

finne’s picture

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)

DamienMcKenna’s picture

Does this need more tests?

idebr’s picture

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

#12 Added a functional test to the Toolbar module

The last submitted patch, 13: 2899392-13-test-only.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 13: 2899392-13.patch, failed testing. View results

idebr’s picture

Status: Needs work » Needs review
FileSize
971 bytes
7.05 KB

Note to self: include new files when creating patches.

The last submitted patch, 16: 2899392-16-test-only.patch, failed testing. View results

firewaller’s picture

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.

Narretz’s picture

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

neclimdul’s picture

relating older issue that was stagnant.

neclimdul’s picture

  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.

Wim Leers’s picture

Title: User hook_toolbar implementation makes all pages uncacheable » user_hook_toolbar() makes all pages uncacheable
Status: Needs review » Needs work

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.

idebr’s picture

Status: Needs work » Needs review
FileSize
971 bytes
6.91 KB
2.96 KB

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.

The last submitted patch, 23: 2899392-23-test-only.patch, failed testing. View results

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

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.

catch’s picture

Status: Reviewed & tested by the community » Needs work

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

idebr’s picture

Status: Needs work » Needs review
FileSize
1.82 KB
977 bytes
6.96 KB

#27 Renamed ToolbarHandler to ToolbarLinkBuilder, and added class description

The last submitted patch, 28: 2899392-28-test-only.patch, failed testing. View results

neclimdul’s picture

Status: Needs review » Reviewed & tested by the community

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

  • catch committed ae262e1 on 8.6.x
    Issue #2899392 by idebr, neclimdul, rgpublic: user_hook_toolbar() makes...
catch’s picture

Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

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

  • catch committed 5d27615 on 8.5.x
    Issue #2899392 by idebr, neclimdul, rgpublic: user_hook_toolbar() makes...
Wim Leers’s picture

Issue tags: +8.5.0 highlights

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 :)

dsnopek’s picture

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.

Wim Leers’s picture

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.

dsnopek’s picture

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

neclimdul’s picture

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!

Berdir’s picture

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.

Wim Leers’s picture

#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.

samuel.mortenson’s picture

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?