This issue was the result of a 'hard problems' discussion at DrupalCon Amsterdam, see #2350943: [Meta] Untangle Drupal 8 page rendering for the meta issue.

Problem/Motivation

HtmlFragment was added to represent an HTML string and associated metadata, however, it did not take into account Drupal's render caching in the Render API, which was introduced in Drupal 7.

#post_render_cache (added in Drupal 8) implies that it must always be possible to cache HTML strings with placeholders, with the #post_render_cache callbacks run on the string retrieved from cache, before it's returned to the user. This is true except for the single exception of the page cache, which currently deals only with raw HTML strings.

HtmlFragment contains a fully rendered HTML string, as returned from drupal_render().

This makes it the final representation of that string, after all caching/#post_render_cache callbacks have run.

HtmlFragment as the final representation of an Html string + attached stuff was fine as an interface. However, being the final representation of a string, means that it's not possible to have an HtmlFragment inside any lower level than the entire page response.

For example, #2334219: Make blocks expose HtmlFragmentInterface directly would have made HtmlFragment be the return value for blocks. Blocks however, can be nested — the most obvious (but not only) example is mini panels. Having blocks return an HtmlFragment would make it impossible to implement caching for an entire mini-panel (unless we added #post_render_cache support to HtmlFragment which would require duplicating further functionality from render arrays).

HtmlPage, as the final representation of a page was also fine in theory. However, in practice this creates confusion with #2218271: Theme preprocess functions have to futz with $page_object = $variables['page']['#page'] in weird ways, where the page object is passed into template_preprocess_page() as $variables['page']['#page'].

Page templates are not fundamentally different from other templates in Drupal, so template_preprocess_page() should have the same ability, and restrictions, that other preprocess functions do (for 8.x this should ideally mean adding dynamic classes/attributes, and assets that specifically relate to the template, and not much else) — the reality isn't there yet, but it's slowly getting closer.

HtmlFragment to represent the concept of a page region (as opposed to a block), would have provided a structure object to interact with at that level, with the benefits of an interface that render arrays don't have. However, there are only a handful of modules that ever deal with regions, so the DX benefits would not trickle down in any way to most contributed and custom modules.

Additionally, there was no goal to replace render arrays with HtmlFragments, so even with a clearer DX (but, again, only for the very narrow case of building pages, not other templates), they represent a duplication and additional layer of abstraction on top of render arrays — adding to the complexity of the overall system rather than reducing it.

Additionally, arguments such as the page array being 'huge', while true for Drupal 7, are no longer the case for Drupal 8. Render arrays returned from blocks are very small, structured pseudo-objects with #pre_render equivalent to a build() method (which is in fact how blocks are implemented). This is a result of the move to render caching of both entity and block views, which represent most HTML on the page.

After talking through all of these various issues, the conclusion was to remove the concept.

To improve the DX of render arrays at all levels, we will continue to add interfaces such as:

https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Cache!CacheableIn...

This takes the functionality of providing #keys for render arrays, which is often the most confusing part, and puts it into a nice interface with methods that can later be used to extract a valid render array.

The advantage of this bottom-up approach is that the system as a whole is only dealing with a single class of object (the render array), not a combination of both HtmlFragment and render arrays at different levels.

Proposed resolution

As per #2350943: [Meta] Untangle Drupal 8 page rendering, we decided to remove HtmlFragment & HtmlPage.

The patch in #78 was profiled, see #81.

Remaining tasks

Get child issues committed, to make the patch on this issue more reviewable:

  1. #2357937: Remove {{ feed_icons }} from page template (page.html.twig)
  2. #2361681: drupal_render(): invert second argument ($is_recursive_call -> $is_root_call) => more strict, better DX/TX
  3. #2239003: Remove the '_http_statuscode' request attribute
  4. #2361693: AjaxEnhancer is dead code, remove it
  5. #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
  6. #2363025: Push usage of drupal_set_page_content() higher: out of blocks and page display variants
  7. #2363139: _content controllers may only return render arrays, not strings
  8. #2364127: Merge AjaxResponseRenderer into AjaxController
  9. #2364189: One service per format + no hardcoded format to service mapping, but tagged services
  10. #2366147: Introduce a page display selection event

After that:

  1. Build consensus for this patch
  2. Commit

User interface changes

None.

API changes

  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. Removed \Drupal\Core\Controller\HtmlControllerBase [MAIN-CONTENT-CONTROLLERS]
  9. Removed \Drupal\Core\Controller\HtmlPageController (superseded by \Drupal\Core\Controller\HtmlController) [MAIN-CONTENT-CONTROLLERS]
  10. 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]
  11. Added \Drupal\Core\Display\Annotation\PageDisplayVariant: annotation for the next item. [PAGE-VARIANTS]
  12. Added \Drupal\Core\Display\PageVariantInterface, subclass of \Drupal\Core\Display\VariantInterface, specifically for "page variants" (it adds a setMainContent() method). [PAGE-VARIANTS]
  13. Removed Drupal\Core\EventSubscriber\HtmlViewSubscriber. [MAIN-CONTENT-CONTROLLERS]
  14. Removed \Drupal\Core\Page\DefaultHtmlFragmentRenderer. [MAIN-CONTENT-CONTROLLERS]
  15. Removed \Drupal\Core\Page\DefaultHtmlPageRenderer. [MAIN-CONTENT-CONTROLLERS]
  16. Removed \Drupal\Core\Page\FeedLinkElement (Split off to #2357937: Remove {{ feed_icons }} from page template (page.html.twig).)
  17. Removed \Drupal\Core\Page\HeadElement. [MAIN-CONTENT-CONTROLLERS]
  18. Removed \Drupal\Core\Page\HtmlFragment. [MAIN-CONTENT-CONTROLLERS]
  19. Removed \Drupal\Core\Page\HtmlFragmentInterface. [MAIN-CONTENT-CONTROLLERS]
  20. Removed \Drupal\Core\Page\HtmlFragmentRendererInterface. [MAIN-CONTENT-CONTROLLERS]
  21. Removed \Drupal\Core\Page\HtmlPage. [MAIN-CONTENT-CONTROLLERS]
  22. Removed \Drupal\Core\Page\HtmlPageRendererInterface. [MAIN-CONTENT-CONTROLLERS]
  23. Removed \Drupal\Core\Page\LinkElement. [MAIN-CONTENT-CONTROLLERS]
  24. Removed \Drupal\Core\Page\MetaElement. [MAIN-CONTENT-CONTROLLERS]
  25. Removed \Drupal\Core\Page\RenderHtmlRenderer. [MAIN-CONTENT-CONTROLLERS]
  26. Removed \Drupal\Core\Page\RenderHtmlRendererInterface. [MAIN-CONTENT-CONTROLLERS]
  27. Added \Drupal\Core\Render\BareHtmlPageRenderer, successor of \Drupal\Core\Page\DefaultHtmlPageRenderer. [HTML-RENDERING]
  28. Added \Drupal\Core\Render\BareHtmlPageRendererInterface, see previous item. [HTML-RENDERING]
  29. Changed \Drupal\block\Plugin\DisplayVariant\FullPageVariant: renamed to BlockPageVariant and now implements \Drupal\Core\Display\PageVariantInterface. [PAGE-VARIANTS]
  30. Added \Drupal\block\Plugin\DisplayVariant\DemoBlockPageVariant. [PAGE-VARIANTS]
  31. Added \Drupal\system\Event\PageDisplayVariantSelectionEvent. [PAGE-VARIANTS]
  32. Added \Drupal\system\Event\SystemEvents: to add the "page display variant selection" event. [PAGE-VARIANTS]
  33. 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]
  34. Changed html.html.twig. [PAGE-VS-HTML]
Files: 

Comments

catch’s picture

Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes

Adding formatting and commas.

catch’s picture

Issue summary: View changes
beejeebus’s picture

wow. awesome work, solutions derived from a thorough understanding of Drupal's actual challenges for the win.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

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

jibran’s picture

Wim Leers’s picture

Wim Leers’s picture

Wim Leers’s picture

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

giorgio79’s picture

Wish I could hit a like on Drupal posts, but since I cannot, let me just say Thank you Wim!

Fabianx’s picture

Indeed, thank you so much, Wim!

The issue summary looks great, the whole problem space reminds me of this xkcd: http://xkcd.com/927/

I am looking forward to the patch.

almaudoh’s picture

Wim++

Wim Leers’s picture

Issue summary: View changes
Status: Active » Needs review
Issue tags: +TX (Themer Experience)
FileSize
286.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,479 pass(es). View
87.6 KB
39.26 KB
6.89 KB
70.41 KB

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.

Drupal 8 render pipeline flowchart.

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:

This at least somewhat helps:


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:

Related:


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:


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:


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);
dawehner’s picture

Great work in genera ... here is some quick feedback.

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.

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.

#2092647: Consolidate _form, _controller, _content, _entity_form, etc. into just _controller?

How?

controller.page -> main_content_controller.html
controller.ajax -> main_content_controller.ajax
controller.dialog -> main_content_controller.dialog + main_content_controller.modal

Why the hack that?

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

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

Fabianx’s picture

Issue tags: +Twig

Thanks Wim.

The proposal looks great to me, but I agree with dawehner we should split things up into little manageable patches.

- E.g. there is an issue open for changing signature of drupal_render() back again, this can go in independently.

- The feed icons mini issue can go independently.

- For the page vs. html issue, we will need themer feedback, too - generally I tend to agree as themers will copy html.html.twig anyway as all classes are added in the templates directly per convention.

- Putting a lot of things like headers to #attached makes a lot of sense.

So lets discuss this proposal some, then come up with manageable child issues of this task and split those out?

Wim Leers’s picture

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.

Fabianx’s picture

#16: \Drupal\Core\Controller\HtmlController

The class is simple enough that I think it is okay to just have one class to delegate the task of building a page to an event, collect page assets and render page_top and page_bottom.

Further feedback for untangling drupal_render():

- The render_stack should in the ideal case be setup before the first controller call is made and injected or gotten via the container and be made a first class citizen (this can be follow up).
- Then every drupal_render() call is a non-root call, just in different stages of the page and in varying recursive levels, where the root is the render stack itself.

This allows to just do:

// Injected $renderStack
$root_context = $renderStack->pop();
// and in the ideal case even $renderStack->getCacheTags();
$cache_tags = $root_context['#cache']['tags'];

for the final X-Drupal-Cache-Tags and the render cache starting at recursion level 1 when initialized / constructed.

Things like: drupal_render($page['top']); will then just naturally work - without any $is_root call needed, but as said I am okay with intermediate step.

Wim Leers’s picture

Wim Leers’s picture

Issue summary: View changes
moshe weitzman’s picture

I read through WIm's comment carefully and this is a huge improvement IMO.

I'm going to apply the patch and step through the flow in a debugger and see if I can provide more detailed feedback. Stay tuned!

yched’s picture

Largely above my head, I won't interfere with the discussions here - just : that diagram in #14 is insanely helpful, we should really make sure it ends up in a prominent place in the docs.

Also : impressive work, Wim :-°

Wim Leers’s picture

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

moshe weitzman’s picture

I stepped through in a debugger and it is pretty straightforward. There's a little dance with the drupal_render() calls in \Drupal\Core\Controller\HtmlController::prepareMainContent but it is well contained. I don't have any issues with the patch.

Fabianx’s picture

I agree that the diagram is awesome, the flow is really simple and clean.

It also makes things like using #pre_render pattern for main request content much simpler.

yched’s picture

Hem, said I wouldn't interfere but ... :-)

Sticking to the flowchart, the steps mentioned in the "_content / HtmlController::handle()" swimline seem oddly named :
Step 1, "getMainContent()" is about retrieving the main content, fine.
Step 2. "prepareMainContent()" is about preparing a full-fledged #type = 'page' render array
Step 3. "renderMainContent()" is about wrapping in a #type = 'html' and rendering it

So it looks like the progression is not so much "get / prepare / render the main content" but rather "get the main content / assemble the page content / render the html content" ?

Wim Leers’s picture

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

yched’s picture

@Wim: Thanks for the updated chart. Seeing ajax / dialog alongside is even better :-)

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 , which is represented by page.html.twig

Got that - but still we are using the same terminology ("Main Content" / $main_content) for related-but-importantly-different things across the various steps of the call stack.

MainContentControllerBase::handle() basically does (simplified, comments are mine) :

// Get $main_content as the result of the _content controller - which,
// nicely enough, is also what is commonly referred to as "the content 
// of the main block" (short version: "the main content").
$main_content = $this->getMainContent($request);

// We "prepare" that $main_content, and call the result ... $main_content :)
// Yet, in our most common case (pages) it's now something
// conceptually fairly different : the full 'page'.
$main_content = $this->prepareMainContent($main_content);

// We then "render" that new $main_content - side note : what was in the
// initial $main_content has actually been rendered in the previous step ;)
return $this->renderMainContent($main_content);

In short, this introduces a terminology that looks "official" and specific ("*main* content", instead of just "content"), but uses that terminology indifferently for all the (importantly) different states the "content" goes through.

Trying to take step back :
- From its phpdoc, MainContentControllerInterface is "The interface for "main content" (_content) controllers".
- So "MainContent" == _content (as opposed to "the <body> for HTML pages") - right ?
- So we need another terminology for "the final content wrapping / built from (and in some cases in fact equal to) that main content" ?
Why not just "content" ? It seems intuitive that a "main content" is called "main" because it's a subpart of a larger "content" ?
For HtmlController, that "content" is a #type 'html', fair too.

Then MainContentControllerBase::handle() does (simplified again) :

// 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);

?

Wim Leers’s picture

Issue summary: View changes

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

Fabianx’s picture

I agree with the new naming in #29.

I still think we need to start the render chain / setup the stack before getting the main content, but that can be followup and works nicely with this proposal.

Thanks for the new child issue and thanks so much for breaking this up into manageable chunks!

catch’s picture

Component: base system » request processing system
Wim Leers’s picture

FileSize
238.76 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,407 pass(es). View
3.22 KB
183.84 KB

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

Issue summary: View changes
FileSize
238.87 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,408 pass(es). View
12.66 KB
183.95 KB
122.46 KB
67.49 KB
9.66 KB
106.97 KB

And here it is, #29 implemented.

Wim Leers’s picture

yched’s picture

Yay !

No biggie, just curious :

+ * The three steps of handling a request for a main content controller are part
+ * of the interface:
+ * 1. getMainContent(): get the main content from the (_content) sub-controller
+ * 2. prepareContent(): apply any preparations/transformations
+ * 3. renderContentIntoResponse(): turn the content render array into a response

Is there a reason "wrap in #type 'html' " and "run hook_page_(top|bottom)()" are done in step 3 rather than in step 2 ?

Also - those three methods are a handy decomposition of what happens in MainContentControllerBase::handle(), that provides clean override mounting points for the concrete subclasses. But they are only that - helpers for handle() that are never called from the outside, and thus have no real reason to be public methods promoted to an interface ?

In practice, MainContentControllerInterface is then just ... handle($request, $route_match, $controller_definition) ?
Which in turn makes it quite unspecific to "_content / MainContent" (I was actually surprised that there isn't an existing, generic interface for that definition of handle()) ?
The real structuring concept for "_content / MainContent" seems to be MainContentControllerBase, rather than the interface ?

Wim Leers’s picture

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

yched’s picture

@Wim: Yeah, those objectives sound valid.

I'd think, If the MainContentControllerInterface only had handle() :
- other "_content controllers" would still be strongly encouraged to use MCCBase
(e.g. because we'd say so in the doc of the interface, and also because in order to be a "_content controller" you need the non-fully-trivial code in MCCBase::getMainContent())
- MCCBase then strongly encourages you to follow its internal code flow of (protected) getMainContent() / prepareContent() / renderContentIntoResponse(),
- but you can still override handle() if you want to bend that flow a bit ?

Point is, looking at an interface gives you a notion of what the outside world is going to do with the objects. So you tend to understand the various methods listed there as "independant tasks called independantly". Seeing 4 methods, 3 of which are in fact only intended to be called by the 4th one, is a bit misleading about the actual code flow.

Anyway - at this point, this does feel like bikeshedding, so I'll leave, er, others more familiar with the problem space :-) comment on the actual code...

Wim Leers’s picture

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!

Wim Leers’s picture

FileSize
222.85 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,504 pass(es). View
167.93 KB
Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

Wim Leers’s picture

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.

Wim Leers’s picture

FileSize
206.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,733 pass(es). View
150.8 KB
effulgentsia’s picture

Here's my suggestion for how to resolve #36-#38:

- Create a new sub-namespace: Drupal\Core\Render\Response (i.e., the part of the Render system that deals with rendering a render array to a Response).
- Move MainContentControllerInterface to Drupal\Core\Render\Response\ResponseRendererInterface and (Html|Ajax|Dialog|Modal)Controller to Drupal\Core\Render\Response\*ResponseRenderer.
- The above interface should have only two methods: prepare() and renderResponse().
- Rename the service tag from main_content_controller to render.response_renderer.
- Move the implementation of MainContentControllerBase::getMainContent() and MainContentControllerBase::handle() into ContentControllerSubscriber, and remove MainContentControllerBase. #37 suggests doing that with a closure, but they could also be methods (e.g., $request->attributes->set('_controller', array($this, 'handle'))). A closure has the benefit of not needing the methods to be public, so another option is a thin closure around a protected method. I'll leave that choice to you, Wim.

The benefit of above is that per #15, if we want to, we'll be able to in the future (maybe even in 8.0 or 8.1) change the implementation from a controller wrapper to a KernelEvents::View listener without that affecting any interfaces or the response renderer implementations.

An additional suggestion:

+++ b/core/lib/Drupal/Core/Controller/MainContentControllerInterface.php
@@ -0,0 +1,92 @@
+   * @return array
+   *   An array with three values:
+   *   0. The prepared render array representing the (actual) content.
+   *   1. The title.
+   *   2. Key-value pairs with custom options for this main content controller.
+   */
+  public function prepareContent(array $main_content, Request $request, RouteMatchInterface $route_match);

That's an ugly return value. How about making the return value just a render array? For HtmlResponseRenderer, the title can be on the page element's #title and $custom is unused. For DialogResponseRenderer, how about introducing a '#type' => 'dialog' type that can wrap the content element and have a #title, #target, and #options?

Wim Leers’s picture

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.

moshe weitzman’s picture

I have done stuff like triple return values in the past and not felt bad about it. I think it is clear and useful in situations like these.

Wim Leers’s picture

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

almaudoh’s picture

#46 + #45 sounds good

yched’s picture

I like the new proposed organization too, but - terminology again :
Drupal\Core\Render\MainContent\*Renderer makes those a little disconnected from "this is what is fired in ContentControllerSubscriber::VIEW to produce a Response" ?

"MainContent" is a bit vague too.
- In the previous proposal, it was always "MainContentController(Interface)", which grounded it with some more specific context "Controllers for routes that massage the output of a 'main content' callback".
- Does "Content" in ContentControllerSubscriber correspond to "MainContent" in Drupal\Core\Render\MainContent\*Renderer ?
Is there a way we could unify ?

Wim Leers’s picture

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?

larowlan’s picture

Instead of a triple value return, let's create a value object like we did in #2242749: Port Form API security fix SA-CORE-2014-002 to Drupal 8

effulgentsia’s picture

rename ContentControllerSubscriber to MainContentRendererSubscriber

Good idea, but how about MainContentViewSubscriber (since it'll be subscribed to the ::VIEW event)? Note, we can then rename (in a separate issue) our existing ViewSubscriber to something like DataViewSubscriber (since it's about returning a plain data object, like for an autocomplete response, as opposed to a "main content" render array).

In the previous proposal, it was always "MainContentController(Interface)", which grounded it with some more specific context "Controllers for routes that massage the output of a 'main content' callback".

Yep, but per #51, it's specifically the "controller" part of that terminology that we want to remove now, in order to change the implementation from a controller wrapper to a view listener. Good call on retaining the "MainContent" part though.

Instead of a triple value return, let's create a value object

+1, though that starts looking a bit like HtmlFragment again, just with a much scaled down responsibility :)

effulgentsia’s picture

starts looking a bit like HtmlFragment again

So, just to be clear, let's NOT use that name. Not because it's a bad name, but because it's too good a name to squat for our specific use case. A properly generic HtmlFragment API would be great to continue exploring in contrib and maybe fold back into core for 8.1, 8.2, or 9.0. For our use case, perhaps the value object could be \Drupal\Core\Render\MainContent\PreparedContent? With methods like getTitle() and getContent()? And then perhaps a PreparedDialogContent could extend that with getTarget() and getOptions()?

larowlan’s picture

#53 and #54 yep prepared content sounds good +1

Fabianx’s picture

+1 on all MainContentViewSubscriber, also +1 on PreparedContent.

Wim Leers’s picture

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

effulgentsia’s picture

+1 to dropping prepare() (as a public API) and closing #2364189: One service per format + no hardcoded format to service mapping, but tagged services in favor of doing it as part of this larger issue.

Wim Leers’s picture

Issue summary: View changes
FileSize
195.02 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,490 pass(es). View
141.03 KB
57.53 KB
152.79 KB
76.39 KB
11.3 KB
417.42 KB

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.

Fabianx’s picture

The diagram is great!

Once we have this implemented, please publish a blog post on it as the request timeline never has been clearer!

It also makes all a lot of sense.

Wim Leers’s picture

Issue summary: View changes

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

Fabianx’s picture

Not reviewed full patch, yet, but this stood out particularly:

+++ b/core/includes/theme.inc
@@ -1710,35 +1708,27 @@ function template_preprocess_html(&$variables) {
+  $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);

IIRC there was a good reason we moved that to an object and rendered it lazily, but I don't remember.

Searching the git log for RenderWrapper() should give back something ...

Oh, yes it was the removal of the hook_template_process() layer.

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

So can we keep that in a value object still?

Also as spoken on IRC would like the bare_html_renderer to perhaps be factored out as its a good and on-its-own pre-step - that is independently useful.

dawehner’s picture

Great diagramm and great general flow. Its great to see the separation of concerns for the different main content renderers.

Kinda nitpick: I'm curious why you decided to use an event to determine the page display variant and not
using a chain of responsibility pattern, so a tagged services approach. The first one who says yes, wins.
But yeah this is a small detail.

  1. +++ b/core/includes/batch.inc
    @@ -136,7 +135,7 @@ function _batch_progress_page() {
         // as content is not allowed to appear after </html> anyway.
    -    $fallback = DefaultHtmlPageRenderer::renderPage($fallback, $current_set['title'], 'maintenance', array(
    +    $fallback = \Drupal::service('bare_html_page_renderer')->renderMaintenancePage($fallback, $current_set['title'], array(
    

    +1 for having a proper service here, this helps a lot, especially in terms of brittleness.

  2. +++ b/core/includes/theme.inc
    @@ -1669,20 +1669,18 @@ function template_preprocess_html(&$variables) {
    -  if ($page->hasTitle()) {
    +  if (!empty($variables['html']['page']['#title'])) {
    ...
    -      'title' => SafeMarkup::set(trim(strip_tags($page->getTitle()))),
    +      'title' => SafeMarkup::set(trim(strip_tags($variables['html']['page']['#title']))),
    

    Just curious: Can we make that kind of stuff easier accessible?

  3. +++ b/core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php
    @@ -0,0 +1,109 @@
    +/**
    + * View subscriber rendering main content render arrays into responses.
    + *
    + * Additional target rendering formats can be defined by adding another service
    + * that implements \Drupal\Core\Render\MainContent\MainContentRendererInterface
    + * and tagging it as a @code render.main_content_renderer @endcode, then
    + * \Drupal\Core\Render\MainContent\MainContentRenderersPass will detect it and
    + * use it when appropriate.
    + *
    + * @see \Drupal\Core\Render\MainContent\MainContentRendererInterface
    + * @see \Drupal\Core\Render\MainContentControllerPass
    + */
    +class MainContentViewSubscriber implements EventSubscriberInterface {
    

    Maybe we should consider linking the flow diagram for here or somewhere else? I think this would be SUPER helpful to understand things.

  4. +++ b/core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php
    @@ -0,0 +1,109 @@
    +   * @var array
    ...
    +   * @param array $main_content_renderers
    +   *   The available main content renderer services, keyed per format.
    

    Do those renderer have nothing like a common interface? MainContentRendererInterface seems to be one.

  5. +++ b/core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php
    @@ -0,0 +1,109 @@
    +    // Render the controller result into a response if it's a render array.
    +    if (is_array($result)) {
    

    Is there any way how we can be a little bit more specific her? An array of domain objects for example could be a usecase someone has.

  6. +++ b/core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php
    @@ -0,0 +1,109 @@
    +   * Registers the methods in this class that should be listeners.
    +   *
    +   * @return array
    +   *   An array of event listener definitions.
    +   */
    +  static function getSubscribedEvents() {
    

    Nitpick: you can just use @inheritdoc here.

  7. +++ b/core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php
    @@ -0,0 +1,109 @@
    +    $events[KernelEvents::VIEW][] = array('onViewRenderArray', 30);
    

    Just curious, why do we have a priority set here? Maybe 30 is just coming from KernelEvents::REQUEST earlier?

  8. +++ b/core/lib/Drupal/Core/Render/BareHtmlPageRenderer.php
    @@ -0,0 +1,78 @@
    +   * {@inheritdoc}
    +   */
    +  public function renderMaintenancePage($content, $title, array $page_additions = []) {
    +    if (!is_array($content)) {
    +      $content = ['#markup' => $content];
    +    }
    +    $attributes = [
    +      'class' => [
    +        'maintenance-page',
    +      ],
    +    ];
    +    return $this->renderBarePage($content, $title, $page_additions, $attributes, 'maintenance_page');
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function renderInstallPage($content, $title, array $page_additions = []) {
    +    $attributes = [
    +      'class' => [
    +        'install-page',
    +      ],
    +    ];
    +    return $this->renderBarePage($content, $title, $page_additions, $attributes, 'install_page');
    +  }
    +
    

    This is beatiful and well be even less, once we move these classes into the templates.

  9. +++ b/core/lib/Drupal/Core/Render/BareHtmlPageRenderer.php
    @@ -0,0 +1,78 @@
    +    // We must first render the contents of the html.html.twig template, see
    +    // \Drupal\Core\Render\MainContent\HtmlRenderer::renderPage() for details.
    +    drupal_render_root($html['page']);
    +    return drupal_render($html);
    

    This is kinda confusing. So drupal_render_root() is actually not the last call :) Maybe we should explain why its okay to "just" call drupal_render() for $html

  10. +++ b/core/lib/Drupal/Core/Render/BareHtmlPageRendererInterface.php
    @@ -0,0 +1,73 @@
    +/**
    + * Bare HTML page renderer.
    + *
    + * By "bare HTML page", we mean that the following hooks that allow for "normal"
    + * pages are not invoked:
    + * - hook_page_attachments()
    + * - hook_page_attachments_alter()
    + * - hook_page_top()
    + * - hook_page_bottom()
    + *
    + * Examples of bare HTML pages are:
    + * - install.php
    + * - update.php
    + * - authorize.php
    + * - maintenance mode
    + * - exception handlers
    + *
    + * i.e. use this when rendering HTML pages in limited environments. Otherwise,
    

    It would be great if you could add some documentation that these renderes should rely on as less as possible states, like having routing system being executed. We had some crazy bugs in the installer at some point for example.

  11. +++ b/core/lib/Drupal/Core/Render/BareHtmlPageRendererInterface.php
    @@ -0,0 +1,73 @@
    + * use a @code _content @endcode route, thiswill cause a main content controller
    

    thiswill => this will

  12. +++ b/core/lib/Drupal/Core/Render/BareHtmlPageRendererInterface.php
    @@ -0,0 +1,73 @@
    + */
    +interface BareHtmlPageRendererInterface {
    +
    

    Given how similar their implementations are, I wonder, whether the callers should instead distinct the behaviour between install/maintenance, so we end up with just a single renderBarPage($content, $title, $page_additions, $attributes, $page_theme_property)

  13. +++ b/core/lib/Drupal/Core/Render/Element/Page.php
    @@ -8,7 +8,11 @@
    + * Provides a render element for the content of an HTML page.
    + *
    + * This represents the "main part" of the HTML page's body; see html.html.twig.
    + *
    + * @todo Rename this to 'body'?
      *
      * @RenderElement("page")
    

    Interesting idea ...

  14. +++ b/core/lib/Drupal/Core/Render/MainContent/DialogRenderer.php
    @@ -0,0 +1,92 @@
    +   * @var \Drupal\Core\Controller\TitleResolver
    ...
    +   * @param \Drupal\Core\Controller\TitleResolverInterface $title_resolver
    

    nitpick: let's use the interface here

  15. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -0,0 +1,284 @@
    + * @file
    + * Contains \Drupal\Core\Render\MainContent\HtmlRenderer.
    + */
    +
    ...
    +class HtmlRenderer {
    
    +++ b/core/modules/system/src/Event/PageDisplayVariantSelectionEvent.php
    @@ -0,0 +1,55 @@
    +namespace Drupal\system\Event;
    ...
    +class PageDisplayVariantSelectionEvent extends Event {
    
    +++ b/core/modules/system/src/Event/SystemEvents.php
    @@ -0,0 +1,23 @@
    +
    +namespace Drupal\system\Event;
    +
    +/**
    ...
    + */
    +final class SystemEvents {
    +
    

    Maybe I'm stupid but I don't see why these events should be part of system module.

  16. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -0,0 +1,284 @@
    +      // Select the page display variant to be used to render this main content,
    +      // default to the built-in "simple page".
    +      $event = new PageDisplayVariantSelectionEvent('simple_page');
    +      $this->eventDispatcher->dispatch(SystemEvents::SELECT_PAGE_DISPLAY_VARIANT, $event);
    +      $variant_id = $event->getPluginId();
    
    +++ b/core/modules/block/src/EventSubscriber/BlockPageDisplayVariantSubscriber.php
    @@ -0,0 +1,64 @@
    +  public function onSelectPageDisplayVariant(PageDisplayVariantSelectionEvent $event) {
    +    if ($this->routeMatch->getRouteName() != 'block.admin_demo') {
    +      $event->setPluginId('block_page');
    +    }
    

    Did you considered to expand the event to have the route match available? It would be just much nicer DX if you would not need to inject it.

  17. +++ b/core/lib/Drupal/Core/Render/MainContent/MainContentRenderersPass.php
    @@ -0,0 +1,33 @@
    +    }
    +    $container->setParameter('main_content_renderers', $main_content_renderers);
    +  }
    

    So there seems to be just a single usecase for it, ... did you considered to instead call a method on the main content renderers injecting in there or alternating the arguments of the service. Using the parameter is just one small level of additional indirection.

  18. +++ b/core/modules/block/block.module
    @@ -62,44 +62,22 @@ function block_theme() {
    +function block_page_top(array &$page_top) {
    +  if (\Drupal::routeMatch()->getRouteName() === 'block.admin_demo') {
    +    $theme = \Drupal::theme()->getActiveTheme()->getName();
    +    $page_top['backlink'] = array(
           '#type' => 'link',
           '#title' => t('Exit block region demonstration'),
           '#options' => array('attributes' => array('class' => array('block-demo-backlink'))),
           '#weight' => -10,
         );
         if (\Drupal::config('system.theme')->get('default') == $theme) {
    -      $page['page_top']['backlink']['#url'] = Url::fromRoute('block.admin_display');
    +      $page_top['backlink']['#url'] = Url::fromRoute('block.admin_display');
         }
         else {
    -      $page['page_top']['backlink']['#url'] = Url::fromRoute('block.admin_display_theme', ['theme' => $theme]);
    +      $page_top['backlink']['#url'] = Url::fromRoute('block.admin_display_theme', ['theme' => $theme]);
         }
       }
     }
    
    +++ b/core/modules/block/src/EventSubscriber/BlockPageDisplayVariantSubscriber.php
    @@ -0,0 +1,64 @@
    +      $event->setPluginId('block_demo_page');
    

    Just curious, this is just actually for a specific page variant. Did you considered to be able to move this logic into the block admin demo page variant?

  19. +++ b/core/modules/block/src/EventSubscriber/BlockPageDisplayVariantSubscriber.php
    similarity index 94%
    rename from core/modules/block/src/Plugin/DisplayVariant/FullPageVariant.php
    
    rename from core/modules/block/src/Plugin/DisplayVariant/FullPageVariant.php
    rename to core/modules/block/src/Plugin/DisplayVariant/BlockPageVariant.php
    
    rename to core/modules/block/src/Plugin/DisplayVariant/BlockPageVariant.php
    index 9ae99ed..1b58773 100644
    
    index 9ae99ed..1b58773 100644
    --- a/core/modules/block/src/Plugin/DisplayVariant/FullPageVariant.php
    
    --- a/core/modules/block/src/Plugin/DisplayVariant/FullPageVariant.php
    +++ b/core/modules/block/src/Plugin/DisplayVariant/BlockPageVariant.php
    
    +++ b/core/modules/block/src/Plugin/DisplayVariant/BlockPageVariant.php
    +++ b/core/modules/block/src/Plugin/DisplayVariant/BlockPageVariant.php
    @@ -2,7 +2,7 @@
    
    @@ -2,7 +2,7 @@
     
    

    That renaming seems a little bit out of scope but well whatever you want.

  20. +++ b/core/modules/system/src/Controller/BatchController.php
    @@ -81,46 +38,13 @@ public function batchPage(Request $request) {
    +      $page = [
    +        '#type' => 'page',
    +        '#show_messages' => FALSE,
    +        'content' => $output,
    +      ];
           return $page;
    

    So good!

  21. +++ b/core/modules/system/src/Plugin/DisplayVariant/SimplePageVariant.php
    @@ -0,0 +1,47 @@
    + */
    +class SimplePageVariant extends VariantBase implements PageVariantInterface {
    

    Nor, why th simple page should not be part of core.

  22. +++ b/core/modules/system/src/Tests/Common/PageRenderTest.php
    @@ -61,14 +62,14 @@ function testHookPageAttachmentsAlter() {
    +    HtmlRenderer::invokePageAttachmentHooks($page);
    ...
    +      HtmlRenderer::invokePageAttachmentHooks($page);
    
    @@ -82,7 +83,7 @@ function assertPageRenderHookExceptions($module, $hook) {
    +      HtmlRenderer::invokePageAttachmentHooks($page);
    

    Ah this is the reason why these are static methods ... I think you can also move it to the service, make it a public method, but not be on the interface ... we are testing core code here so its fine to assume the actual congrete implementation.

  23. +++ b/core/modules/system/system.routing.yml
    @@ -419,13 +419,13 @@ system.admin_config:
    -# @todo system.batch_page.html does not work due to some ordering issues.
    -system.batch_page.normal:
    +system.batch_page.html:
    

    nice!

  24. +++ b/core/modules/system/tests/modules/system_test/system_test.module
    index 3087c5f..37b769e 100644
    --- a/core/modules/system/theme.api.php
    
    --- a/core/modules/system/theme.api.php
    +++ b/core/modules/system/theme.api.php
    
    +++ b/core/modules/system/theme.api.php
    @@ -302,6 +302,80 @@
      *
    

    Oh this is maybe a good place for this diagramm.

  25. +++ b/core/modules/views/views.module
    @@ -325,20 +325,8 @@ function views_preprocess_page(&$variables) {
    -  if (!empty($variables['page']['#views_contextual_links']) && isset($variables['attributes']['class'])) {
    -    /** @var \Drupal\Core\Page\HtmlPage $page_object */
    -    $page_object = $variables['page']['#page'];
    -    $attributes = $page_object->getBodyAttributes();
    -    $class = $attributes['class'] ?: array();
    -
    -    $key = array_search('contextual-region', $variables['attributes']['class'] instanceof AttributeArray ? $variables['attributes']['class']->value() : $variables['attributes']['class']);
    -    if ($key !== FALSE) {
    -      /** @var \Drupal\Core\Page\HtmlPage $page_object */
    -      unset($class[$key]);
    -      $attributes['class'] = $class;
    -      $attributes['data-views-page-contextual-id'] = $variables['title_suffix']['contextual_links']['#id'];
    -      $variables['#attached']['library'][] = 'views/views.contextual-links';
    -    }
    +  if (!empty($variables['html']['page']['#views_contextual_links'])) {
    +    $variables['attributes']['data-views-page-contextual-id'] = _contextual_links_to_id($variables['html']['page']['#contextual_links']);
       }
     }
    
    @@ -460,6 +448,12 @@ function views_add_contextual_links(&$render_element, $location, ViewExecutable
    +          // If we're setting contextual links on a page, for a page view, for a
    +          // user that may use contextual links, attach Views' contextual links
    +          // JavaScript.
    +          if ($location === 'page' && $render_element['#type'] === 'page' && \Drupal::currentUser()->hasPermission('access contextual links')) {
    +            $render_element['#attached']['library'][] = 'views/views.contextual-links';
    +          }
    

    This is indeed also better to understand.

  26. +++ b/core/themes/seven/css/theme/install-page.css
    @@ -5,7 +5,7 @@
    -.install-background {
    +.install-page {
    
    +++ b/core/themes/seven/css/theme/maintenance-page.css
    @@ -2,7 +2,7 @@
    -.maintenance-background {
    +body.maintenance-page {
    

    Oh, this is a bit confusing, can you maybe add it to the IS why we need to change it?

Wim Leers’s picture

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

Wim Leers’s picture

Issue summary: View changes
FileSize
140.74 KB
76.04 KB
11.55 KB
160.72 KB

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.

Wim Leers’s picture

FileSize
196.01 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,650 pass(es). View
142.02 KB
21.65 KB

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

Issue summary: View changes
FileSize
145.53 KB
81.8 KB
12.46 KB
321.45 KB

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.
cosmicdreams’s picture

OMG I can't believe I'm saying this but, if you find yourself with extra time on your hands it might also make sense to break out the different workflows into different pages. For example, it would would great to show a Symfony developer:

red = Here's the workflow if you want to handle it yourself (just to show that their expectations aren't violated).
blue = Here's an example of the workflow if you want to work with Drupal a little
green = Here's how Drupal typically responds to requests.

Love the combined diagram, awesome to see it.

P.S: It needs a legend.

Wim Leers’s picture

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

I've been discussing this in depth with Wim both before and after this issue was opened.

Looking through the patch it's better than I expected in terms of the changes necessary, and the diagram is fantastic for understanding the overall flow. Gets us extremely close to doing things which have been a goal of the entire release, like removing the drupal_get_*() functions and only 1600 new lines of code makes this very reviewable.

The main wart is handled by #2359901: Discourage $main_content['#title'] in favor of route titles and title callbacks, happy to leave that problem in that issue.

Very minor nit.

+++ b/core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php
@@ -0,0 +1,106 @@
+        $event->setResponse(new Response('Not Acceptable. Supported MIME types: ' . implode(', ', $supported_mimetypes) . '.', 406));

This doesn't actually throw an exception as stated in the @throws.

cosmicdreams’s picture

#69.1: Yes one diagram per workflow.

#69.2: Legend could describe the separate workflows with a short description. No need of a legend that describes the parts, just what they represent together.

joelpittet’s picture

Re #62/#64

I'm fine with preprocess_html not being able to do attached as d7/d6 didn't have this feature. The "process" layer did do this as a "flattening process" mostly and that's why we removed the process layer (confusion on what it did, and early rendering/flattening, templates didn't have access to structured data) RenderWrapper was a stop gap to remove the process layer, glad it's finally gone and don't want it back because it was not a particularly intuitive DX, being a generic "call this callback function when printed with args held inside" object (AND because my poor class naming skills).

Also, there is enough context available in other preprocess functions further down to do most any conditional asset attachment necessary. page is high enough, IMO.

I'll do my best to have a review of this at BADCamp.

joelpittet’s picture

+++ b/core/modules/system/templates/html.html.twig
@@ -29,18 +29,18 @@
-    {{ page.head }}
+    {{ html.head }}
...
-    {{ page.styles }}
-    {{ page.scripts }}
+    {{ html.styles }}
+    {{ html.scripts }}
...
-    {{ page_top }}
-    {{ page.content }}
-    {{ page_bottom }}
-    {{ page.scripts('footer') }}
+    {{ html.page_top }}
+    {{ html.page }}
+    {{ html.page_bottom }}
+    {{ html.scripts_bottom }}

The reason these were prefixed with 'page.' was because it was coming in as an object and using Twig's magic method detection for page->getContent() and page->getScripts(). We don't need the 'html.' and this can now be turned back to what we had in D7 and simpler unless there is another reason for the variable changes?

I suggesting the following to revert back before #2272279-38: Kill RenderWrapper class

{{ head }}
{{ styles }}
{{ scripts }}
{{ page_top }}
{{ content }}
{{ page_bottom }}
{{ scripts_bottom }} 

{{ scripts_bottom }} is the only new one and I'm glad it's getting added.

dawehner’s picture

Pair review of @joelpittet and @dawehner, continuued later

  1. +++ b/core/includes/common.inc
    @@ -1117,9 +1117,6 @@ function drupal_get_css($css = NULL, $skip_alter = FALSE, $theme_add_css = TRUE)
       );
    -  if (!empty($setting)) {
    -    $styles['#attached']['js'][] = array('type' => 'setting', 'data' => $setting);
    -  }
    

    HAHA, this is a nice one! We do need css settings, absolutely!

  2. +++ b/core/includes/theme.inc
    @@ -1710,35 +1708,27 @@ function template_preprocess_html(&$variables) {
    +  $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);
    

    Can we add a follow up here to not rely on this really implicit behaviour?

  3. +++ b/core/includes/theme.inc
    @@ -2161,7 +2134,7 @@ function drupal_common_theme() {
    -      'variables' => array('page_object' => NULL),
    +      'render element' => 'html',
    

    We (joelpittet and dawehner) don't understand 'render element'. it would be cool if someone could enlighten us

  4. +++ b/core/lib/Drupal/Core/ContentNegotiation.php
    @@ -14,6 +14,7 @@
      * @todo Replace this class with a real content negotiation library based on
      *   mod_negotiation. Development of that is a work in progress.
    + *   https://www.drupal.org/node/1505080
      */
    

    I literally hate that because it requires #2331919: Implement StackNegotiation to be rerolled

  5. +++ b/core/lib/Drupal/Core/EventSubscriber/MainContentViewSubscriber.php
    @@ -0,0 +1,106 @@
    +        $supported_formats = array_keys($this->mainContentRenderers);
    +        $supported_mimetypes = array_map([$request, 'getMimeType'], $supported_formats);
    +        $event->setResponse(new Response('Not Acceptable. Supported MIME types: ' . implode(', ', $supported_mimetypes) . '.', 406));
    

    I try to understand why we don't throw an exception in that case and provide a nicer page ... maybe we could render it nicely as json for example.

joelpittet’s picture

  1. +++ b/core/lib/Drupal/Core/Render/BareHtmlPageRenderer.php
    @@ -0,0 +1,80 @@
    + * Default bare HTML page renderer
    

    .

  2. +++ b/core/lib/Drupal/Core/Render/BareHtmlPageRenderer.php
    @@ -0,0 +1,80 @@
    +  public function renderMaintenancePage($content, $title, array $page_additions = []) {
    +    if (!is_array($content)) {
    +      $content = ['#markup' => $content];
    +    }
    +    $attributes = [
    +      'class' => [
    +        'maintenance-page',
    +      ],
    +    ];
    +    return $this->renderBarePage($content, $title, $page_additions, $attributes, 'maintenance_page');
    +  }
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function renderInstallPage($content, $title, array $page_additions = []) {
    +    $attributes = [
    +      'class' => [
    +        'install-page',
    +      ],
    +    ];
    +    return $this->renderBarePage($content, $title, $page_additions, $attributes, 'install_page');
    

    The classes we'd like those on the templates.

    After that there is literally not really a big difference.

    Both @tim and @dawehner thinks that the concept of a bare html page should be not be connected with maintenancen or install itself.

  3. +++ b/core/lib/Drupal/Core/Render/Element/Html.php
    @@ -27,4 +33,103 @@ public function getInfo() {
    +  public static function preRenderHtml(array $element) {
    ...
    +    // Attach favicon.
    +    if (static::themeGetSetting('features.favicon')) {
    +      $favicon = static::themeGetSetting('favicon.url');
    +      $type = static::themeGetSetting('favicon.mimetype');
    +      $element['#attached']['html_head_link'][][] = array(
    +        'rel' => 'shortcut icon',
    +        'href' => UrlHelper::stripDangerousProtocols($favicon),
    +        'type' => $type,
    +      );
    +    }
    +
    +    // Get the major Drupal version.
    +    list($version, ) = explode('.', \Drupal::VERSION);
    +
    +    // Attach default meta tags.
    +    $meta_default = array(
    +      // Make sure the Content-Type comes first because the IE browser may be
    +      // vulnerable to XSS via encoding attacks from any content that comes
    +      // before this META tag, such as a TITLE tag.
    +      'system_meta_content_type' => array(
    +        '#tag' => 'meta',
    +        '#attributes' => array(
    +          'name' => 'charset',
    +          'charset' => 'utf-8',
    +        ),
    +        // Security: This always has to be output first.
    +        '#weight' => -1000,
    +      ),
    +      // Show Drupal and the major version number in the META GENERATOR tag.
    +      'system_meta_generator' => array(
    +        '#type' => 'html_tag',
    +        '#tag' => 'meta',
    +        '#attributes' => array(
    +          'name' => 'Generator',
    +          'content' => 'Drupal ' . $version . ' (http://drupal.org)',
    +        ),
    +      ),
    +      // Attach default mobile meta tags for responsive design.
    +      'MobileOptimized' => array(
    +        '#tag' => 'meta',
    +        '#attributes' => array(
    +          'name' => 'MobileOptimized',
    +          'content' => 'width',
    +        ),
    +      ),
    +      'HandheldFriendly' => array(
    +        '#tag' => 'meta',
    +        '#attributes' => array(
    +          'name' => 'HandheldFriendly',
    +          'content' => 'true',
    +        ),
    +      ),
    +      'viewport' => array(
    +        '#tag' => 'meta',
    +        '#attributes' => array(
    +          'name' => 'viewport',
    +          'content' => 'width=device-width, initial-scale=1.0',
    +        ),
    +      ),
    +    );
    +    foreach ($meta_default as $key => $value) {
    +      $element['#attached']['html_head'][] = [$value, $key];
    +    }
    

    It is super confusing / a bug that we don't add it using hook_page_attachments, because you cannot alter them using hook_page_attachments_alter if you do it in #pre_render

  4. +++ b/core/lib/Drupal/Core/Render/Element/Page.php
    @@ -8,7 +8,11 @@
    + * @todo Rename this to 'body'?
    

    follow up?

  5. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -0,0 +1,299 @@
    +class HtmlRenderer {
    

    The order of the methods in this class is kinda weird. The main thing you do is renderResponse ... but its not the first thing. If you follow the code then you have to jump over. It should be renderResponse ... prepare ... invokePageAttachmentHooks

  6. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -0,0 +1,299 @@
    +      // @todo Remove this once https://www.drupal.org/node/2359901 lands.
    

    This may be an issue for contitional titles like the ones which come from the front page view.

  7. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -0,0 +1,299 @@
    +        $main_content = [
    ...
    +          '#attached' => $main_content['#attached'],
    +          '#cache' => ['tags' => $main_content['#cache']['tags']],
    +          '#post_render_cache' => $main_content['#post_render_cache'],
    +          '#title' => isset($main_content['#title']) ? $main_content['#title'] : NULL,
    

    Why do we not just use $main_content += ['#title' => NULL];

  8. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -0,0 +1,299 @@
    +          '#markup' => SafeMarkup::set($main_content['#markup']),
    

    #markup is always defined to be safe anyway. ... so we could drop the full file.

  9. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -0,0 +1,299 @@
    +    // $page is now fully built. Find all non-empty page regions, and add a
    +    // theme wrapper function that allows them to be consistently themed.
    +    $regions = system_region_list(\Drupal::theme()->getActiveTheme()->getName());
    +    foreach (array_keys($regions) as $region) {
    +      if (!empty($page[$region])) {
    +        $page[$region]['#theme_wrappers'][] = 'region';
    +        $page[$region]['#region'] = $region;
    +      }
    +    }
    

    Is this the responsibility of the PageVariant for things like panels and ds?

  10. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -0,0 +1,299 @@
    +      $function = $module . '_page_top';
    
    +++ b/core/modules/basic_auth/src/Authentication/Provider/BasicAuth.php
    @@ -125,7 +125,7 @@ public function authenticate(Request $request) {
    -    return NULL;
    +    return [];
    

    This may be out of scope?

  11. +++ b/core/lib/Drupal/Core/Render/PageDisplayVariantSelectionEvent.php
    @@ -0,0 +1,77 @@
    +class PageDisplayVariantSelectionEvent extends Event {
    

    Did we considered to override isPropagationStopped()? If you want to override choose a higher priority. We could stop propagation if a page variant got set. This potentially allows to speed things up a bit.

  12. +++ b/core/modules/block/src/Plugin/DisplayVariant/DemoBlockPageVariant.php
    @@ -0,0 +1,134 @@
    +      $description = '<div class="block-region demo-block">' . $visible_regions[$region] . '</div>';
    

    Cheater!

  13. +++ b/core/modules/block/src/Plugin/DisplayVariant/DemoBlockPageVariant.php
    @@ -0,0 +1,134 @@
    +      $build[$region]['block_description'] = array(
    +        '#markup' => $description,
    +        '#weight' => 15,
    +      );
    

    Can we open a follow up to swap that either to a template or a inline_template?

  14. +++ b/core/modules/system/src/Controller/BatchController.php
    @@ -81,46 +38,13 @@ public function batchPage(Request $request) {
    +      $page = [
    +        '#type' => 'page',
    +        '#show_messages' => FALSE,
    +        'content' => $output,
    +      ];
           return $page;
         }
    

    We were wondering whether the block admin demo could be implemented using a type page?

  15. +++ /dev/null
    @@ -1,85 +0,0 @@
    -class MainContentFallbackTest extends WebTestBase {
    

    We wonder whether we can keep the test to ensure that the main content is rendered even if block module is uninstalled ...

  16. +++ b/core/modules/system/tests/modules/system_module_test/system_module_test.module
    @@ -5,3 +5,23 @@
    +/**
    + * Implements hook_element_info_alter().
    + */
    +function system_module_test_element_info_alter(&$types) {
    +  $types['html']['#pre_render'][] = 'system_module_test_html_pre_render';
    +}
    +
    +/**
    + * Additional #pre_render callback for 'html' elements.
    + */
    +function system_module_test_html_pre_render(array $element) {
    +  // Remove the HTML5 mobile meta-tags.
    +  $meta_tags_to_remove = ['MobileOptimized', 'HandheldFriendly', 'viewport', 'cleartype'];
    +  foreach ($element['#attached']['html_head'] as $index => $parts) {
    +    if (in_array($parts[1], $meta_tags_to_remove)) {
    +      unset($element['#attached']['html_head'][$index]);
    +    }
    +  }
    +  return $element;
    +}
    

    Ideally we should use hook_page_attachments_alter()

  17. +++ b/core/modules/system/theme.api.php
    @@ -302,6 +302,80 @@
    + * @section render_pipeline The Render Pipeline (or: how Drupal renders pages)
    

    Someone should read the documentation :p

  18. +++ b/core/themes/seven/css/theme/maintenance-page.css
    @@ -2,7 +2,7 @@
    -.maintenance-background {
    +body.maintenance-page {
    

    The selector is too specific.

  19. +++ b/core/themes/seven/seven.theme
    @@ -10,24 +10,33 @@
    +  // If on a node add or edit page, add a node-layout class.
    +  $path_args = explode('/', \Drupal::request()->getPathInfo());
    +  if ($suggestions = theme_get_suggestions($path_args, 'page', '-')) {
    

    We should file a follow up to not rely on the incoming path but actually the system path.

joelpittet’s picture

Status: Needs review » Needs work

I guess we should status this issue too (from @YesCT) ;-)

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
196.32 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,074 pass(es). View
142.33 KB

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

Wim Leers’s picture

FileSize
192.2 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,006 pass(es). View
141.28 KB
40.37 KB

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

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

joelpittet’s picture

@Wim Leers re #73 and #78
{{ content }} to {{ page }} That's cool with me thanks for the change.

Sorry I didn't realize that the keys for page_bottom and page_top were nested in 'html' key and you had to pull them up out of that, was aiming for a smaller patch size with that suggestion. I'll dig into that a bit later. It appears as that shouldn't be the case but... huh.

Wim Leers’s picture

Issue summary: View changes
FileSize
304.56 KB

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

Nice profiling! Yeah I must be missing something because those variables are not in page.html.twig but html.html.twig, that's where I'm lost.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Re #74:

#2 Well ... type HTML should rather get the CSS/JS explicit as variable passed in, but sure this is so out of scope here.
It is just one thing we could do in the future.

#3: Ah, I can imagine how people thought this might be a sane idea but yeah I don't think that additional level of abstraction is worth in general.
We just stumbled upon it, while reading the code, so we tried to understand it. This was not a critique against the code at all.

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

Ah, this is a good point, now I am though confused why you changed it. I will reread your and my answer in another timezone ..., sorry.

#75:

#2:
Well, the distinction could be made by the caller itself, well I don't want to block this really important patch for that,
so let's file a follow up and try to come up with a simpler solution in the future.
#6:
Still think that the other issue just don't care about the crazy dynamicness of Drupal. Drupal is based around features, not around clean architecture.

#8:
I assume that this was always the case ... but yeah this is the way to work around double escaping everywhere :)

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

+1 for follow ups

#10:

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.

Well, its not only the optimization, it is about avoiding the logic. Priority should be the way to go, not anti-priority.
The normal request flow also uses that kind of propagation logic, so the render flow should work similar.
I will RTBC this patch, in case we agree to discuss that and maybe still be able to change that behaviour later.

#13

No need for a follow-up, did that right here. This probably also addresses point 12 then?

Thank you.

#14

Now that is interesting! You're absolutely right!

High five!

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

Another plus point, we decreased the patch size with this.

#17:

Someone should read the documentation :p

Oh well, we just did not read the documentation together ... I think we maybe should have a dedicated "review" follow up to read the documentation.

#19:

Ok, feel free to file that follow-up; this is just moving existing code around, so completely out of scope.

Valid response.

#81:

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.

So the previous level of abstractions HTmlFragment/Page has never been a performance issue, right?

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

Okay, feel free to be killed by mark. There is no point in running AB here ... you want to test drupal not apache. You could use just a timer in
your index.php and be done.

Fabianx’s picture

The profiling is fine. I disagree that ab is not giving a good idea of an direction of where performance is headed, it is not precise, but it gives a general idea and here it was not used as the single thing.

In most cases ab benchmarks did match up with xhprof-kit / XHProf Lib analysis very much.

Of course we can say AB, XHProf, etc. all screw the statistics with their overhead, but in the end this is not about being statistically correct, but to ensure we don't have a huge performance regression or even some improvement. And that do those tools ... Just my 2c.

To the patch: RTBC + 1

znerol’s picture

When profiling with ab, then use -k (keep-alive = no connect overhead) and -c1 (no concurrency). Only use -c > 1 when load testing servers (i.e. verifying MaxClients settings, etc.).

Wim Leers’s picture

#83:


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

catch’s picture

#81.A yes there's no performance issue in terms of where HtmlFragment/HtmlPage are used currently.

However HtmlFragment being the final representation of string plus assets with no support for nesting/post_render_cache means it is not applicable anywhere outside of the very top level of page rendering - see the discussion in #2334219: Make blocks expose HtmlFragmentInterface directly which was part of prompted this one. i.e. if HtmlFragment was applied anywhere without adding at least some of the features of render arrays in terms of support for nested elements, there would have been a serious and unresolvable cacheability issue. No more ability to cache a mini-panel at the block level, for example.

The remaining argument for having it was 'DX', but there's two issues with that - 1. The implementation was incomplete and added complexity/confusion rather than removing it, see for example #2218271: Theme preprocess functions have to futz with $page_object = $variables['page']['#page'] in weird ways 2. Very few modules interact with the top level of page rendering and would be able to use the API, the rest all deal with render arrays everywhere anyway. The eventual answer to that will probably be #1843798: [meta] Refactor Render API to be OO instead, and we can make incremental, bottom-up steps towards that during the 8.x cycle, and already have done to an extent with things like CacheableInterface.

Wim Leers’s picture

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

catch’s picture

I'll probably commit this later today or early tomorrow, since discussion here has slowed down and there's wide agreement on RTBC.

Gave it one more look through and had the following minor nitpicks:

  1. +++ b/core/lib/Drupal/Core/Render/MainContent/DialogRenderer.php
    @@ -0,0 +1,92 @@
    +class DialogRenderer {
    

    And shouldn't this implement MainContentRendererInterface?

  2. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -0,0 +1,304 @@
    +class HtmlRenderer {
    

    And here too.

  3. +++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
    @@ -0,0 +1,304 @@
    +    static::invokePageAttachmentHooks($page);
    

    Why static:: here? Can't see a good reason, need a comment if there actually is one.

Wim Leers’s picture

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

I'll provide a reroll.

Fabianx’s picture

+++ b/core/lib/Drupal/Core/Render/MainContent/HtmlRenderer.php
@@ -0,0 +1,304 @@
+    // The special page regions will appear directly in html.html.twig, not in
+    // page.html.twig, hence add them here, just before rendering html.html.twig.
+    static::buildPageTopAndBottom($html);
+
...
+   */
+  public function buildPageTopAndBottom(array &$html) {
+    // Modules can add render arrays to the top and bottom of the page.

This does not inhibit RTBC status, but I was very confused when I read that ...

as the function is just public and not public static ...

I wonder how that does even work ...

dawehner’s picture

According to http://3v4l.org/8ArS9 this seems to be "just" a strict error, but sure fatals, in case $this is called.

Wim Leers’s picture

FileSize
192.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,153 pass(es). View
141.36 KB
2.47 KB

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

catch’s picture

Yes PHP lets you do that, just about, I've always assumed it's because the same syntax is used for parent:: which works for both static and non-static methods (and with no other option). Which isn't a good argument but seems plausible.

catch’s picture

Interdiff is missing

static::invokePageAttachmentHooks($page);
Wim Leers’s picture

FileSize
192.28 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,195 pass(es). View
141.36 KB
800 bytes

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

  • catch committed 488116d on 8.0.x
    Issue #2352155 by Wim Leers: Remove HtmlFragment/HtmlPage.
    
catch’s picture

Status: Reviewed & tested by the community » Fixed

OK that's all the nitpicks I had, and the rest has been reviewed extensively by several people now. Committed/pushed to 8.x, thanks!

See you in the follow-ups :)

Fabianx’s picture

Yeah!

almaudoh’s picture

Great job, guys! You rock!!

amateescu’s picture

Status: Fixed » Closed (fixed)

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