I ran into the issue of admin_toolbar_tools being incompatible with dynamic page cache - #2897309: admin_toolbar_tools module makes all pages uncacheable. After some debugging I figured the cause are the CSRF tokens in the links. However those where already correctly place-holdered, still they ended up being rendered too early causing the 'session' cache context being added in, making the requests uncachable.

Turns out, problem is that toolbar forces replacing placeholders. Fixing that resolves the problem admin_toolbar_tools and it works just fine.

Steps to reproduce based on #62:

  1. Log in as administrator
  2. Install admin_toolbar_tools
  3. View home page and check headers
  4. Apply patch
  5. View home page and check headers

Without patch:

X-Drupal-Dynamic-Cache: UNCACHEABLE

With patch (after reloading at least once):

X-Drupal-Dynamic-Cache: HIT

CommentFileSizeAuthor
#77 Screenshot 2022-02-11 at 2.18.35 PM.png17.53 KBjoshua1234511
#77 Screenshot 2022-02-11 at 2.17.39 PM.png15.74 KBjoshua1234511
#77 Screenshot 2022-02-11 at 2.18.35 PM.png17.53 KBjoshua1234511
#75 2949457-9.4.x-75.patch7.51 KBkim.pepper
#75 2949457-9.3.x-75.patch7.51 KBkim.pepper
#71 2949457-71.patch6.26 KBSuresh Prabhu Parkala
#66 2949457-66_8.9.patch6.31 KBdungahk
#65 interdiff.2949457.60-65.txt740 bytesdungahk
#65 2949457-65.drupal.Enhance-Toolbars-subtree-caching-so-that-menu-links-with-CSRF-token-do-not-need-one-subtree-cache-item-per-session.patch7.51 KBdungahk
#60 2949457-60.interdiff.txt3.76 KBneclimdul
#60 2949457-60.patch7.51 KBneclimdul
#59 interdiff_57-59.txt2.58 KBravi.shankar
#59 2949457-59.patch5.27 KBravi.shankar
#57 2949457-57.patch5.26 KBSam152
#53 2949457-53.patch5.28 KBidebr
#53 2949457-53-test-only.patch2.8 KBidebr
#53 interdiff-50-53.txt795 bytesidebr
#50 2949457-50.patch5.05 KBidebr
#50 2949457-50-test-only.patch2.57 KBidebr
#43 2949457-43.patch5.05 KBjibran
#43 interdiff-3792e3.txt2.3 KBjibran
#37 2949457-37.patch4.57 KBjibran
#36 2949457-36.patch4.55 KBjibran
#29 2949457-29-manual_testing-do-not-test.patch1.49 KBWim Leers
#28 2949457-28.patch4.29 KBWim Leers
#28 interdiff.txt4.18 KBWim Leers
#21 2949457-21.patch5.7 KBidebr
#21 2949457-21-test-only.patch2.64 KBidebr
#21 interdiff-16-21.txt901 bytesidebr
#16 interdiff-14-16.txt962 bytesidebr
#16 2949457-16.patch5.67 KBidebr
#16 2949457-16-test-only.patch2.64 KBidebr
#14 2949457-14.patch5.67 KBidebr
#14 2949457-14-test-only.patch1.9 KBidebr
#9 2949457-9.patch5.67 KBidebr
#9 2949457-9-test-only.patch2.13 KBidebr
#7 d8_8.6.x-toolbar_rendering.patch2.92 KBfago
#2 d8_toolbar_cache.patch958 bytesfago
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fago created an issue. See original summary.

fago’s picture

Status: Active » Needs review
FileSize
958 bytes

Patch fixes the problem for me, applies to 8.4.x, 8.5.x, 8.6.x.

fago’s picture

Note that the fix of #2899392: user_hook_toolbar() makes all pages uncacheable is also required to make it work with 8.4.x

Status: Needs review » Needs work

The last submitted patch, 2: d8_toolbar_cache.patch, failed testing. View results

MiroslavBanov’s picture

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

I am reproducing this same issue on Drupal 8.6.x. When I comment toolbar_page_top() dynamic page cache starts working.

This can easily be reproduced in simplytest.me with just installing base drupal:

  1. Go to simplytest.me
  2. Choose project: Drupal core, version: 8.6.x
  3. Optionally add patch - it doesn't change anything.
  4. Log in as admin
  5. Reload front page
  6. Look at front page headers - X-Drupal-Dynamic-Cache: UNCACHEABLE
  7. Uninstall toolbar
  8. Reload front page
  9. Look at front page headers - X-Drupal-Dynamic-Cache: MISS
  10. Reload front page
  11. Look at front page headers - X-Drupal-Dynamic-Cache: HIT

I tried the patch above - it doesn't make any difference for me. I don't really understand why it would make a difference, and what exactly CSRF tokens you are referring to, and where they are coming from.

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

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

fago’s picture

Status: Needs work » Needs review
FileSize
2.92 KB

Maybe in your scenario something else is preventing dynamic cache to work in addition.

> I don't really understand why it would make a difference, and what exactly CSRF tokens you are referring to, and where they are coming from.

Well, in my case it was devel toolbar adding links with CSRF tokens. That's usually fine as they should be lazy-placeholdered, but toolbar's wrong call to renderPlain() forces the placesholders to be applied to early, and thus makes the render array dependent on cache context that prevents dynamic page cache.

As the testbot has shown, there was a problem in my previous patch though. The access handler tries to use that internal code without an active render context. I updated the patch to fix that also.

fago’s picture

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

I guess bug-fixes can still make it into 8.6.x? -> Changing version.

idebr’s picture

Title: Toolbar is incompatible with dynamic page cache » Toolbar's renderPlain() is incompatible with dynamic page cache
Version: 8.6.x-dev » 8.7.x-dev
Issue tags: +D8 cacheability, +Performance
FileSize
2.13 KB
5.67 KB

@fago Thanks for the patch and clearly describing the issue at hand. I added appropriate issue tags for increased visibility. If I am not mistaken, bug reports are fixed in the latest release branch (currently 8.7.x) and then cherry-picked to earlier supported branches (currently 8.6.x and 8.5.x) when applicable.

Attached patch adds failing test to show the Toolbar currently handles rendering incorrectly.

The last submitted patch, 9: 2949457-9-test-only.patch, failed testing. View results

andypost’s picture

Test-only failed to apply

nils.destoop’s picture

At first sight, this patch was not working for me. But it was because shortcut was also enabled. Shortcut always adds the user context. Uninstalled shortcut, and dynamic page cache . was ok.

MiroslavBanov’s picture

#12

Thanks for pointing that out. Uninstalling Shortcut module fixes the dynamic page cache, for when installing just Standard profile. That was the problem in #5. Also found the issue here #2914110: Shortcut hook_toolbar implementation makes all pages uncacheable

idebr’s picture

Rerolled #9 so the patches can be applied to 8.7.x

Status: Needs review » Needs work

The last submitted patch, 14: 2949457-14.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

idebr’s picture

Status: Needs work » Needs review
FileSize
2.64 KB
5.67 KB
962 bytes
  1. Fixed incomplete test-only patch
  2. Fixed code sniffer violations

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

akalam’s picture

#9 is working for us on d8.5

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

This looks awesome, great work everyone!

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/toolbar/src/Controller/ToolbarController.php
@@ -48,7 +77,12 @@ public function subtreesAjax() {
-    $expected_hash = _toolbar_get_subtrees_hash()[0];
+    // As subtree-hashes are calculated during rendering, there must be an
+    // active render context. Thus provide a one to avoid rendering to bail out
+    // due to missing render contexts.
+    $expected_hash = $this->renderer->executeInRenderContext(new RenderContext(), function () {

This needs a reword, for example "thus provide one to avoid an early rendering exception" - if that's the correct explanation.

idebr’s picture

The solution to render the subtree in a new RenderContext is similar to the implementation of media embeds rendering in #2831944: Implement media source plugin for remote video via oEmbed, so I updated the comment accordingly.

The last submitted patch, 21: 2949457-21-test-only.patch, failed testing. View results

andypost’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/system/tests/modules/csrf_test/csrf_test.links.menu.yml
    @@ -0,0 +1,13 @@
    +csrf_test.help:
    +  title: 'Tools'
    +  route_name: <front>
    +  menu_name: admin
    +  parent: system.admin
    +  weight: -100
    

    Is this necessary? I can't see why it is. I'm probably missing something.

  2. +++ b/core/modules/toolbar/src/Controller/ToolbarController.php
    @@ -48,7 +77,12 @@ public function subtreesAjax() {
    +    // As subtree-hashes are calculated during rendering, there must be an
    +    // active render context. Provide a new render context so that the
    +    // cacheability metadata of the rendered HTML will be captured correctly.
    +    $expected_hash = $this->renderer->executeInRenderContext(new RenderContext(), function () {
    +      return _toolbar_get_subtrees_hash()[0];
    +    });
    
    +++ b/core/modules/toolbar/tests/src/Functional/ToolbarCacheContextsTest.php
    @@ -62,7 +62,7 @@ protected function setUp() {
    -    $this->installExtraModules(['dynamic_page_cache']);
    +    $this->installExtraModules(['csrf_test', 'dynamic_page_cache']);
    

    Whilst this patch is current adding test coverage it looks a bit fragile because we're not actually asserting that the additional cacheability metadata is captured.

idebr’s picture

Status: Needs work » Needs review

#24.1 The conflict with Dynamic Page Cache becomes apparent when the Toolbar displays a menu link with a CSRF token. This is the case when a site uses the Admin Toolbar module. The test coverage adds menu links with a CSRF token to simulate this use case.

#25.2 Actually the patch removes the 'session' cache context, so Dynamic Page Cache starts working again. This is checked using the existing assertion in the 'X-Drupal-Dynamic-Cache' response header in \Drupal\Tests\toolbar\Functional\ToolbarCacheContextsTest::testCacheIntegration().

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

I'll review this. Will try to do it before the end of the week.

Fabianx’s picture

RTBC + 1 from my side and great catch. (leaving final RTBC to Wim due to it having been RTBC twice already before)

executeInRenderContext is correct to use here for the access check as the result is not actually rendered and the inner cacheability hence does not matter.

i.e. the only reason for using renderPlain() in the first place was to avoid the Exception thrown when no render context is active during the access check.

Therefore this should be good to go.

Wim Leers’s picture

Sorry for the delay, got a lot of very high priority things on my plate.

  1. +++ b/core/modules/toolbar/toolbar.module
    @@ -306,7 +306,7 @@ function toolbar_get_rendered_subtrees() {
    -  \Drupal::service('renderer')->renderPlain($data);
    +  \Drupal::service('renderer')->render($data);
    
    @@ -336,7 +336,7 @@ function _toolbar_do_get_rendered_subtrees(array $data) {
    -      $output = \Drupal::service('renderer')->renderPlain($subtree);
    +      $output = \Drupal::service('renderer')->render($subtree);
    

    👍

    This is changing
    render this, including replacing placeholders for things like CSRF tokens in links, and also, don't bubble cacheability
    to:
    render this, but don't replace placeholders just yet for things like CSRF tokens in links, and also, letting cacheability bubble is fine

    (This is also what the original path in #2 did.)

  2. +++ b/core/modules/toolbar/src/Controller/ToolbarController.php
    @@ -48,7 +77,12 @@ public function subtreesAjax() {
    -    $expected_hash = _toolbar_get_subtrees_hash()[0];
    +    // As subtree-hashes are calculated during rendering, there must be an
    +    // active render context. Provide a new render context so that the
    +    // cacheability metadata of the rendered HTML will be captured correctly.
    +    $expected_hash = $this->renderer->executeInRenderContext(new RenderContext(), function () {
    +      return _toolbar_get_subtrees_hash()[0];
    +    });
    

    👎 Because of the change above, this change is necessary, but not for the reason described in this comment. The reason that we need to execute this in a render context is to avoid the bubbling of cacheability when merely calculating the hash to check access.

    We wouldn't need any of these changes if #1 would be executing its render logic in a render context. Those are the places concerned with rendering, not this one. The need for the changes in this hunk shows we're making an abstraction leaky.

This fixes that. Makes the patch much simpler.

Wim Leers’s picture

Title: Toolbar's renderPlain() is incompatible with dynamic page cache » Enhance Toolbar's subtree caching so that menu links with CSRF token do not need one subtree cache item per session
Assigned: Wim Leers » Unassigned
Status: Needs review » Needs work
FileSize
1.49 KB

Far more importantly though, is the question of whether this still results in correct caching. Yes, perhaps the appearance of a link with a CSRF token resulted in an uncacheable toolbar, but … is making it cacheable actually correct?

So let's test with D8 HEAD with the csrf_test module enabled and the two hunks from the above patch applied to it (in 2949457-29-manual_testing-do-not-test.patch, attached)

HEAD
The following 4 cache items exist:
toolbar:[languages:language_interface]=en:[session]=4ODqTRyHK_ypQzGtUfVeXgRInoCPOc9uwQw43kpOE8c:[theme]=seven:[user.permissions]=8f5ff07a7aa52925ee935f27fab962760a193419d0d476010cddf8e6db939a11:[user.roles:anonymous]=is-super-user
toolbar:[languages:language_interface]=en:[theme]=seven:[user.permissions]=8f5ff07a7aa52925ee935f27fab962760a193419d0d476010cddf8e6db939a11
toolbar_rendered_subtrees:[languages:language_interface]=en:[session]=4ODqTRyHK_ypQzGtUfVeXgRInoCPOc9uwQw43kpOE8c:[theme]=seven:[user.permissions]=8f5ff07a7aa52925ee935f27fab962760a193419d0d476010cddf8e6db939a11
toolbar_rendered_subtrees:[languages:language_interface]=en:[theme]=seven:[user.permissions]=8f5ff07a7aa52925ee935f27fab962760a193419d0d476010cddf8e6db939a11

The first two are for the toolbar that you see at the top of the page. The last two are for the admin menu subtree, which is accessible in the vertical orientation of the toolbar tray labeled "manage".

The 1st and 3rd are "redirecting" cache items, where bubbled cache contexts are collected to point to the appropriate variation (the 2nd and 4th, respectively).

Note how the 2nd and 4th contain: [session]=4ODqTRyHK_ypQzGtUfVeXgRInoCPOc9uwQw43kpOE8c. That means that the session cache context is part of their CID.

This is what allows them to cache the entire subtree: because the subtree contains a CSRF token, it ought to vary by it. Otherwise a user with the same permissions would be able to see this user's CSRF token.

HEAD + patch
toolbar:[languages:language_interface]=en:[theme]=seven:[user.permissions]=8f5ff07a7aa52925ee935f27fab962760a193419d0d476010cddf8e6db939a11
toolbar:[languages:language_interface]=en:[theme]=seven:[user.permissions]=8f5ff07a7aa52925ee935f27fab962760a193419d0d476010cddf8e6db939a11:[user.roles:anonymous]=is-super-user
toolbar_rendered_subtrees:[languages:language_interface]=en:[theme]=seven:[user.permissions]=8f5ff07a7aa52925ee935f27fab962760a193419d0d476010cddf8e6db939a11
toolbar_rendered_subtrees:[user.permissions]=8f5ff07a7aa52925ee935f27fab962760a193419d0d476010cddf8e6db939a11

Note the absence of the session cache context! Now, this is fine, if the contents then also do not vary by session, i.e. if the cached contents don't contain a CSRF token.

Sadly, they do!

So this patch "fixes" the symptom of responses not being cacheable by Dynamic Page Cache. But it turns out that this was happening to guarantee cache correctness.

Also, this means that the issue title was quite misleading. Fixed.

Wim Leers’s picture

Priority: Major » Normal

I also don't see how this is Major. How many people truly are affected by this? Who adds a CSRF link to their admin menu?

Wim Leers’s picture

idebr’s picture

I also don't see how this is Major. How many people truly are affected by this? Who adds a CSRF link to their admin menu?

The Admin Toolbar has a submodule Admin Toolbar Tools that adds menu links that require a CSRF token, such as 'Run cron'. Drupal.org does not report on submodule usage, but I imagine a large portion of module installs has this submodule enabled as well. This means many site builders and developers are currently working without Dynamic Page Caching.

rgpublic’s picture

@Wim: Well, I thought this is due to the menu items added by admin_tools. Flush cache, run cronjobs, etc. They all have CSRF tokens AFAIK. So this issue here effectively means: For all people with administrator privileges, all pages are uncacheable. Is it "Major"? Well, perhaps, no. Is it still quite significant? I think so, because it's totally unexpected that you get completely different caching behavior depending on whether you see some menu items or not. After D8 was launched, I took my quite a while to figure out why seemingly no pages were cached whenever I was logged in. It looked like the Dynamic Cache didnt work at all.

Wim Leers’s picture

Priority: Normal » Major

#32 + #33: Aha! Thanks for adding that information to this issue :) Evidently increasing priority to Major again!

I think that pretty much all of the Toolbar's caching should be reassessed. It was written in a time when render caching was pretty new, when cache contexts did not exist yet and more … all because it finished in ~2012/2013, at the very start of the Drupal 8 development cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jibran’s picture

jibran’s picture

FileSize
4.57 KB

Correct reroll.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

neclimdul’s picture

+++ b/core/modules/toolbar/toolbar.module
@@ -296,6 +297,10 @@ function toolbar_get_rendered_subtrees() {
+  $renderer = \Drupal::service('renderer');
+  $renderer->executeInRenderContext(new RenderContext(), function() use ($renderer, $data) {
+    $renderer->render($data);
+  });
   \Drupal::service('renderer')->renderPlain($data);

this seems to renders twice? That doesn't seem right.

I'm trying to catch up and this seems pretty complicated(caching often is). What's the problem we're trying to solve out of #29?

jibran’s picture

We have CSRF links in the Toolbar when Admin Toolbar Tools is enabled which is a submodule Admin Toolbar but CSRF link calls kill switch and make the toolbar uncacheable which in 8.8 makes the whole page uncacheable in 8.7 and before it was just toolbar.

neclimdul’s picture

right, that's how i got here :) just trying to figure out how to move it from NW to NR/RTBC ;)

larowlan’s picture

jibran’s picture

Status: Needs work » Needs review
Issue tags: +DrupalSouth 2019
FileSize
2.3 KB
5.05 KB

Fixed #39 and added asserts for session context. Turns out we need to render the link with CSRF token to get the session context.

kualee’s picture

Reviewing this now at @Drupal South 2019. Just commenting to avoid duplication :)

kualee’s picture

I have tested the patch in #43 and it applied cleanly and worked fine on a fresh install of 8.8, with admin toolbar enabled. Though I have to uninstall Shortcut module like mentioned in #12 and #13
Does this patch mean to work even with the Shortcut module enabled?

Berdir’s picture

I'm a bit confused about the status here, is there something that needs to be done based on the feedback from @Wim or is that OK now?

jibran’s picture

I think the latest patch addressed issue discussed in #29.

acbramley’s picture

I've applied #43 to a couple of projects and it works well. Along with admin_toolbar_tools, pages are now cacheable. Without the patch all pages for logged in users were marked as UNCACHEABLE.

fago’s picture

Yep, I can confirm the patch is still needed to keep dynamic page cache working.

idebr’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
2.57 KB
5.05 KB

The feedback in #29 was addressed: the cache context now includes the session. I did another manual test to confirm the patch now works as intented.

Uploading test-only patch to show the current behaviour. The patch itself is unchanged.

The last submitted patch, 50: 2949457-50-test-only.patch, failed testing. View results

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 50: 2949457-50.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

idebr’s picture

Added legacy annotation to test, since it uses a module with a deprecated route parameter.

Wim Leers’s picture

+++ b/core/modules/toolbar/src/Controller/ToolbarController.php
@@ -112,7 +114,9 @@ public static function preRenderGetRenderedSubtrees(array $data) {
+        $output = $renderer->executeInRenderContext(new RenderContext(), function() use ($renderer, $subtree) {

+++ b/core/modules/toolbar/toolbar.module
@@ -296,7 +297,10 @@ function toolbar_get_rendered_subtrees() {
+  $renderer->executeInRenderContext(new RenderContext(), function() use ($renderer, &$data) {
  1. Why is one by reference and not the other? Let's document that.
  2. Why are we throwing away the render context without merging its cacheability? Let's document that. (See \Drupal\Core\Render\MainContent\HtmlRenderer::renderResponse() vs \Drupal\Core\Render\MainContent\HtmlRenderer::prepare() for respectively an example where we do throw its cacheability away and where we don't.)

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Sam152’s picture

Rerolling for 9.2.

Status: Needs review » Needs work

The last submitted patch, 57: 2949457-57.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

ravi.shankar’s picture

Status: Needs work » Needs review
FileSize
5.27 KB
2.58 KB

Fixed failed tests and some CS issues of patch #57.

neclimdul’s picture

bump deprecations because I'm being hopeful.

re: #53.1 I don't know the answer to why the second one isn't passed by reference but I documented the first. Seems like if there was additional cache metadata deeper in subtrees that would get lost right?

re: #53.2 I'm not sure I understand the question but I get the feeling that's probably related to me not understanding why the pre-process method isn't rendering the subtrees by reference.

Kristen Pol’s picture

Status: Needs review » Needs work
Issue tags: +Bug Smash Initiative

Thanks for the update.

1) Patch still applies cleanly.

2) Reviewed the interdiff and see the version change, type hint, and new comment. IMO this part isn't super clear:

// The pre_render do the

Maybe should be something like:

The pre_render process populates $data during the render pipeline.

3) Tested as follows:

  • Applied patch
  • Logged in
  • Viewed home page and checked headers
  • Uninstalled toolbar
  • Viewed home page and checked headers

In both cases, when first viewing the home page, I get:

X-Drupal-Dynamic-Cache: MISS

and when reloading:

X-Drupal-Dynamic-Cache: HIT

as expected.

3) While a comment was added for the pass-by-reference noted in #54.1, #54.2 isn't addressed as is noted in #60.

Thus, while this appears to be working, marking back to needs work to address that documentation.

Kristen Pol’s picture

I didn't test properly above so here are the correct steps:

  • Log in as administrator
  • Install admin_toolbar_tools
  • View home page and check headers
  • Apply patch
  • View home page and check headers

Without patch:

X-Drupal-Dynamic-Cache: UNCACHEABLE

With patch (after reloading at least once):

X-Drupal-Dynamic-Cache: HIT
Kristen Pol’s picture

Btw, regarding #54.2:

Why are we throwing away the render context without merging its cacheability? Let's document that. (See \Drupal\Core\Render\MainContent\HtmlRenderer::renderResponse() vs \Drupal\Core\Render\MainContent\HtmlRenderer::prepare() for respectively an example where we do throw its cacheability away and where we don't.)

I don't see the render context used in prepare, only in renderResponse.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

dungahk’s picture

dungahk’s picture

tunic’s picture

Status: Needs work » Needs review

Tests passed, moving to needs review.

neclimdul’s picture

Status: Needs review » Needs work

Still need to address Wim's comment note and now the deprecation version needs to be bumped to 9.3

dungahk’s picture

Just adding that this has worked for a site I've tested as well.

kim.pepper’s picture

Issue tags: +#pnx-sprint
Suresh Prabhu Parkala’s picture

A re-rolled patch against 8.9.x. Please review.

Suresh Prabhu Parkala’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 71: 2949457-71.patch, failed testing. View results

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Kristen Pol’s picture

Issue summary: View changes
Issue tags: +Needs manual testing

Updated issue summary with steps to reproduce from #62 and tagging for manual testing.

joshua1234511’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
17.53 KB
15.74 KB
17.53 KB

Manually tested the patch provided in #75 with the steps from issue summary.

Loged in as administrator
Install admin_toolbar_tools
checked home page and check headers
Applied the patch cleared cache
Checked home page and check headers

Test results for 9.3
Pass - Correct header is applied

Test results for 9.4
Pass - Correct header is applied

  • catch committed f2669a3 on 10.0.x
    Issue #2949457 by idebr, jibran, Wim Leers, dungahk, fago, kim.pepper,...

  • catch committed df171a1 on 9.4.x
    Issue #2949457 by idebr, jibran, Wim Leers, dungahk, fago, kim.pepper,...

  • catch committed e24cd89 on 9.3.x
    Issue #2949457 by idebr, jibran, Wim Leers, dungahk, fago, kim.pepper,...
catch’s picture

Version: 9.4.x-dev » 9.3.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs manual testing

Committed/pushed the 9.4.x patch to 10.0.x and cherry-picked back, and committed the 9.3.x patch too. Good to see this one ready to go.

Wim Leers’s picture

Wow, blast from the past! Nice to see this land indeed :)

Status: Fixed » Closed (fixed)

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