Problem/Motivation

Split off from #2543334: Auto-placeholdering for #lazy_builder with bubbling of contexts and tags to make that one more easily reviewable. Details to follow.

Proposed resolution

Details to follow.

Remaining tasks

Details to follow.

User interface changes

None.

API changes

None.

Data model changes

None.

Why this should be an RC target

From #10:

Without this, max-age=0 blocks will not be placeholdered automatically, which means that any page with uncacheable blocks will not be able to use Dynamic Page Cache.

This would therefore IMO be a great RC target.

CommentFileSizeAuthor
#50 2559847-49.patch15.41 KBayalon
#48 reroll_diff_2559847_46-48.txt14.93 KBankithashetty
#48 2559847-48.patch15.42 KBankithashetty
#47 2559847-46.patch14.66 KBayalon
#44 2559847-44.patch15.13 KBayalon
#41 2559847-41.patch15.44 KBneclimdul
#41 2559847-41.interdiff.txt637 bytesneclimdul
#39 2559847-39.patch15.1 KBneclimdul
#39 2559847-39.interdiff.txt1.47 KBneclimdul
#31 2559847-31.patch15.08 KBMerryHamster
#27 auto-placeholdering-step3-max-age-2559847-27.patch15.08 KBsavkaviktor16@gmail.com
#15 interdiff.txt939 byteswim leers
#15 auto-placeholdering-step3-max-age-2559847-15.patch15.47 KBwim leers
#5 interdiff.txt2.24 KBwim leers
#5 auto-placeholdering-step3-max-age-2559847-5.patch15.61 KBwim leers
#4 auto-placeholdering-max-age.patch15.34 KBwim leers

Comments

Wim Leers created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented
Status: Postponed » Active
catch’s picture

catch
he/him
Primary language English
commented
Title: [PP-1] Auto-placeholdering for #lazy_builder with bubbling of max-age » Auto-placeholdering for #lazy_builder with bubbling of max-age
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new auto-placeholdering-max-age.patch15.34 KB

Here's a patch, directly extracted from #2543334-44: Auto-placeholdering for #lazy_builder with bubbling of contexts and tags.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
StatusFileSize
new auto-placeholdering-step3-max-age-2559847-5.patch15.61 KB
new interdiff.txt2.24 KB

The last submitted patch, 4: auto-placeholdering-max-age.patch, failed testing.

The last submitted patch, 4: auto-placeholdering-max-age.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 5: auto-placeholdering-step3-max-age-2559847-5.patch, failed testing.

The last submitted patch, 5: auto-placeholdering-step3-max-age-2559847-5.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +rc target triage

Without this, max-age=0 blocks will not be placeholdered automatically, which means that any page with uncacheable blocks will not be able to use Dynamic Page Cache.

This would therefore IMO be a great RC target.

But since it's a pure behavior change, and not an API change, it could also land in 8.1.0.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes
dawehner’s picture

dawehner
Primary language German
commented
Issue tags: +Needs tests

Given that this just touches one single class this could be easily implemented by a contrib module, right?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: -Needs tests

Test coverage is already included.

In theory this can be implemented in a contrib module until 8.1.x, yes. But virtually nobody will install such a module.

Without this, any block (or anything else) that has max-age=0 will cause the entire page to not be cached.

It's totally fine to postpone this to 8.1.x, it would merely be unfortunate.

The last submitted patch, 5: auto-placeholdering-step3-max-age-2559847-5.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Needs review
StatusFileSize
new auto-placeholdering-step3-max-age-2559847-15.patch15.47 KB
new interdiff.txt939 bytes 
+++ b/core/lib/Drupal/Core/Render/RenderCache.php
@@ -94,10 +95,24 @@ public function set(array &$elements, array $pre_bubbling_elements) {
+   * @param array $elements
+   *   Unlike ::set(), this is not passed by reference, which means side-effects
+   *   do not exist anymore here; they're isolated to set().

I got overzealous with enforcing simplicity here. This was possible initially, but then I also did this:

+++ b/core/lib/Drupal/Core/Render/RenderCache.php
@@ -94,10 +95,24 @@ public function set(array &$elements, array $pre_bubbling_elements) {
+  protected function doSet(array $elements, array $pre_bubbling_elements) {
+    $cid = $this->createCacheID($elements);

And it's createCacheID()'s side effect that must work: it updates $elements by reference when necessary. In the first iteration, the createCacheID() still lived in the set() call. Later, I then had to move the createCacheID() call into doSet(), thus invalidating the code comment I quoted at the beginning.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: -rc target triage

 

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented

Came here from https://www.drupal.org/docs/8/api/render-api/auto-placeholdering.

I'm debugging an issue with a view. The view's cache setting is set to off, which from what I can tell sets the max-age to 0 for the views render array. However the whole page is still cached via the Page Cache (it does not appear to be cached in the Dynamic Page Cache). Is that due to this issue? I'm currently at 8.1.8.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Nope. You are looking for #2352009: Bubbling of elements' max-age to the page's headers and the page cache, you can see there about the challenges. See also http://drupal.stackexchange.com/questions/219568/stop-full-page-html-fro...

dawehner’s picture

dawehner
Primary language German
commented
Issue tags: +Needs issue summary update
m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented

Thanks @Berdir!

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

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

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should 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.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch no longer applies

savkaviktor16@gmail.com’s picture

savkaviktor16@gmail.com commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new auto-placeholdering-step3-max-age-2559847-27.patch15.08 KB

Re-rolled

Status: Needs review » Needs work

The last submitted patch, 27: auto-placeholdering-step3-max-age-2559847-27.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented 
+++ b/core/lib/Drupal/Core/Render/PlaceholderingRenderCache.php
@@ -131,6 +155,28 @@ public function set(array &$elements, array $pre_bubbling_elements) {
+    if ($result === FALSE && $this->placeholderGenerator->canCreatePlaceholder($pre_bubbling_elements) && $this->placeholderGenerator->shouldAutomaticallyPlaceholder($elements) && $this->requestStack->getCurrentRequest()->isMethodSafe() && !$this->maxAgeAllowsCaching($elements) && $this->maxAgeAllowsCaching($pre_bubbling_elements) && $cid = $this->createCacheID($elements)) {

Calls like this should apparently use isMethodCacheable() now.

MerryHamster’s picture

MerryHamster commented
Version: 8.5.x-dev » 8.6.x-dev
MerryHamster’s picture

MerryHamster commented
StatusFileSize
new 2559847-31.patch15.08 KB

Here is an only reroll for 8.6.x-dev

MerryHamster’s picture

MerryHamster commented
Status: Needs work » Needs review

Status: Needs review » Needs work

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

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.

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.

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.

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.

neclimdul’s picture

neclimdul
He/Him
commented
Status: Needs work » Needs review
StatusFileSize
new 2559847-39.interdiff.txt1.47 KB
new 2559847-39.patch15.1 KB

as noted by Berdir, the isMethodCacheable change. Haven't really reviewed the patch yet, wanted to see if this fixed all the failures first to set a baseline.

Found this in some documentation as an important upcoming change (in 8.6) and it _is_ marked major but is the summary still accurate? Seems like some people would be running into really annoying caching problems without it but there's no traction in a long time.

Status: Needs review » Needs work

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

neclimdul’s picture

neclimdul
He/Him
commented
Status: Needs work » Needs review
StatusFileSize
new 2559847-41.interdiff.txt637 bytes
new 2559847-41.patch15.44 KB

This should make it green. Just the old trusted callback changes for lazy builders.

neclimdul’s picture

neclimdul
He/Him
commented

Ok, so following back to the previous issue there's an epic comment by wim that kinda hints at what's going on here and I feel like I got the faintest hint of what's going on hear but I don't think I grok the impact of this rendering process well enough to get the interaction. I don't think the logic in arrays is helping any either.

That said, the fact that with the amount of testing we have in core at this point, nothing failing is a solid sign. I can also provide a bit of a code review:

  1. +++ b/core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php
@@ -375,13 +383,42 @@ public function providerPlaceholders() {
+    $cases[] = [
...
+    $cases[] = [

    Seem's like these provider datasets could use useful keys. 

  2. +++ b/core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php
@@ -573,6 +610,9 @@ protected function assertPlaceholderRenderCache($cid_parts, array $bubbled_cache
+      if ($expected_data['#cache']['max-age'] === 0) {
+        $this->assertEquals(-1, $cached->expire, 'When a bubbled max-age=0 is cached, it is cached permanently for the given set of ');

    The comment doesn't seem helpful as an assertion and is incomplete. This should assert the constant.

Unfortunately this was kind-of a distraction so I might not come around to reviewing this further for a while but it looks like a cool problem with some interesting performance implications so interest peaked :-D

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.

ayalon’s picture

ayalon commented
StatusFileSize
new 2559847-44.patch15.13 KB

Reroll for 9.2.2

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.

ayalon’s picture

ayalon commented

Reroll for 9.3.2

ayalon’s picture

ayalon commented
StatusFileSize
new 2559847-46.patch14.66 KB

Reroll for 9.3.2

ankithashetty’s picture

ankithashetty
Location Karnataka, India
commented
StatusFileSize
new 2559847-48.patch15.42 KB
new reroll_diff_2559847_46-48.txt14.93 KB

Updated the rerolled patch, also fixed the phpcs errors observed during the checks run locally (using sh ./core/scripts/dev/commit-code-check.sh).

Checking core/lib/Drupal/Core/Render/PlaceholderingRenderCache.php


FILE: core/lib/Drupal/Core/Render/PlaceholderingRenderCache.php
------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------
 81 | ERROR | [x] Expected "null|int" but found "NULL|int" for @var tag in member variable comment
------------------------------------------------------------------------------------------------------------------


Checking core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php


FILE: core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php
-------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
-------------------------------------------------------------------------------------------------------------------------
 796 | ERROR | [x] There should be no white space before a closing ")"

Thanks!

Status: Needs review » Needs work

The last submitted patch, 48: 2559847-48.patch, failed testing. View results

ayalon’s picture

ayalon commented
StatusFileSize
new 2559847-49.patch15.41 KB

I forgot to rename a function call that leads to fatal. Fixed now.

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

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

sinn’s picture

sinn commented

Not sure it is relevant anymore. Based on my tests blocks with max-age = 0 are placeholdered in Drupal 9.5 and can use Dynamic page cache. Or we need to add test to show the use case of the issue.

rosk0’s picture

rosk0
he/him
Primary language Russian
Location Wellington
commented
Issue tags: +Needs tests

NT based on #54.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.