Problem/Motivation

Several of the things that template_preprocess_html() does (because it must) should not be done in the theme preprocess layer. They actually belong on a HtmlResponse class; but since we don't have that, because HtmlRenderer::renderResponse() returns a plain Symfony Response, that's not yet the case.
(See #2368797-34: Optimize ajaxPageState to keep Drupal 8 sites fast on high-latency networks, prevent CSS/JS aggregation from taking down sites and use HTTP GET for AJAX requests.3.)

More importantly: HEAD makes implementing render strategies extremely difficult, i.e. it blocks #2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts (per #8 + #79) and allows for significant simplifications in #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!).

So this patch paves the way for one of the most important steps towards BigPipe, and simplifies SmartCache.

Proposed resolution

Introduce a HtmlResponse class.

Remaining tasks

Review & commit.

User interface changes

None.

API changes

None. (Internal API changes only.)

Possibly: template_preprocess_html() is now called before the main 'page', 'page_top' and 'page_bottom' have been rendered. Not sure we need a CR for that as its not TRUE API, but rather a side effect.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because neither bug nor feature, but significantly paves the way for SmartCache/BigPipe.
From the IS:

More importantly: HEAD makes implementing render strategies extremely difficult, i.e. it blocks #2469431: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts (per #8 + #79) and allows for significant simplifications in #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!).

So this patch paves the way for one of the most important steps towards BigPipe, and simplifies SmartCache.

Issue priority Major because blocks BigPipe, both of which are major.
Prioritized changes The main goal of this issue is performance — indirectly, by paving the way for BigPipe.
Disruption None. All internal.
Files: 
CommentFileSizeAuthor
#122 interdiff.txt4.05 KBWim Leers
#122 move_attachment-2407195-122.patch72.93 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,116 pass(es). View
#40 interdiff.txt15.6 KBFabianx
#105 consider_removing-reroll-2407195-105.patch67.18 KBWim Leers
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,033 pass(es). View
#120 move_attachment-2407195-120.patch68.95 KBWim Leers
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Comments

Wim Leers’s picture

Issue summary: View changes
dawehner’s picture

Well, the HtmlResponse class should not be a response but rather a domain object which gets the necessary preprocess stuff from outside.

Wim Leers’s picture

Status: Postponed » Active

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

Wim Leers’s picture

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!)

Fabianx’s picture

joelpittet’s picture

@Wim Leers this idea you have sounds really promising. Is it still something you'd like to see happen for 8.0.x?

Not sure I could architect such a thing but i'm a big +1 to this. Maybe I can start a patch with something completely wrong as a starter:D or anything you think is more helpful to make this fly.

Then we can actually put assets in preprocess_html() no?

Fabianx’s picture

Yes and yes.

We still plan to do that, not sure we can get rid of template_preprocess_html() in full though ... (But could be of everything below '// Collect all attachments. This must happen in the preprocess function for')

The idea is to have CacheableHtmlResponse + AttachmentsResponseInterface and then just using placeholder string (for CSS / JS) instead.

In a FinishResponseSubscriber we then replace the static placeholders with the scripts (header / footer) and styles and html head.

And that is it.

That means to create a response with assets (in HtmlRenderer):

$response = new CacheableHtmlResponse($build['#markup'], 200);
$response->setCacheableMetadata(CacheableMetadata::fromRenderArray($build));
$response->setAttachments($build['#attached']);

That is consistent with AjaxResponse then! (Tada - Consistency!)

Bonus points if you allow passing a render array in the constructor (or add a helper method):

$response = new CacheableHtmlResponse($build, 200);

or

$response = new CacheableHtmlResponse('', 200);
$response->setRenderArray($build);

Part of our overall performance plan.

Fabianx’s picture

Status: Active » Needs review
FileSize
18.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 57,808 pass(es), 15,307 fail(s), and 7,739 exception(s). View

I needed this for my 'rendering strategies' work, so gone ahead and implemented the minimum needed for it, so could as well post it.

Strong WIP, only for test-bot.

Status: Needs review » Needs work

The last submitted patch, 8: consider_removing-2407195-8.patch, failed testing.

Fabianx’s picture

Status: Needs work » Needs review
FileSize
19.75 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,577 pass(es), 300 fail(s), and 0 exception(s). View

It helps to commit the CacheableHtmlResponse class ...

Still only for testbot.

Status: Needs review » Needs work

The last submitted patch, 10: consider_removing-2407195-10.patch, failed testing.

Fabianx’s picture

Status: Needs work » Needs review
Issue tags: +DX (Developer Experience)
FileSize
27.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,202 pass(es), 615 fail(s), and 387 exception(s). View
18.9 KB

Still has hacks, but this one is much nicer in terms of DX while being 100% BC compatible in terms of the Response class:

  // @todo Make renderRoot return a cacheable render array directly.
  $this->renderer->renderRoot($html);
  $content = $this->renderCache->getCacheableRenderArray($html);

  $response = new CacheableHtmlResponse($content, 200);

This is very close to what Crell and me envisioned - just renderRoot should return a cacheable render array directly. Then there would be no longer any gap between the render and the response system.

--

The only thing changing here is the BareHtmlPageRenderer, which needs to support some responses that are send directly for which a special BareHtmlPageRenderer::filterResponse is added to hide that complexity.

Status: Needs review » Needs work

The last submitted patch, 12: consider_removing-2407195-12.patch, failed testing.

Fabianx’s picture

Status: Needs work » Needs review
FileSize
28.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,236 pass(es), 639 fail(s), and 55 exception(s). View
1.64 KB

Batch API wants some love, too ...

Status: Needs review » Needs work

The last submitted patch, 14: consider_removing-2407195-14.patch, failed testing.

Fabianx’s picture

Status: Needs work » Needs review
FileSize
1.75 KB
30.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,727 pass(es), 229 fail(s), and 7 exception(s). View

And I missed the installer.

Status: Needs review » Needs work

The last submitted patch, 16: consider_removing-2407195-16.patch, failed testing.

Fabianx’s picture

Status: Needs work » Needs review
FileSize
30.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,136 pass(es). View
4.06 KB

- Silly mistakes like removing the Response 'use' statement when its still used within the file.
- Forgetting to use CacheableHtmlResponse
- Wrong argument to renderBarePage due to internal re-factoring

But what is interesting about this one is the change to twig.engine as it immediately makes Twig wrapped exceptions more useful, could be split out into its own issue too, but is necessary to fix ErrorHandlerTest.

Fabianx’s picture

Assigned: Unassigned » Wim Leers

Assigning to Wim for a review with the following caveats that I am myself still unhappy about, but that can be resolved by mere re-organization of the code:

- Hardcoding strings to replace later feels / is wrong
- Calling the FinishResponseSubscriber from BareHtmlPageRenderer feels very wrong, should really be its own service
- Adding some much new logic to FinishResponseSubscriber feels wrong, should be its own injected service

But what to name this service?

- HtmlAssets?
- AttachmentsResponseProcessor?

This issue paves the way to removing _all_ the globals and drupal_process_attached() in a follow-up.

So whatever it is called needs to take into account that we want to be able to retrieve the headers (set via ['#attached']['http_header']) from that Service, too.

Cottser’s picture

Issue tags: +Twig

Affects Twig land, tagging.

Wim Leers’s picture

Status: Needs review » Needs work

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?

Fabianx’s picture

#21

1. No, 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.
2. Yes, as discussed would probably use ->prepare().
3. Yes, as discussed, but for that we probably need a service, unless I render it and store it via #attached html_response_placeholders directly (have to try that - yes, thats likely better).
4. Yes, lets use HtmlResponse and as discussed in IRC, lets see if the logic can live in HtmlResponse::prepareAttachments(), called by ::prepare().
5. Will be fixed.
6. Better to call $response->prepare()
7. Will be fixed.
8. Will be fixed.
9. :-D
10. Will be fixed.
11. Yes, that is correct, this removes the chicken-egg problem by moving the asset rendering into the response realm.
12. That one is tricky, but in a nut-shell:

Before the renderer rendered the children directly, hence we got the original exception, but now 'page' is rendered as part of a twig template and hence Twig simplifies the Exception by showing the template and line wrapping the original Exception in a new Exception, but unfortunately we totally loose the function() that caused the Exception that way.

For generic twig that is the only option, but this being Drupal, we can do better and just show the twig message as a first message, then show the original Exception.

That makes the ErrorHandlerTest pass as now we show both:

- The twig template name and line
- The original Exception with the function name, line, etc.

So we get the best out of both worlds.

Wim Leers’s picture

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 :)

Crell’s picture

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
    @@ -158,6 +155,54 @@ public function onRespond(FilterResponseEvent $event) {
    +  public static function processAttachments($response) {
    +    // @todo Move to a service.
    

    This should not be in HtmlResponse. It should be its own service, as the @todo suggests.

    Responses should be value objects. They represent data. They should not be doing anything interesting themselves. My experience working with value objects is that it is *extremely tempting* to let them have "simple" functionality. It's so easy, it's right there... But don't listen to that siren song. It will trap you and ensnare you and riddle your code with hard-to-fix design flaws. :-)

    (FabianX noted in IRC that Symfony's Response class already has a fair bit of logic in it. That's true. Given the above, I am not going to defend that as a good design decision. Quite the opposite. That's why PSR-7's ResponseInterface has no prepare() method, or similar. That should be its own service, too.)

    Especially in this case, there are calls out to various services. A value object should *never ever ever* call out to services, because it's complecting "be" and "do". Doing so is a very clear code smell. (Side note: An astute reader may note that Entities in Drupal 8 all have service dependencies. Take a look at the existence of DependencySerializationTrait. That proves my point.)

    This code should either be its own service, or part of another existing render service. The main question there is whether this logic would ever reasonably change between different service implementations. Is this generic to all render strategies, or strategy-specific? If strategy specific, put it in the appropriate render strategy class. If it's universal, make it its own class/service. (It has its own dependencies so should not be a trait.)

    Reading Wim's reply now, if AjaxResponse has too much attachment handling logic then perhaps that may need to get moved out, too? Definitely if it has any dependencies, although it didn't the last time I looked.

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

    This ship may have sailed, but is there no way to break this out a bit more, and make the different types of attachments explicit? Just from the name, all I see here is "array of random stuff".

  3. +++ b/core/lib/Drupal/Core/Render/AttachmentsResponseInterface.php
    @@ -0,0 +1,39 @@
    +  /**
    +   * Gets attachments set for this response.
    +   *
    +   * @return array $attachments
    +   *   An #attached array.
    +   */
    +  public function getAttachments();
    

    Unclear: Do I get back

    ['#attached' => [
    '#html_head' => [...],
    '#library' => [...],
    ]];
    
    // or
    
    [
    '#html_head' => [...],
    '#library' => [...],
    ]
    
    

    (Related to previous note, #attached still has significant DX problems around discoverability. In this case it's probably a documentation question.)

  4. +++ b/core/lib/Drupal/Core/Render/AttachmentsResponseTrait.php
    @@ -0,0 +1,41 @@
    +  protected $attachments = [
    +    'library' => [],
    +    'drupalSettings' => [],
    +    'html_head' => [],
    +    'feed' => [],
    +    'html_head_link' => [],
    +    'http_header' => [],
    +  ];
    

    A master list of the keys in attachments! It's Christmas! :-)

  5. +++ 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) {
    +    FinishResponseSubscriber::processAttachments($response);
       }
    

    This seems a poor method name. It's very easy to configure with the FilterResponseEvent class that Symfony uses for the kernel.response event. (Why it uses such bizarre names for its event classes I don't know, but it does.)

    There's also no, well, filtering going on here that I can see. (And I'd make the same comment about kernel.response, too.)

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

    These lines make me warm and fuzzy inside.

Wim Leers’s picture

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

Wim Leers’s picture

#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. :)
Fabianx’s picture

Assigned: Wim Leers » Fabianx

1. I found some more resources in terms of value objects (https://pragprog.com/articles/tell-dont-ask + http://en.wikipedia.org/wiki/Law_of_Demeter)

The TL;DR is:

In fact, according to the Law of Demeter for Methods, any method of an object should only call methods belonging to:

  1. itself.
  2. any parameters that were passed in to the method.
  3. any objects it created.
  4. any composite objects.

which gave me a way better understanding of why e.g. calling a service in a value object is bad, it couples the value object to a specific implementation.

However the HtmlResponseSubscriber::prepareAttachments() can be called by e.g. the bare_html_page_renderer service, because even though the main task of that service is to listen and filter responses, the bare_html_page_renderer can declare a dependency (composite) and hence use that service without violating LoD.

Nice, thanks for the information, Crell!

Crell’s picture

All of this basically boils down to: Don't ever use Response::prepare(), the Symfony folks were drunk when they added that..

I cannot speak to Fabien's level of intoxication, other than observing that he is French. :-) prepare() doesn't have any external dependencies so it's less of an issue, but yes, PSR-7 deliberately eschewed even that, for a reason. I'd say more "don't ever mimic Response::prepare()".

(When possible, isolate crap, so that the rest of your code doesn't need to smell too!)

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

Wim Leers’s picture

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

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

Fabianx’s picture

Filed #2497115: ajax_page_state is not taken into account for normal GET requests, not fixing in here, because of needed test coverage for this.

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
@@ -158,6 +155,54 @@ public function onRespond(FilterResponseEvent $event) {
+      $assets = AttachedAssets::createFromRenderArray($all_attached);
+      // Take Ajax page state into account, to allow for something like Turbolinks
+      // to be implemented without altering core.
+      // @see https://github.com/rails/turbolinks/
+      $ajax_page_state = \Drupal::request()->request->get('ajax_page_state');
+      $assets->setAlreadyLoadedLibraries(isset($ajax_page_state) ? explode(',', $ajax_page_state['libraries']) : []);
+      // Optimize CSS/JS if necessary, but only during normal site operation.
+      $optimize_css = !defined('MAINTENANCE_MODE') && \Drupal::config('system.performance')->get('css.preprocess');
+      $optimize_js = !defined('MAINTENANCE_MODE') && \Drupal::config('system.performance')->get('js.preprocess');
+      // Render the asset collections.
+      $asset_resolver = \Drupal::service('asset.resolver');
+
+      $variables = [];
+      $variables['styles'] = \Drupal::service('asset.css.collection_renderer')->render($asset_resolver->getCssAssets($assets, $optimize_css));
+      list($js_assets_header, $js_assets_footer) = $asset_resolver->getJsAssets($assets, $optimize_js);
+      $js_collection_renderer = \Drupal::service('asset.js.collection_renderer');
+      $variables['scripts'] = $js_collection_renderer->render($js_assets_header);
+      $variables['scripts_bottom'] = $js_collection_renderer->render($js_assets_footer);
+
+      // Handle all non-asset attachments.
+      drupal_process_attached($all_attached);
+      $variables['head'] = drupal_get_html_head(FALSE);

It would be great if this was a standalone part of a service. I've been copy/pasting this part for my own hacky purposes.

Fabianx’s picture

Status: Needs work » Needs review
FileSize
28.91 KB
33.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 93,061 pass(es), 1,053 fail(s), and 82 exception(s). View

#31:

Yes, new patch coming soon. Just dealing with a contextual views links problem right now. Have converted all to nice injected services already ...

Hm, should probably just post the patch and see if any new tests fail ...

@Tim: Let me know if #32 helps you or if you need some more helper methods or such on the service.

Status: Needs review » Needs work

The last submitted patch, 32: consider_removing-2407195-32.patch, failed testing.

Fabianx’s picture

Issue summary: View changes

- Addressed all points, generating unique placeholders now
- Added HtmlResponseSubscriber (yeah!)
- Fixed test failures (silly missing class)
- Fixed views contextual links (was the only preprocess_html callback that depended on the old behavior that page was rendered first, while it is now the opposite)

Fabianx’s picture

Status: Needs work » Needs review
FileSize
2.11 KB
35.03 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,264 pass(es), 1 fail(s), and 0 exception(s). View
Wim Leers’s picture

Status: Needs review » Needs work

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?

The last submitted patch, 35: consider_removing-2407195-35.patch, failed testing.

Crell’s picture

Because this representation of attachments is specific to HTML responses.

Then check the interface and the content-type header. Type hinting on a class is a bad idea.

Fabianx’s picture

Status: Needs work » Needs review
FileSize
15.6 KB
36.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,261 pass(es). View

All feedback addressed, also fixed the tests.

Notable:

-    $placeholder = '<drupal-html-response-placeholder key="' . $key . '" token="' . $token . '"></drupal-html-response-placeholder>';
+    $placeholder = SafeMarkup::format('<drupal-html-response-attachment-placeholder type="@type" token="@token"></drupal-html-response-attachment-placeholder>', [
+      '@type' => $type,
+      '@token' => $token,
+    ]);

This is future-proofing the code for when #markup is XSS filtered to ensure this patch does not conflict with that one.

Fabianx’s picture

#39: Yeah, its either:

A:

if ($response instanceof HtmlResponse) { }

OR

B:

if ($response instanceof AttachmentsResponseInterface && !($response instanceof AjaxResponse)) { }

However given that its called HtmlResponseSubscriber AND it is not the most usual thing to override HtmlResponse at all (could subclass) AND Symfony does not provide a ResponseInterface either AND even if you really needed a different class you could easily subclass HtmlResponseSubscriber and just call $this->prepareAttachments() for yourself ...

I would agree with Wim in this special case that A) is better - unless you insist that we write a HtmlResponseInterface.

tim.plunkett’s picture

+++ b/core/lib/Drupal/Core/EventSubscriber/HtmlResponseSubscriber.php
@@ -0,0 +1,232 @@
+  protected function processAssetLibraries(array $attached, array $placeholders) {

I'd like this to be public. I mimicked this in some code I was working on for a hackathon: https://github.com/acquia/acquia_lift/blob/cardstack-hackathon/src/Contr...

Wim Leers’s picture

Status: Needs review » Needs work

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.

Wim Leers’s picture

#42:

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

Isn't this enough?

Fabianx’s picture

Assigned: Fabianx » Unassigned
Issue tags: +Novice

#43:

3. Nope, else drupal_get_http_header() fails, need to move 'http_header' out of drupal_process_attached first.

---

Unassigning for now and tagging Novice for someone else to help bring this over the finish line.

Wim Leers’s picture

Issue tags: +Needs reroll

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

Crell’s picture

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

    Are we not still moving this to a service? It's way too much code to put in a listener, IMO.

    Also, when doing so I would strongly recommend returning the resulting response. Yes it's an object so it passes by handle so it would be slightly redundant, but we should be getting in the habit of returning values, not using input/output parameters. IMO in modern code those are a code smell, because that behavior is highly non-obvious. It also makes pipeing or chaining behavior much more difficult. Return values are your friend. :-)

  2. +++ b/core/lib/Drupal/Core/Render/HtmlResponse.php
    @@ -0,0 +1,47 @@
    +  public function setContent($content) {
    

    You'll probably hate me for saying this, but..

    setContent on Response() assumes a string, or string-able. An array is not itself stringable. we're providing the stringification. Should this instead be a setRenderContent() method, or similar, so that we can type hint the parameter to array?

    ... Actually... never mind. That would make it arguably harder to evolve the arrays into something more structured in the future. (Eg, the return of HtmlFragment :-) ).

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/EventSubscriber/HtmlResponseSubscriber.php
    @@ -0,0 +1,232 @@
    + * Definition of Drupal\Core\EventSubscriber\HtmlResponseSubscriber.
    

    nitpick ... Contains \...

  2. +++ b/core/lib/Drupal/Core/EventSubscriber/HtmlResponseSubscriber.php
    @@ -0,0 +1,232 @@
    +    $this->config = $config_factory->get('system.performance');
    

    I try to avoid using anything in constructors because this potentially triggers a chain of things. Especially a configfactory get, has potential for a lot of things going on. Pratically it doesn't matter because this service will just be initialized on KernelEvents::RESPONSE, but we could easily add a different event and then up with a hard to debug problem.

  3. +++ b/core/modules/views/views.module
    @@ -288,32 +288,16 @@ function views_theme_suggestions_container_alter(array &$suggestions, array $var
    - * Implements hook_element_info_alter().
    - *
    - * @see views_page_display_pre_render()
    - * @see views_preprocess_page()
    - */
    -function views_element_info_alter(&$types) {
    -  $types['page']['#pre_render'][] = 'views_page_display_pre_render';
    -}
    -
    -/**
    - * #pre_render callback to set contextual links for views using a Page display.
    + * Implements MODULE_preprocess_HOOK().
      */
    -function views_page_display_pre_render(array $element) {
    +function views_preprocess_html(&$variables) {
       // If the main content of this page contains a view, attach its contextual
       // links to the overall page array. This allows them to be rendered directly
       // next to the page title.
       if ($view = views_get_page_view()) {
    -    views_add_contextual_links($element, 'page', $view, $view->current_display);
    +    views_add_contextual_links($variables['page'], 'page', $view, $view->current_display);
       }
    -  return $element;
    -}
    

    Is that hunk needed? Seems a little bit out of scope or at least some form of API change as part of teh patch

lauriii’s picture

Status: Needs work » Needs review
Issue tags: -Novice, -Needs reroll
FileSize
37.47 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 92,894 pass(es), 1,140 fail(s), and 282 exception(s). View
5.5 KB
36.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,352 pass(es). View

Rerolled & fixed #43 and #48.1

The last submitted patch, 49: consider_removing-2407195-49.patch, failed testing.

Wim Leers’s picture

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…

Wim Leers’s picture

FileSize
38.17 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,984 pass(es), 1 fail(s), and 0 exception(s). View

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

Wim Leers’s picture

#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);
Wim Leers’s picture

FileSize
38.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,247 pass(es), 1,167 fail(s), and 258 exception(s). View

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…

The last submitted patch, 52: consider_removing-reroll-2407195-52.patch, failed testing.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

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 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.

Wim Leers’s picture

FileSize
38.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View
5.89 KB

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.

The last submitted patch, 54: consider_removing-reroll-2407195-54.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 57: consider_removing-reroll-2407195-57.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
41.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,878 pass(es), 1 fail(s), and 0 exception(s). View
34.91 KB

(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.
Wim Leers’s picture

FileSize
58.68 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,904 pass(es), 5 fail(s), and 0 exception(s). View
19.19 KB

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.

The last submitted patch, 60: consider_removing-reroll-2407195-60.patch, failed testing.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Issue tags: +Needs issue summary update
FileSize
57.81 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,875 pass(es), 5 fail(s), and 0 exception(s). View
6.54 KB

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.

Wim Leers’s picture

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

Wim Leers’s picture

The last submitted patch, 61: consider_removing-reroll-2407195-61.patch, failed testing.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

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

Status: Needs review » Needs work

The last submitted patch, 63: consider_removing-reroll-2407195-63.patch, failed testing.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs work » Needs review
FileSize
62.48 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,903 pass(es). View
4.77 KB

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

Fabianx’s picture

Great changes!

Love the new patch! :)

Having BareHtmlRenderer directly return a Response is so much nicer and makes so much more sense!

#63.4:

Rename to AjaxResponseSubscriber makes sense to me. BC break should be limited as it is just a listener. If there is BC concerns can add class alias and service alias.

#63.12:

Yes, responses are cached via PageCache, which serializes the Response as is. Don't think we want to store the whole render tree, do we?

Little thing I found in my self-review:

+++ 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;
+  }

I thought about this again:

It won't fly.

What if a form throws a RedirectResponse?

We surely don't want to display messages for that.

However Twig_Error_Runtime eats most of the useful information.

There is two ways:

a) Create a new \RuntimeException / custom Twig Exception with both messages concatenated.

b) Have the ErrorHandler unwrap all previous Exceptions and display them separately as messages.

I think unwrapping would be useful anyway.

c) Decide its out-of-scope here and just fix the test and open a follow-up to make Exception output more useful (again).

Wim Leers’s picture

  • #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.
Crell’s picture

Title: Consider removing template_preprocess_html() by modifying HtmlRenderer::renderResponse() and introducing HtmlResponse » Move attachment processing to services and per-type response subclasses

Overall this is looking really good! At this point I feel safe retitling the issue as well, since we want to go through with this.

Some comments below. I don't fully understand the Twig issue so defer to FabianX on that front.

  1. +++ b/core/includes/errors.inc
    @@ -241,12 +241,11 @@ function _drupal_log_error($error, $fatal = FALSE) {
    -      }
    -      else {
    -        $output = \Drupal::service('bare_html_page_renderer')->renderBarePage(['#markup' => $message], 'Error', 'maintenance_page');
    +        exit;
           }
    

    We've been actively trying to expunge the word "exit" from the code base, because it breaks many many things, like some testing strategies. Even in error handling. Why do we need to add one here?

  2. +++ b/core/lib/Drupal/Core/Ajax/AjaxResponseAttachmentsProcessor.php
    @@ -0,0 +1,185 @@
    +    // Aggregate CSS/JS if necessary, but only during normal site operation.
    +    $optimize_css = !defined('MAINTENANCE_MODE') && $this->config->get('css.preprocess');
    +    $optimize_js = !defined('MAINTENANCE_MODE') && $this->config->get('js.preprocess');
    

    Likely off-topic, but haven't we pushed Maintenance Mode into a container parameter or something by now? I know we moved some of our other constants there. A global constant here is sad-making.

  3. +++ b/core/lib/Drupal/Core/Ajax/AjaxResponseAttachmentsProcessor.php
    @@ -0,0 +1,185 @@
    +      ->setAlreadyLoadedLibraries(isset($ajax_page_state) ? explode(',', $ajax_page_state['libraries']) : [])
    

    This variable will always be set. It may be null, or some other falsy value, but it will always be set on line 122.

  4. +++ b/core/lib/Drupal/Core/Ajax/AjaxResponseAttachmentsProcessor.php
    @@ -0,0 +1,185 @@
    +    // Render the HTML to load these files, and add AJAX commands to insert this
    +    // HTML in the page. Settings are handled separately, afterwards.
    +    $settings = [];
    +    if (isset($js_assets_header['drupalSettings'])) {
    +      $settings = $js_assets_header['drupalSettings']['data'];
    +      unset($js_assets_header['drupalSettings']);
    +    }
    +    if (isset($js_assets_footer['drupalSettings'])) {
    +      $settings = $js_assets_footer['drupalSettings']['data'];
    +      unset($js_assets_footer['drupalSettings']);
    +    }
    

    The comment seems out of sync, as the code right after it is handling the settings it says are later.

  5. +++ b/core/lib/Drupal/Core/Ajax/AjaxResponseAttachmentsProcessor.php
    @@ -0,0 +1,185 @@
    +    if (!empty($css_assets)) {
    

    Minor point: Presumably $css_assets is an array, so an empty array is already false, so the !empty() is unnecessary. Same for the other blocks below.

  6. +++ b/core/lib/Drupal/Core/Ajax/AjaxResponseAttachmentsProcessor.php
    @@ -0,0 +1,185 @@
    +    \Drupal::moduleHandler()->alter('ajax_render', $commands);
    

    This is an injected class. Just inject the Module Handler, too.

  7. +++ b/core/lib/Drupal/Core/Render/AttachmentsResponseTrait.php
    @@ -0,0 +1,41 @@
    +  public function setAttachments(array $attachments) {
    +    $this->attachments = $attachments;
    +    return $this;
    +  }
    

    To improve predictability, shouldn't this merge the array rather than just overwrite it? That is, if someone passes in an array that has onl library and feed, then we end up with an array that loses those other properties. That makes the structure of the return value of getAttachments() unpredictable.

    We can probably just do:

    $this->attachments = $attachments + $default_array;

    Where $default_array is the same as the, er, default value of $this->attachments. That way, $this->attachments (and thus getAttachments()'s return value) is always guaranteed to have all 6 keys.

  8. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
    @@ -0,0 +1,217 @@
    +    if (!$response instanceof HtmlResponse) {
    +      throw new \InvalidArgumentException('\Drupal\Core\Render\HtmlResponse instance expected.');
    +    }
    

    Perhaps add a @todo that once we start using assertions this seems like a good place for one. (More involved type checking than PHP natively allows.)

  9. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
    @@ -0,0 +1,217 @@
    +    $variables = [];
    +    $variables += $this->processAssetLibraries($attached, $placeholders);
    

    Seems these lines don't need to be separate...

  10. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
    @@ -0,0 +1,217 @@
    +    // Handle all non-asset attachments. This populates drupal_get_html_head()
    +    // and drupal_get_http_header().
    +    $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);
    +    }
    +
    

    I'm sure I sound like a broken record by now, but... why are these functions still here? Isn't the whole point of the renderers and such that we can finally get rid of these damned test-breaking things?

  11. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
    @@ -0,0 +1,217 @@
    +      // Symfony special-cases the 'Status' header.
    +      if ($name === 'status') {
    +        $response->setStatusCode($value);
    +      }
    

    The comment is incorrect. Status is not a header in the first place. It's PHP native and Drupal that get it wrong. :-) The comment should reflect that we're tracking status as if it were a header for, um, historical reasons I guess?

  12. +++ b/core/modules/system/src/Tests/Ajax/CommandsTest.php
    @@ -133,7 +136,15 @@ public function testAttachedSettings() {
    -      $response->prepare(new Request());
    +      $ajax_response_attachments_processor = \Drupal::service('ajax_response.attachments_processor');
    +      $subscriber = new AjaxSubscriber($ajax_response_attachments_processor);
    +      $event = new FilterResponseEvent(
    +        \Drupal::service('http_kernel'),
    +        new Request(),
    +        HttpKernelInterface::MASTER_REQUEST,
    +        $response
    +      );
    +      $subscriber->onResponse($event);
    

    The kernel and processor can be mocked, making this a proper unit test of the subscriber. Also, faster. :-)

  13. +++ b/core/tests/Drupal/Tests/Core/Ajax/AjaxResponseTest.php
    @@ -81,7 +84,15 @@ public function testPrepareResponseForIeFormRequestsWithFileUpload() {
    -    $response->prepare($request);
    +    $ajax_response_attachments_processor = $this->getMock('\Drupal\Core\Render\AttachmentsResponseProcessorInterface');
    +    $subscriber = new AjaxSubscriber($ajax_response_attachments_processor);
    +    $event = new FilterResponseEvent(
    +      $this->getMock('\Symfony\Component\HttpKernel\HttpKernelInterface'),
    +      $request,
    +      HttpKernelInterface::MASTER_REQUEST,
    +      $response
    +    );
    +    $subscriber->onResponse($event);
    

    Kind of like this, in fact. :-)

Wim Leers’s picture

FileSize
63.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh --clean. View
12.6 KB

#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?
Fabianx’s picture

#63.12: Can we leave the getCacheableRenderArray for now with a @todo on the renderRoot() issue. I realize it is superfluous, but putting the argument instead of the result into the response feels more wrong to me ...

2. Leave it out and see 1 test fail ;). You'll then know way more exactly of what I am talking about.

Wim Leers’s picture

FileSize
62.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: failed to clear checkout directory. View
1.07 KB
  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
Wim Leers’s picture

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.

Crell’s picture

#76: +1!

#73.7: I think I wasn't clear about what is being merged then. With the current code, you get this:

$r = new HtmlResponse();
$before = $r->getAttachments();
// $before has 6 keys, all of which are empty arrays.
$r->setAttachments(['http_header' => [...]]);
$after = $r->getAttachments();
// $after has ONE key, 'http_header', which has some value.

What we want instead is for $after to contain the same 6 keys, 5 of which are empty arrays and one of which is the value provided by the caller. That way, code calling getAttachments() can rely on there always being 6 keys (no isset() calls) and that the unused keys have a falsy (empty array) value.

Fabianx’s picture

#76: +1 to AttachmentsInterface :)

#77: Lets call it:

addAttachments() then instead?

Fabianx’s picture

This makes implementing RenderStrategies (and hence BigPipe) much simpler, because the 3 separate renderings of the page array then one rendering afterwards make it really complicated to properly render placeholders after the main processing has been done. As essentially the render array is rendered 4 times, but three times with renderRoot, then once without ...

Wim Leers’s picture

Issue summary: View changes

Updating the IS per #79.

Fabianx’s picture

It also makes SmartCache simpler, because it just needs to change renderRoot() to render() and then just needs to execute the placeholders after the cache get instead of having to deal with the 3 regions, because template_preprocess_html() runs too early.

It also makes it possible to remove / deprecate drupal_process_attached() in a follow-up. (I found no more blockers for it.)

Wim Leers’s picture

FileSize
64.14 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh --clean. View
9.66 KB

#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()

The last submitted patch, 73: consider_removing-reroll-2407195-73.patch, failed testing.

The last submitted patch, 75: consider_removing-reroll-2407195-75.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 82: consider_removing-reroll-2407195-82.patch, failed testing.

Fabianx’s picture

If we move AttachmentsResponseTrait to AttachmentsTrait, could BubbleableMetadata use that then? :))))))

Also lets not forget we have that namespaced, it is actually \Drupal\Core\Render\AttachmentsInterface which is obviously way way more specific ...

The last submitted patch, 82: consider_removing-reroll-2407195-82.patch, failed testing.

The last submitted patch, 82: consider_removing-reroll-2407195-82.patch, failed testing.

The last submitted patch, 82: consider_removing-reroll-2407195-82.patch, failed testing.

Crell’s picture

#87: I like the way you think. :-)

Wim: That we're doing it wrong elsewhere is no reason to keep doing it wrong. :-) But if that must get split to a follow-up, so be it. Can you file? (I don't know all the places that we'd need to change.) (Insert stock rant about value objects being better than arrays here.) An addAttachments() Is fine; I don't much care, but it's a separate question.

#73.12: Won't somebody think of the llamas!

The last submitted patch, 82: consider_removing-reroll-2407195-82.patch, failed testing.

The last submitted patch, 82: consider_removing-reroll-2407195-82.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
66.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,028 pass(es), 1 fail(s), and 0 exception(s). View
6.83 KB

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

Status: Needs review » Needs work

The last submitted patch, 98: consider_removing-reroll-2407195-95.patch, failed testing.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

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.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs work » Needs review
FileSize
67.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,022 pass(es). View
1.05 KB

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?

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

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

However overall the Exception will always be more helpful than the line where it occurred in the template.

"Yes, we printed a {{ content }} array, AND?"

--

nit:

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

Wim Leers’s picture

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.

Wim Leers’s picture

FileSize
67.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,040 pass(es). View
786 bytes

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

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
67.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,033 pass(es). View
774 bytes

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.

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC, thanks so much!

Crell’s picture

#98 looks like you meant to paste a link to a follow up, but didn't? I think that point was my last blocking objection, so if the follow up exists then I'm +1 on committing here. Thanks Wim and Fabian!

Wim Leers’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
  1. The patch needs a reroll.
  2. Missing a beta evaluation
  3.  core/includes/errors.inc                           |   9 +-
    ...
     use Drupal\Core\Asset\AttachedAssets;
     use Drupal\Core\Render\BubbleableMetadata;
     use Drupal\Core\Render\Renderer;
    +use Drupal\Core\Render\AttachmentsInterface;
    +use Drupal\Core\Render\AttachmentsTrait;
     use Symfony\Component\DependencyInjection\ContainerInterface;
     use Symfony\Component\HttpFoundation\JsonResponse;
     use Symfony\Component\HttpFoundation\Request;
    

    There's quite a few unused use statements now amongst this lot.

  4. +++ b/core/authorize.php
    @@ -152,16 +154,18 @@ function authorize_access_allowed(Request $request) {
     if (!empty($content)) {
    ...
     }
    

    I think the empty($content) check is superfluous - but I guess not part of this patch.

  5. +++ b/core/includes/batch.inc
    @@ -19,6 +19,7 @@
    +use Drupal\Core\Render\HtmlResponse;
    

    Not used.

  6. +++ b/core/includes/theme.inc
    @@ -1308,41 +1309,23 @@ function template_preprocess_html(&$variables) {
    +  foreach ($types as $type) {
    +    $token = Crypt::randomBytesBase64(55);
    

    Does each placeholder need a separate token? Is this more secure?

  7. +++ b/core/includes/theme.inc
    @@ -1308,41 +1309,23 @@ 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,
    +    ]);
    

    Is it necessary to for the result of this to be in the list of safe strings?

  8. +++ b/core/includes/theme.inc
    @@ -1308,41 +1309,23 @@ function template_preprocess_html(&$variables) {
    -  $assets = AttachedAssets::createFromRenderArray($all_attached);
    

    AttachedAssets is no longer used in this include.

  9. +++ b/core/lib/Drupal/Core/Ajax/AjaxResponseAttachmentsProcessor.php
    @@ -0,0 +1,197 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function processAttachments(AttachmentsInterface $response) {
    +    // @todo Convert to assertion once https://www.drupal.org/node/2408013 lands
    +    if (!$response instanceof AjaxResponse) {
    +      throw new \InvalidArgumentException('\Drupal\Core\Ajax\AjaxResponse instance expected.');
    +    }
    +
    +    $request = $this->requestStack->getCurrentRequest();
    +
    +    if ($response->getContent() == '{}') {
    +      $response->setData($this->buildAttachmentsCommands($response, $request));
    +    }
    +  }
    

    I think this needs to return the response to conform to the interface.

  10. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
    @@ -0,0 +1,218 @@
    +      $variables['head'] = drupal_get_html_head(FALSE);
    

    This is deprecated for 8.0.x and this is fresh code - anything we can do here?

  11. +++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
    @@ -0,0 +1,218 @@
    +    $headers = drupal_get_http_header();
    

    This is deprecated for 8.0.x and this is fresh code - anything we can do here?

  12. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -119,39 +127,18 @@ public function renderResponse(array $main_content, Request $request, RouteMatch
    -    $default = new CacheableMetadata();
    

    CacheableMetadata is not used anymore.

  13. +++ b/core/lib/Drupal/Core/Render/AttachmentsResponseProcessorInterface.php
    @@ -0,0 +1,30 @@
    +  public function processAttachments(AttachmentsInterface $response);
    

    This has to be both a response and an attachementsinterface - do we think it is worthwhile creating a ResponseAttachmentsInterface?

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
65.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,050 pass(es). View

Here's the re-roll.

joelpittet’s picture

FileSize
65.89 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh --clean. View
3.56 KB

Resolved the smaller bits: from #109

  1. DONE
  2. TBD
  3. DONE
  4. DONE
  5. DONE
  6. TBD
  7. TBD
  8. DONE
  9. TBD
  10. Not sure
  11. Not sure
  12. DONE
  13. Not sure

Status: Needs review » Needs work

The last submitted patch, 111: move_attachment-2407195-111.patch, failed testing.

The last submitted patch, 111: move_attachment-2407195-111.patch, failed testing.

joelpittet’s picture

Hmm, didn't do much just removed use statements that PHP storm was pointing out in the files mentioned in #109 Sorry for breaking things:S

joelpittet’s picture

Status: Needs work » Needs review
FileSize
68.22 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,065 pass(es). View

Hmm maybe I messed something up in that patch. This one seems to work better.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
67.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,056 pass(es). View
1.33 KB

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.

Wim Leers’s picture

Issue summary: View changes

Oops, forgot the beta evaluation.

The last submitted patch, 111: move_attachment-2407195-111.patch, failed testing.

Wim Leers’s picture

FileSize
1.09 KB
68.95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

#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.

The last submitted patch, 120: move_attachment-2407195-120.patch, failed testing.

Wim Leers’s picture

FileSize
72.93 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,116 pass(es). View
4.05 KB

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 :)

Fabianx’s picture

Back to RTBC

Crell’s picture

Issue tags: +Avoid commit conflicts

Tagging because this seems reroll-fragile based on the last 24 hours or so. :-)

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Avoid commit conflicts

Committed 42c693a and pushed to 8.0.x. Thanks!

Thanks for adding the beta to the issue summary.

  • alexpott committed 42c693a on 8.0.x
    Issue #2407195 by Wim Leers, Fabianx, joelpittet, lauriii, Crell: Move...
Wim Leers’s picture

  • alexpott committed 59469bf on 8.0.x
    Issue #2512382 by Wim Leers: Follow-up for #2407195: #attached['...
Wim Leers’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

cilefen’s picture