Auto-placeholdering for #lazy_builder without bubbling

Overall looks really truly fantastic, I agree to include one conversion here.

Just had one quick remark:

+++ b/sites/default/default.services.yml
@@ -84,6 +84,14 @@ parameters:
+    # @default ['session', 'user']
+    high_cardinality_cache_contexts: ['session', 'user']

Either this default needs to live in core.services.yml as well OR

in the __construct() we do something like:

$this->config = $config + [
'required_cache_contexts' => [ '...' ],
'high_card_...' => ['...'],
]

Patch looks great. Just some minor nits and questions:

1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -636,6 +645,29 @@ protected function replacePlaceholders(array &$elements) {
   +   * @see sites/default/default.services.yml
   

   I don't think we need that. We don't do it for other container parameters.

2. +++ b/core/modules/comment/src/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
   @@ -184,30 +184,23 @@ public function viewElements(FieldItemListInterface $items) {
   +          // All anonymous users get the same form, but authenticated users get
   +          // personalized forms.
   

   Why? I realize this is already the case in HEAD, but I think a comment explaining why comment forms are personalized would be helpful. Is it just the CSRF token or something else? If it's the CSRF token, why doesn't FAPI set the 'user' cache context on that token element? No need to refactor that here, but a comment, and if applicable, a @todo linking to an issue, would help.

3. +++ b/core/modules/comment/src/Tests/CommentTranslationUITest.php
   @@ -38,10 +38,9 @@ class CommentTranslationUITest extends ContentTranslationUITestBase {
   -    'user.permissions',
   -    'user.roles'
   +    'user'
   +++ b/core/modules/node/src/Tests/NodeTranslationUITest.php
   @@ -27,13 +27,12 @@ class NodeTranslationUITest extends ContentTranslationUITestBase {
   -    'user.permissions',
   -    'user.roles'
   +    'user'
   

   Is this because of the CommentDefaultFormatter change or due to something else? If due to the CommentDefaultFormatter change, then a) why does that affect the translation UI, and b) how was HEAD working prior to this patch, given that the comment form was personalized there as well?

1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -337,6 +340,12 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   +    // Determine whether to do auto-placeholdering.
   +    if (isset($elements['#lazy_builder']) && (!isset($elements['#create_placeholder']) || $elements['#create_placeholder'] !== FALSE)) {
   +      if ($this->hasPoorCacheability($elements)) {
   +        $elements['#create_placeholder'] = TRUE;
   +      }
   +    }
   

   Why is this 2 separate conditionals? Seems like it's just one long set of them...

2. +++ b/core/modules/comment/src/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
   @@ -184,30 +184,23 @@ public function viewElements(FieldItemListInterface $items) {
   +            '#lazy_builder' => ['comment.lazy_builders:renderForm', [
   

   Since I can't keep up with all the moving parts of render API, am I reading this correctly that the lazy builder can/should be a service name and method? (The correct answer is yes.)

3. +++ b/core/modules/comment/src/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
   @@ -184,30 +184,23 @@ public function viewElements(FieldItemListInterface $items) {
   +          // All anonymous users get the same form, but authenticated users get
   +          // personalized forms.
   +          if ($this->currentUser->isAuthenticated()) {
   +            $output['comment_form']['#cache']['contexts'][] = 'user';
              }
   

   I'm confused. Why do we need to add the user context only if the user is authenticated? All anonymous users should get the same form, presumably, and all have the same user ID, so why would adding the user context in all cases be a problem?

4. +++ b/sites/default/default.services.yml
   @@ -84,6 +84,14 @@ parameters:
   +    # The Renderer will automatically placeholder cacheable render arrays that
   +    # have a #lazy_builder callback, to ensure fragments that vary by these
   +    # cache contexts do not affect the containing fragments.
   +    #
   +    # @default ['session', 'user']
   +    high_cardinality_cache_contexts: ['session', 'user']
   

   This is a file that people will be customizing early in their Drupal experience. To someone who hasn't already written large portions of the render API, this paragraph is completely useless if not counter-productive with how much it assumes. Even I only barely follow it, and I am reading this patch. :-)

   Alternative text:

   Drupal allows portions of the page to be automatically deferred when rendering to improve cache performance. That is especially helpful for cache contexts that vary widely, such as the active user. On some sites those may be different, however, such as sites with only a handful of users. If you know what the high-cardinality cache contexts are for your site, specify those here. If you're not sure, the defaults are fairly safe in general.

   And then maybe a @see to something that explains about #lazy_builder (which I've not actually heard about until just now, to show how obscure that reference is).

Re #11.3: On the list of things I have a strong feeling about it's fairly low on the list :-), but it does strike me as something that is going to confuse a lot of people who stare at it and go "what's the subtle reason we have to only add it conditionally, what am I missing?" which is exactly what I did. If the answer is "meh, no particular reason", then let's skip the confusing part and just always add it.

If there is some subtle reason for it, the that needs to be better documented.

But I think this is a bit conceptually clearer, and is more in line with the original logic that this patch now simplifies.

I share the same confusion as Crell. In HEAD, there's an if/else for anonymous/authenticated, because one uses #lazy_builder and the other doesn't. But the patch unifies to always use #lazy_builder. So, given that, I'm confused how:

+++ b/core/modules/comment/src/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
@@ -184,30 +184,25 @@ public function viewElements(FieldItemListInterface $items) {
+            '#cache' => [
+              'contexts' => [
+                'user.roles:authenticated',
+              ],
+            ],
+          ];
+          if ($this->currentUser->isAuthenticated()) {
+            $output['comment_form']['#cache']['contexts'][] = 'user';
           }

has any functional difference to:

+            $output['comment_form']['#cache']['contexts'][] = 'user';

So all I see is 8 lines of code performing what could be done with 1.

Well, we only conditionally vary per user (i.e. if the user is authenticated).

Huh? All anonymous users have uid=0, so what does it mean to "only conditionally vary per user if the user is authenticated"?

On the other hand:

+++ b/core/modules/comment/src/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
@@ -184,30 +184,25 @@ public function viewElements(FieldItemListInterface $items) {
+          // All anonymous users get the same form, but authenticated users get
+          // personalized forms (instead of showing a "Your name" field where
+          // the user can enter their name, the field displays the name of the
+          // current user instead, which is of course different per user.)
+          if ($this->currentUser->isAuthenticated()) {
+            $output['comment_form']['#cache']['contexts'][] = 'user';
           }

Does it make sense to disconnect this context assignment from the "Your name" field? Shouldn't we do this within CommentForm::form() instead, at the point that we actually perform the personalization? Is doing it here just a temporary artifact of this issue's title ending with "without bubbling"? If so, then maybe leaving it as a separate conditional makes sense, if we add a @todo to the "with bubbling" issue?

Re #16, how about this?

+++ b/core/modules/comment/src/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
@@ -184,30 +184,25 @@ public function viewElements(FieldItemListInterface $items) {
+          // All anonymous users get the same form, but authenticated users get
+          // personalized forms (instead of showing a "Your name" field where
+          // the user can enter their name, the field displays the name of the
+          // current user instead, which is of course different per user.)

I'm curious, could this username display be a placeholder for itself so most of the form could be cached? Is there an issue for that?

1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -170,6 +170,9 @@ protected function renderPlaceholder($placeholder, array $elements) {
   +    // Prevent it from being auto-placeholdered again.
   

   Prevent the renderable array? Or the placeholder?

2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -636,6 +643,27 @@ protected function replacePlaceholders(array &$elements) {
   +    return
   +      // Auto-placeholder if max-age === 0.
   +      (isset($element['#cache']['max-age']) && $element['#cache']['max-age'] === 0)
   +      ||
   +      // Auto-placeholder if a high-cardinality cache context is set.
   +      (isset($element['#cache']['contexts']) && array_intersect($element['#cache']['contexts'], $this->rendererConfig['high_cardinality_cache_contexts']));
   

   I think the readability would be better if you split this if up into 2 different if statements.

   if (...) { return true; }
   if (...) { return true; }
   return false;
   

Responded as a new issue #2544560: Try to make the comment form cacheable. I think its solveable.

I'm comfortable with where this is ending up. Nice work, all!

+++ b/sites/default/default.services.yml
@@ -96,9 +96,19 @@ parameters:
+      # Max-age at or below which caching is not considered worthwhile.
+      #
+      # @default 0

Should add:

Disable by setting to max-age: -1

and for the other two:

Disable by specifying [].

Overall looks great! :)

Back to RTBC. Patch looks great!

Still +1 to RTBC. Nice tweakage.

Added a proposed commit message, patch indeed is still RTBC.

Proposed commit message:

Issue #2543332 by Wim Leers, Fabianx, effulgentsia, Crell, dawehner, borisson_: Auto-placeholdering for #lazy_builder without bubbling

Committed/pushed to 8.0.x, thanks!

This appears to have caused some weirdness with messages. See #2547127: [regression] Empty messages container appearing when a Notice occurs

I tracked it to this commit.

After performing drush cr with the current HEAD i get an Undefined index once.

Notice: Undefined index: auto_placeholder_conditions in Drupal\Core\Render\Renderer->shouldAutomaticallyPlaceholder() (line 658 of core/lib/Drupal/Core/Render/Renderer.php).

I didn't saw any side-effects along with this notice.