Move attachment processing to services and per-type response subclasses

The parent issue landed, we can work on this now.

My original thinking, was that rendering html.html.twig is a special case; it's not the regular theme system/render API that is being used here. Well, it is, but special rules apply, since this is the final level: there is no level above this to bubble things to. All of the "global" things are happening for html.html.twig: assets are bubbled up to the response-level "thing", which is html.html.twig. Which is why it's currently template_preprocess_html()'s responsibility to take that response-level metadata and turn it into a string of HTML.

But if there were a HtmlResponse class, then we could bubble the assets to that, which both makes semantical sense and is analogous to AjaxResponse.

This rough idea of a HtmlResponse dates back as far as #2352155: Remove HtmlFragment/HtmlPage.

But… today, when I tried to make it work that way, I got stuck: turns out that html.html.twig is using certain things that the theme system provides (like $variables['logged_in']). It only became more clear 3 days though, with the commit of #2329753: Move html classes from preprocess to templates.
I think it's still conceptually a good idea to have a HtmlResponse class to encapsulate a bunch of the "HTML response" things that have to happen (like transform the attached assets into <script> and <link rel="stylesheets"> tags). But since such a HtmlResponse class would also have to contain hacks — just like template_preprocess_html() feels hacky today, I'm not super convinced anymore it's worth it.

Plus, I also have no idea what dawehner means with #2 (could you provide a tiny prototype patch?), and AFAICT that indicates further that HtmlResponse is not a good idea.

But then what is left to be done here? (That prototype patch would be helpful!)

Looks great overall!

1. +++ b/core/includes/errors.inc
   @@ -241,12 +241,18 @@ function _drupal_log_error($error, $fatal = FALSE) {
   +        exit;
   

   I think this is a debugging leftover? :) Because install_display_output() already calls exit.

2. +++ b/core/includes/install.core.inc
   @@ -992,7 +994,9 @@ function install_display_output($output, $install_state) {
   +  $bare_html_page_renderer->filterResponse($response);
   

   This could also use the "Filter the response as it does not go via the usual chain […]" comment that you have elsewhere.

3. +++ b/core/includes/theme.inc
   @@ -1311,41 +1311,11 @@ function template_preprocess_html(&$variables) {
   +  $variables['styles']['#markup'] = '<!-- X-DRUPAL-STYLES-PLACEHOLDER -->';
   +  $variables['scripts']['#markup'] = '<!-- X-DRUPAL-SCRIPTS-PLACEHOLDER -->';
   +  $variables['scripts_bottom']['#markup'] = '<!-- X-DRUPAL-SCRIPTS-BOTTOM-PLACEHOLDER -->';
   +  $variables['head']['#markup'] = '<!-- X-DRUPAL-HEAD-PLACEHOLDER -->';
   

   Too easy to create collisions, needs something like Renderer::generateCachePlaceholder().

4. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
   @@ -158,6 +155,54 @@ public function onRespond(FilterResponseEvent $event) {
   +    if ($response instanceof AttachmentsResponseInterface && !($response instanceof AjaxResponse)) {
   

   Once this is in a service, we could move the burden to CacheableHtmlResponse to call this.

   And we could also move this chunk of logic into a separate event subscriber if we'd like.

   But most of this is actually specific to HTML responses, just like we have kinda-similar-but-really-quite-different-stuff for AJAX responses. Because HTML and AJAX responses handle/load attachments (out-of-band data) very differently.

   So, IMO ideally this could live on CacheableHtmlResponse.

   Except it then would not work for non-cacheable HTML responses, i.e. generic Response objects. But… if you're sending those kinds of responses, you don't want our treatment anyway, because if you construct a Response, you just want that to be sent.

   It's only when you're building a response using a render array, that you want this logic to run. But we can guarantee that that is always a CacheableHtmlResponse. So, AFAICT we should be able to move this into CacheableHtmlResponse, much like AjaxResponse has some attachment-handling logic?

   And that reminds me: why CacheableHtmlResponse and not just HtmlResponse?

5. +++ b/core/lib/Drupal/Core/Render/AttachmentsResponseInterface.php
   @@ -0,0 +1,39 @@
   +  public function getAttachments();
   +
   +
   +}
   

   Should be one newline, not two.

6. +++ b/core/lib/Drupal/Core/Render/BareHtmlPageRenderer.php
   @@ -55,16 +65,32 @@ public function renderBarePage(array $content, $title, $page_theme_property, arr
   +  public function filterResponse(AttachmentsResponseInterface $response) {
   ...
   +  public function filterRenderArray(array $build) {
   

   These are kind of ugly. Wouldn't it be better to manually trigger the KernelEvents::RESPONSE event in those places where we are currently manually calling these functions?

7. +++ b/core/lib/Drupal/Core/Render/BareHtmlPageRenderer.php
   @@ -55,16 +65,32 @@ public function renderBarePage(array $content, $title, $page_theme_property, arr
   +  }
   +
   +
    }
   

   2 -> 1 \n.

8. +++ b/core/lib/Drupal/Core/Render/CacheableHtmlResponse.php
   @@ -0,0 +1,47 @@
   + * Supports Drupal's idea of #attached metadata: libraries, settings, http_header and html_head.
   

   80 cols.

9. +++ b/core/lib/Drupal/Core/Render/CacheableHtmlResponse.php
   @@ -0,0 +1,47 @@
   +class CacheableHtmlResponse extends Response implements CacheableResponseInterface, AttachmentsResponseInterface {
   +
   +  use CacheableResponseTrait;
   +  use AttachmentsResponseTrait;
   

   Looks great!

10. +++ b/core/lib/Drupal/Core/Render/CacheableHtmlResponse.php
    @@ -0,0 +1,47 @@
    +  }
    +}
    

    Needs newline in between.

11. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -119,45 +120,22 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
    -    // The three parts of rendered markup in html.html.twig (page_top, page and
    -    // page_bottom) must be rendered with drupal_render_root(), so that their
    -    // #post_render_cache callbacks are executed (which may attach additional
    -    // assets).
    -    // html.html.twig must be able to render the final list of attached assets,
    -    // and hence may not execute any #post_render_cache_callbacks (because they
    -    // might add yet more assets to be attached), and therefore it must be
    -    // rendered with drupal_render(), not drupal_render_root().
    -    $this->renderer->render($html['page'], TRUE);
    -    if (isset($html['page_top'])) {
    -      $this->renderer->render($html['page_top'], TRUE);
    -    }
    -    if (isset($html['page_bottom'])) {
    -      $this->renderer->render($html['page_bottom'], TRUE);
    -    }
    -    $content = $this->renderer->render($html);
    ...
    +    $this->renderer->renderRoot($html);
    ...
    -    // Bubble the cacheability metadata associated with the rendered render
    -    // arrays to the response.
    -    foreach (['page_top', 'page', 'page_bottom'] as $region) {
    -      if (isset($html[$region])) {
    -        $response->addCacheableDependency(CacheableMetadata::createFromRenderArray($html[$region]));
    -      }
    -    }
    

    So this is the key change. And I want to make sure I understand it.

    AFAICT the reason this is okay, is that assets are now no longer rendered by Renderer/drupal_render(), but by FinishResponseSubscriber.

    Correct?

    Wonderful simplification.

12. +++ b/core/themes/engines/twig/twig.engine
    @@ -62,6 +62,17 @@ function twig_render_template($template_file, array $variables) {
    +  catch (\Twig_Error_Runtime $e) {
    +    // In case there is a previous exception, we just display the message and
    +    // show the original Exception so that the original function that fails is
    +    // shown.
    +    $previous_exception = $e->getPrevious();
    +    if ($previous_exception) {
    +      drupal_set_message($e->getMessage(), 'error');
    +      throw $previous_exception;
    +    }
    +    throw $e;
    +  }
    

    Why is this change necessary now?

I don't fully grok point 12, but that's fine for now. I'll step through it with a debugger in a later review. Looking forward to the reroll :)

It will trap you and ensnare you and riddle your code with hard-to-fix design flaws. :-)

I misread the "It" as "I", which makes it a whole lot more funny :D

#24:

1. But it is relevant only to HTML responses. So that's why Fabianx and I concluded in IRC that we need a HtmlResponseSubscriber, that does all of the HTML response-specific stuff. We should then also make AjaxResponse have a corresponding AjaxResponseSubscriber.
   All of this basically boils down to: Don't ever use Response::prepare(), the Symfony folks were drunk when they added that..
2. Yeah, indeed, that's a tricky one. The ship has kinda sailed on that one, I fear. Better to be consistent about it at this point, IMO. It's unfortunate, but at least it is now isolated awfulness. (When possible, isolate crap, so that the rest of your code doesn't need to smell too!)
3. See previous point.
4. Hah!
5. Yeah, +1, I already commented to the same effect earlier. Fabian is going to clean it up :)
6. :)

Someone was listening to my DrupalCon San Francisco presentation on OOP... :-)

Nope, the eyjafjallajökull eruption prevented me from doing so :(

Looking great!

1. +++ b/core/includes/batch.inc
   @@ -135,9 +136,21 @@ function _batch_progress_page() {
   +    // Prepare attachments, because this does not go the usual way
   
   +++ b/core/includes/errors.inc
   @@ -241,12 +241,17 @@ function _drupal_log_error($error, $fatal = FALSE) {
   +      // Prepare attachments, because this does not go the usual way
   
   +++ b/core/includes/install.core.inc
   @@ -992,7 +994,12 @@ function install_display_output($output, $install_state) {
   +  // Prepare attachments, because this does not go the usual way
   

   80 cols.

2. +++ b/core/includes/batch.inc
   @@ -135,9 +136,21 @@ function _batch_progress_page() {
   +    // via the Symfony chain, but is send directly.
   
   +++ b/core/includes/errors.inc
   @@ -241,12 +241,17 @@ function _drupal_log_error($error, $fatal = FALSE) {
   +      // via the Symfony chain, but is send directly.
   
   +++ b/core/includes/install.core.inc
   @@ -992,7 +994,12 @@ function install_display_output($output, $install_state) {
   +  // via the Symfony chain, but is send directly.
   

   s/send/sent/

3. +++ b/core/includes/theme.inc
   @@ -1311,41 +1312,20 @@ function template_preprocess_html(&$variables) {
   +    $placeholder = '<drupal-html-response-placeholder key="' . $key . '" token="' . $token . '"></drupal-html-response-placeholder>';
   

   I think e.g. <drupal-html-response-attachment-placeholder type="$type" token="$token"> would make for a slightly better name.

4. +++ b/core/lib/Drupal/Core/EventSubscriber/HtmlResponseSubscriber.php
   @@ -0,0 +1,169 @@
   + * Response subscriber to handle html responses.
   

   s/html/HTML/

5. +++ b/core/lib/Drupal/Core/EventSubscriber/HtmlResponseSubscriber.php
   @@ -0,0 +1,169 @@
   +  public function prepareAttachments($response) {
   

   Missing docs.

6. +++ b/core/lib/Drupal/Core/EventSubscriber/HtmlResponseSubscriber.php
   @@ -0,0 +1,169 @@
   +    if ($response instanceof AttachmentsResponseInterface && !($response instanceof AjaxResponse)) {
   

   if ($response instanceof HtmlResponse) {
   

   Because this representation of attachments is specific to HTML responses.

7. +++ b/core/lib/Drupal/Core/EventSubscriber/HtmlResponseSubscriber.php
   @@ -0,0 +1,169 @@
   +      // Render the attachments into HTML markup
   

   Missing trailing period.

8. +++ b/core/lib/Drupal/Core/EventSubscriber/HtmlResponseSubscriber.php
   @@ -0,0 +1,169 @@
   +      // Take Ajax page state into account, to allow for something like Turbolinks
   

   80 cols.

9. +++ b/core/lib/Drupal/Core/EventSubscriber/HtmlResponseSubscriber.php
   @@ -0,0 +1,169 @@
   +      // @todo Remove and move into this class.
   

   That's unsure, because attached headers may apply to other response formats as well. Let's leave this @todo out, IMO it adds more confusion than anything.

10. +++ b/core/lib/Drupal/Core/EventSubscriber/HtmlResponseSubscriber.php
    @@ -0,0 +1,169 @@
    +      $headers = drupal_get_http_header();
    +      foreach ($headers as $name => $value) {
    +        // Symfony special-cases the 'Status' header.
    +        if ($name === 'status') {
    +          $response->setStatusCode($value);
    +        }
    +        $response->headers->set($name, $value, FALSE);
    +      }
    

    Let's do this last, since it's the only non-placeholder one.

    In fact, let's move this to a protected helper method. And let's move the placeholder replacement logic also to a protected helper method.

    Then you can see the logical units in the code much better.

11. +++ b/core/lib/Drupal/Core/EventSubscriber/HtmlResponseSubscriber.php
    @@ -0,0 +1,169 @@
    +  }
    +}
    

    Needs newline in between.

12. +++ b/core/lib/Drupal/Core/Render/BareHtmlPageRenderer.php
    @@ -55,16 +73,22 @@ public function renderBarePage(array $content, $title, $page_theme_property, arr
    +    // Never cache exception pages.
    +    $content['#cache']['max-age'] = 0;
    

    Feels out of scope.

13. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -119,45 +120,22 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
    +    // @todo Make renderRoot return a cacheable render array directly.
    

    Could you create an issue to link to?

I think this is almost ready now.

+++ b/core/includes/theme.inc
@@ -1314,17 +1314,20 @@ function template_preprocess_html(&$variables) {
+    $placeholder = SafeMarkup::format('<drupal-html-response-attachment-placeholder type="@type" token="@token"></drupal-html-response-attachment-placeholder>', [
+      '@type' => $type,
+      '@token' => $token,
+    ]);

Clever!

1. +++ b/core/lib/Drupal/Core/EventSubscriber/HtmlResponseSubscriber.php
   @@ -0,0 +1,232 @@
   +   * Sets extra headers on HtmlResponse responses.
   

   Not just extra headers.

2. +++ b/core/lib/Drupal/Core/EventSubscriber/HtmlResponseSubscriber.php
   @@ -0,0 +1,232 @@
   +  public function prepareAttachments(AttachmentsResponseInterface $response) {
   

   It feels like "process" would be more appropriate than "prepare". Or even "apply".

3. +++ b/core/lib/Drupal/Core/EventSubscriber/HtmlResponseSubscriber.php
   @@ -0,0 +1,232 @@
   +    // Handle all non-asset attachments.
   +    $all_attached = ['#attached' => $attached];
   +    drupal_process_attached($all_attached);
   +
   +    // Get HTML head elements - if present.
   +    if (isset($placeholders['head'])) {
   +      $variables['head'] = drupal_get_html_head(FALSE);
   +    }
   

   The first 3 quoted lines can be moved into the quoted if-statement.

4. +++ b/core/lib/Drupal/Core/EventSubscriber/HtmlResponseSubscriber.php
   @@ -0,0 +1,232 @@
   +    // Now replace the placeholders in the response content with the real
   +    // data.
   

   80 cols.

5. +++ b/core/lib/Drupal/Core/EventSubscriber/HtmlResponseSubscriber.php
   @@ -0,0 +1,232 @@
   +    foreach ($placeholders as $key => $placeholder) {
   

   s/$key/$type/

6. +++ b/core/lib/Drupal/Core/EventSubscriber/HtmlResponseSubscriber.php
   @@ -0,0 +1,232 @@
   +      $content = str_replace($placeholder, render($variables[$key]), $content);
   

   render()… that should probably be Renderer::renderPlain().

   Also, what happens if $variables[$key] doesn't exist? e.g. if there are no header scripts.

#42:

+++ b/core/lib/Drupal/Core/EventSubscriber/HtmlResponseSubscriber.php
@@ -0,0 +1,232 @@
+  public function prepareAttachments(AttachmentsResponseInterface $response) {

Isn't this enough?

This feels hardly novice, but the remarks in #43 are doable, so I guess you're referring to that.

A bunch of changes broke this patch, most notably #2381277: Make Views use render caching and remove Views' own "output caching". The changes there are significantly complicating the reroll, because the Views contextual links hacks have been changed completely. Trying to devise a solution…

This should do it. No interdiff, this is purely chasing HEAD.

#48.2: I bet this was just c/p'ed from FinishResponseSubscriber, which does exactly the same thing. But, you're right of course. OTOH, there are at least 25 pre-existing occurrences of this (grep for = $config_factory->get), and I can't think of a cleaner solution… so keeping this for now.

#48.3: yes, that hunk is needed, because of the last bullet in #34: in HEAD, #type => page is rendered first, then #type => html — this patch changes that. I.e. this bit:

+++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
@@ -119,39 +120,21 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
-    $this->renderer->render($html['page'], TRUE);
-    if (isset($html['page_top'])) {
-      $this->renderer->render($html['page_top'], TRUE);
-    }
-    if (isset($html['page_bottom'])) {
-      $this->renderer->render($html['page_bottom'], TRUE);
-    }
-    $content = $this->renderer->render($html);
...
+    $this->renderer->renderRoot($html);

Ugh, #52 was based on the green patch in #49. I didn't realize that was the straight reroll, without the interdiff applied. So, restoring the #49 interdiff…

Going over recent reviews, to get this issue going again.

• #43.1, .3, .4, .5 and .6 were addressed.
• #43.2 was only partially addressed: HtmlResponseSubscriber was updated, but BareHtmlPageRenderer and the various "direct rendering" edge cases were not. This is likely the cause of the failures in #49 (and the same failures should be present in #54). Fixed that.
• #43's question at the end is also not addressed. Now fixed

#45: okay, so #49 shouldn't have done #43.3. Restored previous code, with a clarifying comment.

#47:

1. I already explained in #26.1 why I think it's an okay compromise: a generic service doesn't make a whole lot of sense, since every response format (HTML, AJAX …) handles these attachments in its own way. That'd mean creating a service for every format, even when it is tightly coupled to the
   However… given that we are now calling $bare_html_page_renderer->processAttachments($response); in half a dozen places (which is in fact not on the interface of that service), and that itself also just redirects to \Drupal\Core\EventSubscriber\HtmlResponseSubscriber::processAttachments(), which cannot be an interface method… I've come around :)
   Let's just have an HtmlResponseAttachmentsProcessor service, plus an analogous one for AjaxResponse. These services respectively accept only an HtmlResponse and an AjaxResponse, plus the current Request (to be able to send only the additionally needed assets). I'll do that in the next reroll.
2. Hehe ok :)

#48: addressed by #49 + #53.

Code review:

1. +++ b/core/authorize.php
   @@ -159,9 +160,15 @@ function authorize_access_allowed(Request $request) {
   +  $bare_html_page_renderer->prepareAttachments($response);
   
   +++ b/core/includes/batch.inc
   @@ -137,9 +138,21 @@ function _batch_progress_page() {
   +    $bare_html_page_renderer->prepareAttachments($response);
   

   (As mentioned above: s/prepare/process/, fixed in this reroll.)

   We really need to improve this — I think it's the biggest remaining problem in the patch.

2. +++ b/core/core.services.yml
   @@ -903,7 +903,7 @@ services:
   +    arguments: ['@renderer', '@render_cache', '@html_response_subscriber']
   
   +++ b/core/lib/Drupal/Core/Render/BareHtmlPageRenderer.php
   @@ -20,13 +22,29 @@ class BareHtmlPageRenderer implements BareHtmlPageRendererInterface {
   +    $this->renderCache = $render_cache;
   
   @@ -55,16 +73,20 @@ public function renderBarePage(array $content, $title, $page_theme_property, arr
   +    $content = $this->renderCache->getCacheableRenderArray($html);
   

   We inject the RenderCache service just to be able to do this.

   #2495001: [meta] Add a replacement for renderRoot() that returns a cacheable render array or object instead of a string will fix that.

   But I wonder if it's truly necessary? I guess the answer is "no", but we want to do it anyway because 1) it is harmless, 2) it keeps the door open for a value object rather than an ArrayPI?

3. +++ b/core/lib/Drupal/Core/Render/AttachmentsResponseInterface.php
   @@ -0,0 +1,38 @@
   +interface AttachmentsResponseInterface {
   

   Why not just use AttachedAssetsInterface?

   i.e. instead of:

   class HtmlResponse extends Response implements CacheableResponseInterface, AttachmentsResponseInterface
   

   do:
   class HtmlResponse extends Response implements CacheableResponseInterface, AttachedAssetsInterface

   I suspect that will be significantly cleaner.

   …

   Oh, I realized why: because AttachedAssetsInterface only supports libraries/settings, whereas responses also need to support non-asset attachments, such as headers, and HTML <head> tags. Gaahh!

4. +++ b/core/lib/Drupal/Core/Render/BareHtmlPageRenderer.php
   @@ -20,13 +22,29 @@ class BareHtmlPageRenderer implements BareHtmlPageRendererInterface {
   +  protected $htmlResponseSubscriber;
   ...
   +    $this->htmlResponseSubscriber = $html_response_subscriber;
   
   @@ -55,16 +73,20 @@ public function renderBarePage(array $content, $title, $page_theme_property, arr
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function prepareAttachments(AttachmentsResponseInterface $response) {
   +    $this->htmlResponseSubscriber->prepareAttachments($response);
      }
   

   Yeah this is just awful. I hope to be able to get rid of this :)

5. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
   @@ -119,39 +120,21 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
   +    // @todo https://www.drupal.org/node/2495001 Make renderRoot return a
   +    //       cacheable render array directly.
   

   We should also add this todo to BareHtmlPageRenderer.

Next: implementing the bits I pointed out above that need work.

I forgot to attach the patch that is associated with the above comment. It just addresses the tiny bits that went wrong in previous patches.

(Gah, testbot was terminated… the same 1 failure exists as in #54. Testbot--.)

1. Added \Drupal\Core\Render\AttachmentsResponseProcessorInterface
2. Added \Drupal\Core\Render\HtmlResponseAttachmentsProcessor, which implements the above interface, and is the service that Crell has been asking for :)
3. With a small API change to \Drupal\Core\Render\BareHtmlPageRendererInterface — which I think is completely acceptable since only the most esoteric/highly advanced use cases want to use this — we can get rid of the manual processing of attachments in all of those PHP/INC files, and instead let the BareHtmlPageRenderer do that: the API change makes it that we return a HtmlResponse object rather than a string of HTML, which means we can already do the attachment processing in there.

And now AjaxResponse gets the same treatment! Finally, consistency WRT asset handling for different types of responses.

Note that there is still some level of cruft in \Drupal\Core\Ajax\AjaxResponseAttachmentsProcessor (notably the module handler altering), but that's pre-existing, and is out of scope to fix here. It's also too risky to change here, since there's so little test coverage for it.

Fixed the one failing test.

Self-review, with nitpicks already fixed:

1. +++ b/core/lib/Drupal/Core/Ajax/AjaxResponseAttachmentsProcessor.php
   @@ -0,0 +1,188 @@
   + * Processes attachments of HTML responses.
   ...
   +   * Constructs a HtmlResponseSubscriber object.
   

   Oopsie. Fixed.

2. +++ b/core/lib/Drupal/Core/Ajax/AjaxResponseAttachmentsProcessor.php
   @@ -0,0 +1,188 @@
   +    if ($response->getContent() == '{}') {
   

   This is strange, but it's pre-existing strangeness, so I'm keeping it. Improving this is out of scope.

3. +++ b/core/lib/Drupal/Core/Ajax/AjaxResponseAttachmentsProcessor.php
   @@ -0,0 +1,188 @@
   +      $response->setData($this->buildAttachmentsCommands($response, $request));
   

   If the above weirdness disappears, we could inline this method.

4. +++ b/core/lib/Drupal/Core/EventSubscriber/AjaxSubscriber.php
   @@ -20,6 +21,23 @@
    class AjaxSubscriber implements EventSubscriberInterface {
   

   Nit: this should be renamed to AjaxResponseSubscriber for consistency.

   Thoughts?

5. +++ b/core/lib/Drupal/Core/EventSubscriber/AjaxSubscriber.php
   @@ -38,7 +56,43 @@ public function onRequest(GetResponseEvent $event) {
   +      // IE 9 does not support XHR 2 (http://caniuse.com/#feat=xhr2), so
   

   This stuff was moved in here, because it applies to AJAX responses at a more general level: it is completely unrelated to attachment/asset handling, therefore I kept this bit out of the AJAX response attachment processor.

6. +++ b/core/lib/Drupal/Core/EventSubscriber/HtmlResponseSubscriber.php
   @@ -0,0 +1,67 @@
   + * Contains Drupal\Core\EventSubscriber\HtmlResponseSubscriber.
   

   Missing leading backslash, fixed.

7. +++ b/core/lib/Drupal/Core/EventSubscriber/HtmlResponseSubscriber.php
   @@ -0,0 +1,67 @@
   +    // @todo what about subrequests for 404s?
   +    if (!$event->isMasterRequest()) {
   

   Apparently this is not a problem (at least I can't break it), so removing the @todo. Still not happy with all this master request business though, but it exists elsewhere as well, so… fixing this is out of scope.

8. +++ b/core/lib/Drupal/Core/EventSubscriber/HtmlResponseSubscriber.php
   @@ -0,0 +1,67 @@
   +  }
   +
   +
   +  /**
   

   2 newlines, should be 1, fixed.

9. +++ b/core/lib/Drupal/Core/Render/AttachmentsResponseInterface.php
   @@ -0,0 +1,42 @@
   +   * Those can be processed by e.g. HtmlResponseSubscriber for the HtmlResponse
   +   * class or processed directly by e.g. an AjaxResponse.
   

   This is wrong now, and is also quite useless info. Removed.

10. +++ b/core/lib/Drupal/Core/Render/HtmlResponse.php
    @@ -0,0 +1,47 @@
    +use Drupal\Core\Cache\CacheableResponseTrait;
    +
    +use Symfony\Component\HttpFoundation\Response;
    

    Silly newline, removed.

11. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
    @@ -0,0 +1,217 @@
    +   * Constructs a HtmlResponseSubscriber object.
    

    Wrong, fixed.

12. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -119,39 +127,21 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
    +    // @todo https://www.drupal.org/node/2495001 Make renderRoot return a
    +    //       cacheable render array directly.
    ...
    +    $content = $this->renderCache->getCacheableRenderArray($html);
    

    Is this really necessary? It's going to be stored in a HtmlResponse anyway, which already removes the ability to drill down… so I'm not sure what we're gaining here?

    Thoughts?

13. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -119,39 +127,21 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
    +    // Set the generator in the HTTP header.
    +    list($version) = explode('.', \Drupal::VERSION, 2);
    

    Since #2477461: Move X-Generator header to its own listener, this is dead code, since it is handled elsewhere now. Removed.

14. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -119,39 +127,21 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
    +    $response = new HtmlResponse($content, 200,[
    

    Missing space at the end, fixed.

15. +++ b/core/modules/system/src/Controller/DbUpdateController.php
    @@ -13,6 +13,7 @@
    +use Drupal\Core\Render\HtmlResponse;
    

    Unused, removed.

With that, this is now blocked on review.

Next: IS update.

For clarity: I need feedback on points 4 and 12.

Looks like #61 broke something in AJAX. Looking at that.

Failures due to tests still calling AjaxResponse::prepare(). Fixed tests.

• #63.4: ok, will do.
• #63.12: Don't think we want to store the whole render tree, do we? indeed, we don't want that, but:
  

  +++ b/core/lib/Drupal/Core/Render/HtmlResponse.php
  @@ -0,0 +1,46 @@
  +  public function setContent($content) {
  +    // A render array can automatically be converted to a string and set the
  +    // necessary metadata.
  +    if (is_array($content) && (isset($content['#markup']))) {
  +      $this->addCacheableDependency(CacheableMetadata::createFromRenderArray($content));
  +      $this->setAttachments($content['#attached']);
  +      $content = $content['#markup'];
  +    }
  
  +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
  @@ -119,39 +127,18 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
  +    $response = new HtmlResponse($content, 200, [
  

  This means that even if $content (as passed to the HtmlResponse constructor) is still a non-flattened render array (i.e. if we wouldn't do the ::getCacheableRenderArray() step), that during the construction, we only keep the top-level #markup anyway; the rest is thrown away.

  So… that's why I don't understand what you're getting at.

• RE: \Twig_Error_Runtime: I vote option c, decide it is out of scope here, because I frankly don't understand this 100%, whereas the rest of this issue is very understandable. So if you think it is fine to defer it to a follow-up, I think that's preferable.

#70: Did #63.4.

#72:

Glad you like it too, Crell :)

And thank you for the rename, that is a much, much better title :)

1. I asked the same question in #21.1, Fabianx answered it in #22.1 like this: , this is hardening and making that exit() explicit instead of relying on that code not changing and suddenly ending up with what had before been in an else case. — but I'm also don't think it's truly necessary.
2. Yep, off-topic, and yeah, it saddens me too. Definitely out of scope.
3. Pre-existing (copy/pasted) code. But you're right about the uselessness. However, note that we're using a key inside that value, so I bet that that was the original intent. So changed it to isset($ajax_page_state['libraries']).
4. Pre-existing (copy/pasted) code. I'm afraid you're wrong: the code you quoted collects the settings: they might live either in the header, or in the footer, and this makes sure we get them. They're then only actually rendered at the very end. So the comment is correct. But the code sure is confusing. Cleaning that up is out of scope here.
5. Pre-existing (copy/pasted) code. Fixed.
6. Done.
7. Strongly disagree; I think this removes predictability. The name "set" implies just setting, nothing else. The behaviour you're describing is "adding/appending/merging". Keeping this as-is.
8. Oh, yes, good point! :) Did it for both the HTML and AJAX response attachments processors.
9. Fixed.
10. Yes, and annoying one ;) I use the word "annoying", because just like you, I'd love nothing more than to get rid of these pieces of crap. But… we simply haven't gotten to it yet. There have always been more important things to do. In the end, >95% of Drupal code only uses asset libraries and drupalSettings. Those are properly encapsulated, non-globally-polluting-crap-piles. But they've only been like for a number of months. It took that much effort to clean them out.
    Yes, we can get rid of it now. If only I had 100-hour days. But at least it is now contained crap, that we could perhaps clean up during the 8.x cycle.
    So, I invite you to help get rid of them. They're significantly easier to refactor away than the asset libraries in any case. But they're simply not important enough, considering the other problems we need to fix to ship D8.
11. Pre-existing (copy/pasted) code. But… oh, haha :D Drupal--. I never even realized this, I learned this part of HTTP via Drupal… Fixed :)
12. Sure, but… this is in fact a WebTestBase test. I don't want to import PHPUnit's getMock() into WebTestBase, kittens will be slaughtered en masse, WORSE EVEN: llamas will be too!
    (Now, this should be a unit test, no question. But doing that is out of scope.)
13. Yep! :) But this one is a unit test. Hence the difference.

@Crell @Fabianx: AFAIK that leaves two more things:

1. #63.12 (also see that being discussed in #70 & #71)
2. The whole \Twig_Error_Runtime thing. What are the practical consequences of leaving that hunk out of this patch?

1. putting the argument instead of the result into the response feels more wrong to me ...

   That's an excellent point. Ok.

2. I have no idea, but I'm curious now. Done. Let's see :D

I just realized something: AttachmentsResponseInterface should probably be renamed to something like AttachmentsInterface (i.e. not be Response-specific — though the name I am using for now is also definitely not great), then BubbleableMetadata can implement that interface.

Then we'd have a nice symmetry:

• CacheableDependencyInterface: implemented by CacheableMetadata (and BubbleableMetadata) and HtmlResponse
• AttachmentsInterface: implemented by BubbleableMetadata and HtmlResponse + AjaxResponse

It allows us to deal with all of these objects in consistent ways.

Updating the IS per #79.

#77: RE: #73.7: Yes, you could get that, but the code example you give is exactly how I'd expect it to work too :) If you're setting ['headers' => []], then I'd also expect to only get back that just headers, regardless of what the prior value was. That's what "to set" means.

The behavior you want, would have to live in a addAttachments() method. And guess what, that's exactly what BubbleableMetadata does today :)

Having written that, I do see what you mean now though. But it'd be inconsistent with how we've done things in BubbleableMetadata and elsewhere. I think it's better to be consistent for now, and use the same behavior as elsewhere in core; we can choose to change that everywhere (and thus remain consistent across core) in a follow-up.

… and after I already wrote this, I see @Fabianx's #78, which is exactly what I am suggesting above as well :)

So, given that I've got two +1s for #76, plus this discussion with Crell and Fabian's + my stance, which Crell will hopefully agree with, this patch does:

• rename AttachmentsResponseInterface to AttachmentsInterface
• add AttachmentsInterface::addAttachments()
• changed the order of the methods in AttachmentsInterface to match the order in CacheableMetadata and BubbleableMetadata (get, add, set)
• update BubbleableMetadata to implement AttachmentsInterface
• update AttachmentsResponseTrait to implement ::addAttachments()

I forgot to update core.services.yml correctly in #82. It should fail.

#87: Hah :) I also considered this, but decided against it because we cannot do it elegantly until #2505171: Follow-up for #2483433 lands, which cleans up BubbleableMetadata::addAttachments(). I should've known that you'd also think of this :D
Looking again at that issue, I can actually make it work already, no need to block this on that issue. There will be some level of conflict, but it'll be very manageable. Let's do it :)

#93.A: true, but it's debatable whether it's actually wrong :) In any case, it's a detail, and in that case, consistency matters most. So yes, filed a follow-up to address that:

#93.B: :D

So that one test failure is because I removed that pesky Twig hunk (the one that neither Crell nor I understand) from the patch in #75.

Working on it.

So, HEAD does this:

+++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
@@ -119,39 +127,18 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
-    // The three parts of rendered markup in html.html.twig (page_top, page and
-    // page_bottom) must be rendered with drupal_render_root(), so that their
-    // placeholders are replaced (which may attach additional assets).
-    // html.html.twig must be able to render the final list of attached assets,
-    // and hence may not replace any placeholders (because they might add yet
-    // more assets to be attached), and therefore it must be rendered with
-    // drupal_render(), not drupal_render_root().
-    $this->renderer->render($html['page'], TRUE);
-    if (isset($html['page_top'])) {
-      $this->renderer->render($html['page_top'], TRUE);
-    }
-    if (isset($html['page_bottom'])) {
-      $this->renderer->render($html['page_bottom'], TRUE);
-    }
-    $content = $this->renderer->render($html);

(The reason is explained in the quoted comment, but basically: "because asset rendering".)

But this patch changes it to be simply:

+++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
@@ -119,39 +127,18 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
+    $this->renderer->renderRoot($html);

This is part of the clean-up that is possible thanks to encapsulating HTML response logic properly.

However, as a side-effect of this clean-up, the page template (#type => page) is now no longer rendered by the Renderer directly, it is rendered by Twig, which then calls Renderer. (Because we invoke the Render on #type => html, which contains #type => page, rather than calling it on #type => page directly, like HEAD does.)

So, rather than seeing the Renderer's exception, we now see Twig's exception, which is why the test fails.

Fabian outlines 3 possible solutions at the bottom of #70. His problem with his patch was: We surely don't want to display messages for that.. Which is totally true.
But I think there's a much simpler solution: just don't set that message :) Just re-throw the previous exception, and everything continues to work as it does today. (And if desired, we can still choose to improve upon that in a follow-up.)

Thoughts?

Sounds great, lets get a follow-up however to ensure that we don't regress for twig.

Done: #2508309: Improve Twig's handling of exceptions.

We can now remove all get/set except addAttachments, which still uses the renderer, but could be follow-up or fixed on commit.

I truly have no idea what this means.

Fabianx clarified what he meant. I simply forgot to remove BubbleableMetadata's ::addAttachments() implementation, which is now provided by the trait. Oops :)

Worse, I actually forgot to remove BubbleableMetadata's addAttachments() and ::getAttachments() too — i.e. I simply forgot to remove the code in BubbleableMetadata that now lives in the trait that BubbleableMetadata uses. Oops.

#107: Oopsie, done: #2508543: [PP-1] Require that AttachmentsInterface implementations have all attachment types present by default.

Glad you +1'd the RTBC! :) Another step closer to BigPipe!

Thanks, Joël!

• #109.2: beta eval added.
• #109.6: good point, that's unnecessary.
• #109.7: yes, see @Fabianx in #40: This is future-proofing the code for when #markup is XSS filtered to ensure this patch does not conflict with that one.
• #109.9: Fixed.
• #109.10: well, it's not really fresh code, it's just moving existing code around (see the last deleted line in theme.inc)
• #109.11: same as 10.
• #109.13: sure, but that won't help us, as Symfony does not have a ResponseInterface, only the default implementation… :( Next best thing: create an AttachmentsResponseBase class… except that won't help either, since AjaxResponse extends JsonResponse. So: we're doing the best we can, consider the circumstances.

Back to RTBC for committer feedback, consider this was all small stuff.

Oops, forgot the beta evaluation.

#2505171: Follow-up for #2483433 broke this, rerolled.

Note that that patch implemented the order of merging in BubbleableMetadata::addAttachments() inversely, which actually makes less sense. So, tiny change to test coverage too.

Now #1305882: drupal_html_id() considered harmful; remove ajax_html_ids to use GET (not POST) AJAX requests broke a few more things, because a few tests started referring to a constant in AjaxSubscriber, and AjaxSubscriber is renamed in this patch. Hopefully this is the last one :)

Created a follow-up: #2512382: Follow-up for #2407195: #attached['http_header'] being added to Response in two places.

This also allowed me to close #2407201: Consider introducing AjaxResponse::addAttachments().