Move replacing of placeholders from HtmlRenderer to HtmlResponseAttachmentsProcessor

1. +++ b/core/lib/Drupal/Core/Render/HtmlResponse.php
   @@ -36,12 +36,13 @@ public function setContent($content) {
   -      $content += ['#attached' => ['html_response_placeholders' => []]];
   +      $content += ['#attached' => ['html_response_attachment_placeholders' => [], 'placeholders' => []]];
   

   Nit: This would be easier to read if split to multiple lines.

2. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
   @@ -128,6 +134,35 @@ public function processAttachments(AttachmentsInterface $response) {
      /**
   +   * Renders placeholders (#attached['placeholders']).
   +   *
   +   * @param \Drupal\Core\Render\HtmlResponse $response
   +   *   The HTML response whose placeholders are being replaced.
   +   *
   +   * @see \Drupal\Core\Render\Renderer::replacePlaceholders()
   +   * @see \Drupal\Core\Render\Renderer::renderPlaceholder()
   +   */
   

   Let me make sure I am following the logic of this method, as it's rather inside out...

   Given an HtmlResponse object, whose body contains placeholders and has placeholder replacement attachments...

   Convert the HtmlResponse to an equivalent render array.

   renderRoot() the render array, which will swap in the placeholders.

   Reset the body of the rendered array back on the Response, which MAY include new dependencies and other attachments based on the now-executed placeholders.

   Right? If so, let's add some version of the above to the docblock because it took me 10 minutes to write this summary. :-)

   Also, this method should return the modified $response rather than just letting it be an input/output parameter. Both work, but explicit returns make the code easier to follow.

3. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
   @@ -128,6 +134,35 @@ public function processAttachments(AttachmentsInterface $response) {
   +  protected function renderPlaceholders(HtmlResponse $response) {
   

   Shouldn't this be checking against an interface, not a class?

4. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
   @@ -128,6 +134,35 @@ public function processAttachments(AttachmentsInterface $response) {
   +    // RendererInterface::renderRoot() renders the $build render array, it
   +    // updates $build by reference. We don't care about the return value (which
   +    // is just $build['#markup']), but about the resulting render array.
   +    // @todo Simplify this when https://www.drupal.org/node/2495001 lands.
   +    $this->renderer->renderRoot($build);
   

   First sentence is a runon. I think you mean "renderRoot() renders the $build render array and updates it in place."

5. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
   @@ -125,11 +139,28 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
   +      // RendererInterface::render() renders the $html render array, it updates
   +      // $html by reference. We don't care about the return value (which is just
   +      // $html['#markup']), but about the resulting render array.
   +      // @todo Simplify this when https://www.drupal.org/node/2495001 lands.
   

   Same language correction as above.

6. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
   @@ -125,11 +139,28 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
   +    // Also associate the required cache contexts.
   +    // (Because we use ::render() above and not ::renderRoot(), we manually must
   +    // ensure the HTML response varies by the required cache contexts.)
   

   So why don't we use renderRoot()?

+++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
@@ -128,6 +134,55 @@ public function processAttachments(AttachmentsInterface $response) {
+    // updates it in place.. We don't care about the return value (which is just

Extra period. Can fix on commit.

I think that's well-documented enough now. :-) Thanks, Wim.

I've spent some time on IRC discussing this change on IRC with @Wim Leers I'm that happy that we're injecting and using renderer.config into the HtmlRenderer and using it. We discussed several work around including:

• Relying on HtmlResponseAttachmentsProcessor to add them, This does not work because HtmlRenderer needs to return a properly cacheable response with all cache contexts
• Adding a new method on the Renderer that does renderRoot without the placeholdering. This seems suboptimal ... it is alreadt hard to work out when to use renderRoot(), renderPlain() and just plain ol' render().
• Maybe we should rename renderer.config to something that does not seem so specific.

I think only the last one is actually worth pursuing and we can do that as a followup. Fixed the double fullstop on commit. Committed 0549d59 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.