Remove HtmlFragment/HtmlPage

Adding formatting and commas.

Adding related issues.

I'm working on this one today. Hope to post a patch by the end of the day.

Opened a small issue to help pave the way for this one: #2357937: Remove {{ feed_icons }} from page template (page.html.twig).

Another, bigger issue to help pave the way: #2359901: Discourage $main_content['#title'] in favor of route titles and title callbacks.

#2359901: Discourage $main_content['#title'] in favor of route titles and title callbacks is no longer a blocker. I found a work-around.

Tomorrow, I'll be posting a patch at last. Most encountered problems have been fixed; I went from >5000 failed tests to now <30. Expect a very big update tomorrow.

The reason I've been working in relative isolation (I did have frequent check-ins with catch) is that fixing one thing uncovered another thing needing to be fixed, and that thing in turn required another thing to be fixed, etc. It's all still so interwoven, due to the many loose ends that haven't been fixed (both in relation to HtmlPage/HtmlFragment and relatively independent of them). I felt it was very important to formulate a complete, cohesive answer, because that's a big part of the current entangledness in HEAD: many things that make sense in isolation, but not together.
Since this is the closing stone for #2350943: [Meta] Untangle Drupal 8 page rendering (all other child issues have been resolved), and since it aims to close so many issues, I felt it was important that this issue provided complete closure for this problem space: with the patch for this issue committed (even if that needs to happen in parts/child issues) should allow us to look at Drupal 8 page rendering and say page rendering in Drupal 8 is done and preferably even make developers think and say, after reading a single page of docs okay, I understand how Drupal 8 renders pages!.

Speaking at a high-level: as I said in #10: I felt it was very important to formulate a complete, cohesive answer, because that's a big part of the current entangledness in HEAD: many things that make sense in isolation, but not together.
In doing so, I made sure to keep in mind (roughly in order of priority) while working on this patch:

1. that the rendering pipeline should be understandable
2. that the rendering pipeline should not only be understandable for rendering HTML only
3. that it should enable, not prevent SCOTCH
4. that it should have a better DX
5. that it should have a better TX
6. that it's easy to add support for additional formats (besides the already supported HTML/AJAX/Drupal Dialog/Drupal Modal Dialog)
7. that it should allow us to remove the globals for attachment handling

Larry (Crell) and others who worked on HtmlPage/HtmlFragment, I'd like to say again that I feel very sorry that we jointly had to conclude at DrupalCon Amsterdam that we'd have to remove HtmlPage and HtmlFragment. But I think and hope that you will see that your work was not in vain — in many places, the HtmlPage/HtmlFragment patches have actually paved the way for this one. And I hope you'll agree that some of the things you were working towards — like SCOTCH — are now actually closer within reach than ever before!

While working on this and reading the many existing issues around page rendering, I stumbled upon #1594870-24: Decide what KernelEvents::VIEW subscribers should be used for, document it somewhere, and make ViewSubscriber comply and #1594870-26: Decide what KernelEvents::VIEW subscribers should be used for, document it somewhere, and make ViewSubscriber comply, written by effulgentsia a full 1.5 years ago… and it describes rather closely what this patch now effectively does.

The patch

Diffstat: 125 files changed, 2207 insertions(+), 3144 deletions(-). 1000 LoC less, with all outstanding loose ends fixed in page rendering. The difference would have been several hundred LoC greater still if this patch didn't add hundreds of lines of docs.

This is a ~300 KB patch. Unless people think otherwise, I will begin splitting off parts of it that can be split off into child issues, to simplify the review process.

The sections below the "API changes" section will list changes per topic. Some of those topics can be moved into child issues. Many of them are too intertwined, though. But the per-topic descriptions of what's changed should help you understand this huge patch.

Flowchart

I created a flowchart to help make this patch more digestable, and also to just document the hell out of the (proposed) Drupal 8 page rendering pipeline, so that anybody can understand how it works — it literally took me days to understand the rendering pipeline in HEAD.
I want the understanding process to take an hour at most with this proposed patch.

Changes

In the order you encounter them in the patch:

1. Removed _drupal_add_feed(). (Split off to #2357937: Remove {{ feed_icons }} from page template (page.html.twig).)
2. Removed drupal_get_feeds(). (Split off to #2357937: Remove {{ feed_icons }} from page template (page.html.twig).)
3. Removed drupal_set_page_content(). [PAGE-VARIANTS]
4. Removed drupal_prepare_page(). [MAIN-CONTENT-CONTROLLERS]
5. Changed drupal_render()'s signature. [DRUPAL_RENDER]
6. Removed \Drupal\Core\Ajax\AjaxResponseRenderer, is now part of \Drupal\Core\Controller\AjaxController, shared functionality lives in \Drupal\Core\Controller\MainContentControllerBase [MAIN-CONTENT-CONTROLLERS]
7. Added \Drupal\Core\Block\MainContentBlockPluginInterface (for blocks rendering the main content). [PAGE-VARIANTS]
8. Added \Drupal\Core\Controller\ModalController (thin subclass of \Drupal\Core\Controller\DialogController, to comply with \Drupal\Core\Controller\MainContentControllerInterface) [MAIN-CONTENT-CONTROLLERS]
9. Removed \Drupal\Core\Controller\HtmlControllerBase [MAIN-CONTENT-CONTROLLERS]
10. Removed \Drupal\Core\Controller\HtmlPageController (superseded by \Drupal\Core\Controller\HtmlController) [MAIN-CONTENT-CONTROLLERS]
11. Added \Drupal\Core\Controller\MainContentControllerBase and \Drupal\Core\Controller\MainContentControllerInterface (interface and base class, respectively, for \Drupal\Core\Controller\HtmlController, \Drupal\Core\Controller\AjaxController, \Drupal\Core\Controller\DialogController and \Drupal\Core\Controller\ModalController) [MAIN-CONTENT-CONTROLLERS]
12. Added \Drupal\Core\Display\Annotation\PageDisplayVariant: annotation for the next item. [PAGE-VARIANTS]
13. Added \Drupal\Core\Display\PageVariantInterface, subclass of \Drupal\Core\Display\VariantInterface, specifically for "page variants" (it adds a setMainContent() method). [PAGE-VARIANTS]
14. Changed \Drupal\Core\EventSubscriber\ContentControllerSubscriber, to have full documentation rather than a misleading one-liner, and to use the result of the compiler pass rather than a hardcoded list. [MAIN-CONTENT-CONTROLLERS]
15. Changed \Drupal\Core\EventSubscriber\DefaultExceptionHtmlSubscriber to extend \Drupal\Core\EventSubscriber\DefaultExceptionHtmlSubscriber, since the latter now behaves more like the former than before. [403-404]
16. Changed \Drupal\Core\EventSubscriber\DefaultExceptionHtmlSubscriber to work more like \Drupal\Core\EventSubscriber\CustomExceptionHtmlSubscriber: no more hard-coded responses. [403-404] + [HTML-RENDERING]
17. Changed \Drupal\Core\EventSubscriber\FinishResponseSubscriber: Status header handling. [STATUS-HEADER]
18. Removed Drupal\Core\EventSubscriber\HtmlViewSubscriber. [MAIN-CONTENT-CONTROLLERS]
19. Changed \Drupal\Core\EventSubscriber\MaintenanceModeSubscriber to use \Drupal\Core\Render\BareHtmlPageRenderer instead of \Drupal\Core\Page\DefaultHtmlPageRenderer. [HTML-RENDERING]
20. Changed \Drupal\Core\EventSubscriber\ViewSubscriber: it needs to do less work now. [MAIN-CONTENT-CONTROLLERS]
21. Removed \Drupal\Core\Page\DefaultHtmlFragmentRenderer. [MAIN-CONTENT-CONTROLLERS]
22. Removed \Drupal\Core\Page\DefaultHtmlPageRenderer. [MAIN-CONTENT-CONTROLLERS]
23. Removed \Drupal\Core\Page\FeedLinkElement (Split off to #2357937: Remove {{ feed_icons }} from page template (page.html.twig).)
24. Removed \Drupal\Core\Page\HeadElement. [MAIN-CONTENT-CONTROLLERS]
25. Removed \Drupal\Core\Page\HtmlFragment. [MAIN-CONTENT-CONTROLLERS]
26. Removed \Drupal\Core\Page\HtmlFragmentInterface. [MAIN-CONTENT-CONTROLLERS]
27. Removed \Drupal\Core\Page\HtmlFragmentRendererInterface. [MAIN-CONTENT-CONTROLLERS]
28. Removed \Drupal\Core\Page\HtmlPage. [MAIN-CONTENT-CONTROLLERS]
29. Removed \Drupal\Core\Page\HtmlPageRendererInterface. [MAIN-CONTENT-CONTROLLERS]
30. Removed \Drupal\Core\Page\LinkElement. [MAIN-CONTENT-CONTROLLERS]
31. Removed \Drupal\Core\Page\MetaElement. [MAIN-CONTENT-CONTROLLERS]
32. Removed \Drupal\Core\Page\RenderHtmlRenderer. [MAIN-CONTENT-CONTROLLERS]
33. Removed \Drupal\Core\Page\RenderHtmlRendererInterface. [MAIN-CONTENT-CONTROLLERS]
34. Added \Drupal\Core\Render\BareHtmlPageRenderer, successor of \Drupal\Core\Page\DefaultHtmlPageRenderer. [HTML-RENDERING]
35. Added \Drupal\Core\Render\BareHtmlPageRendererInterface, see previous item. [HTML-RENDERING]
36. Changed \Drupal\Core\Render\Element\Html: added a #pre_render callback to assign meta tags. [MAIN-CONTENT-CONTROLLERS]
37. Added \Drupal\Core\Render\MainContentControllerPass. [MAIN-CONTENT-CONTROLLERS]
38. Removed \Drupal\Core\Routing\Enhancer\AjaxEnhancer, already dead code in HEAD, now definitely. [MAIN-CONTENT-CONTROLLERS]
39. Changed block.module: introduced a hook_page_top() implementation for the "backlink" on the demo page. [PAGE-VARIANTS]
40. Added \Drupal\block\EventSubscriber\BlockPageDisplayVariantSubscriber: selects the block page display variant. [PAGE-VARIANTS] + [MAIN-CONTENT-CONTROLLERS]
41. Changed \Drupal\block\Plugin\DisplayVariant\FullPageVariant: renamed to BlockPageVariant and now implements \Drupal\Core\Display\PageVariantInterface. [PAGE-VARIANTS]
42. Added \Drupal\block\Plugin\DisplayVariant\DemoBlockPageVariant. [PAGE-VARIANTS]
43. Changed ckeditor.module: the format_tags setting for each text format must be pre-calculated. Otherwise a "root" call of drupal_render() could happen within a root call, which now triggers an exception. [DRUPAL_RENDER]
44. Changed \Drupal\ckeditor\Plugin\CKEditorPlugin\Internal: see above. [DRUPAL_RENDER]
45. Changed \Drupal\comment\CommentPostRenderCache: bugfix. Fixes #2238835: #post_render_cache + #attached. [DRUPAL_RENDER]
46. Changed \Drupal\views\Plugin\views\style\Rss + \Drupal\node\Plugin\views\row\Rss: "final" rendering because rendering a feed, not HTML. [DRUPAL_RENDER]
47. Changed filter.module: check_markup() does "final" rendering. [DRUPAL_RENDER]
48. Changed node.module: drupal_render() no longer does "final" rendering by default. [DRUPAL_RENDER]
49. Changed \Drupal\rest\Plugin\views\display\RestExport: "final" rendering because rendering REST export, not HTML. [DRUPAL_RENDER]
50. Changed \Drupal\system\Controller\BatchController to use \Drupal\Core\Render\BareHtmlPageRenderer instead of \Drupal\Core\Page\DefaultHtmlPageRenderer. [HTML-RENDERING]
51. Changed \Drupal\system\Controller\DbUpdateController to use \Drupal\Core\Render\BareHtmlPageRenderer instead of \Drupal\Core\Page\DefaultHtmlPageRenderer. [HTML-RENDERING]
52. Added \Drupal\system\Controller\Http4xxController: for the system.403 and system.404 routes. [403-404] + [HTML-RENDERING]
53. Added \Drupal\system\Event\PageDisplayVariantSelectionEvent. [PAGE-VARIANTS]
54. Added \Drupal\system\Event\SystemEvents: to add the "page display variant selection" event. [PAGE-VARIANTS]
55. Changed \Drupal\system\Plugin\Block\SystemMainBlock: to replace the call to drupal_set_page_content() with an implementation of MainContentBlockPluginInterface. [PAGE-VARIANTS]
56. Added \Drupal\block\Plugin\DisplayVariant\SimplePageVariant: to encapsulate the default behavior of drupal_prepare_page(), i.e. to be used when Block module is not installed, and BlockPageVariant cannot be used. [PAGE-VARIANTS] + [MAIN-CONTENT-CONTROLLERS]
57. Changed \Drupal\system\Tests\Common\AddFeedTest because FeedLinkElement and HtmlPage are no more. [MAIN-CONTENT-CONTROLLERS]
58. Changed \Drupal\system\Tests\Routing\ExceptionHandlingTest: was testing in a too-artificial manner, and hence was actually testing the wrong thing. Actual page responses are always prepare()d, which sets the content type automatically, the tests had to be adjusted to reflect that. [403-404]
59. Removed \Drupal\system\Tests\System\MainContentFallbackTest: obsolete, now that we have SimplePageVariant. [PAGE-VARIANTS]
60. Changed system.routing.yml: to add system.403 and system.404, but also to fix system.batch_page.normal: it's only ever intended to return HTML, but because it didn't set a _format, the wrong format could be negotiated, breaking things horribly. [403-404]
61. Changed html.html.twig. [PAGE-VS-HTML]
62. Changed page.html.twig. (Split off to #2357937: Remove {{ feed_icons }} from page template (page.html.twig).)
63. Removed \Drupal\system_module_test\EventSubscriber\HtmlPageSubscriber: was overriding assigned meta tags, this now happens in \Drupal\Core\Render\Element\Html::preRenderHtml() rather than on HtmlPage, hence needed to be changed into #pre_render callbacks. [MAIN-CONTENT-CONTROLLERS]
64. Changed \Drupal\views\EventSubscriber\RouteSubscriber: removed Status header overriding, no longer needs an event subscriber now. [STATUS-HEADER]
65. Changed \Drupal\views\Plugin\views\area\HTTPStatusCode: no longer needs to set a global to override the Status header. [STATUS-HEADER]
66. Changed \Drupal\views\Plugin\views\display\Feed + \Drupal\views\Plugin\views\row\RssFields + \Drupal\views\Plugin\views\style\Rss: "final" rendering because rendering feed, not HTML. [DRUPAL_RENDER]
67. Changed views.module: handling contextual links for "page" displays needs to happen differently now that the global HtmlPage object is gone. [MAIN-CONTENT-CONTROLLERS]
68. Renamed \Drupal\Tests\Core\Ajax\AjaxResponseRendererTest to \Drupal\Tests\Core\Controller\AjaxControllerTest, since AjaxResponseRenderer is now part of AjaxController. [MAIN-CONTENT-CONTROLLERS]
69. Changed bartik.theme. [PAGE-VS-HTML]
70. Changed seven.theme. [PAGE-VS-HTML]

"main content" controllers (+ actually remove HtmlFragment & HtmlPage)

HEAD has controller.page, controller.ajax and controller.dialog. All three are responsible for rendering "main content" (as received from _content controllers) in the respective formats they support: the first is for the html format, the second for drupal_ajax and the third for both drupal_dialog and drupal_modal. That's not very consistent. That's definitely not very discoverable. On the surface, it looks like any other controller, just… a little bit more special.

To clarify that, I introduced MainContentControllerInterface and made each of them implement that. And in fact, I noticed that they all had one method that was identical across all of them (getContentResult()). And after updating each to use the newly introduced interface, even more similarities appeared. So I introduced a common base class: MainContentControllerBase.

The old controllers were actually implicitly coupled to ContentControllerSubscriber, but only by convention, not by any clearer structure. They were literally hardcoded.
To improve that situation — because it's possible somebody might want to add a new format to render the main content into, but also for increased understandability — I've opted to introduce a main_content_controllers_controller service tag, along with a format attribute. This allowed me to add a MainContentControllerPass compiler pass that creates a key-value map of formats to corresponding controllers (stored in the %main_content_controllers% container parameter), and have ContentControllerSubscriber use that instead of a hardcoded list.

End result:

• common interface (MainContentControllerInterface) and base class (MainContentControllerBase), which makes the various "main content" controllers much easier to understand
• renamed services:
  • controller.page -> main_content_controller.html
  • controller.ajax -> main_content_controller.ajax
  • controller.dialog -> main_content_controller.dialog + main_content_controller.modal
• combined logic:
  • \Drupal\Core\Ajax\AjaxResponseRenderer + \Drupal\Core\Controller\AjaxController = \Drupal\Core\Controller\AjaxController (main_content_controller.ajax)
  • \Drupal\Core\Controller\HtmlPageController + \Drupal\Core\EventSubscriber\HtmlViewSubscriber + \Drupal\Core\Page\RenderHtmlRenderer + \Drupal\Core\Page\DefaultHtmlFragmentRenderer + \Drupal\Core\Page\DefaultHtmlPageRenderer = \Drupal\Core\Controller\HtmlController (main_content_controller.html)
• Obsolete logic: the HtmlFragment, HtmlPage and related classes (like HeadElement etc.)
• a main_content_controller service tag plus a format attribute, which then allows ContentControllerSubscriber to use the services tagged as such when determining the controller to use
• Complete documentation for ContentControllerSubscriber that allows you to understand the system, rather than a very vague single line which doesn't help you to understand the system at all.
• Strict checking and throwing of helpful exceptions throughout, for a better DX. Hence strings are no longer valid "main content" — only render arrays are!

This fixes:

• #2334229: Move controller render array -> HtmlFragment conversion to a View listener; eliminate HtmlPageController
• #2092647: Consolidate _form, _controller, _entity_form, etc. into just _controller?

This at least somewhat helps:

• #1505080: [META] Content Negotiation for Accept Headers/#2026431: Make ContentNegotiation a "internal" service, used only by the router, so that core or contrib can implement real negotiation

Page variants: now a first-class citizen

This builds upon [MAIN-CONTENT-CONTROLLERS].

Page variants are about rendering the main content in a certain way. Without Block module, a page will just show the main content, and that's it. With Block module, the main content will be decorated by blocks, and the main content itself will also be rendered in a block (hence allowing you to put the main content anywhere you want).

Page variants are the abstraction necessary to let Page Manager, Panels, and so on be cleanly implemented in Drupal 8 contrib.

Conceptually speaking, page display variants or page variants in short, are the successors to hook_page_build() as they worked in Drupal 7. For lack of a better word, hook_page_build() was already castrated in #2350949: Add hook_page_attachments(_alter)() and deprecate hook_page_build/alter(), because it allowed any module to make any kind of manipulation. This caused a lot of problems in Drupal 7.

Page variants don't suffer from that problem: only a single page variant can be selected, at which point the selected page variant and only that one can determine what the body content will be (how to fill the regions inside page.html.twig).

Clearly, page variants only apply when rendering HTML (i.e. when the request format is html). On top of that, it's only applicable when rendering "main content" (a _content controller).
Combine the two and the only logical place for this to live is in \Drupal\Core\Controller\HtmlController. There, a SystemEvents::SELECT_PAGE_DISPLAY_VARIANT event is dispatched that allows any event subscriber (and hence any module) to select the page variant to be used for the current route.

\Drupal\Core\Controller\HtmlController then uses the selected page display variant plugin to build the page to be rendered.

Display variants are a generic, abstract concept: they're intended to represent different variations of displaying things. For displaying variations of pages, which always have some "main content", it needs to be possible to somehow send that main content to the variant.
In HEAD, that happens through a global static: drupal_set_page_content(). We don't want that to continue (especially because this was nigh impossible to understand), and therefore we want to be able to send the main content to a variant directly. For that purpose, I introduced \Drupal\Core\Display\PageVariantInterface, which subclasses the generic variant interface. Its sole addition: a setMainContent() method.

Now the code invoking a page display variant can send the main content and let the variant deal with it further!

In HEAD, we already have the SystemMainBlock, which is rendering the main content. Until now, that was using drupal_set_page_content(), i.e. that global static again. The Block module's page display variant already receives the main content cleanly (as per the above), so now we need to pass it cleanly to the block(s) rendering the main content as well. For that, a similar interface was introduced: \Drupal\Core\Block\MainContentBlockPluginInterface, a subclass of BlockPluginInterface, with again a sole addition: a setMainContent() method.

So now HtmlController can always call setMainContent() on the page display variant (since page display variants must implement PageVariantInterface), which then in turn has the responsibility of rendering the main content, and in the case of blocks, we detect which block renders the main content by checking if it implements MainContentBlockPluginInterface, and if it does, we again call setMainContent(). Et voilà!

This fixes:

• #1594870: Decide what KernelEvents::VIEW subscribers should be used for, document it somewhere, and make ViewSubscriber comply (open since May 2012!)

Related:

• #2292733: [meta] Introduce Display objects
• #2305199: Create a route for every variant and use a "route filter" to apply selection rules

HTML rendering: consolidated the many HTML renderers

Removed html_view_subscriber (\Drupal\Core\EventSubscriber\HtmlViewSubscriber), render_html_renderer (\Drupal\Core\Page\RenderHtmlRenderer), html_fragment_renderer (\Drupal\Core\Page\DefaultHtmlFragmentRenderer) and html_page_renderer (\Drupal\Core\Page\DefaultHtmlPageRenderer).

Instead, there now are only \Drupal\Core\Controller\HtmlController (used for _content controllers) and \Drupal\Core\Render\BareHtmlPageRenderer (used for "one-off pages").

Rather than using DefaultHtmlPageRenderer::renderPage(), which was a static "temporary shim" method for the one-off pages where it's impossible to use a "main content controller", such as the installer or error pages, we now have bare_html_page_renderer (BareHtmlPageRenderer), where the "bare" indicates that the hook_page_(attachments|attachments_alter|top|bottom)() aren't invoked, nor is a page display variant selected (to allow e.g. blocks to appear).

This fixes:

• #2295609: Break the dependency of MaintenanceModeSubscriber on DefaultHtmlPageRenderer
• #2295367: BatchController should not rely on DefaultHtmlFragmentRenderer directly

drupal_render(): more strict, better DX

Any remaining drupal_render() calls that were called with $is_recursive = FALSE, where they really shouldn't have been, have been fixed.

While fixing some test failures, I then noticed that #post_render_cache callbacks and #attached assets were no longer being applied for the main content. Turns out the root cause was once again "wrong" drupal_render() calls; these caused the stack (that is used internally, see #2273277: Figure out a solution for the problematic interaction between the render system and the theme system when using #pre_render) not to be empty at the end of a "root" (non-recursive) drupal_render() call, which effectively means that some of the bubbleable metadata is being left behind in the stack, and is missing from the final result for the "root" call.

I first embarked on a mission to fix all drupal_render() calls to have $is_recursive = FALSE… but since this is the majority of the calls, it really is:

1. A fool's errand
2. Bad DX, because it means 99% of the time a Drupal developer uses drupal_render() without specifying $is_recursive = FALSE, that that is wrong.

So instead of "fixing" all existing drupal_render() calls, I instead fixed drupal_render() itself: the signature changed from

function drupal_render(&$elements, $is_recursive_call = FALSE) {}

to

function drupal_render(&$elements, $is_root_call = FALSE) {}

i.e. I inverted the second argument's meaning. Now the default is once again drupal_render($build), and only when rendering the final markup (i.e. just before generating a Response). The dozen or so cases in Drupal core that need to do this were updated to do this.

To prevent this problem in the future, even though the above already diminished the chance of this happening again greatly, drupal_render() now throws an exception whenever a root call to it does not end with an empty stack.

(Files with $is_root_call = TRUE calls for drupal_render(): whenever rendering the "final" markup, which happens in authorize.php, update.php, install.php, exception handling, maintenance mode and when rendering render arrays for non-HTML responses, such as feeds.)

As part of this work, it turns out we fixed:

• #2238835: #post_render_cache + #attached

Default 403/404

Changed \Drupal\Core\EventSubscriber\DefaultExceptionHtmlSubscriber now works like \Drupal\Core\EventSubscriber\CustomExceptionHtmlSubscriber already does in HEAD: rather than hardcoding responses, it now performs a sub-request to newly added system.403 and system.404 routes.

Hence \Drupal\Core\EventSubscriber\CustomExceptionHtmlSubscriber now is almost empty; it only needs to subclass \Drupal\Core\EventSubscriber\DefaultExceptionHtmlSubscriber.

This brings more consistency to both exception handling and HTML page rendering: that then leaves only a handful of routes/responses operating in limited environments (the complete list: install.php/update.php/authorize.php/PHP code exceptions (DefaultExceptionSubscriber)/maintenance mode (MaintenanceModeSubscriber)), which is conceptually simpler to understand.

This ties back to [HTML-RENDERING].

Letting main content set the status header

In all of Drupal core, there's only a single case where the main content (_content) wants to set the HTTP Status header (Status: 200 by default).

In HEAD, Views' \Drupal\views\EventSubscriber\RouteSubscriber has an event listener for KernelEvents::VIEW that runs very late, to modify the HtmlPage object's HTTP status code just before it's converted into a response. This is a global in disguise.
But to determine which status to set, some other Views code sets a request attribute (another global in disguise).

So not only does HEAD actually use two globals, this issue of course aims to remove HtmlPage, so this pattern cannot continue.

Drupal already uses ['#attached']['http_header'], which maps to _drupal_add_http_header(), which is retrieved via drupal_get_http_header() (and yes, this is also a global, but it's not a new one), and \Drupal\Core\EventSubscriber\FinishResponseSubscriber applies those headers to the Symfony Response object. The only tricky bit is that Symfony apparently special-cases the Status header, so a tiny addition was made:

	$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);
  }

Hence now the main content can simply do e.g.:

$build['#attached']['http_header'][] =  ['Status', 404];

to render content, but mark it as a 404 to prevent search engines from indexing the response.

'page' vs. 'html'

Part one: the templates

page.html.twig has variables for every region of this form: page.REGION_NAME, i.e.: page.header, page.help, page.content, page.sidebar_first, and so on.

Confusingly, html.html.twig has very similar variables: page.head, page.styles, page.scripts, page.content, but then also page_top and page_bottom (note the underscore instead of period). This is an artefact that AFAICT has two causes:

1. Historically, the boundaries between "the page template" and "the html template" have been rather vague.
2. The HtmlPage object enforced this, because page.scripts automatically mapped to $page->getScripts(), page.content to $page->getContent(), and so on.

So, page.SOMETHING in page.html.twig is operating on a very different "page" than html.html.twig. The latter is operating on the HtmlPage object, the former is operating on a $page render array.
But it seems that html.html.twig is — somehow — rendering whatever is in the $page render array again!

To be clear, the relation of page.html.twig to html.html.twig is that page.html.twig maps exactly to html.html.twig's page.content variable. That's right — if you're confused, you're not alone. It took me quite some frustration to figure that out.

That's why I changed html.html.twig from:

<body{{ attributes }}>
  <a href="#main-content" class="visually-hidden focusable skip-link">
    {{ 'Skip to main content'|t }}
  </a>
  {{ page_top }}
  {{ page.content }}
  {{ page_bottom }}
</body>

to:

<body{{ attributes }}>
  <a href="#main-content" class="visually-hidden focusable skip-link">
    {{ 'Skip to main content'|t }}
  </a>
  {{ html.page_top }}
  {{ html.page }}
  {{ html.page_bottom }}
</body>

This makes things much clearer IMO:

• html.page == the rendered page.html.twig
• html.page_top == the results of hook_page_top()
• html.page_bottom == the results of hook_page_bottom()

This also makes it super clear where the results of hook_page_top()/hook_page_bottom() will end up, and it makes it super explicit that these are NOT page regions.

Part two: consequences of removing HtmlPage for 'page' template preprocessing

Going back to $variables['html_attributes'] and $variables['attributes'] in hook_preprocess_html(), rather than

	/** @var $page \Drupal\Core\Page\HtmlPage */
	$page = $variables['page_object'];
	$variables['attributes'] = $page->getBodyAttributes();
	$body_classes = $variables['attributes']['class'];

It's also no longer possible to specify attributes for #type => 'page' (and hence also not in hook_preprocess_page()) which then automatically become attributes on the <body> tag. The <body> tag is not part of #type => 'page'/page.html.twig and hence it should not be possible to set <body> attributes from there.
This only worked thanks to hacks in the old system. If you want to set <body> attributes, implement hook_preprocess_html() and set $variables['attributes'].

Hence this fixes #2218271: Theme preprocess functions have to futz with $page_object = $variables['page']['#page'] in weird ways.

Attachment handling

Only a single call to drupal_process_attached() remaining when rendering HTML pages (we don't render HTML pages when rendering something for e.g. AJAX responses, so those still have their own calls).
And best of all: it's called in the same place and just before calling drupal_get_(css|js)()! This means that the putting of data into a global static is now in the same place as where we're gathering the data from a global static, hence it will be finally possible with a simple follow-up patch to get rid of those global statics!

In template_preprocess_html():

  drupal_process_attached($all_attached);                        
  
  $variables['html']['styles'] = drupal_get_css();
  $variables['html']['scripts'] = drupal_get_js();
  $variables['html']['scripts_bottom'] = drupal_get_js('footer');
  $variables['html']['head'] = drupal_get_html_head(FALSE);

I'm sorry but please split things up into reviewable and discussed pieces. No wonder you did not wanted help, one patch to rule them all IS NOT the fastest way, especially not when it comes down to spreading the knowledge around.

we should split things up into little manageable patches.

Of course. I agree. I'll repeat what I already said:

I felt it was very important to formulate a complete, cohesive answer, because that's a big part of the current entangledness in HEAD: many things that make sense in isolation, but not together.

This is a ~300 KB patch. Unless people think otherwise, I will begin splitting off parts of it that can be split off into child issues, to simplify the review process.

The sections below the "API changes" section will list changes per topic. Some of those topics can be moved into child issues. Many of them are too intertwined, though. But the per-topic descriptions of what's changed should help you understand this huge patch.

One thing to rule them all is not necesarrily a good architecture. Are we really sure we want to have one really big object?

I understand it's a lot of digest, but please first look at a patch before making claims about it. This is wrong.

HtmlController (281 LoC) is only a bit bigger than just the HtmlPage value object on its own (234 LoC).

I say "A + B + C + D + … = the new thing", but what that really means is bits of B and C and all the rest no longer being necessary at all. By removing the intermediate value objects, and just use drupal_render(), we don't have to convert from one intermediate form to another to yet another anymore.

First general feedback: I think it is wrong that HtmlController::handle does all that ... Didn't we talked about converting stuff on kernel::view
and not directly as part of the _controller callback? ... I still consider it as a requirement for D9 to get rid of _content, _form and what not. Your current architecture does not really allows that.

Well, I kept ContentControllerSubscriber as-is mostly. If other people agree with you on that, I think we could make ContentControllerSubscriber react to KernelEvents::VIEW instead, detect if it's a render array, and if so, apply pretty much exactly the same logic there.

I mentioned #1594870-24: Decide what KernelEvents::VIEW subscribers should be used for, document it somewhere, and make ViewSubscriber comply in my comment, which I recommend you to read. It provides a rationale — written 1.5 years ago — for precisely the way it's implemented right now: render arrays (i.e. rendered content) should use _content routes, which are enhanced to _controller routes. Whereas data (and corresponding domain objects) would indeed use KernelEvents::VIEW to render that data in a certain way.

But as I said, if consensus says we should always use KernelEvents::VIEW, then that's fine, and I'll change the patch to use that instead.

Here are the first child issues:

1. For the drupal_render() topic in #14: #2361681: drupal_render(): invert second argument ($is_recursive_call -> $is_root_call) => more strict, better DX/TX
2. For the HTTP status topic in #14: #2239003-18: Remove the '_http_statuscode' request attribute
3. #2361693: AjaxEnhancer is dead code, remove it

#21 & #22: Thank you :)

I spent several hours today on creating that diagram. It's very much intended to become part of the official documentation! It's the thing I (always) wish(ed) already existed. Hence I've already included a SVG as well. If the diagram needs to be updated because we decide to change some things here, then I'll update it. Hence I've also provided the original format (in OmniGraffle), so that anybody can get the original (from which I generated the PDF, PNG and SVG).

#24 & #25: lovely! :)

#26: :) I welcome your interference!

I'm replying mainly with an updated flowchart. See, the thing is that this is not specific to building a HTML response; the controllers building AJAX/Drupal Dialog/Drupal Modal Dialog responses go through the exact same three steps.

HTML

In the case of rendering HTML, it just happens to be the case that "the main content" from a site builder/developer's POV is what ends up in the main content block. But from an HTML document's POV, "the main content" is not just that main content block, but the <body>, which is represented by page.html.twig. Hence the HTML controller (HtmlController) "prepares" the main content it received by transforming it into #type => 'page'. The final rendering of the HTML document requires #type => 'page' to be wrapped in #type => 'html', since #type => 'html' is how Drupal renders (using drupal_render()) an HTML document: by filling the placeholders in html.html.twig.

AJAX

In the case of the AJAX controller (AjaxController), no preparation is necessary. prepareMainContent() is basically a no-op, we go immediately to renderMainContent().

Drupal Dialog/Modal Dialog

And here (DialogController & ModalController) we again have a preparation step.

But, the dialog should show this much more clearly than I can put into words. I'm only putting it in the IS, not here, because otherwise this issue page will become super slow to load.

#28: All of that makes a lot of sense.

// Get the "main content" (_content).
$main_content = $this->getMainContent($request);
// From that, generate the actual "content".
$content = $this->prepareContent($main_content);
// Render that actual content into a Response.
// OK, that last method name might be a bit far-fetched - yet it's
// fairly accurate...
return $this->renderContentIntoResponse($content);

I'm totally fine with that :)

Thanks for interfering :D

#2357937: Remove {{ feed_icons }} from page template (page.html.twig), #2361681: drupal_render(): invert second argument ($is_recursive_call -> $is_root_call) => more strict, better DX/TX and #2239003: Remove the '_http_statuscode' request attribute are RTBC. The 4th child (#2361693: AjaxEnhancer is dead code, remove it) was trivial and is already committed.

Once either of those children are committed, I'll reroll this patch.

The next child issue, just created it: #2362517: Improve default 403/404 exception HTML subscriber: don't duplicate the page render pipeline, use the routing system — add system.403 and system.404 routes.

#2239003: Remove the '_http_statuscode' request attribute, #2357937: Remove {{ feed_icons }} from page template (page.html.twig) and #2361681: drupal_render(): invert second argument ($is_recursive_call -> $is_root_call) => more strict, better DX/TX got committed, and #2362517: Improve default 403/404 exception HTML subscriber: don't duplicate the page render pipeline, use the routing system — add system.403 and system.404 routes is now also RTBC. We're making good progress :)

Next child issue: #2363025: Push usage of drupal_set_page_content() higher: out of blocks and page display variants.

Currently rerolling this patch now that three (small) patches have been committed, and incorporating #29.

The rerolled patch goes from 293 KB to 244 KB :) Hurray! First, here's a straight reroll. I'm providing only an interdiff for the things that weren't fixed during the merge process. The interdiff shows a crucial part of HtmlController getting a much, much clearer comment.

Also attached: a review-only (do-not-test) patch, which was created with the -D flag to list file deletions, but not list the contents. For some reason, testbot can't deal with this, hence marked as "do-not-test". That should make reviews already somewhat feasible, but most people probably want to wait until more child issues have been committed.

In my next comment:

• reroll that applies #29
• …accompanied by an updated diagram

And here it is, #29 implemented.

Another child issue added: #2363139: _content controllers may only return render arrays, not strings.

An excellent question.

This is one of the things I'm not entirely sure about what's the best way — what matters most is that we have formalized steps to take "the main content" and turn into a Response for the negotiated format (Content-Type). It's one of the things that needs to be discussed here :)

A few thoughts:

• We want to automate those 3 steps, otherwise all "main content controllers" will start to look vastly different again, whereas we want the same pattern for all of them. That's why it's currently in MainContentControllerBase.
• Perhaps we could let ContentControllerSubscriber::onRequestDerivceController() just generate a closure that does these three steps? Then the interface really is just about formalizing those 3 steps. But then the resulting code aren't really controllers.
• OTOH, it may be nice to keep the door open for other "main content controllers" than the ones we have in core (html/ajax/drupal_dialog/drupal_modal) to still have the flexibility to use a fourth step of their own, in which case they'd need to be able to override ::handle().

Hopefully that made sense. I hope you have some good ideas on how to combine those needs in a better way! :)

Another child issue added: #2364127: Merge AjaxResponseRenderer into AjaxController. As you can probably already tell by its title, it's getting more difficult to split this patch up further. I have ideas for 1 or 2 more child issues, at least one of which will be more clear than #2364127.

Rerolling this patch, because #2362517: Improve default 403/404 exception HTML subscriber: don't duplicate the page render pipeline, use the routing system — add system.403 and system.404 routes got committed today!

Another 16 KB of changes removed from this patch, thanks to #2362517: Improve default 403/404 exception HTML subscriber: don't duplicate the page render pipeline, use the routing system — add system.403 and system.404 routes having landed.

And another child: #2364189: One service per format + no hardcoded format to service mapping, but tagged services.

There's only one more thing I can split off into a separate issue, but that's blocked on #2363025: Push usage of drupal_set_page_content() higher: out of blocks and page display variants.

If all four uncommitted patches would be committed, then we'd already be down to a ~110 KB patch to be reviewed. Together with that final child issue that I just mentioned, it should drop below the 100 KB barrier. Then this should be a fairly reviewable, well-focused patch.

#2363139: _content controllers may only return render arrays, not strings landed; straight reroll.

Wow. Awesome! Thanks for your wonderful, clarity-bringing comment :) I agree with your comment, as do dawehner and Fabianx (I talked to them in IRC).

The only thing that that doesn't handle, is when you want to override the handle() method for a certain format. But that should be extremely rare anyway, and can be addressed by subclassing ContentControllerSubscriber, only making it work for your format, and then overriding handle().

I'm currently implementing this. I've got the biggest changes already implemented (including running at ::VIEW instead of at ::REQUEST), now just doing the renames. Now that the code is running at ::VIEW, the change from "controller" to "renderer" in your suggested naming scheme also makes a ton of sense. And with all this done, #2092647: Consolidate _form, _controller, _entity_form, etc. into just _controller? becomes trivial! :)

In that area (naming scheme), I'm not entirely convinced about

Move MainContentControllerInterface to Drupal\Core\Render\Response\ResponseRendererInterface and (Html|Ajax|Dialog|Modal)Controller to Drupal\Core\Render\Response\*ResponseRenderer.

The reason being: this name suggests it's something strongly related to AjaxResponse, but it's not; it's completely independent.

The part that's missing in your suggested naming scheme for me — on second thought, because I initially was very happy with it — is the "main content" part.

What about MainContentRendererInterface and \Drupal\Core\MainContent\(Html|Ajax|Dialog|Modal)Renderer? I think that more accurately captures what this is about. The service tag would then become main_content_renderer.

RE: additional suggestion: I totally agree it's ugly. But '#type' => 'dialog' is not an option; we don't render the entire dialog using drupal_render(), we render it into an AJAX command. Introducing '#type' => 'dialog' would therefore be misleading.
We could just piggyback everything onto the main content render array, since that piggy backing is very much self-contained. It's choosing between piggybacking (which is ugly) and an ugly triple of return values (which is also ugly). The latter is more explicit though, so that's why I went with that.
If people prefer the piggybacking, then I'll update the patch to do that instead.

I meant to say \Drupal\Core\Render\MainContent\*.

I had a long but productive call with effulgentsia to resolve #45/#46 — we agreed on what I proposed in #46, but with the service tag being namespaced, so: render.main_content_renderer.

We also agreed that it makes sense to convert from what all preceding patches did (generating a Response from the _controller set during ::REQUEST) to letting _content controllers return render arrays and then during ::VIEW converting them into a Response. dawehner has pointed out repeatedly that this is how it was intended to work, that that was said during DC Amsterdam (which I forgot — my apologies!), and I realized that that is in fact how HEAD already works. So it's not an actual change relative to HEAD.

I'm currently still making the necessary changes, but that will take a while longer; especially because I will also need to update the diagram and #2363025: Push usage of drupal_set_page_content() higher: out of blocks and page display variants has landed in the mean time.

I will probably not be able to still post a patch today, but definitely expect one tomorrow! :)

Does "Content" in ContentControllerSubscriber correspond to "MainContent" in Drupal\Core\Render\MainContent\*Renderer ?

Yes. This has bothered me also. IMO: rename ContentControllerSubscriber to MainContentRendererSubscriber. dawehner, Fabianx, effulgentsia: what do you think?

+1 for MainContentViewSubscriber.

I think a value object with such a limited lifetime a waste of CPU cycles and not very helpful at all, since only <1% of developers will ever write their own main content renderer. The code literally looks like this without the value object:

    list($content, $title, $custom) = $renderer->prepare($main_content, $request, $route_match);
    return $renderer->renderResponse($content, $title, $custom);

Introducing a value object in between there does not make sense to me. Especially not if we're going to add methods for each property. If we'd be using value objects like C-structs, then I'd understand it more. But in Drupal 8, we prefer to always have methods for some reason.
Hence We'd change that code to this:

    $prepared_content = $renderer->prepare($main_content, $request, $route_match);
    return $renderer->renderResponse($prepared_content->getContent(), $prepared_content->getTitle(), $prepared_content->getOptions());

And if passing in $prepared_content wholesale, then we'd still have to do that unpacking in the main content renderer. I just don't see the value.

Actually, with all those changes, we're down from 4 methods (getMainContent(), prepareContent(), renderContentIntoResponse() and handle() to use the first 3) to 2 (prepare(), renderResponse()). And prepare() is arguably mostly/only there for the html case. The ajax case doesn't need it at all. The dialog and modal cases use it, but could easily live without it. So perhaps it's better to drop prepare() and keep only renderResponse(). The html case (HtmlRenderer) can just have a protected prepare() method. KISS.

Talked to dawehner about this in IRC, he agreed. That makes MainContentRendererInterface very clear, very focused, with only:

interface MainContentRendererInterface  {

  /**
   * Renders the main content render array into a response.
   *
   * @param array $main_content
   *   The render array representing the main content.
   * @param \Symfony\Component\HttpFoundation\Request $request
   *   The request object, for context.
   * @param \Drupal\Core\Routing\RouteMatchInterface $route_match
   *   The route match, for context.
   *
   * @return \Symfony\Component\HttpFoundation\Response
   *   The Response in the format that this implementation supports.
   */
  public function renderResponse(array $main_content, Request $request, RouteMatchInterface $route_match);

}

And it lets us simply avoid the whole PreparedContent discussion. :)

#2364127: Merge AjaxResponseRenderer into AjaxController apparently landed half an hour ago. Together with #2363025: Push usage of drupal_set_page_content() higher: out of blocks and page display variants having landed a few days ago, that should bring this patch down to 175 KB total, 120 KB to be reviewed (i.e. with git diff -D).

That leaves just #2364189: One service per format + no hardcoded format to service mapping, but tagged services, but that's very closely related to the discussion we're now having here (which is why it's marked as postponed: I postponed it yesterday precisely because we're having this discussion here).

I think it might be wiser to do the remainder of the work in this issue, because everything that remains is truly focused on the render pipeline, without tangents. We'll see what works best. But since we're already having this discussion here, that just feels more appropriate for me now.

Addressed everything from #36–#58 as well as #15 — thanks for this excellent discussion, all! :)

This now really paves the way for #2092647: Consolidate _form, _controller, _entity_form, etc. into just _controller?. (Because the main content renderers now work during KernelEvents::VIEW rather than as a _controller during KernelEvents::REQUEST.)

The interdiff is quite large, because there's lots of moving around of things. It also means that the patch size hasn't decreased as much as predicted with those two additional child issues having landed.

Highlights:

1. MainContentControllerBase is gone.
2. ContentControllerSubscriber was kept, but only for the KernelEvents::REQUEST events of 1) negotiating a format (which will go away once #2331919: Implement StackNegotiation lands) and 2) simply assigning _content to _controller unconditionally (which will go away once #2092647: Consolidate _form, _controller, _entity_form, etc. into just _controller? lands). This allows MainContentViewSubscriber to stay true to its name: be a KernelEvents::VIEW subscriber, and nothing else. It also means we'll be able to simply delete ContentControllerSubscriber once we've cleaned up our cruft.
3. These changes have unveiled a subtle but important bug in HEAD: when making a REST request (application/hal+json) to a route that either A) doesn't have a REST module route, B) doesn't have supported_formats set or C) doesn't have support_auth set, all tests in HEAD expect a 404. But this is wrong; in all cases in HEAD, there is the canonical (non-REST module) route, which doesn't specify any formats! Hence AcceptHeaderMatcher actually considers them a (low-quality/non-preferred) match! With this interdiff, when REST module is not set up (or incorrectly) for an entity, MainContentViewSubscriber will correctly get to handle the request, but will be forced to bail with a 406 since it doesn't know how to render a render array into a HAL+JSON response.

Once again, the diagram has been updated. However, since we're now effectively depending on two rather than one Symfony events, I felt it became more important to show the timeline of the different Symfony events. More generally, I felt it was important to visualize which bits of request handling and rendering are pure Symfony and which bits are Drupally. For the Symfony part, I'm leaning heavily on http://symfony.com/doc/current/components/http_kernel/introduction.html, and using exactly those headings to indicate the different steps of HttpKernel::handle()
In doing so, I also realized that it would be valuable to truly capture the entire request handling flow. Hence I included index.php as well. So the diagram now shows index.php, Symfony's HttpKernel, the controller in Drupal, MainContentViewSubscriber and the various main content renderers. I hope you like it :) (And if you have ideas on how to make the diagram significantly clearer, let me know!)

An unfortunate side-effect is that the diagram has gotten quite a bit bigger (30% wider, 10% taller). When printed on A4 paper with a standard (non-marginless) printer, you can still read it just fine, albeit the font size is very small.

#60: Glad you like it! :) And absolutely, I will write a blog post about this!

Added a new child issue: #2366147: Introduce a page display selection event , which I can do now that #2363025: Push usage of drupal_set_page_content() higher: out of blocks and page display variants has landed. That should take another 20KB out of this patch.

#62

It was related to drupal_render() putting the metadata for assets in global statics by calling drupal_process_attached(). As of #2273277: Figure out a solution for the problematic interaction between the render system and the theme system when using #pre_render, drupal_render() doesn't call drupal_process_attached() anymore. It just bubbles up the attached assets from children up to the root of the render array. The root of the render array now contains what those global statics used to contain. So now that complexity/cruft is gone, and we don't need to deal with it anymore.

The code cited in #62 omits the few lines that come before it, and that helps explain why the code works this way:

// Collect all attachments. This must happen in the preprocess function for
// #type => html, to ensure that attachments added in #pre_render callbacks
// for #type => html are included.
$attached = $variables['html']['#attached'];
$attached = drupal_merge_attached($attached, $variables['html']['page']['#attached']);
if (isset($variables['html']['page_top'])) {
  $attached = drupal_merge_attached($attached, $variables['html']['page_top']['#attached']);
}
if (isset($variables['html']['page_bottom'])) {
  $attached = drupal_merge_attached($attached, $variables['html']['page_bottom']['#attached']);
}

// Render the attachments into HTML markup to be used directly in the template
// for #type => html: html.html.twig.
$all_attached = ['#attached' => $attached];
drupal_process_attached($all_attached);

$variables['html']['styles'] = drupal_get_css();
$variables['html']['scripts'] = drupal_get_js();
$variables['html']['scripts_bottom'] = drupal_get_js('footer');
$variables['html']['head'] = drupal_get_html_head(FALSE);

We basically have all the required attachments in #attached, then put them in a global statics because that's what we have in HEAD, and then immediately pull them out of the statics. Because the code is already next to each other, it will now be trivial to finally get rid of the global statics :)

Also see the "attachment handling" section in #14.

You remove the ability to add/change css/js via preprocess_html() this way.

This is true. But this is by design.
The html.html.twig template is responsible for rendering the final HTML document. Including <HEAD>. Hence also including <SCRIPT> tags to load JS assets, analogously for loading CSS assets. Therefore, if we're going to allow HOOK_preprocess_html() to attach additional assets… how are we ever supposed to be able to render the HTML to load JS assets etc (<SCRIPT> tags etc)?
This is essentially a slight change back to how it worked before: you cannot attach things to html.html.twig, but you can attach them to page.html.twig (just like before). This also makes conceptual sense: you can attach assets to the page (page.html.twig ~= <BODY>), which are then bubbled up to the level of the HTML document (html.html.twig).
And finally, this is also consistent with the concept of drupal_render(), which is rendering recursively. This patch makes '#type' => 'html' a render element like any other again, no more special casing with a special pre-built HtmlPage object being inserted and the nasty TX consequences of that. Basically, we need to populate {{ html.scripts }} in html.html.twig. The way Drupal/drupal_render() allows you to populate that variable, is through the preprocess function associated with that template. Hence template_preprocess_html() MUST be able to calculate $html['scripts']. Hence by that time, all attachments be known. Hence, we cannot allow additional assets to be added in hook_preprocess_html(). Q.E.D.

One last thing: if a theme wants to attach assets to all pages, a theme should just specify assets to be loaded. If a module or theme wants to attach assets conditionally, use hook_page_attachments()/hook_page_attachments_alter(). There really is no need for hook_preprocess_html() AFAICT.

I hope that made sense (and that I didn't make any mistakes, all that was from memory).

#63

WOW!!! Thank you for such a complete review! I will probably not be able to finish addressing those remarks today, but I'm so looking forward to addressing your feedback :) Thanks!

Diagram updated based on feedback from msonnabaum in chat. Now consumes less vertical space, by reducing the height of events 2/3/4 in HttpKernel::handle(), since they're not important to understand the whole, only step 5 is (and step 2 is even confusing, since it might be mistaken for "apply routing to determine which controller to use"). A few arrows are made clearer.

RE: nitpick about page display variant selection event being an event: The first one who says yes, wins. doesn't work because SimplePageVariant by definition must alway say yes because it's the fallback. BlockPageVariant will also always say yes. It'll be up to Page Manager/Panels/… to have whatever precise/subtle conditions they want to use to say "yes", hence events feel more appropriate.

1. :)
2. Do you mean aliasing $variables['html']['page']['#title'] to $variables['title'], so that people can use {{ title }} in html.html.twig to print the plain title? Not yet added here, but +1 to that.
3. ABSOLUTELY! That's why I'm already proving a SVG version, because I'm hoping that's acceptable to be committed. OTOH, I fear people will just say "upload it to the handbook pages", which is a possibility, but then makes it too easy for the logic to change and the diagram to go outdated. So I'm really hoping we can commit a SVG to Drupal core… but not sure if that will fly.
4. Clarified docs.
5. Excellent question.
   Unfortunately not AFAICT. A render array doesn't have to have a #type. A render array might also only contain render child arrays, e.g. ['a' => ['#type' => 'something'], 'b' => ['#type' => 'something else']] — which means that we'd have to recurse into the array to find any render array-like characteristics. Finally, the empty array also is a valid render array.
   This is unfortunate, but it's not that important in the grand scheme of things. I think it makes more sense to wrap arrays of domain object in a meaningful value object than it makes sense to wrap render arrays in a value object. Because then you can indicate for those domain objects what type of collection it is: an ordered list, unordered list, set, stack, linked list, and whatnot, which then allows multiple view subscribers to render that same collection in very different, more meaningful ways. Render arrays are what they are, but for domain objects, we can store them in a richer, more meaningful collection than "PHP array".
6. YAY! Fixed :)
7. Yep — fixed!
8. Indeed :) And, thanks!
9. Agreed. This "just" uses the exact same pattern as used in HtmlRenderer::renderResponse() (the current comment still refers to ::renderPage(), which is wrong). I now clarified the comment that the exact same pattern is used in that other place, with a detailed explanation. DRY: I don't want to write a near-exact duplicate comment.
10. Good point; documented.
11. Fixed.
12. I wondered the exact same thing. The reason I chose not to do this, is that it's then too easy to forget to pass in the right attributes, and end up with inconsistent/broken styling for that type of page. This is why BareHtmlPageRenderer is so nice/easy to understand (as you said in point 8). IIRC this is also why I did 26: those (install|maintenance)-background classes were added by Seven in preprocess functions because those provided by core were inconsistent/broken. This guarantees that the (install|maintenance)-page classes are always set, without fail. Hence I was also able to do the small clean-up in #26.
    If you're not convinced by this argument, and others also don't, then I'm fine with doing what you described. But I remember trying exactly that first and running into problems, or at least brittleness.
13. :)
14. Oops, absolutely. Fixed.
15. No stupidity to be seen; except on my side. I don't know what I was thinking. Fixed, after having discussed it with both dawehner and Fabianx in IRC, by doing the following renames:
    1. \Drupal\system\Event\SystemEvents -> \Drupal\Core\Render\RenderEvents
    2. \Drupal\system\Event\PageDisplayVariantSelectionEvent -> \Drupal\Core\Render\PageDisplayVariantSelectionEvent
16. It's not guaranteed that all subscribers for this event need the route match. That could be a future API addition. But I think you're right, e.g. Page Manager/Panels would also override on a per-route basis I think… so: done!
17. Ohh! :) There might be more use cases though, like a UI showing all the available formats, or demonstrating the various different formats next to each other, or …
18. hook_page_top() maps to {{ html.page_top }}. Page display variants map to {{ html.page }}. Therefore page display variants can't set things in {{ html.page_top }} or {{ html.page_bottom }}. This is not new, this is already the case in HEAD. But with hook_page_build(), you were able to treat them as the same, even though they are treated very differently by Drupal. page_top and page_bottom are NOT actual regions! (This is also why they don't show up in the Block UI.)
19. Well, it makes the names of the page display variants logical and consistent, which helps with understandability. I actually opened a child issue for this stuff, but now that you've reviewed it here, I'll either need to reroll that, or we just close that in favor of this. (See #2366147: Introduce a page display selection event .)
20. :)
21. You're right again, of course — moved to \Drupal\Core\Render\Plugin\DisplayVariant\SimplePageVariant.
22. Discussed in chat with dawehner; kept public, no longer static, marked as @internal. Now using an injected module handler rather than resorting to \Drupal::moduleHandler().
23. :)
24. Agreed, I want to refer to it — and preferably embed it — in this documentation… but how? See earlier.
25. Glad you like this!
26. Answered in point 12.

Another diagram update. This time mostly based on feedback from effulgentsia, but also some from Gábor Hojtsy.

He roughly made two suggestions:

1. it'd be clearer if the "Event handling" column was between the Symfony and Drupal columns, because the arrows would have to go less far. Plus, it makes conceptual sense, because Symfony fires events, to which Drupal reacts. Events are the glue between Symfony and Drupal.
2. Most developers will care in particular how to get their stuff rendered by Drupal. Hence we must make it super clear which paths the different types of return values from their controllers will follow throughout the diagram. But in doing so, we must particularly stress the typical flow (that for render arrays). The best solution I could come up was a set of colored, dashed overlay lines that show the different paths.

#68:

1. Do you mean different diagrams, one per workflow? As in, 3 diagrams, one for each of the three colors in the current diagram? If so, that can easily be done, by copy/pasting the OmniGraffle file, removing the ones you don't want and saving. I'm willing to do that once this issue is finalized :)
2. RE: needing a legend. A legend for what? For the different shapes? This is loosely based on the diagram "language" called "flowchart". I had to make my own additions/changes to that to 1) make it sufficiently visually clear on the small space it's in, 2) to succinctly describe things. It should not need a legend, IMHO.

Thanks for all the reviews!

First, a straight reroll to chase HEAD, since the following changes in HEAD broke this patch:

1. #2230121: Remove exit() from FormBuilder
2. #2367579: Move retrieval of visible blocks to a standalone service
3. #2329783: Move comment classes from preprocess to templates
4. #2366645: Drupal\Tests\Core\Controller\AjaxControllerTest has wrong @covers.

Next: addressing all the reviews. (Only setting to NR so testbot verifies it still passes.)

#70: Fixed; that was a wrong/leftover @throws.

#72: yep, agreed, thanks for the extra context :)

#73: Correct, the HtmlPage $page object was the reason for the Twig template variables being prefixed with page.. I'm fine with what you propose, with only one difference: I don't want {{ content }}, but {{ page }}, because that accurately and clearly reflects that {{ page }} corresponds to page.html.twig.

There is one subtle "regression" in your proposal: the hierarchy is less clear. For example: page.html.twig renders the regions, and therefore contains the regions, and this is reflected in the Twig variables: {{ page.content }}, {{ page.sidebar_first }}, etc.

This logic/reasoning also was present until you asked me to remove it: html.html.twig renders the page, page top and page bottom, and therefore contains them, but is now no longer reflected in the Twig variables ({{ html.page }} -> {{ page }}).

#74:

1. :)
2. Not sure what you want to see added here. The thing is that this already is a huge clean-up compared to HEAD. this doesn't change anything functionally relative to HEAD. So what "really implicit behavior" do you mean?
3. Oh boy. I understand that this is tricky to understand. But this is most definitely out of scope. The simple explanation: because that's what it looked like before #2068471: Normalize Controller/View-listener behavior with a Page object changed it. The more accurate explanation: if a (Twig) template is to be populated by a single render array, we use render_element, otherwise we use variables. The exact "how and why" I don't know, but the key information is at:
   

     // If a renderable array is passed as $variables, then set $variables to
     // the arguments expected by the theme function.
     if (isset($variables['#theme']) || isset($variables['#theme_wrappers'])) {
       …
       }
       else {
         $variables[$info['render element']] = $element;
         // Give a hint to render engines to prevent infinite recursion.
         $variables[$info['render element']]['#render_children'] = TRUE;
       }
     }
   

   and the docs for #render_children:

    *   - The main render phase to produce #children for this element takes place:
    *     - If this element has #theme defined and #theme is an implemented theme
    *       hook/suggestion then _theme() is called and must render both the element
    *       and its children. If #render_children is set, _theme() will not be
    *       called. #render_children is usually only set internally by _theme() so
    *       that we can avoid the situation where drupal_render() called from
    *       within a theme preprocess function creates an infinite loop.
   

4. Fair; removed that update to that docblock!
5. Why would we render this as JSON? We were unable to render it since it's an unsupported format… so why would rendering it in any format make sense over rendering a plain text message?
   My own answer: because then machines could be programmed to know that Drupal sites return a JSON response with the available formats/MIME types so they can repeat the request with a different Accept header, silly!
   Note: I went with the current code because that was suggested on StackOverflow (http://stackoverflow.com/questions/4422980/how-to-properly-send-406-stat...).
   Done!

#75:

1. .
2. I agree that the concept of a "bare html page" should not be connected with the maintenance or install pages directly. I just couldn't think of a cleaner way to do this earlier.

   And I still can't.

   Because those classes that you'd like to see in the template… they can't live in the template, because they don't apply to the maintenance-page.html.twig template! They are meant to become classes on the <body> tag. And the <body> tag is owned by html.html.twig, not maintenance-page.html.twig. Just like page.html.twig ends up in {{ page }} in html.html.twig, maintenance-page.html.twig and install-page.html.twig do, too.

   Hence this reroll doesn't change that at all.

   Also: this is still a big step forward in clarity/simplicity for rendering maintenance and install pages, so I don't believe we should hold this up over that.

3. You're absolutely right! That is an excellent remark. In fact, let's remove \Drupal\Core\Render\Element\Html::preRenderHtml() altogether and just move that into system_page_attachments(). That's more consistent.
4. Agreed; follow-up created, @todo removed since this is not a certainty but up for debate: #2372581: Rename 'page' render element to 'body', page.html.twig to body.html.twig.
5. Fixed. (This was because of the architecture change ignited by yched et al., the order used to make sense, but since then, it doesn't anymore. I kept the interdiff sizes to a minimum by not moving it around, but now that this patch is close to landing, it's the right time to do just that.)
6. Yes, and that's precisely why this defers to #2359901: Discourage $main_content['#title'] in favor of route titles and title callbacks to fix it. If we're unable to find a work-around in that issue, then that issue should simply remove this @todo.
7. Because of what the comment above it says:
   

         // We must render the main content now already, because it might provide a
         // title. We set its $is_root_call parameter to FALSE, to ensure
         // #post_render_cache callbacks are not yet applied. This is essentially
         // "pre-rendering" the main content, the "full rendering" will happen in
         // ::renderContentIntoResponse().
   

   In other words: we're about to call the page display variant's build() method. We need to render the main content already, because the main content may set a title. But if we pass in the original $main_content, the one on which we invoked drupal_render(), we'd be making it possible for the page display variant to:

   1. intentionally modify the main content, in which case the title we'd have used would be wrong
   2. accidentally re-render the main content, in which case we'd suffer from bad performance

   By enforcing the main content to be rendered already, we prevent those problems. But since we're working in a world of render arrays, we pass around that rendered HTML as #markup. Of course, that #markup has associated metadata that needs to be bubbled. Hence #attached, #cache and #post_render_cache.

8. Oh, ok, that changed then :) Fixed!
9. Not at the moment, because HEAD also does this automatically, in HEAD it also doesn't live in Block module's page display variant. Should it be? In any case, that'd be a very simple follow-up.
10. It looks like it is, but it's necessary; it's the one hunk I missed to do as part of the #2363139: _content controllers may only return render arrays, not strings child issue.
11. No, we did not :) That's interesting. But I think it's a premature optimization. We can still add that later if necessary. AFAICT zero other events in Drupal core override isPropagationStopped(), so I don't think this one should either. Plus, if we'd override it, that'd prevent event subscribers from being able to use stopPropagation() and have that take effect.
12. Why? :) this is exactly like HEAD…
13. No need for a follow-up, did that right here. This probably also addresses point 12 then?
14. Now that is interesting! You're absolutely right!
15. Good point. Though with one caveat: it needs to be fixed, to no longer assume the dreadful/awful drupal_set_page_content(). Restored the test!
16. Done; this is a must-have once #3 is done.
17. Eh? :P
18. Fixed.
19. Ok, feel free to file that follow-up; this is just moving existing code around, so completely out of scope.

+++ b/core/modules/system/tests/modules/system_module_test/system_module_test.module
@@ -6,22 +6,15 @@
-  return $element;
+

Ugh, missed this during my own review. Will fix that in the next reroll.

#80: that's because {{ page_top }} and {{ page_bottom }} aren't actually page regions. They're really {{ page_prefix }} and {{ page_suffix }}. They don't live inside page.html.twig, they're the prefix and suffix for that very template. This is also why we have a separate hook_page_top() and hook_page_bottom() hook: they populate these "pseudo regions".

I really wanted to do benchmarking + profiling of this patch — after all, we don't want to regress performance-wise while improving the DX. Since we shift around quite a few things, we must *definitely* test this.

I expected the numbers to stay pretty much identical, since the same amount of work needs to be done. A few layers of abstraction and passing around have been removed, but at the same time another one is introduced (the page display variant selection event). So: should remain pretty much identical perf-wise.

My expectation was confirmed:

profiling (PHP 5.5.11, APC class loader, XHProf, OpCache, frontpage, anonymous, best of 10 runs)

	HEAD	Patch in #78	Diff	Diff%
Number of Function Calls	64,969	64,805	-164	-0.3%
Incl. Wall Time (microsec)	219,384	219,161	-223	-0.1%
Incl. MemUse (bytes)	22,106,336	22,090,312	-16,024	-0.1%
Incl. PeakMemUse (bytes)	22,174,928	22,193,328	18,400	0.1%

benchmarking (PHP 5.5.11, APC class loader, OpCache, frontpage, anonymous, best of 10 runs of ab -c 10 -n 100 http://tres/)

• Before:
  

  Requests per second:    14.62 [#/sec] (mean)
  Time per request:       684.148 [ms] (mean)
  Time per request:       68.415 [ms] (mean, across all concurrent requests)
  Transfer rate:          173.37 [Kbytes/sec] received
  
  Connection Times (ms)
                min  mean[+/-sd] median   max
  Connect:        0    1   3.0      0      18
  Processing:   303  668 110.8    689     886
  Waiting:      285  589  97.4    595     797
  Total:        303  669 111.3    689     886

• After:
  

  Requests per second:    15.30 [#/sec] (mean)
  Time per request:       653.690 [ms] (mean)
  Time per request:       65.369 [ms] (mean, across all concurrent requests)
  Transfer rate:          181.39 [Kbytes/sec] received
  
  Connection Times (ms)
                min  mean[+/-sd] median   max
  Connect:        0    1   7.1      0      49
  Processing:   233  639  93.0    658     803
  Waiting:      218  580  83.7    596     735
  Total:        233  641  93.7    660     804

#83:

• #74.2: ah, that makes sense :) We could indeed do that in a follow-up. Opened #2373729: Consider injecting {{ styles }}, {{ scripts }} and {{ scripts_bottom }} into #type 'html'.
• #74.5: haha :) Because the spec actually says it's okay to answer in a different format, as long as you set the Content-Type header correctly. Basically, the HTTP spec doesn't have a strong opinion about this, it's kind of unknown territory and hence unspecified behavior. Returning a JSON response is more structured and more machine-friendly, so I went with that.
• #75.2: Thanks! Opened #2373735: Simplify/clean up BareHtmlPageRenderer.
• #75.6: That's fair, but many (I'd almost say "most") titles are actually broken in D8 HEAD. That patch not only tries to remove "crazy dynamicness", it also tries to bring some consistency to handling titles. In literally DOZENS of routes, we set a title in the *.routing.yml file, only to override it later. I'd be fine with rescoping that patch to only do that sort of clean-up.
• #75.9: opened #2373737: Consider moving the 'region' theme wrapper into page display variants, no longer doing it automatically.
• #75.10: yep, I'm okay with refining that later.
• #75.14: :D
• #75.15: indeed :)
• #75.17: opened #2373741: Move bulk of render pipeline documentation to d.o handbooks
• #81.A: I don't think I ever said it was a performance issue. It's an understandability issue. Note that this issue is NOT tagged as being a performance issue :)

#85: I was repeating the same practice we used in the past, but thanks for that. I will do testing using -k -c 1 from now on.

Oh and one possible reason we aren't seeing a slight performance improvement (that isn't a margin of error kind of improvement, which is how the "improvements" in #81 should be seen) is that we introduce one new event: the page display variant selection event. That wasn't happening in HEAD, and event dispatching is not cheap, so it could be costing us a net improvement. But it fixes one of the big remaining questions, and since this issue aims to leave us with an understandable architecture, it was important for it to be done as part of this issue.

So, in summary: this patch presents a status quo performance-wise, but cleans up the DX, removes loose ends (page display selection a.o.) and paves the way for cleaning up more loose ends (removing drupal_get_js() and friends).

• 1+2: Oops… absolutely. Same goes for AjaxRenderer.
• 3: Another oversight. Should be $this->.

I'll provide a reroll.

Reroll, as promised.

#91: that's what #89.3 gets at, and it's fixed in this reroll. Regarding why that even works: PHP works in mysterious ways :)

Arghhh! So stupid! A final reroll for a single line change…

Hurray! :)

Now that this is committed, catch and I have teamed up and updated all issues that needed to be:

• bumped
  1. #2092647-14: Consolidate _form, _controller, _entity_form, etc. into just _controller?
  2. #2369987-6: Remove SafeMarkup::set() from 'head' title on template_preprocess_html (and pinged jhogdon)
  3. #2362987-17: Remove hook_page_build() and hook_page_alter()
  4. #2373741-1: Move bulk of render pipeline documentation to d.o handbooks
• closed
  1. #2334213: Split HtmlMainFragment off of HtmlFragment for the page body
  2. #2295363: Blocks should be added to the page via an HtmlFragmentRenderer
  3. #2334217: Internalize drupal_prepare_page()
  4. #2334229: Move controller render array -> HtmlFragment conversion to a View listener; eliminate HtmlPageController
  5. #2334233: Move Modal/Dialog detection to a View listener
  6. #2334235: Replace AjaxController with a View listener
  7. #2334225: Move CSS/JS preprocessing to a View listener
  8. #1871596: Create a PartialResponse class to encapsulate html-level data [Moving to sandbox]
  9. #2334191: Move main content fallback into the block configuration level
  10. #2334207: Revisit naming of "renderers" in the page building pipeline
• closed-but-not-entirely-certain-if-that's-ok:
  1. #2305199: Create a route for every variant and use a "route filter" to apply selection rules
  2. #2292733: [meta] Introduce Display objects