Problem/Motivation
Proposed resolution
Remaining tasks
User interface changes
API changes
During the DrupalCon Amsterdam discussion about page rendering, we decided that the following steps are necessary to fix #2327277: [Meta] Page rendering meta discussion:
Critical
- Remove
HtmlFragment&HtmlPage, but make sure that'#type' => 'page'works (because controllers should be able to return entire pages, without having the content be surrounded by blocks) — see #2352155: Remove HtmlFragment/HtmlPage - Prevent crazy asset handling and crazy page manipulation:
- For the valid use cases of adding content to the page, we introduce
hook_page_top()andhook_page_bottom(): for adding content to the very top or bottom of the page (not regions!) — see #2350949: Add hook_page_attachments(_alter)() and deprecate hook_page_build/alter() - Introduce
hook_page_attachments(), which may be implemented by both modules and themes, to allow assets to be added to pages conditionally. For assets unconditionally added to pages by themes,hook_preprocess_page()must be used. We keephook_page_build()around, for backwards compatibility — but we only allow assets to be added — see #2350949: Add hook_page_attachments(_alter)() and deprecate hook_page_build/alter()
- For the valid use cases of adding content to the page, we introduce
- Document how to attach assets correctly:
- Allow preprocess functions to cleanly attach assets for the associated template — see #2346369: Support special '#attached' variable for attaching assets in preprocess functions
- Documentation:
hook_page_attachments()to conditionally attach assets to pages,hook_preprocess_page()to unconditionally attach assets to pages — see #2350949: Add hook_page_attachments(_alter)() and deprecate hook_page_build/alter()
- Remove
drupal_add_html_head(),drupal_add_html_head_link()anddrupal_add_feed(), accept['#attached']['head'],['#attached']['head_link'],['#attached']['feed']instead — see #2350877: Deprecate/rename drupal_add_feed(), drupal_add_html_head(), drupal_add_html_head_link(), drupal_add_http_header(), and allow to be set declaratively in #attached
Nice-to-have
- Convert all
pagetemplate (page.html.twig) variables to blocks - Extract
RenderableInterface extends CacheableInterfacefromBlockPluginInterface. Begin with just abuild()method that returns a render array, during D8 cycle, gradually add more methods so that the render array doesn't contain asset metadata anymore, etc. — until nothing remains, at which point we'll have done a lot of the work of moving away from the Render ArrayPI.
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | tags.png | 84.75 KB | danny englander |
Comments
Comment #1
catchComment #2
wim leersUpdating as per the long "Hard Problem" meeting we had in Amsterdam. (Attendees: msonnabaum, dawehner, Crell, catch, effulgentsia, Wim Leers.)
Directly created from my conclusion notes, which I triple-checked with all attendees. This is the plan.
Comment #3
wim leersComment #4
wim leersComment #5
dawehnerOn my way home I tried to apply the following strategy: use render arrays everywhere where html page/fragments are used. This seems to be a) quite hard and b) does not solve really the problems we discussed.
One alternative would be to try to go back to the code how it looked like before HtmlPage.
Comment #6
dawehnerComment #7
catchComment #8
fabianx commentedSo the most important for the rendering chain is to have:
Begin of request:
$render = array(
'#pre_render' => 'process_page_etc',
);
drupal_render($render, TRUE);
To _ensure_ that everything that is rendered is part of the rendering stack, which
a) Allows #post_render_cache everywhere and that #post_render_cache is resolved at the last possible point
b) Allows e.g. different rendering strategies to work on the whole rendered page with a complete #post_render_cache set
It does not matter if the render chain is interrupted in between as long as the $stack is correct.
Comment #9
andypost+1 overall!
I see twig templates as pieces of render cache too, so here's another layer of complexity.
It's not clear how attached assets will bubble up to page template and the reason to use "head_link"? To be alterable?
The only question what to do if controller returns string?
Comment #10
catch@andypost preprocess gets to do #2346369: Support special '#attached' variable for attaching assets in preprocess functions as of today - for adding #attached for specific templates.
#2352155: Remove HtmlFragment/HtmlPage deals with the final bubbling up and that's the trickiest part of this. The current situation in HEAD gives the impression of handling bubbling partially, but it actually relies on the static variables in the various _drupal_add_*() functions as well as passing page/fragments in and out of render arrays and preprocess variables.
I actually think we could get rid of controllers returning strings and standardize on response vs. render array. Strings was a backwards compatibility layer for Drupal 6.
However the stacking in drupal_render() does allow early calls to still bubble up to the page level so it 'works' but not perfectly.
Comment #11
wim leersComment #12
moshe weitzman commentedI agree.
Comment #13
fabianx commentedI think controllers could return a RenderArrayResponse object for now to wrap the render array?
Also please note:
- We want to get away from calling the controller directly, but instead use a #pre_render callback to call its callable later, but have the route context available for all blocks (and the main controller): https://www.drupal.org/node/2274167#comment-9244301
It seems to be feasible with only around 630 test failures.
Where controller and block need to communicate they need to do so via contexts instead of global state like now (views_get_page_view() / views_set_page_view())
Comment #14
joelpittetI probably don't know enough about this, but why do we want to get rid of returning strings from Controllers?
That sounds really handy for prototyping if something is going to work or not and getting up and running and may have some use-cases were the output has been already generated and just needs to be sent to the browser.
Again, maybe there is a way around this and I'm overreacting a bit.
This is a "sanity check" piece of code I'd hope would work:
Which would shortly be followed by a more elaborate bit of code..., but also, I could dump an early return with some test data to the output of this controller.
I'm likely "doing it wrong" or something, but not being able to do that seems like a DX loss.
Couldn't this just be a
is_scalar($var) ? new Response($var);wrap on the "support this" side?Trying to avoid these kind of questions a bit too: (Warning ASP.NET code, NSW)
http://stackoverflow.com/questions/553936/in-mvc-how-do-i-return-a-strin...
Comment #15
wim leersOpened a small additional child issue: #2357937: Remove {{ feed_icons }} from page template (page.html.twig). It helps pave the way for #2352155: Remove HtmlFragment/HtmlPage and also helps with .
Comment #16
danny englanderI am adding a viewport tag from within my contrib theme and I came across this issue
but I am not sure if I should open a new issueor if this one covers the problem I am encountering. Update, opened a new issue. #2359987: Overriding core metatag using hook_page_attachments_alter results in two tagsComment #17
miroslavbanov commented@Danny Englander
You should probably open a separate issue. You want to override a meta tag, and it makes perfect sense to do that in page_attachments_alter, but you can't. This is related, but a separate issue is better.
Comment #18
wim leersAnother, bigger child issue to help pave the way: #2359901: Discourage $main_content['#title'] in favor of route titles and title callbacks.
Comment #19
catch#2352155: Remove HtmlFragment/HtmlPage is RTBC and the last remaining critical issue here, and reviews show that rendering is sufficiently untangled by that issue and the other sub issues of this one, so closing this meta out.