Updated: Comment #N
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.
html.html.twig and page.html.twig to page.html.twigtemplate_preprocess_html() and template_preprocess_page() to template_preprocess_page().None
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)
| Comment | File | Size | Author |
|---|---|---|---|
| #33 | 2090967-21-combine-html-page-templates.patch | 34.15 KB | nicholasruunu |
Comments
Comment #1
richardj commentedCopying my comment from issue #1982256
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).
Comment #2
dbazuin commentedWe 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.
Comment #3
richardj commented@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):
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?Comment #4
dbazuin commentedThe 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.
Comment #4.0
dbazuin commentedUpdated issue summary.
Comment #5
star-szrTwig blocks work, updated issue summary.
Comment #6
star-szrI really think we should do this.
Comment #7
lewisnymanIf 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.
Comment #8
BarisW commentedI 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.
Comment #9
BarisW commentedIt 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.
Comment #10
BarisW commentedFor those who can continue with this issue, I've applied the patch.
Comment #11
BarisW commentedComment #12
BarisW commentedComment #13
joelpittetThis 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.
Comment #14
BarisW commentedThanks so much joelpittet. I'll test it asap and continue working on it.
Comment #15
joelpittetupdated summary with template.
Comment #16
joelpittetComment #17
star-szrDefinitely qualifies as consolidation!
Comment #18
BarisW commentedGetting 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.
Comment #19
nicholasruunu commentedOk, 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.
Comment #20
BarisW commentedAh, great catch nicholas. That was indeed the problem.
I've fixed Seven now as well. Let's see what the testbot thinks.
Comment #21
BarisW commentedComment #23
rteijeiro commented20: 2090967-20-combine-html-page-templates.patch queued for re-testing.
Comment #24
nicholasruunu commentedI 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.
Comment #26
joelpittet@nicholasruunu Could you let us know where the two places you found the template_preprocess_page were getting called? That may help.
Comment #27
nicholasruunu commented@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.
Comment #28
nicholasruunu commentedOk, 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.
Comment #29
tim.plunkettIf 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:
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.
Comment #30
nicholasruunu commentedI 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.
Comment #31
joelpittetRe #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.
Comment #32
nicholasruunu commentedAh, thanks for the clarification @joelpittet, makes more sense than what we had in D7.
Comment #33
nicholasruunu commentedOk, these tests are driving me crazy, let the bot run them.
Merged in the new changes and removed notices, see interdiff.
Comment #35
tim.plunkettWhere 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.
Comment #36
joelpittetThis 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:(
Comment #37
sunI 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:
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.
Comment #38
mortendk commentedok im kinda ok with that, if it turns out to be a huge pita
Comment #39
nicholasruunu commentedI actually don't think @sun's concerns is an issue, but I'll look into it more, just haven't had the time.
Comment #40
joelpittetI 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!
Comment #41
markhalliwellI 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.
Comment #42
nicholasruunu commentedDid 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.
Comment #43
sunTwig'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...
Comment #44
nicholasruunu commented@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?
Comment #45
joelpittet@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.
Comment #46
markhalliwell@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.Comment #47
markhalliwellAlso changing the title. We're not actually "merging" the files.
Comment #48
markhalliwellComment #49
joelpittetGood 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
Comment #50
star-szrRemoving from focus board for now. I thought this would be fairly straightforward, I was wrong.
Comment #51
star-szrPostponing because 9.x.
Comment #52
joelpittetGood 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;)
Comment #53
andypostLooks mostly all asset-related issues are solved, so now it's time re-think this
Comment #54
andypostThere's green #2231853: Rename maintenance-page template into page--maintenance + install-page into page--maintenance--install
And workaround for #2372581: Rename 'page' render element to 'body', page.html.twig to body.html.twig
Suppose it's not hard to implement
extendnow, at least to tryComment #55
andypostComment #58
Jeff Burnz commentedI 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.
Comment #59
lauriiiMoving to D9 since I couldn't think of how this could be done with a BC layer.
Comment #61
xjmThis is a minor-only addition. Since 8.9.x and 9.0.x are now in beta, I'm moving this to 9.1.x. Thanks!