Support placeholder render strategies so contrib can support BigPipe, ESI…

AFAICT this is wholly a duplicate of #2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts by now.

Oh, wait, is this supposed to be the issue representing step 3 of #2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts, i.e. render strategies? If so, let's make it a child of that one, and link to this from that issue's IS.

Clarifying title. And adding to the IS of [#2469431.

The IS definitely needs an overhaul.

First: glad to hear that :)

Second: wow, you weren't kidding -- this patch is ridiculously simple!

Here's an initial code review.

1. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
   @@ -124,11 +133,13 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
   -    // @todo https://www.drupal.org/node/2495001 Make renderRoot return a
   -    //       cacheable render array directly.
   -    $this->renderer->renderRoot($html);
   +    // This uses render() instead of renderRoot() to ensure that placeholders
   +    // are not replaced.
   +    $this->renderer->render($html);
   

   Once #2450993: Rendered Cache Metadata created during the main controller request gets lost lands, this will then have to be executed in a render context. But that's fine.

2. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -118,10 +118,8 @@ public function renderPlain(&$elements) {
   @@ -211,12 +209,6 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   
   @@ -211,12 +209,6 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
          $cached_element = $this->renderCache->get($elements);
          if ($cached_element !== FALSE) {
            $elements = $cached_element;
   -        // Only when we're in a root (non-recursive) Renderer::render() call,
   -        // placeholders must be processed, to prevent breaking the render cache
   -        // in case of nested elements with #cache set.
   -        if ($is_root_call) {
   -          $this->replacePlaceholders($elements);
   -        }
            // Mark the element markup as safe. If we have cached children, we need
            // to mark them as safe too. The parent markup contains the child
            // markup, so if the parent markup is safe, then the markup of the
   @@ -233,6 +225,14 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   
   @@ -233,6 +225,14 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
            // Render cache hit, so rendering is finished, all necessary info
            // collected!
            $this->bubbleStack();
   +
   +        // Only when we're in a root (non-recursive) Renderer::render() call,
   +        // placeholders must be processed, to prevent breaking the render cache
   +        // in case of nested elements with #cache set.
   +        if ($is_root_call) {
   +          $this->replacePlaceholders($elements);
   +        }
   +
            return $elements['#markup'];
          }
        }
   

   Can you explain why this needed to be moved?

3. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -470,6 +470,15 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   +    if ($is_root_call) {
   +      if (static::$stack->count() !== 1) {
   +        throw new \LogicException('A stray drupal_render() invocation with $is_root_call = TRUE is causing bubbling of attached assets to break.');
   +      }
   +    }
   +
   +    // Rendering is finished, all necessary info collected!
   +    $this->bubbleStack();
   +
        // Only when we're in a root (non-recursive) Renderer::render() call,
        // placeholders must be processed, to prevent breaking the render cache in
        // case of nested elements with #cache set.
   @@ -481,14 +490,8 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   
   @@ -481,14 +490,8 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
        // that is handled earlier in Renderer::render().
        if ($is_root_call) {
          $this->replacePlaceholders($elements);
   -      if (static::$stack->count() !== 1) {
   -        throw new \LogicException('A stray drupal_render() invocation with $is_root_call = TRUE is causing bubbling of attached assets to break.');
   -      }
        }
    
   -    // Rendering is finished, all necessary info collected!
   -    $this->bubbleStack();
   

   This I don't understand at first sight either. Again a bit of the background info/reasoning would be welcome :)

4. +++ b/core/lib/Drupal/Core/Render/Strategy/RenderStrategyInterface.php
   @@ -0,0 +1,27 @@
   +   * Processes placeholders to render them with different strategies.
   
   +++ b/core/lib/Drupal/Core/Render/Strategy/RenderStrategyManager.php
   @@ -0,0 +1,84 @@
   + * Provides a class which allows to render placeholders.
   

   "process" vs. "render"

   Let's be consistent.

5. +++ b/core/lib/Drupal/Core/Render/Strategy/RenderStrategyInterface.php
   @@ -0,0 +1,27 @@
   +   *   The placeholders to process keyed by $placeholder_id with the render
   +   *   cache array as value.
   
   +++ b/core/lib/Drupal/Core/Render/Strategy/RenderStrategyManagerInterface.php
   @@ -0,0 +1,26 @@
   +   *   The render cache array containing #markup and #attached placeholders.
   

   s/render cache array/placeholder render array/

   (Or whatever the exact name is that HEAD uses. In any case *not* "render cache array".)

6. +++ b/core/lib/Drupal/Core/Render/Strategy/RenderStrategyInterface.php
   @@ -0,0 +1,27 @@
   +   *   The new values for the placeholders, keyed by $placeholder_id => $render_array.
   

   80 cols.

7. +++ b/core/lib/Drupal/Core/Render/Strategy/RenderStrategyManager.php
   @@ -0,0 +1,84 @@
   +  /**
   +   * The RenderStrategy services.
   +   *
   +   * @var Drupal\Core\Render\Strategy\RenderStrategyInterface[]
   +   */
   +  protected $renderStrategies;
   ...
   +  public function addRenderStrategy(RenderStrategyInterface $strategy) {
   +    $this->renderStrategies[] = $strategy;
   +  }
   

   I think it's worth calling out that this is a *list*, i.e. an array where order matters, because it determines render strategy execution order.

   This should mention it relies on service priorities etc.

8. +++ b/core/lib/Drupal/Core/Render/Strategy/RenderStrategyManager.php
   @@ -0,0 +1,84 @@
   +   *   The strategy to add to the render strategies.
   

   The render strategy to add.

9. +++ b/core/lib/Drupal/Core/Render/Strategy/RenderStrategyManager.php
   @@ -0,0 +1,84 @@
   +  public function renderPlaceholders(&$elements) {
   

   array

10. +++ b/core/lib/Drupal/Core/Render/Strategy/RenderStrategyManager.php
    @@ -0,0 +1,84 @@
    +    // Now render the new placeholders.
    

    s/new/processed/

11. +++ b/core/lib/Drupal/Core/Render/Strategy/SingleFlushRenderStrategy.php
    @@ -0,0 +1,24 @@
    +  }
    +}
    

    Missing newline in between.

+++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
@@ -124,11 +133,13 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
+    // This uses render() instead of renderRoot() to ensure that placeholders
+    // are not replaced.
+    $this->renderer->render($html);
     $content = $this->renderCache->getCacheableRenderArray($html);
 
+    $this->renderStrategyManager->renderPlaceholders($content);

This won't work anymore now that #2450993: Rendered Cache Metadata created during the main controller request gets lost is in.

You need to wrap the ->render() call in executeInRenderContext().

Then the $this->renderStrategyManager->renderPlaceholders() call can still happen as it is right now.

I thoroughly reviewed this issue/patch. The patch in #11 was green — per #13 it needs to be rerolled, but that doesn't mean the patch isn't already functionally working.

1. The patch makes small internal changes in Renderer, none of which cause API changes.
2. The patch makes a small internal change in HtmlRenderer, none of which cause API changes.
3. Everything else in the patch are pure additions.

Therefore, AFAICT, this patch does not cause any API changes, which means it can definitely remain major.

Patch review:

1. +++ b/core/core.services.yml
   @@ -1479,6 +1479,22 @@ services:
   +    lazy: true
   

   Why lazy? Almost every page contains at least one placeholder: that for the status messages. So we don't actually gain anything AFAICT.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/HtmlRenderStrategySubscriber.php
   @@ -0,0 +1,74 @@
   +class HtmlRenderStrategySubscriber implements EventSubscriberInterface {
   

   Why an event subscriber, and not what I proposed in #2429617-207: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!).3 (i.e. letting HtmlResponseAttachmentsProcessor call the render strategy manager)?

3. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -272,12 +272,6 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   -        // Only when we're in a root (non-recursive) Renderer::render() call,
   -        // placeholders must be processed, to prevent breaking the render cache
   -        // in case of nested elements with #cache set.
   -        if ($is_root_call) {
   -          $this->replacePlaceholders($elements);
   -        }
            // Mark the element markup as safe if is it a string.
            if (is_string($elements['#markup'])) {
              $elements['#markup'] = SafeString::create($elements['#markup']);
   @@ -288,6 +282,14 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   
   @@ -288,6 +282,14 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
            // Render cache hit, so rendering is finished, all necessary info
            // collected!
            $context->bubble();
   +
   +        // Only when we're in a root (non-recursive) Renderer::render() call,
   +        // placeholders must be processed, to prevent breaking the render cache
   +        // in case of nested elements with #cache set.
   +        if ($is_root_call) {
   +          $this->replacePlaceholders($elements);
   +        }
   

   Why does this need to be moved?

4. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -525,6 +527,16 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   +    if ($is_root_call) {
   +      // @todo remove as part of https://www.drupal.org/node/2511330.
   +      if ($context->count() !== 1) {
   +        throw new \LogicException('A stray drupal_render() invocation with $is_root_call = TRUE is causing bubbling of attached assets to break.');
   +      }
   +    }
   
   @@ -536,15 +548,8 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   -      // @todo remove as part of https://www.drupal.org/node/2511330.
   -      if ($context->count() !== 1) {
   -        throw new \LogicException('A stray drupal_render() invocation with $is_root_call = TRUE is causing bubbling of attached assets to break.');
   -      }
   

   And why does this need to be moved?

5. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -525,6 +527,16 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   +    // Rendering is finished, all necessary info collected!
   +    $context->bubble();
   
   @@ -536,15 +548,8 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   -    // Rendering is finished, all necessary info collected!
   -    $context->bubble();
   

   I think you're moving this simply because there's no need for that to happen so late anymore, now that Renderer::render() no longer is replacing placeholders?

6. +++ b/core/lib/Drupal/Core/Render/Strategy/RenderStrategyInterface.php
   @@ -0,0 +1,27 @@
   +interface RenderStrategyInterface {
   ...
   +  public function processPlaceholders(array $placeholders);
   

   Render strategies are specifically about placeholders.

   So, shouldn't we rename them to "placeholder render strategies"?

   Or do you foresee additional things besides placeholder rendering in the future?

7. +++ b/core/lib/Drupal/Core/Render/Strategy/RenderStrategyManager.php
   @@ -0,0 +1,76 @@
   +  public function processPlaceholders(array $placeholders) {
   +    if (empty($placeholders)) {
   +      return [];
   +    }
   +
   +    $new_placeholders = [];
   +
   +    // Give each render strategy a chance to replace all not-yet replaced
   +    // placeholders.
   +    foreach ($this->renderStrategies as $strategy) {
   +      $processed_placeholders = $strategy->processPlaceholders($placeholders);
   +      $placeholders = array_diff_key($placeholders, $processed_placeholders);
   +      $new_placeholders += $processed_placeholders;
   +
   +      if (empty($placeholders)) {
   +        break;
   +      }
   +    }
   +
   +    return $new_placeholders;
   +  }
   

   This needs a unit test.

8. +++ b/core/lib/Drupal/Core/Render/Strategy/RenderStrategyManagerInterface.php
   @@ -0,0 +1,26 @@
   +interface RenderStrategyManagerInterface {
   

   This interface is effectively identical to RenderStrategyInterface. Do we really need it? If we remove it, the pattern here would match that of e.g. OutboundPathProcessorInterface being implemented by PathProcessorManager. In other words: the manager is just a special aggregating strategy, rather than a separate thing.

Interdiff review:

1. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -255,6 +255,8 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   +      // This is inlined for performance reasons. Also see
   +      // applyRequiredCacheContexts() function below.
   

   s/Also see FUNC function below/@see FUNC/

2. +++ b/core/lib/Drupal/Core/Render/RendererInterface.php
   @@ -365,6 +365,21 @@ public function hasRenderContext();
   +   * Applies the required cache contexts to a render array.
   +   *
   +   * Note: This function should only be needed in edge-case circumstances; e.g.
   +   * when interacting directly with the RenderCache class or by rendering
   +   * without replacing placeholders.
   

   This strange documentation is precisely why I'd argue we should have ::getRequiredCacheContexts() instead. This would then be usable by any code, regardless of the RendererInterface implementation.

   (e.g. a module that does some automated cacheability analysis; it'd need to know the required cache context of the renderer, and should not have

2. Ok! (I can't think of anything needed in between, but that sure is a good general best practice.)

3+4. Can we then add test coverage for this?

6. Okay. But then the docs should specifically say "placeholder render strategies".

8. Ok :)

2. Ok :)

Reflecting the current state of this issue.

I moved the parts of the SmartCache patch that blocked this issue (since #15, see that comment) to #2551989: Move replacing of placeholders from HtmlRenderer to HtmlResponseAttachmentsProcessor, which just got committed. So this is now fully unblocked!

3 + 4: thanks, I now see why you did it this way. In fact, your previous reply to my questions was crystal clear. I don't know why I didn't get that before.

1. +++ b/core/core.services.yml
   @@ -1507,6 +1507,20 @@ services:
   +  # Render strategies for rendering placeholders.
   
   +++ b/core/lib/Drupal/Core/EventSubscriber/HtmlRenderStrategySubscriber.php
   @@ -0,0 +1,74 @@
   + * HTML Response subscriber to handle placeholder render strategies.
   

   The first doesn't mention "HTML", the second does.

   Reading this patch again, it still feels like it'd be clearer if this were called "placeholder render strategies" and not "render strategies".

   Because "render strategies" implies a significantly broader level of impact, but we really are just talking about placeholder render strategies.

2. +++ b/core/lib/Drupal/Core/Render/Strategy/RenderStrategyInterface.php
   @@ -0,0 +1,27 @@
   +interface RenderStrategyInterface {
   

   So then this would be called PlaceholderRenderStrategyInterface. Or even just PlaceholderStrategyInterface. If "render strategy" is okay, then surely "placeholder strategy" is too?

3. +++ b/core/lib/Drupal/Core/Render/Strategy/RenderStrategyManager.php
   @@ -0,0 +1,59 @@
   +    // Give each render strategy a chance to replace all not-yet replaced
   +    // placeholders.
   

   This should stress that the different render strategies are called in a specific order. If that order were not strictly defined, this functionality would work in an undeterministic way.

4. +++ b/core/lib/Drupal/Core/Render/Strategy/SingleFlushRenderStrategy.php
   @@ -0,0 +1,24 @@
   + * This is the last strategy that always replaces all remaining placeholders.
   

   Why is it the last? Because its priority is -1000.

   So this comment should rather say something like "this is designed to be the fallback strategy, so should have the lowest priority".

#28: oh, ok!

Behavior if someone removed all RenderStrategies or had only one render strategy that did not process any placeholders is to throw away those placeholder

Having only one placeholder render strategy does not seem to be a problem at all? Especially considering that the SingleFlush strategy is guaranteed to be always available.
So: a) like currently - discard any remaining placeholders (though that should never happen as SingleFlushRenderStrategy comes last) seems to be the answer already.

- Behavior if there is not a single render strategy registered is throwing a notice due to NULL not being an array.

Again, the SingleFlush strategy is guaranteed to be always available. So I don't see how/when/why this can be a real-world problem.

+++ b/core/tests/Drupal/Tests/Core/Render/Placeholder/PlaceholderStrategyManagerTest.php
@@ -43,6 +43,12 @@ public function providerProcessPlaceholders() {
+      'ignore-me' => ['#markup' => 'I-am-a-llama-that-will-be-ignored-by-the-placeholder-strategy-manager.'],

Poor llama :D

(Also: strange llama :P)

Fixed lots of nits:

1. +++ b/core/lib/Drupal/Core/EventSubscriber/HtmlResponsePlaceholderStrategySubscriber.php
   @@ -0,0 +1,74 @@
   +   * Processes placeholders for HtmlResponse responses.
   

   s/HtmlResponse responses/HTML responses/

2. +++ b/core/lib/Drupal/Core/Render/Placeholder/PlaceholderStrategyInterface.php
   @@ -0,0 +1,27 @@
   +  /**
   +   * Processes placeholders to render them with different strategies.
   +   *
   +   * @param array $placeholders
   +   *   The placeholders to process keyed by $placeholder_id with the render
   +   *   cache array as value.
   +   *
   +   * @return array $placeholders
   +   *   The new values for the placeholders, keyed by $placeholder_id => $render_array.
   +   */
   

   This needs quite a bit of clean-up.

3. +++ b/core/lib/Drupal/Core/Render/Placeholder/PlaceholderStrategyManager.php
   @@ -0,0 +1,65 @@
   +use Drupal\Core\Render\RendererInterface;
   

   Unused.

4. +++ b/core/lib/Drupal/Core/Render/Placeholder/PlaceholderStrategyManager.php
   @@ -0,0 +1,65 @@
   +   * @var Drupal\Core\Render\Placeholder\PlaceholderStrategyInterface[]
   

   Missing leading slash.

5. +++ b/core/lib/Drupal/Core/Render/Placeholder/PlaceholderStrategyManager.php
   @@ -0,0 +1,65 @@
   +  protected $placeholderStrategies;
   ...
   +  public function addPlaceholderStrategy(PlaceholderStrategyInterface $strategy) {
   ...
   +    // placeholders. The order of placeholder strategies is well defined
   +    // and this uses a variation of the "chain of responsibility" design pattern.
   

   What's not clear from the member's documentation nor the method's documentation is that it's the service priority that determines the order.

6. +++ b/core/lib/Drupal/Core/Render/Placeholder/PlaceholderStrategyManager.php
   @@ -0,0 +1,65 @@
   +   * @param PlaceholderStrategyInterface $strategy
   

   Missing FQCN.

And these still need to be addressed:

1. +++ b/core/lib/Drupal/Core/Render/Placeholder/PlaceholderStrategyManager.php
   @@ -0,0 +1,65 @@
   +    // In case there are no placeholder strategies, return all placeholders.
   +    if (empty($this->placeholderStrategies)) {
   +      return $placeholders;
   +    }
   
   +++ b/core/lib/Drupal/Core/Render/Placeholder/SingleFlushStrategy.php
   @@ -0,0 +1,25 @@
   +class SingleFlushStrategy implements PlaceholderStrategyInterface {
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function processPlaceholders(array $placeholders) {
   +    // Return all placeholders as is; they should be rendered directly.
   +    return $placeholders;
   +  }
   +}
   

   These are effectively equivalent.

   On the one hand, it is impossible to not have the Single Flush strategy, so the if (empty() always evaluates to false.

   On the other hand, if that evaluated to true (because some module modified the container), then it'd effectively still be the same as the single flush strategy.

   Therefore I think we should have either, but not both. I'll leave it to you on how you want to solve this.

2. +++ b/core/lib/Drupal/Core/Render/Placeholder/PlaceholderStrategyManager.php
   @@ -0,0 +1,65 @@
   +      $processed_placeholders = $strategy->processPlaceholders($placeholders);
   +      $placeholders = array_diff_key($placeholders, $processed_placeholders);
   

   Let's add an assertion here that ensures that array_keys($processed_placeholders) is a subset of array_keys($placeholders). We want to prevent additional placeholders being added, or existing placeholders being changed, because that could break things.

3. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -163,10 +163,8 @@ public function renderPlain(&$elements) {
   -  protected function renderPlaceholder($placeholder, array $elements) {
   +  public function renderPlaceholder($placeholder, array $elements) {
   

   AFAICT nothing in this patch calls this, so there's no need to do this here. Let's do that in a separate issue. Even more so because it needs to update RendererInterface too.

   That keeps the scope of this issue much clearer.

4. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -275,12 +273,6 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   -        if ($is_root_call) {
   -          $this->replacePlaceholders($elements);
   -        }
   
   @@ -291,6 +283,14 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   +        if ($is_root_call) {
   +          $this->replacePlaceholders($elements);
   +        }
   

   Then these changes could also be done in that separate issue.

5. +++ b/core/tests/Drupal/Tests/Core/Render/Placeholder/PlaceholderStrategyManagerTest.php
   @@ -0,0 +1,129 @@
   +    $prophecy = $this->prophesize('\Drupal\Core\Render\Placeholder\PlaceholderStrategyInterface');
   +    $single_flush_strategy = $prophecy->reveal();
   

   Let's assert this is not ever called.

This looks great now, and very simple. The latest patch makes it very clear this is a pure, simple, thin API addition. Let's also update the IS to bring it in sync with the latest patch. Then I think this is ready :)

1. +++ b/core/lib/Drupal/Core/Render/Placeholder/PlaceholderStrategyManager.php
   @@ -39,10 +39,8 @@ public function processPlaceholders(array $placeholders) {
   +    assert('!empty($this->placeholderStrategies)', 'At least one placeholder strategy needs to be present.');
   

   Let's just be more explicit and state that we always expect the \Drupal\Core\Render\Placeholder\SingleFlushStrategy strategy to be present.

   Also: s/need to/must/

2. +++ b/core/lib/Drupal/Core/Render/Placeholder/PlaceholderStrategyManager.php
   @@ -51,6 +49,7 @@ public function processPlaceholders(array $placeholders) {
   +      assert('array_intersect_key($processed_placeholders, $placeholders) === $processed_placeholders', 'Processed placeholders need to be a subset of all placeholders.');
   

   s/need to/must/

3. +++ b/core/lib/Drupal/Core/Render/Renderer.php
   @@ -164,8 +164,10 @@ public function renderPlain(&$elements) {
   +   * @todo Make public as part of https://www.drupal.org/node/2469431
   
   @@ -554,8 +544,15 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
   +      // @todo remove as part of https://www.drupal.org/node/2511330.
   

   Thanks!

Let's make it clear that this is a contributed project blocker: it blocks BigPipe/ESI/… modules in D8 contrib. With this change, they'll be able to work automatically: Drupal 8 core already has the concept of placeholders, they'd just effectively take over placeholder rendering from Drupal core. This patch makes that possible.

It therefore unblocks #2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts — which at this point is unlikely to happen for D8 core, but with this simple, thin patch, it can actually be done in D8 contrib, so that it can mature there first before moving into D8 core at a later time (think 8.1.0, 8.2.0 …).

I think this is RTBC-worthy. I'll ask @moshe to also take a look at this.

Just one more thing:

+++ b/core/lib/Drupal/Core/EventSubscriber/HtmlResponsePlaceholderStrategySubscriber.php
@@ -0,0 +1,74 @@
+/**
+ * HTML response subscriber to allow for different  placeholder strategies.
+ */
+class HtmlResponsePlaceholderStrategySubscriber implements EventSubscriberInterface {

I think this could use a paragraph, perhaps two, explaining what exactly this does. (Part of what #35's first paragraph says, plus some of the technical details.)

Then we have a place where we document *in* code how this relates to Renderer, HtmlResponse, HtmlResponseAttachmentProcessor, etc.

1. +++ b/core/lib/Drupal/Core/EventSubscriber/HtmlResponsePlaceholderStrategySubscriber.php
   @@ -0,0 +1,74 @@
   + * HTML response subscriber to allow for different  placeholder strategies.
   

   Nitpick alarm: two spaces :P

2. +++ b/core/lib/Drupal/Core/Render/Placeholder/PlaceholderStrategyManager.php
   @@ -0,0 +1,64 @@
   +class PlaceholderStrategyManager implements PlaceholderStrategyInterface {
   

   I'm curious whether we can come up with a better name than "Manager". Manager pretty much tells you nothing about it

3. +++ b/core/lib/Drupal/Core/Render/Placeholder/SingleFlushStrategy.php
   @@ -0,0 +1,25 @@
   +/**
   + * Defines the 'single_flush' placeholder strategy.
   + *
   

   It would be nice to define what single flush means

4. +++ b/core/tests/Drupal/Tests/Core/Render/Placeholder/PlaceholderStrategyManagerTest.php
   @@ -0,0 +1,169 @@
   +    $data = [];
   ...
   +    $data[] = [[], [], []];
   ...
   +    $data[] = [[$dev_null_strategy], $placeholders, []];
   ...
   +    $data[] = [[$single_flush_strategy], $placeholders, $placeholders];
   ...
   +    $data[] = [[$esi_strategy], $placeholders, $result];
   ...
   +    $data[] = [[$esi_strategy, $single_flush_strategy], $placeholders, $result];
   ...
   +    $data[] = [[$esi_strategy, $single_flush_strategy], $placeholders, $result];
   

   For future reference, you can name these entries by providing an array key

Patch looks great to me. Just a couple tiny nits:

1. +++ b/core/lib/Drupal/Core/EventSubscriber/HtmlResponsePlaceholderStrategySubscriber.php
   @@ -0,0 +1,80 @@
   +    $event->setResponse($response);
   

   Do we need this? Isn't $response still the same object (just mutated) that's already in the event?

2. +++ b/core/tests/Drupal/Tests/Core/Render/Placeholder/PlaceholderStrategyManagerTest.php
   @@ -0,0 +1,169 @@
   +  public function testProcessPlaceholdersSingleFlush($strategies, $placeholders, $result) {
   

   This test function now does a lot more than just testing single flush, so should be renamed, right?

Besides the patch, I'm curious about this claim in the beta eval:

Major because it blocks BigPipe and other things that can make Drupal 8 way faster.

How does this patch block BigPipe? Can't this entire patch be just as effectively implemented in a contrib module (e.g., placeholder_strategy_manager.module)? Not saying we need to punt this to 8.1, but should we? Would there be benefit to letting this evolve a bit in contrib first, in case that once BigPipe, ESI, etc. modules start using it, things get discovered that would be harder to change in core?

Re: #39.2 > I'm curious whether we can come up with a better name than "Manager". Manager pretty much tells you nothing about it

How about PlaceholderStrategyDelegator?

I'm curious whether we can come up with a better name than "Manager". Manager pretty much tells you nothing about it

Twitter seems to have concluded "chain": https://twitter.com/da_wehner/status/638776059664728064. Chain makes more sense than "delegator" (or "manager"), because it is implemented as a chain.

NW for #41, #42 and this comment.

+++ b/core/lib/Drupal/Core/Render/Placeholder/PlaceholderStrategyManager.php
@@ -0,0 +1,64 @@
+/**
+ * Renders placeholders using different strategies depending on priorities.
+ */
+class PlaceholderStrategyManager implements PlaceholderStrategyInterface {
+

Core is rather inconsistent on the naming here, but if we want to avoid "Manager" (we do), then "ChainedPlaceholderStrategy" is probably the next logical name. Or, really, it should be a verb so ChainedPlaceholderProcessor?

That said, I'm curious of the point of this patch. It appears to allow multiple placeholder replacement systems to run concurrently, so that a given request could have some inline, some bigpipe, and some ESI placeholders all in a single response. I am struggling to see where that is useful.

That is also why I think it would be great to have this in core, so that the
coordinator / chained placeholder processor is something that core and
contrib can rely on.

If so, do we want to make that logic live :
- in "just a regular Strategy that happens to call the other strategies that registered using the right service tag" (i.e ChainedPlaceholderStrategy as @Crell suggest in #44),
- or in something that is not a Strategy itself and lives "above" strategies (the current "Manager" - could be PlaceholderResolver if we don't like Manager ?)
?

This patch now looks beautiful and is wonderfully simple and elegant. Great job :)

1. +++ b/core/core.services.yml
   @@ -1503,6 +1503,20 @@ services:
   +  placeholder_strategy.chained:
   +    class: Drupal\Core\Render\Placeholder\ChainedPlaceholderStrategy
   +    tags:
   +      - { name: service_collector, tag: placeholder_strategy, call: addPlaceholderStrategy }
   +  placeholder_strategy.single_flush:
   +    class: Drupal\Core\Render\Placeholder\SingleFlushStrategy
   +    tags:
   +      - { name: placeholder_strategy, priority: -1000 }
   

   This makes a lot of sense! :) Much better, much more elegant than before :)

2. +++ b/core/lib/Drupal/Core/Render/Placeholder/ChainedPlaceholderStrategy.php
   @@ -0,0 +1,64 @@
   + * Renders placeholders using different strategies depending on priorities.
   

   Nit: Renders placeholders using a chain of placeholder strategies.

1. +++ b/core/core.services.yml
   @@ -1503,6 +1503,20 @@ services:
   +  placeholder_strategy.chained:
   +    class: Drupal\Core\Render\Placeholder\ChainedPlaceholderStrategy
   +    tags:
   +      - { name: service_collector, tag: placeholder_strategy, call: addPlaceholderStrategy }
   

   There's no need to call this .chained. The site's placeholder strategy is what's registered at placeholder_strategy and has the PlaceholderInterface. That it by default happens to be a chained implementation is irrelevant to anyone but that class. That's by design.

   Our router service is router, not router.chained, even though we're using a chained router there, too. (And in core have only a single implementation of it.)

2. +++ b/core/lib/Drupal/Core/Render/Placeholder/ChainedPlaceholderStrategy.php
   @@ -0,0 +1,64 @@
   +class ChainedPlaceholderStrategy implements PlaceholderStrategyInterface {
   

   Yep, that's exactly how it's done elsewhere, as Druplicon intended. :-)

   In some cases we also have an extra interface for the add*() method, but I'm not sure if we're doing that everywhere.

In some cases we also have an extra interface for the add*() method, but I'm not sure if we're doing that everywhere.

Yeah IMHO this is an antipattern to do that.

Not a full review. This looks good to me in general. Only one real question:

1. +++ b/core/lib/Drupal/Core/EventSubscriber/HtmlResponsePlaceholderStrategySubscriber.php
   @@ -0,0 +1,79 @@
   +    if (!$event->isMasterRequest()) {
   

   Does comment permalink rendering still use a subrequest?

2. +++ b/core/tests/Drupal/Tests/Core/Render/Placeholder/ChainedPlaceholderStrategyTest.php
   @@ -0,0 +1,168 @@
   +      'remove-me' => ['#markup' => 'I-am-a-llama-that-will-be-removed-sad-face.'],
   

   :)

Tagging RC target because this is a proper API addition that we'd probably only be able to do in a minor release and at the very latest for 8.0.0 during RC if we decide the disruption is minimal - we need a new tag for this like 'minor release target' or something.

#53.1: Yes, see \Drupal\comment\Controller\CommentController::commentPermalink().

#54 if it does, I'd expect us to still want placeholdering to work there. Not sure what the implications are, possibly not using a subrequest would be best.

#55: No. HtmlResponseSubscriber is what does placeholder replacement in HEAD, and it also doesn't run for subrequests. And everything is working fine. The subrequest is only happening to retrieve the actual response to send, to do in-app redirection (rather than sending a redirect response), but it is then "promoted" to become the master response. Therefore using the same pattern here is totally fine. :)

EDIT: I forgot to mention that I stepped through both HEAD and the patch to make sure that what I wrote above is actually the case.

but it is then "promoted" to become the master response.

OK this is what I couldn't remember properly at all. So isMasterRequest() returns TRUE even though the comment code uses a subrequest. Not confusing at all :P

But not at all the fault of this patch, slightly my fault because the subrequest for comments is ported from one of the very first core patches I worked on ;)

@catch suppose you mean related #2113323: Rename Comment::permalink() to not be ambiguous with ::uri()

Testbotfail:

Drupal\system\Tests\Update\UpdatePathTestBaseFilledTest	
GET http://ec2-52-88-182-40.us-west-2.compute.amazonaws.com/checkout/update.php/run returned 0 (0 bytes).	
…

I read through again and didn't find anything noteworthy.

So.. committed/pushed to 8.0.x, thanks!