Updated: Comment #N

Problem/Motivation

In Drupal 7, a separation between html.tpl.php and page.tpl.php was introduced. This separation seems unneeded and makes it harder to theme Drupal. Merging the files back to how it was before D7 would make theming Drupal sites easier and faster, and it gives the themer more control of the HTML.

Proposed resolution

  • Combine html.html.twig and page.html.twig to page.html.twig
  • Use twig blocks for the maintenance page (and name it page--maintenance-page.twig).
  • Combine template_preprocess_html() and template_preprocess_page() to template_preprocess_page().
  • Update drupal_common_theme() to combine 'html' and 'page' theme settings and re-use the existing page render classes.

Remaining tasks

  • Combine html.html.twig into page.html.twig Twig Templates
  • Combine *_preprocess_html() into *_preprocess_page() and adjust variables for the page.html.twig template
  • Make the pages render correctly
  • Wrap twig blocks around the variable parts of the template, likely head and body blocks?

User interface changes

None

API changes

  • Removes template_preprocess_html().

Original report by @mortendk

Why:
1 themers arent idiots & can understand that "dont remove {{ page_top }} {{ page }} {{ page_bottom }}
if you do this - stuff wont show up- there done

2. Less files makes it easier to figure out wtf is going on

3. if you wanna do variations over the same base fx adding in elements in the html.html.twig file and then have variations with the page.twig files you can do that with "twigblocks" (blogs in twigs)

blockers:
twigblocks must work (they do)

Comments

richardj’s picture

Copying my comment from issue #1982256

I would say, keep the html.html.twig and the page.html.twig, the thought behind this is: you put your html and head into html.html.twig so that is kind of a static file, then within that you load all your different page.html.twig files.

I think most framework work with this in mind and personally i don't want to keep track of all my html / head elements in all the different page.html.twig files (as we did in Drupal 6).

To elaborate, I don't know the exact possibilities with Twig, but i do know that in the past (Drupal 6 and down) everything happened in the page.tpl, when copying this you had the problem of multiple ways of loading that part of your html. When merging these files again you will add the overhead of having to load everything in depending on the page.html.twig you are using.

As with less files, we are talking about one html.html.twig file that you don't even have to use (because of the core one).

dbazuin’s picture

We could put the html stuff in blocks in page and so make it easy for people to just override that part.
Then we have one less template and still have the same (or maybe even higher) level of flexibility.

richardj’s picture

@dbazuin Please elaborate on the blocks part you are talking about, is it Drupal blocks or Twig blocks or other blocks?

Doctype and things in the are things that normally you don't have to change that often, not a lot of if else statements in there, it just is, when we are merging this into one file this means we will have multiple times the doctype declaration / in code.

So basically what other systems are doing is (and i'm using pseudo code for this):

<!DOCTYPE html>
  <head>
   head stuff
  </head>

  <body>
    <? LOAD_STUFF ?>
  </body>
</html>

So @mortendk, themers are not stupid, but having separation of code is a way of keeping it cleaner in my opinion, also it doesn't create extra points of failure.

But if blocks can be used to load that in page.html.twig with {{ page_top }} I would be fine with that as well, but where is the page_top declared?

dbazuin’s picture

The term block is a bit confusing I understand that now.

I mend twig blocks or maybe some other snart twig thingy.
I not sure what twig things are also working with Drupal.

Maybe this is something for the Bof tomorrow.

dbazuin’s picture

Issue summary: View changes

Updated issue summary.

Cottser’s picture

Title: merge html.html.twig & page.html.twig into one file » Merge html.html.twig & page.html.twig into one file
Issue summary: View changes

Twig blocks work, updated issue summary.

Cottser’s picture

Category: Feature request » Task
Issue tags: +sprint

I really think we should do this.

LewisNyman’s picture

If you want to keep your code DRYer you could easily create your own includes. This change brings our templates closer to other frameworks (eg. Jekyll) so it's a good thing in my mind.

BarisW’s picture

Assigned: Unassigned » BarisW

I will start working on this now (in the DrupalCamp Twig sprint). The idea is to remove the html.html.twig and move its variables & regions into page.html.twig, and combine the template_preprocess_page & template_preprocess_html accordingly.

BarisW’s picture

It seems that this is a lot more complicated than anticipated. I need some help here from a developer.

I've combined template_preprocess_html() and template_preprocess_page(), and merged the .twig files. However, the page renderer (DefaultHtmlPageRenderer) is not run anymore so this needs to be changed first. See 'html' and 'page' too in drupal_common_theme().

So, how to move on? I can upload my changes as a patch, but if you apply the patch then, you won't see any output.

BarisW’s picture

Assigned: BarisW » Unassigned
FileSize
28.64 KB

For those who can continue with this issue, I've applied the patch.

BarisW’s picture

Issue summary: View changes
BarisW’s picture

Issue summary: View changes
joelpittet’s picture

This will make things show but there is a bit of nested doll thing going on because the

DefaultHtmlFragmentRenderer::render function passes the page back into the #content. It's a bit late but thought I'd nudge this forward a bit.

BarisW’s picture

Assigned: Unassigned » BarisW

Thanks so much joelpittet. I'll test it asap and continue working on it.

joelpittet’s picture

Issue summary: View changes

updated summary with template.

joelpittet’s picture

Issue summary: View changes
Cottser’s picture

Issue tags: +Template consolidation

Definitely qualifies as consolidation!

BarisW’s picture

Assigned: BarisW » Unassigned
FileSize
32.57 KB

Getting there bit by bit. The patch in #13 didn't apply anymore to HEAD, so here's a re-roll. Also, it contained a double theme_preprocess_page(). With the attached patch, you see some output (header, footer, tabs, etc) except for the 'content' region. I can't get that to output.

So again; if there's a developer that could help out, please do.

nicholasruunu’s picture

Status: Active » Needs review
FileSize
32.58 KB
592 bytes

Ok, the problem was/is that template_preprocess_page is being called another time in DefaultHtmlPageRenderer.
I managed to render some content atleast, hopefully it helps someone to understand this.

BarisW’s picture

FileSize
34.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,488 pass(es), 299 fail(s), and 52 exception(s). View
1.71 KB

Ah, great catch nicholas. That was indeed the problem.
I've fixed Seven now as well. Let's see what the testbot thinks.

BarisW’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 20: 2090967-20-combine-html-page-templates.patch, failed testing.

rteijeiro’s picture

nicholasruunu’s picture

I noticed that node/add and other admin pages is still crashing, just got some content out on the startpage from fresh install.
This is probably not even the right way to go about it, just thought it might help with debugging.

The last submitted patch, 20: 2090967-20-combine-html-page-templates.patch, failed testing.

joelpittet’s picture

@nicholasruunu Could you let us know where the two places you found the template_preprocess_page were getting called? That may help.

nicholasruunu’s picture

@joelpittet
In patch 18 it was \Drupal\Core\Page\HtmlFragment and \Drupal\Core\Page\HtmlPage

Update:
Actually, let me look this over tonight, might have been me experimenting with drupal_page_render.

nicholasruunu’s picture

Ok, I confirmed that template_preprocess_page was being called twice in patch 18.
The debug backtrace is appended.

Update:
I don't know if it was because I hadn't cleared cache or @BarisW fixed it, but node/add and so on seems to work now.

Update:
Ok, the same tests seem to fail with the 8.x branch.
One problem I found was that the root_user doesn't get the administrator role in the test, so you get 403 on the admin pages it tries to test.

tim.plunkett’s picture

If you read through #469242: Move <head> outside page.tpl.php, which was where they were originally split out, it says that the split also helped resolve a security issue: #449142: SA-CORE-2009-005 and SA-CORE-2009-006 - Drupal core - Cross site scripting

Furthermore, I take issue with this statement:

themers arent idiots & can understand that "dont remove {{ page_top }} {{ page }} {{ page_bottom }}

When I started Drupal as a themer, one of the first mistakes I made (other than putting contrib modules in /modules!) was to delete page_bottom from my page.tpl.php.

The resulting bugs you get from doing that are not obvious, and can go undetected for a long time.

Also, looking through themes I've worked on recently, the natural split between template_preprocess_html and template_preprocess_page is a nice one, and keeps things clean.

Finally, on the off chance I've used a template suggestion for page templates, it was really nice have one less set of changes to make to all page templates if something had to change in the <head> element (like recently, removing markup for Google Chrome Frame).

Despite not being known in the Drupal community as a themer, it still is a large part of my $dayjob, and I can say that I'm firmly -1 to reverting back to the D6 single template approach.

nicholasruunu’s picture

I kind of agree with @tim.plunkett actually, doesn't make sense to have to change code in more files than one when working with stuff in <head>.
There's still some stuff you have to change there, for OG tags and so on.

joelpittet’s picture

Re #29.

I agree with the deleting variables sentiment, I've done that with $messages, $title_suffix and realized that I have no debugging or contextual links.

Right now we are only merging and getting that working but the idea is that we can still split the head/foot wrapping the page with twig blocks @see http://twig.sensiolabs.org/doc/tags/extends.html

A side benefit is the variables would be shared for both templates as there would be only one preprocess. This would make a bit more flexible where things get put in the templates.

So yes the separation is really nice in D7 and we don't want the D6 code duplication but do want the simplicity back. So this is an attempt to provide both.

nicholasruunu’s picture

Ah, thanks for the clarification @joelpittet, makes more sense than what we had in D7.

nicholasruunu’s picture

Status: Needs work » Needs review
FileSize
34.15 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,720 pass(es), 313 fail(s), and 52 exception(s). View
561 bytes

Ok, these tests are driving me crazy, let the bot run them.
Merged in the new changes and removed notices, see interdiff.

Status: Needs review » Needs work

The last submitted patch, 33: 2090967-21-combine-html-page-templates.patch, failed testing.

tim.plunkett’s picture

Where is the issue for what is described in #31?
Because while I've *never* needed template suggestions for html.tpl.php, I would not be sure how to have suggestions for twig blocks.

joelpittet’s picture

This is the issue for what is described in #31. And if you have never needed suggestions for html.tpl.php then this should be great because when you split the blocks out as the example shows on the twig documentation the suggestions and preprocess would only apply to page.html.twig making page the editable set of variables and markup and the duplicated parts in html.html.twig separated out similar to what we have now. The sensio docs show an example better than I can explain it.

This is kind of an experiment to see what is possible with twig. And from the looks of it the patch is getting really close, nice work @nicholasruunu and @BarisW. I can actually get around the pages now with no duplication or errors. Once those testbot errors are cleared up, we can attempt the twig block splitting.

Wish I could help a bit more than fielding questions, bit backlogged at work:(

sun’s picture

I agree with @tim.plunkett.

In addition, the issue summary needs a fundamental/substantial clarification on how you propose to resolve the following problem space:

The split between the page and html templates is what allows render arrays to #attach asset libraries and add other elements to the HTML <head> when they are rendered in the page template. template_process_page() and other preprocess functions are adding/manipulating those elements, too.

The only reason this works is because the HTML <head> has not been printed out yet.

In order to merge the page and html templates, you need to resolve a circular dependency in the page rendering chain:

  1. The HTML head is rendered before the page content.
  2. The page content is rendered after the HTML head and needs to be able to add or change the HTML head.
  3. Both the HTML head and the page content need to stay drillable variables, because if they'd be pre-rendered strings then the entire template would be useless.

I'd recommend to move this issue to D9 or won't fix it.


FWIW, #2218039: Render the maintenance/install page like any other HTML page eliminates the two special cases of the maintenance_page and install_page templates, which are the only page templates that include the html template. The code paths for those currently require tons of complex and ugly code, in order to get anything printed at all, and themes are not able to work with those pages in normal ways.

mortendk’s picture

ok im kinda ok with that, if it turns out to be a huge pita

nicholasruunu’s picture

I actually don't think @sun's concerns is an issue, but I'll look into it more, just haven't had the time.

joelpittet’s picture

I agree with @nicholasruunu, we may run into those issues @sun mentioned and they are good to be aware of during this but I'll jump on this later on if nobody gets around to it and if it gets pushed to d9 so be it. I'd like to see this issue through to either yay moment or a I give up moment;)

And I don't give up easily!

markcarver’s picture

I agree with @tim.plunkett and @sun here. There are very specific reasons why this has been done. Speaking from personal experience, changing the html template is very, very rare and usually never has suggestion overrides (none that I have ever seen either). Changes on this level are global and encompass the site as a whole.

Page templates OTH often get overridden (at the very least once or twice per a moderately sized site, more if a larger site). These changes usually are very specific to a node type or context/section of the site that isn't global. Merging these would then require that the header code is duplicated in every page--*.tpl.php as well. This makes replicating a change even more of a pain than it already is when dealing with page templates. This would actually decrease the DX tremendously. I certainly do not want to keep track of all that code.

This has been debated and the reasonings behind separating them in the first place has also already been stated very clearly. Marking as won't fix.

nicholasruunu’s picture

Did anyone even read @joelpittet's link? That's how Twig is meant to be used:
http://twig.sensiolabs.org/doc/tags/extends.html

There would still be clear separation.
We need to get away from the thinking we've had with PHPTemplate/pure HTML, this is a compiled view language.

Again, this change wouldn't make you have multiple html templates, you'd extend one html template.

sun’s picture

Twig's support for block + extends are certainly able to mitigate the code duplication problem.

However, they are not able to resolve #37, unless you'd require page templates to produce output in a very weird processing order; i.e., the "page" block would have to be generated first + upfront, and the result of that is embedded into the actual main page output later on (which all happens within a single template).

I'm surprised that the current patch even got this far in terms of passing tests. Most likely, we're missing some fundamental test coverage...

nicholasruunu’s picture

@sun, Maybe I'm lacking some understanding about Drupal, but Twig shouldn't generate output separately, everything should be generated the last thing that happens.
Why isn't that an option?

joelpittet’s picture

Status: Closed (won't fix) » Active

@markcarver I think that was a bit premature to close. There are people actively looking at this issue and working on it, it's not stale and has some potential. Find me on irc to discuss.

I'm not against moving it to d9 but don't want to close the discussion off.

markcarver’s picture

@joelpittet, yes. Sorry. It wasn't clear that we'd be using Twig's {% extend ... %} functionality. I literally thought we were moving code into page.html.twig. Regardless, given the current situation of the theme system/render "api". I really don't think this would work very well. Maybe something we can work on in contrib and move into 9.x for sure. Adding some more related issues too.

markcarver’s picture

Title: Merge html.html.twig & page.html.twig into one file » Extend page.html.twig with html.html.twig

Also changing the title. We're not actually "merging" the files.

markcarver’s picture

Status: Active » Needs work
joelpittet’s picture

Good call on the title change I was thinking that as well. It seemed to cause some negative reactions:S.
The merge needs to happen before the split again... so yes merging, but no not merging!:D

Cottser’s picture

Issue tags: -sprint

Removing from focus board for now. I thought this would be fairly straightforward, I was wrong.

Cottser’s picture

Status: Needs work » Postponed

Postponing because 9.x.

joelpittet’s picture

Good idea @Cottser thanks. It would be nice to have the page--maintenance.html.twig and page--install.html.twig page variation consolidation before this too.

But shouldn't stop people from giving the idea a try in their own sites;)

andypost’s picture

Looks mostly all asset-related issues are solved, so now it's time re-think this

andypost’s picture

andypost’s picture

Version: 8.0.x-dev » 8.1.x-dev

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Jeff Burnz’s picture

I don't see how this can possibly be backwards compatible, especially with regard to preprocess, I use both and do not want this feature taken away from D8.

The whole issue is a bit of a strawman to be frank. I see no evidence the current situation making theming harder, and I work with a lot of newbie themers.

The IS actually predicates this issue on "This separation seems unneeded", which is basically wrong.

This is 9.x material, not sure why this is even being entertained for D8.

lauriii’s picture

Version: 8.3.x-dev » 9.x-dev
Status: Needs review » Active

Moving to D9 since I couldn't think of how this could be done with a BC layer.