problem:
<head>
<?php print $styles ?>
<?php print $scripts ?>
...
</head>
<body>
<?php drupal_add_css(...) ?> // no effect
<?php drupal_add_js(...) ?> // no effect
...
</body>
you may say don't use those functions inside page*.tpl.php, but if you consider a code like this:
<?php print views_embed_view('myview') ?>
is the same as drupal_add_css() and drupal_add_js() as it needs additional js and css files, then it is quite a big limitation of page*.tpl.php
proposed feature: new template file
html.tpl.php
<head>
...
<?php print $styles ?>
<?php print $scripts ?>
...
</head>
<body>
<?php print $page ?>
</body>
this way the $page can be rendered first, and it would not require hacks to use special elements inside page*.tpl.php.
Comment | File | Size | Author |
---|---|---|---|
#294 | 294-html_template.patch | 33 KB | Nick Lewis |
#292 | 1701-html_template.patch | 31.28 KB | Nick Lewis |
#290 | html_template_no_281.patch | 32.97 KB | Nick Lewis |
#278 | html_template-242-again.patch | 32.97 KB | alexanderpas |
#277 | stark.zip | 20.1 KB | Nick Lewis |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis is partially related to #449142: SA-CORE-2009-005 and SA-CORE-2009-006 - Drupal core - Cross site scripting. One of the ideas in this other issue is to manage all the headers in a $head variable (thus including $head_title, $head, $styles and $scripts).
Comment #2
Pasquallethat would not fix this problem
the page needs to be processed before printing
<head>
EDIT: but yes, you are right, it is partially related to that issue..
Comment #3
Damien Tournoud CreditAttribution: Damien Tournoud commentedI have not said that it was solving the problem. I said it was related, and it is.
Comment #4
Pasquallesorry, little misunderstanding from my part..
Comment #5
Pasqualleproblems:
only changed the garland theme (and the $ie_styles is missing now. isn't there a better way to add ie_styles without changing the base html template?)
the #theme change inside drupal_render_page() seems like a hack
the maintenance page should be improved in another issue
the body classes moved to
<div id="page" class="...">
but IE does not support #id.class so the CSS changed from body.class to .classbut:
it works and the page.tpl.php is much more cleaner (no base html related variables) and much more usable (like views_ember_view() drupal_add_css() can be used inside it)
Comment #6
PasqualleComment #7
moshe weitzman CreditAttribution: moshe weitzman commentedGreat patch! We discussed this problem at length at #455844: Allow more granular theming of drupal_render'ed elements and we never thought of this. /me slaps head.
This patch can now undo some of the inconsistency we added there with #theme=page. Specifically, we can now stop rendering all the regions in template_process_page() and instead let the page.tpl.php do render($page['right'), render($page['highlight') and so on. even more flexibility for template authors. if render() looks unfamiliar to anyone, then see #455844: Allow more granular theming of drupal_render'ed elements.
Comment #8
yched CreditAttribution: yched commentedI don't get why those changes are needed:
Even though the templates are split in two, the structure of the rendered HTML stays the same, right ?
Comment #9
moshe weitzman CreditAttribution: moshe weitzman commentedCleaned up the patch to user render() in page.tpl.php. Also we now invoke html.tpl.php via
'#theme_wrapper' => 'html',
instead of #theme => html and doing an awkward theme('page) call in template_preprocess_html(). Cleaner this way.@yched i think there is a slight html change to accomodate $classes but i will let pasquelle handle that question.
Comment #11
Pasqualle@yched: I do not like the sloppy CSS selector either, but the problem is that
<body>
should be in html.tpl.php and the current body classes (like: front page-node one-sidebar sidebar-left) logically belongs to page.tpl.php.. So the answer in no, the rendered html is different now: body.class moved to #page.classComment #12
NaheemSays CreditAttribution: NaheemSays commentedIf there is going to be a required page-top-region on themes (#468534: Add a region at the top of the page above the header region.), would it be a good idea to have that set as part of this too?
Also, plus one to $closure being a part of this as otherwise it is easy to forget.
Comment #13
Pasqualle@moshe: I like the render(region) thing, but as I see the concept is not finished and probably should be done in other issue..
from the patch #9:
I understand, this is just mistyped, but you can see why I think the concept is not finished..
the #theme_wrapper is really great, I did not know this option..
Comment #14
moshe weitzman CreditAttribution: moshe weitzman commentedFixed that missing render(). I am coding here to current standards of HEAD. granular theming function name may or may not change. thats not really our concern here. hopefully this gets in first and then the other issue will patch our page.tpl.php if needed.
Comment #16
PasqualleI still not understand why do we have to fix two different issues in one patch, the patch is large enough for review already. So I left out the region rendering changes.
fixed:
garland IE style
css changes for all themes
changed "body.class" to "body .class" in css, I think it is better than a simple ".class". But I am still unsure about this.
Comment #17
Pasquallewith unix style line ends this time
fixed a missing "body " for 1 css selector in garland's style.css
Comment #18
moshe weitzman CreditAttribution: moshe weitzman commentedNo problem - I will elaborate. The only reason why page.tpl.php currently uses print instead of render is because we were losing the css and js that were added during rendering. This patch fixes that problem (hurray!). So, we are now free to do whatever we want in page.tpl.php But we have already made the decision of how to handle rendering in templates. See #455844: Allow more granular theming of drupal_render'ed elements. We have chosen to use the render() function in order to give the template author flexibility. Thats in HEAD, and thats our strategy going forward. All Drupal patches have to adopt HEAD strategies like "insert queries use db_insert(), filtering on output ....". Granular themeing is a new strategy, but it is just as strong and valid. this patch cannot choose to ignore it.
A site builder can certainly choose to do rendering in preprocess if he likes his templates cleaner. But core is not doing it that way (after much debate).
Comment #19
Pasquallepatch rerolled with render()
Comment #21
Pasquallefixed garland header and the maintenance page
Comment #22
PasqualleComment #23
yched CreditAttribution: yched commentedThe patch moves from (simplified):
to
Meaning the styles of elements in top-region can not rely on the $classes anymore. Might be surprising.
I'm not sure I completely understand why the $classes cannot remain on the body tag (thus, moving from page.tpl to html.tpl).
But if there's a strong reason, then it seems the whole page.tpl.php needs to be wrapped in a new div that receives the $classes ?
As for the CSS changes : well,
body .drag {
is just a more verbose.drag {
, right ?Couldn't we have something like :
I know IE6 doesn't parse
.foo.bar
selectors, I don't remember if#foo.bar
is OK...Comment #24
Pasqualle#foo.bar does not work in IE6, but this might be interesting: http://blog.solutionset.com/wpmu/2008/02/15/internet-explorer-id-class-bug/
I do not know why page_top was put outside the #page, it seems like it always needs a full width..
the $classes depends on page variables, it is constructed in template_preprocess_page, it should be used in page.tpl.php. It has no connection to html.tpl.php, which is the simple html skeleton only. It would be like adding node variables to page.tph.php.
But I am definitely open to ideas, as I do not like the "body .class" selector either..
Comment #25
alpritt CreditAttribution: alpritt commented1. #484820: Initial D7UX admin header went in with CSS hacks for IE6 since there was no easy way to add IE conditional comments to every theme. This looks like an opportunity to rectify that. (In a follow up issue, of course.)
2. On a related note, the current patch also breaks the new admin header because any references to body classes in the CSS and JavaScript files are now incorrect. (I haven't checked if anything else is similarly affected, but it is likely.) So either this needs to be updated, or we should perhaps question if it actually is the logical choice to move the body classes into the #page div.
Comment #26
NaheemSays CreditAttribution: NaheemSays commentedNow that #468534: Add a region at the top of the page above the header region. is actually in, this patch could potentially refactor that out and into html.tpl.php too?
It might make the life of themers/there users easier if things that cannot be changed are taken away from them to change. Just like moving the $closure out of page.tpl.php that this patch does.
Comment #27
tic2000 CreditAttribution: tic2000 commentedA re-roll with improvements.
I used a wrapper div with id body that wraps everything (the toolbar uses $classes too). I modified toolbar.css, toolbar.js, tabledrag.js and tableheader.js to use the #body div instead of the body.
This one solves #510108: Call theme closure function in process_page, not preprocess_page too.
Comment #29
PasqualleOk, this body.class problem is too much to solve here. I just moved the class back to body. I noticed, there are node variables in page.tpl.php so this page $classes variable in html.tpl.php won't be something new..
I used drupal_static() to copy $classes_array from template_preprocess_page() to template_preprocess_html(), I am not sure if it is the right solution.
Comment #30
PasqualleComment #31
catchReally like the look of this, only skimmed through the patch but as long as we keep body.class the rest looks nice.
Comment #33
tic2000 CreditAttribution: tic2000 commented@Pasqualle the patch in #27 solves the class problem.
New patch, this should pass the tests.
Comment #34
PasqualleIt may solve the problem but it looks really ugly:
Comment #35
catchI don't think there's any semantic issue with putting body $classes in html.tpl.php - modules add all kinds of stuff to $classes based on things within the page, that's part of what makes it useful.
Comment #36
tic2000 CreditAttribution: tic2000 commentedYou can replace that with only #body if you really want to.
Comment #37
PasqualleRe #29 test fail:
the CascadingStylesheetsTestCase->testRenderInlineFullPage() in common.test seems to be wrong.
it searches for the string
but the actual html is now:
I think using drupal_add_css() is better than using drupal_add_html_head() to add the IE specific styles, as modules may want to change it.
Comment #38
tic2000 CreditAttribution: tic2000 commentedThe patch in #29 doesn't apply cleanly to HEAD.
And about the test error, even if strictly you are right, in real world that code wouldn't work because
<link>
inside<style>
doesn't work (you won't get a validation error because it thinks it's a comment, but IE 6 will ignore that too).I think we are stuck with drupal_add_html_head(), but honestly, I don't want modules to mess with my theme style.
I opened #510460: Improve "Cascading stylesheets" tests for the test.
Comment #39
Pasquallehmm, the same code is used in the devel.module
ok, there is some new magic in drupal_get_css() for inline styles in Drupal 7. There was no
<style>
attribute in D6.http://api.drupal.org/api/function/drupal_get_css/7
#259368: Allow Inline CSS in drupal_add_css
So, is it possible to use drupal_add_css() here somehow? I don't see it. I demand browser detection in Drupal :)
Comment #40
tic2000 CreditAttribution: tic2000 commentedCan you make a re-roll against HEAD with drupal_add_html_head() if we can't find a better solution?
I actually tried the code in IE6, so I know for a fact it doesn't work.
Comment #41
PasqualleComment #42
tic2000 CreditAttribution: tic2000 commentedEverything works OK except one minor problem. The "toolbar" class is not added.
To solve this you need to change toolbar_preprocess_page() to toolbar_preprocess_html()
I wonder, wouldn't be easier and cleaner to just move the opening body tag to page.tpl.php even if the closing tag will be missing?
Comment #43
tic2000 CreditAttribution: tic2000 commentedComment #44
Pasqualleno, the tpl.php files should be valid html on their own. For example I can validate the template files in eclipse now, that change would break it..
Comment #45
tic2000 CreditAttribution: tic2000 commentedWell, then move things in hook_preprocess_html, if they don't break anything (at least for toolbar that needs to be done)
Comment #46
Frando CreditAttribution: Frando commentedGreat idea and really solves the problem of not being able to late-render regions in current HEAD. Awesome.
To avoid the problems with body classes, why not just move the body tag into page.tpl.php?
So html.tpl.php would be
and page.tpl.php
Comment #47
tic2000 CreditAttribution: tic2000 commented@Frando
I think because $closure has to be in html.tpl.php
Comment #48
tic2000 CreditAttribution: tic2000 commentedHere is a patch with toolbar build move to toolbar_html_alter.
I hope some smarter people than me can point me what other $classes variables can be safely moved in hook_html_alter, or if none, to have this in soon.
Comment #49
tic2000 CreditAttribution: tic2000 commentedWe need a review, don't we?
Comment #50
Pasquallethis is wrong:
you wanted to change toolbar_preprocess_page(), but that would not be necessary either, if we move
from template_preprocess_page to template_process_page..
can someone confirm that it is ok to use drupal_static() here (to copy $classes_array from preprocess_page to preprocess_html), or should we choose another solution?
Comment #51
tic2000 CreditAttribution: tic2000 commentedAh, true :). And true if we move to process also
Comment #52
RobLoachOMG... Subscribing.
Comment #53
tic2000 CreditAttribution: tic2000 commentedA new version.
Moved drupal_static() to template_process_page()
Comment #54
JohnAlbinsubscribe
Comment #55
moshe weitzman CreditAttribution: moshe weitzman commentedI haven't looked too closely, but the copying of classes looks strange to me. Can't we ask folks to use preprocess_html instead of preprocess_page?
Comment #56
tic2000 CreditAttribution: tic2000 commentedWe can ask that in the future, but for now, to core modules we should do it in the patch.
Comment #57
quicksketchI like most of the ideas here and the code itself is an improvement (switching to render() calls), however I'm very hesitant about this split. The page.tpl.php file is the one file in all of Drupal that makes sense to new developers that are moving from Dreamweaver/HTML background. If we take this away and split it up into two files, we're loosing one of the valuable teaching tools in Drupal and further obscuring the rendering cycle. Overall, I'd prefer to not have the splitting, despite it's programmatic advantages.
I think we'll need more themer input on this before basing our decision on the developer merits alone.
Comment #58
tic2000 CreditAttribution: tic2000 commentedAdding tag so we get a review from themers
Comment #60
Pasquallerender() needs to be changed to print render()
http://drupal.org/cvs?commit=236980
Comment #61
tic2000 CreditAttribution: tic2000 commentedLet see where we stand.
I'll try to play around with some of the classes and se what can be moved and what can't.
Comment #63
tic2000 CreditAttribution: tic2000 commentedI missed some prints.
Comment #64
tic2000 CreditAttribution: tic2000 commentedNew patch.
1. It moves what I could from the $classes to template_preprocess_html. I don't know if $variables['node'] is used for osomething else than adding clases to body and nobody answered to the question on IRC.
2. I added html.tpl.php in garland folder too. It helped in adding the ie styles like garland does now, only that it uses a hook_preprocess_html instead of hook_preprocess_page. I think it will also answer to quicksketch concern, giving the themers the wrapper file too.
3. Changed toolbar module to add the classes in hook_preprocess_html too.
TODO
Move all $classes in template_preprocess_html and get rid of the drupal_static.
Comment #65
xmacinfoI am a themer and will review this later unless someone RTBC it before I get my hands on this.
I understand quicksketch concerns and my first reaction would be to forget about the splitting.
However, on the other side of the coin, I would like this split to happen. For example, on some themes, I may need 2 or 3 zzz.page.tpl.php files. For example, one for the frontpage, one for the regular pages and maybe one for the forum page. Actually a themer can create many zzz.page.tpl.php files. So by splitting the
<head>
from the<body>
we could define the<head>
in a single place in a single document and not replicate the code over many page.tpl.php files.My guess here is that all page templates would share the same doctype. But that's another issue.
This is my two cents. :-)
Comment #66
PasqualleI do not like that the html.tpl.php is added to the garland theme, that makes a wrong guide. Themes do not need to change anything in the html.tpl.php file.
Themers are getting comfortable with template files easily, and skinning the (original) page.tpl.php only, does not make a Drupal theme, so I do not see the split as a barrier for beginners..
Comment #67
tic2000 CreditAttribution: tic2000 commented@Pasqualle
I think it's a very good example. "Now the html is split and this are the files". You don't have to "dig" to find out where the head is rendered. I think html.tpl.php should be in Garland theme even if it's identical to the one in system directory.
Comment #68
Pasquallewhy would a themer care about where the html head is rendered?
Comment #69
NaheemSays CreditAttribution: NaheemSays commentedAs an occasional themer, this patch will allow me to forget about magic invocations of code to add all the stylesheets etc in. I see it as a good thing and IMO as much of the duplicated code as possible should be put in here so that themers cannot mess it up - which this patch is doing by moving $closure our of page.tpl.php.
So plus 1 and more for this as I think it will make things easier/moar better for themers.
Comment #70
tic2000 CreditAttribution: tic2000 commentedBecause he want to add IE specific styles for example, like Garland does.
Comment #71
RobLoachRegarding IE-specific styles like Garland, should we consider moving parts of Conditional Styles into core?
Comment #72
kika CreditAttribution: kika commentedCould we refer to html.tpl.php in page.tpl.php comment header and vice versa?
Comment #73
xmacinfoI think that html.tpl.php should not be included by default in a theme, or for that matter required.
However, nothing should prevent a themer to add this template to his theme folder, like he already does for all kinds of other templates.
As for Garland, if the html.tpl.php file is the exact replicate of the one in the system, I think we should not put in in Garland. But if in Garland we need to add this template to override some values, or for that matter force new CSS files to load, well, then we need to add this template in Garland. :-)
Comment #74
Pasquallehow the IE stylesheet is added in garland-6.x is just wrong. The solution to include an IE stylesheet is much cleaner in patch #63 and earlier.. The solution in "conditional styles" module is not clean enough either.
the best solution would be a browser detection and something like:
This way the css file would be ins the $css array as every other stylesheet.
And this would even work inside page.tpl.php. I wonder why themers do not use drupal_add_css() inside templates?
Comment #75
tic2000 CreditAttribution: tic2000 commented@Rob Loach
That would be a good idea.
@xmacinfo
html.tpl.php is not required in a theme folder as not required is page.tpl.php (not even in D6).
@Pasqualle
When I'm new to a "system" being a CMS to create a site or a piece of software on my PC that I can customize I like to have working examples. If I would be new to Drupal I would like to know where the html file is generated and I would like to see it in the default theme that comes with Drupal. But if people think it's not needed I will revert that to #63.
Comment #76
ipwa CreditAttribution: ipwa commentedI agree that conditional comments in core would be awesome. I can see the benefits of this patch, but I think it would add confusion to themers who are starting out. With the growing design community within Drupal we'll get a lot more people trying out Drupal theming for the first time. It is hard enough to get them to not be so afraid of tpl.php files, especially the page.tpl.php which is exactly like an html page, I think they would be a lot more scared of modifying the templates because they won't look very familiar. I saw this issue on #drupal-themes and here is a quick excerpt of my conversation with @ultimateboy:
[21:43] ipwa: I can see al the advantages of what's being proposed
[21:43] ipwa: but as a themer I have to agree with what quicsketch said completely
[21:44] ipwa: I would hate having to explain this to someone I'm teaching how to theme for Drupal
[21:44] ultimateboy: ipwa: indeed.. its my biggest fear with that patch. I love the idea, but it adds yet another layer of abstraction
[21:45] ipwa: I think the page.tpl.php is a great advantage over other theming systems because it's just like an html page, and when css/html designers finally have enough guts to open it, they see that everything is very familiar
[21:45] ultimateboy: ipwa: indeed.. and now they would open it and be confused
But I'm torned about this because it really is a good idea and I would like to have the additional template file, but I think we would make it a lot harder on newcomers, it'd be to good to get more themers to weight in on this. I'll ask one of my students if this change would confuse him, and comment back.
Comment #77
Manuel Garcia CreditAttribution: Manuel Garcia commentedOK, I understand somewhat what is being requested here, but I am against this move for these reasons:
I understand some CMS's do call tons of functions in such files, but drupal is heavy on separating logic from presentation, and is one of the reasons why I love it so much...
Comment #78
Pasqualle@Manuel Garcia:
1. there is no logic in the page.tpl.php file, and let me keep my option to write logic into template file when the solution is much cleaner than writing new variables for every small thing..
And you are now really late for asking "no function calls" in template files. Garland always had function calls and now there is the render() function which is a demanded request
2. no, that is not true. read the issue description, comments #7 and #65 for example
3. 100 or 101, what is the difference?
4. no it can't be, if you do not know which file (or even dynamic js Drupal.settings) you need to add like for example when using
views_embed_view('myview')
ortheme('quicktabs', $quicktabs)
. These are simple renderers and should work in page.tpl.php also as it works in every template file.I saw modules recreating the $css array in template_preprocess_page just to make the module work.. The page.tpl.php is crippled compared to other templates..
5. same problem as 4.
Comment #79
RobLoachRegarding IE6 CSS compatibility, I created #522006: Conditional Styles in .info files, since drupal_add_css has it. Let's move that discussion there and not derail this thread ;-) .
Comment #80
JohnAlbinD6 Garland has never been a good example of best practices. And, in fact, in D7 we removed all that programming cruft from its page.tpl.
Pasqualle, you, of course, can continue to do what you want in your tpls, but putting logic in tpls isn't a best practice. So let me spell out the best practices for theming:
print
,t()
, andif/elseif/else/endif
. And with the new granular theming patch, we addshow()
,hide()
, andrender()
to that list.Those are the rules that core themes must follow.
So your argument is incorrect; embedding a view in the page.tpl is not something we need to fix. And embedding a view inside a preprocess function has been solved in D7 since js and css aren't rendered until template_process_page().
So, I would mark this “won’t fix” except for Moshe’s comment in #7; page.tpl is inconsistent with regards to
render()
.Finally, I would say the solution to move all of the
<body>
tags contents into a separate tpl from the doctype, html tag, and head content is not a good solution. My reasons are the same as Quicksketch’s, ipwa’s, and ultimateboy’s. I also talked to Zarabadoo and a few others in IRC and they agreed on this point. The existing page.tpl has all the familiar markup that new themer's expect to see: doctype, html, body, divs, etc. Its sprinkled with a little PHP, but hopefully not enough to scare them off. Splitting the page.tpl as proposed destroys this gentle introduction to Drupal theming.Making things easier for programmers is not a good trade for making things harder for new themers. The programmers can handle the small difficulties better.
Comment #81
tic2000 CreditAttribution: tic2000 commentedThen this is a won't fix.
Comment #82
NaheemSays CreditAttribution: NaheemSays commentedgrrr... I disagree with this being a wontfix.
But it is all magic!
Have you just asked people used to existing drupal 6 theming what they think or new people too? I would think existing people may not be as happy - but the same people may also balk at the show(), hide() and render() changes - they are powerful, but they also take away from the ease of theming (and are functions). Of course these also violate:
But most people seem happy with this change as it allows more granular theming.
This patch on the other hand helps remove magic the the template file - and after that, the core themes do not need to have any logic in the page.tpl.php file!
This patch also fixes another big problem - it makes sure that template essentials are more likely to be preserved - removing $closure from a theme (more likely not adding it in the first place) is all way too easy and the only time you realise is when there is a module out there such as admin_menu that needs it and it breaks on the theme.
I think this patch makes theming much much simpler than before as it stops the need to copy set sets of code into new tpl files for theming leaving behind what actually is the theming data.
Comment #83
PasqualleComment #84
Pasqualleit is wrong that I can't do something in page.tpl.php what I can do in the node.tpl.php. like render() and even views_embed_view()
It is a bug, which needs to be fixed. The reasoning that new users "may" (there is no evidence) not be comfortable with it, is just wrong.
bug reports, I can find quickly:
#338588-6: Post access counter removes default views.css link from header if "view post access counter" is not set.
#332895-13: render quicktab programatically
please do fix them.. Please don't tell me how to hide the bug, fixing it is the right solution.
Comment #85
quicksketchThere definitely is a real problem here, this issue should not have been marked won't fix after JohnAlbin's comment, which explicitly said:
I agree with John (and Pasqualle and nbz), this is a real problem and it does need to be fixed. Especially considering that
render()
has moved into the "best practices" category, there's no reason why it shouldn't work everywhere (or as many places as we can make it) if it's a best practice. So I'd like to say (also in agreement with John) that this is not a "good" solution.However, if this the only solution we have (I certainly can't think of others) then I'm in support of it because it fixes a bigger problem than it causes. I'm not in the "beginner" themer category, so I'm not sure exactly how much more difficult this will make things. But I have taught several hundred people through theming courses, and the problem of drupal_add_css() not working in [theme]_preprocess_page() and page.tpl.php always comes up. I think with this patch it will be much less likely to be a problem, and with the introduction of render() I think it might be something we have to accept.
Comment #86
moshe weitzman CreditAttribution: moshe weitzman commentedlast patch looks good to me. just some old API docs lying around mention
in template_preprocess_page(). we don't do that anymore.
this is really close. lets not lose it.
Comment #87
tic2000 CreditAttribution: tic2000 commented@Pasqualle
Those are not good example of best practices. Like JohnAlbain said that should be in preprocess/process functions or even better a module, not in a template file.
The fact that it can be done in node.tpl.php is not a good argument too for the same reason.
As a developer I like the freedom, but I can't force that as an argument.
But aside that there are other good arguments for the change.
In this patch I removed drupal_static, but I don't like the duplication of code so I'm open to suggestions on how to better achieve the same goal.
Comment #88
tic2000 CreditAttribution: tic2000 commentedCNR of course.
Comment #90
tic2000 CreditAttribution: tic2000 commentedThis one should pass, but...
Comment #91
Pasqualle@tic2000: those are good examples, you will do it also in D7, I promise..
Comment #92
tic2000 CreditAttribution: tic2000 commentedI don't even use Views because I prefer to create my custom module for pages and blocks I need. So highly unlikely I will do it.
And even if I will, I'm a developer, not a themer, so it wouldn't be an argument.
But also the fact that you can do it it's not a good themer argument against it because you are not forced to do it, it just gives the freedom for those who want it or need it.
In the end the problem is if the split makes the life of themers so much harder or not. Does the benefits of it outweigh the difficulties it brings from a themer POV?
Comment #93
NaheemSays CreditAttribution: NaheemSays commentedShould that be moved from page.tpl.php to html.tpl.php? This is a region was made hidden by default in #511284: Add html_top and move the admin toolbar into html_top and it is a "magic region" that will need to be supported by all themes.
Comment #94
moshe weitzman CreditAttribution: moshe weitzman commentedIMO it shouldn't. It is a standard region. It's only special feature is that it does not appear in block UI.
Attached patch fixes up some comment text. i think it is ready to go. I changed to this bug fix as we are trying to fix render() calls in the page template.
Comment #95
Pasquallethis change makes life easier for a themers not harder..
re #80: a simple example why it is wrong to use _preprocess_page for embedding a view.
vs
so which solution would you teach to the themer? embedding a view in the page.tpl is something we need to fix
sorry for the off topic comment, but it is frustrating if I get criticized when I am right.
Comment #97
PasqualleI think we will need more PHPdoc for page_top and content regions. The content region is mandatory for all themes, but I do not know what happens if page_top is missing.. This will probably create some confusion.. I know, these regions were not created with this issue, so it could be solved later..
Comment #98
tic2000 CreditAttribution: tic2000 commentedTo move or not to move page_top regions is for another issue. Also making $closure a page_bottom hidden region like page_top is.
@moshe
You should base you patch on that from #90. To be goot to go we need a patch that applies and passes.
Comment #99
moshe weitzman CreditAttribution: moshe weitzman commentedSorry about that. I am just uploading tic's patch in #90 since my changes were less than trivial. Also marking this as RTBC as it passed bot today and bot will retest in a few mins anyway.
Comment #100
sunNice patch.
There was no leading white-space previously in page.tpl.php, can we keep it that way?
Comment #101
webchickI decided to do some research on this since the opinions are pretty split down the middle on whether this change is going to be too hard for new themers to grasp.
Something that's interesting to note is that WordPress already has a similar split, although instead of html.tpl.php, they have both header.php and footer.php, which respectively contain everything up until and after the "content" area, which btw is inexplicably in 'index.php'. There's also a file called 'theloop.php'... why is Drupal theming harder than WordPress again? :P I've uploaded some samples from whatever the first theme was in the list for reference.
Apparently WordPress themes are all about sticking whatever PHP function randomly comes to mind directly into their templates. We should therefore assume that new Drupal themers are going to want to do what Pasqualle describes and try and stick function calls directly in their .tpl.php files, even if they ought to ideally be using the best practice method of preprocess functions or modules.
With that knowledge, I'm a little more comfortable moving in this direction, but I'd like to get Dries's two cents since it's quite a fundamental shift from what we've been doing.
I'll need to read this a bit more in-depth but I'm a little confused why we're rendering out every single variable in page.tpl.php when it seems like a print will do fine. For example:
IMO, $page_top should be part of html.tpl.php. It is analogous to $closure, which needs to be renamed to $page_bottom and converted to a proper system region in another issue for symmetry. There's also a blank line before the DOCTYPE line which will cause XHTML validation errors and needs to be removed.
Comment #102
tic2000 CreditAttribution: tic2000 commented@all
Can we leave the page_top and page_bottom for a follow-up issue?
If we don't we will have another 100 comments discussing if that is a good or bad move and if the implementation is correct or not. And on top of that we will just forget about the kittens and get a puppy instead.
I promise I'll open an issue and provide a patch the moment I see this got committed.
@webchick
$highlight is a region. You need to use drupal_render or render on it before you output it like you do for any region. The code that did that in the process function was removed and all render functions are now in page.tpl.php. A simple print $highlight will do nothing since $highlight no longer exits inside $variables. A print $page['highlight'] will output Array().
Here's a patch without the empty line.
Comment #103
tic2000 CreditAttribution: tic2000 commentedOnly an empty line removed and test bot is happy so back to RTBC
Comment #104
Crell CreditAttribution: Crell commentedUm. "People coming from another system with a lame theme layer are going to expect ours to be equally lame, so we should let them do stupid things." No, we should make it as easy as possible for them to stop doing stupid things and learn a much better approach. Function calls in template files should work, yes, but we should by no means be encouraging it. (That's why Garland was cleaned up to not be everything we say not to do.)
Comment #105
eaton CreditAttribution: eaton commentedBecause these elements are completely optional in Wordpress theming. You can include a 'header.php' and 'footer.php' and 'sidebar.php' file if you want to split things out into multiple files, but there's nothing preventing you from implementing your entire theme in a single php file. This is arguably bad for reuse, but the fact that you don't have to use dozens of very granular template files to assemble a proper design is one of the things that smooths the learning curve.
IMO, we Drupal developers have spent a long time obsessing about the fact that PHP Functions are called in template files, and trying to eliminate as much of that as possible. Along the way, though, we've arrived at a place where a given page is constructed out of -- literally -- many dozens of separate tpl.php files scattered all over a drupal installation. There's no way to create a 'high level' template that consolidates all of them, due to our inside-out rendering pattern.
Learning the deeply nested Russian-Dolls structure of a Drupal theme is, IMO, the hardest part for new designers. Far harder than the idea of calling a PHP function.
That's one of the reasons I'm very concerned about splitting the head tag off from page.tpl.php. Unless I'm missing something, we gain very little from this change. The ability to have the admin header bar located above the main content in the page source, and the ability to call drupal_add_js() in a template file. That's it. Am I not seeing something?
Comment #106
mfer CreditAttribution: mfer commentedMaybe I'm missing something. The original premise was to support things like views_embed_view() usage in something like a page.tpl.php file. The rule for how our theme system works is that this shouldn't be in a template file. That goes in a preprocess function. And, since the scripts and styles are built in a process function now (which files after the preprocess functions) the styles and css will just be added.
I already head complaints about how granular the template files are for themers/designers. Do we really want to add more levels to this? (I'm asking themers/designers)
Comment #107
webchickSounds like this still needs some more discussion.
Comment #108
NaheemSays CreditAttribution: NaheemSays commentedas a part time theme - for me, I think this makes things easier - html.tpl.php is a file that I would in most cases not have to touch at all. It also has the added bonus of getting rid of a lot of magic from page.tpl.php - wordpress for example has all the stylesheets hard coded in the head section, drupal does not.
with this patch I will not need to remember about $styles, $scripts, (eventually ) $page['page_top'] or $closure and I see that as a massive win.
I cannot see how this makes things harder for themers - it makes things simpler for me (and I am sort of dreading the render changes... I understand that it adds power, but it also adds a layer of complexity).
(as for wordpress not needing multiple files - that does not make things easier - the themers still have to come to terms with "the loop" and all blocks are hardcoded into the theme - you move them by editing the theme.)
I assume there is no logical place where we can add a poll for to get a polling of the themers? Just in case there is any doubt, I am emphatically in favour of this change and think it makes things easier.
Comment #109
NaheemSays CreditAttribution: NaheemSays commentedcross posted.
(chances are this will need an executive decision)
Comment #110
webchickI agree that it will probably need an executive decision, but said decisions are easier to make when lots of people affected by said change chime in to say whether they're for or against and why. :)
Comment #111
Zarabadoo CreditAttribution: Zarabadoo commentedI do not like this approach. I feel it is an unnecessary level of separation. For one, I sit squarely in the camp that most function calls should not be called in a tpl.php file. That is what preprocess is for. Better yet, if it is something like embedding a view and is actually adding content to the site, it should be done via module.
Yes, this possibly makes things more difficult for someone coming from another system. I will have to echo Crell on this and say we should not encourage what we are trying to avoid with Drupal's theme layer. I have always viewed this system to be more of a "push" style system where systems like Wordpress are more of a "pull". It is my understanding that the Drupal module system is for the generation of content that can them be sent to the theme layer for arrangement and display. That strikes me as a huge advantage over a system like Wordpress that seems to have a like of site functionality bundled right into the theme.
In my opinion, this seems to be more of a documentation issue than a heavy handed code issue. The philosophy that people are too dumb to understand, so a code based solution is the best solution is a bad one. We are going to have people that embed the php right into the tpl.php file and we should not close that door, but it should definitely not be encouraged.
Comment #112
Zarabadoo CreditAttribution: Zarabadoo commentedSorry... didn't mean to change the status.
Comment #113
Pasqualle@webchick: would you or anyone else revert #455844: Allow more granular theming of drupal_render'ed elements?
if not, then I really do not know what are we trying to discuss here.. The page.tpl.php is broken and the only solution is to "Move
<head>
outside page.tpl.php".Comment #114
mfer CreditAttribution: mfer commented@Pasqualle can you explain a case for why page.tpl.php is broken other than the initial issue with views_embed_view and drupal_add_js/css which should not be in a template file in the first place?
I think some of us may be failing to see the problem.
Comment #115
sun1) This separation allows us to add JS/CSS/whatever in page.tpl.php. Entirely not possible today.
2) Many users FAIL to insert $closure and sometimes even some other of the required variables into their custom themes. I cannot count the number of false bug reports and support requests that were solely caused by this. This patch almost eliminates this possibility, because I guess that people overriding html.tpl.php will know what they do.
Comment #116
stephthegeek CreditAttribution: stephthegeek commented+1 for sun's comments in 115. It allows for new flexibility/features, greater code reuse, and simplifies the main tpl file that a new themer needs to understand, while still leaving the html.tpl.php file available for those who need it.
Comment #117
mfer CreditAttribution: mfer commentedI'm not against this patch per say. A couple things...
1) Why would you add JS/CSS in a page.tpl.php? The drupal way, as it has always been explained to me is to NOT have those function calls in a template file but instead the template.php file. If you do this I don't see the issue.
2) In many of the themes I work on we have conditional stylesheets for IE. To add these you'll need to modify html.tpl.php or a template_process_html (with programming knowledge required). I would assume a lot of sites will be overriding what core does here.
I'm starting to wonder if the support requests will just change to different types of support requests. This adds another layer of complexity and removes some control.
For example, what if a theme has different information in the head based on different page-*.tpl.php files. You make that more difficult and require programming knowledge to do that here.
I will admit that having an html wrapper defined like all the other tags in drupal does make sense from a programatic point of view and that the current way deviates from the way they rest of the system works. I actually like this part.
But, it does add another layer of complexity to designers/themers who are already complaining about all the layers they have to deal with. We need to have an elegant solution and one that solves the problems of most of the people who are the front end developers.
Comment #118
xmacinfo+1 for sun's and stephthegeek comments in #115 and #116.
I've seen many newbies errors that could have been solved quickly by this split.
Comment #119
sun@mfer: We can entirely eliminate your use-case for altering html.tpl.php: #522006: Conditional Styles in .info files, since drupal_add_css has it
Comment #120
Pasqualle@mfer: Re #114
so if we can use a render() in node.tpl.php, then page.tpl.php is broken.
even if somehow you manage to hack render() into page.tpl.php, you still have problem with drupal_add_css() and drupal_add_js()
-> render()
--> drupal_render()
--->
----> theme () equals drupal_add_css() and drupal_add_js()
Comment #121
JohnAlbin@webchick Indeed, we need more discussion.
@quicksketch:
This is already fixed in D7. And we didn't need to slaughter our page.tpl's readability to fix it. Go ahead and try it. :-)
@nbz:
Actually, no. If you read the original thread where show/hide were added it was because they were seen as making things easier for new themers. Because those new functions are easier to grasp then preprocess/process functions.
The reason I am unhappy with this patch is because it removes all the basic position-able divs out of the HTML in the basic tpl.
@Pasqualle:
Beginning themer's are not doing this. You are talking about experienced themers coming from a different platform; that's not who I'm talking about. And it’s not a bug, it’s a documentation issue. (which needs fixing.) Actually, I'm already starting work on this documentation problem. As I've already got a full outline written for a tutorial-based Theme handbook; I've just been too busy to flush it out while D7 patches need writing. After code freeze I’ll get some docs people to help out.
Now regarding the user you were talking about, the “new-to-Drupal themers” that have experience with other systems, I completely agree with Crell’s statement:
@Pasqualle:
page.tpl is not broken. It’s just inconsistent in that we can’t use render/show/hide inside the tpl like we can in other tpls. But core is only using them in preprocess_page now and not in the tpl, so its not a bug, just a limitation. And its the same limitation we have in D6's page.tpl: don't do complex php in the page.tpl.
Since we already hackily add the
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
, we could also hackily add the scripts and stylesheets after page.tpl is rendered. Those who don't like that particular preg_replace, I think we could replace it with a less-CPU-hungryexplode()
. So, that's an alternative solution to breaking up the page.tpl.Comment #122
moshe weitzman CreditAttribution: moshe weitzman commentedIn addition to the improvements mentioned earlier, this issue brings:
1) Granular themeing of page.tpl.php (and all its variants). Template authors can choose to print a block here or a block there or a block there, or discard a region, or whatever. They can print a menu or login/register div when they want to.
2) Consistency. Granular theming works on all our other templates. It is real unfortunate for it to sort of work but in reality it drops your css/js/library on the floor.
I know that you can do everything in preprocess that you could imagine with granular rendering. Thats beside the point. Folks who want to work that way should work that way. Granular theming is a superset of "preprocess rules". Granular theming is appeals to folks who understand templates and don't want to work constantly switch between preprocess and template layers. They probably think it is more understandable to centralize the presentation info in a template file.
IMO, we have already committed to granular theming. This patch just brings it to page.tpl.php.
Comment #123
NaheemSays CreditAttribution: NaheemSays commentedIs one of the major reason against this that it allows themers to use something that is not considered "best practice"? If so, that seems an artificial limitation - people who want to stick to best practice can. others will find the fastest hack they can and question why it does not work.
That sounds like a (very) big gotcha to me - "you can do this in all the other 'tpl.php' files, but not this one."
(and yes, I really do think this patch makes theming easier.)
Comment #124
NaheemSays CreditAttribution: NaheemSays commentedcross posted
Comment #125
JohnAlbinActually, I think Moshe crossposted with me. I intended to mark it "needs review" while we continue to discuss.
As I said, its an existing gotcha from D6: Whatever your base tpl is, you can't do complex PHP in it because scripts and stylesheets come before page content. And I think this patch’s negatives outweigh its positives.
Comment #126
tic2000 CreditAttribution: tic2000 commentedLet not make this a discussion about best practices and just forget what the OP uses as arguments.
moshe in #122 points out the main reasons this should go in. I don't consider those points just additional. Also @sun in #115.2 provides also another good reason even if not as strong as those made by moshe (I think drupal doesn't babysits broken code).
So to summarize. This patch allows granular theming in page.tpl.php and provides consistency with other tpl.php files (it will move the inconsistency to html.tpl.php, but that file will not need it as much).
The fact that you will now be able to do drupal_add_js/css or use other function inside page.tpl.php is only a side effect, and it will not affect themers, but give freedom to those who want it or need it (I think I already said that).
The side effect is that now we don't have a full html file for themers, but let's not assume themers are so dumb so they will not get that the head tag and an important region is inside html.tpl.php and everything they need to change to get the desired layout is inside page.tpl.php.
@JohnAlbin
page.tpl.php is still the main tpl file used for layout. You can make your basic layout here and add regions inside the divs to position them.
@mfer
We can provide template suggestions for html.tpl.php like we do for page.tpl.php.
While the discussion develops here I reopened #519782: Change $closure to become a hidden region like page_top to make closure a region.
Comment #127
PasqualleI have a proof that a themer (who knows about preprocess functions) is teaching how to use views_embed_view() in template files:
http://mustardseedmedia.com/podcast/episode31
http://mustardseedmedia.com/podcast/episode35
I can only recommend the same. It is the fastest and cleanest way to embed a view. It is wrong to pollute a _preprocess function with that. I know I said it many times in this issue, but yes, that is my original problem. If you can't convince me you won't convince themers who saw the tutorials and knows how to embed a view easily..
If you check the "Theme snippets" handbook pages or Drupal themes, then you can see that functions and theme() calls are absolutely common inside template files. It is not something what only Wordpress people do.. I would really like to see a handbook page where the page.tpl.php "inconsistency" is explained, so I can just point every bug report to that page. I hope users will be satisfied with the answer that "this is not something we need to fix"..
Comment #128
stephthegeek CreditAttribution: stephthegeek commentedI think it's easy to forget that the majority of people creating Drupal themes are not what we'd call themers. They are not professional coders. They're creating a site for their business, their boss, their neighbour's sister's business, or the organization they volunteer for. They are lucky to know some CSS and work their way around PHP files, and will grasp onto any snippet or tutorial they can find that simply works.
I think adding consistency to page.tpl is a worthwhile step to increase the number of early successes for those learning theming.
Comment #129
mfer CreditAttribution: mfer commentedWhile we debate how this whole issue should work out I did notice a problem....
in html.tpl.php should be $content for consistency. We use $content everywhere else and the html element here could be used for places other than the standard page.
Comment #130
Crell CreditAttribution: Crell commentedThis actually does now tie into something else I've dealt with. We use the word "page" in about 15 places, to mean different things. Recently I was explaining something to a new Drupal developer, and realized that I used the word "page" to mean 3 different things in a single sentence. That's a serious DX WTF.
IF we are going to split page.tpl.php up, how about we split it into html.tpl.php and body.tpl.php and drop the "page.tpl.php" file name entirely? The page is "the whole thing that the we server returns", and also "a type of node", and also a few other things. If we can eliminate some uses of it, we can make the system easier to understand. (And html.tpl.php and body.tpl.php map then to the HTML tags that they more or less represent, as I understand it.)
Comment #131
eaton CreditAttribution: eaton commentedA huge +1 for that idea, Crell. I think it could (at the very least) make the division much clearer.
Comment #132
kika CreditAttribution: kika commentedI was about to suggest the same page --> body renaming but two things held me back:
1. <body> tag is not inside page|body.tpl.php (it's in html.tpl.php), while <code> is inside html.tpl.php
2. Are we sure we want to tie ourselves to html tag names? One could whisk together a theme based on any XML markup (say Atom) and CSS + XSL. In that case file names and markup tag names do not correlate. Also, what is our roadmap in future? Will we see HTML5 style section.tpl.php and aside.tpl.php?
Comment #133
moshe weitzman CreditAttribution: moshe weitzman commentedI'm fine with renaming page to body.
@kika - the theme functions page and html are specifically built for emititng HTML. If one wanted to emit XML, one would use page alter to swap theme and theme_wrapper on the top most element of $page. Thats our thinking for how we start supporting web services in a RESTful fashion. But there are loads of issues to be worked out.
@kika - i don't think it is complicated for themers to understand than body.tpl.php represents the contents of . body is a pretty special tag.
Comment #134
Frando CreditAttribution: Frando commentedBody doesn't necessarily have to refer to the HTML tag name. Even if we were doing XML or even JSON, you could still refer to the body of the page. Thumbs up for html.tpl.php and body.tpl.php!
Comment #135
tstoecklerJust wanted to randomly throw in that we now have a field called "Body" by default in every Drupal install.
Comment #136
NaheemSays CreditAttribution: NaheemSays commentedThis needs to be rerolled to account for other changes, including #519782: Change $closure to become a hidden region like page_top
(I tried to do it but I could not get the toolbar to work...)
Comment #137
tic2000 CreditAttribution: tic2000 commentedOK, I'll try to roll a patch later, but I sense another 100 posts before it gets in or 9/1 comes (whichever comes first)
Comment #138
tic2000 CreditAttribution: tic2000 commentedOK, let see how it went. I hope I didn't forgot anything.
For now no change from page to body, I want to see what the bot have to say first.
Comment #140
tic2000 CreditAttribution: tic2000 commentedForgot to change a $left to $page['left']
Comment #142
tic2000 CreditAttribution: tic2000 commentedComment #144
tic2000 CreditAttribution: tic2000 commentedAnd we try again.
Comment #146
tic2000 CreditAttribution: tic2000 commentedComment #148
tic2000 CreditAttribution: tic2000 commentedComment #150
tic2000 CreditAttribution: tic2000 commentedI give up. The bot is not in a good mood.
Comment #151
tic2000 CreditAttribution: tic2000 commentedComment #152
tic2000 CreditAttribution: tic2000 commentedOne more try.
Comment #154
tic2000 CreditAttribution: tic2000 commentedComment #156
tic2000 CreditAttribution: tic2000 commentedComment #158
tic2000 CreditAttribution: tic2000 commentedForgot we have a new theme now.
Comment #159
tic2000 CreditAttribution: tic2000 commentedNow that the tests pass.
If we changing $page to $body it's easy. The question do we rename $page_top/bottom to $body_top/bottom? If we do that we rename the regions too?
Making page.tpl.php become body.tpl.php. Do we rename just the file or do we rename everything? That means preprocess/preocess_page become preprocess/process_body. If we do that do we also rename hook_page_alter to hook_body_alter?
Comment #160
eaton CreditAttribution: eaton commentedIdeally, I'd prefer to see a single $page construct, with the HTML template added as a #theme_wrapper on the page object.
Comment #161
tic2000 CreditAttribution: tic2000 commented@eaton
It's already like that in the patch. And it will be like that no matter what solution we adopt.
But we can do
and we only change page.tpl.php to body.tpl.php (renaming $page to $body inside html.tpl.php is trivial).
Or we can do
which renames page.tpl.php to body.tpl.php and all preprocess_page/process_page functions to preprocess_body/process_body.
After that we can also discuss about changing hook_page_alter to hook_body_alter for naming consistency.
I repeat, no matter what is the approach, html.tpl.php will be a wrapper of that using #theme_wrapper.
What is the desired approach? And more important, we do that in this issue or another one?
Comment #162
moshe weitzman CreditAttribution: moshe weitzman commentedIMO, body.tpl.php sounded like a good idea until tstoeckler reminded us that we have a new body Field in core. A rename form page.tpl.php to body.tpl.php just trades once confusion for another. IMO, this patch is RTBC. I'll wait a bit for more feedback before marking it so.
Comment #163
NaheemSays CreditAttribution: NaheemSays commentedagreed with moshe - further if such a rename happens, (especially if it is option 2 from comment #161), it may be better as a separate issue from this.
Comment #164
tic2000 CreditAttribution: tic2000 commentedRe-roll after #226587: Default sidebar region labels are confusing (wrong) for RTL languages
Comment #166
tic2000 CreditAttribution: tic2000 commentedHere we go again :)
Comment #168
tic2000 CreditAttribution: tic2000 commentedNew try.
Comment #169
tic2000 CreditAttribution: tic2000 commentedRe-roll after #433992: Radios missing class form-radios, theme_radios() never called
Comment #170
moshe weitzman CreditAttribution: moshe weitzman commentedThanks for the endless re-rolls, tic2000. RTBC.
Comment #171
JohnAlbinI'm still not happy about this patch, but at least it should be done right before committing. So here's my review.
Poor maintenance_page.tpl, everyone always forgets about you. :-( Leaving maintenance_page.tpl.php as-is is inconsistent with this proposed split.
Since
<?php if (!empty($page['sidebar_first'])): ?>
and<?php if ($page['sidebar_first']): ?>
are equivalent when we know the region key exists, we should be using the simpler form in our tpl. The current patch has a mixture of the two forms.We also have some regression when the favicon code got moved from
template_preprocess_page()
totemplate_preprocess_html()
.Looks like I'm running out of time to do a full review; I've got a plane to the states to catch in 2 hours. But I can go through the rest next week.
Comment #172
PasqualleIf someone would compare how the maintenance page is created and how normal page is created, would see that those are two completely different things, therefore I would go for full separation from page.tpl.php, and total clean-up for the maintenance page. The maintenance page code is really ugly and it could get only worse if we try to continue on the current path.
Comment #173
tic2000 CreditAttribution: tic2000 commentedThis patch takes care of the regression and of the "empty" problem. Let see what the bot things.
Waiting for the full review.
Comment #175
tic2000 CreditAttribution: tic2000 commentedNow it should come back green.
Comment #176
tic2000 CreditAttribution: tic2000 commentedOne problem from using the theme_wrapper option is the fact that the wrapping theme is not aware of changes made by wrapped theme. This is not a problem when the theme is a function and not a template (or it may be, but I can't think of anything now). When using templates files you have preprocess and process function which may change, add or remove things, changes that are not passed to the wrapper template.
I only have one example (maybe my brain is tired, but I can't think of another use case again). This example is is the classes_array variable. I had to move all that code from preprocess_page to preprocess_html because there is no way I can just pass them from one function to another (unless I use drupal_static, or... more about it later). Very well you will tell, what's the problem then? The problem is that for example if I want to convert the maintenance page to use the same wrapper I don't know how to pass
$variables['classes_array'][] = 'in-maintenance';
to the html wrapper, other than using again a drupal_static.To make things short (who said "it was about time?"). I found a way to allow passing changes from the wrapped theme to the wrapping theme (if the themes are templates). My question is, there is a use case for that, apart this classes_array variable, or if you need to do that is just ok to do a drupal_static? And I mean cases where you can't achieve the same result using hook_page_alter.
Comment #177
Pasquallejust tell the idea, before you forget it. If it is too complex we can create a new issue for it. I think it would be nice if some variables could be shared between the template and the wrapper template (without using drupal_static).
Comment #178
tic2000 CreditAttribution: tic2000 commented@Pasqualle
I have a patch created.
I will actually attach it as text file for those who want to take a look.
Comment #179
tic2000 CreditAttribution: tic2000 commentedTo quote moshe "at some point the cure is worst than the disease" and I agree with him.
Comment #181
moshe weitzman CreditAttribution: moshe weitzman commented@tic2000 - are you still available to reroll this? At this point, #169 has not been tested in a while.
@JohnAlbin - tic200 worked for a while to get maint template to use the same system and it was really messy. Thats when I said "the cure is worse than the disease." I don't think we can achieve that right now ... Hope you can give more feedback when you get a chance. If you can't, let us know so we can move along.
Comment #182
tic2000 CreditAttribution: tic2000 commentedReroll. One addition to the patch is that now it adds suggestion for HTML template files.
Comment #184
tic2000 CreditAttribution: tic2000 commentedComment #186
tic2000 CreditAttribution: tic2000 commentedI don't like green :)
Comment #187
xmacinfoI love green!
Comment #188
moshe weitzman CreditAttribution: moshe weitzman commentedonce again, rtcb
Comment #190
moshe weitzman CreditAttribution: moshe weitzman commentedUgh. Any chance you can re-roll, tic2000?
Comment #191
tic2000 CreditAttribution: tic2000 commentedComment #192
moshe weitzman CreditAttribution: moshe weitzman commentedthanks tic2000. still rtbc.
Comment #194
moshe weitzman CreditAttribution: moshe weitzman commentedOh noes. Bot says "failed Failed: Failed to apply patch."
Comment #195
tic2000 CreditAttribution: tic2000 commentedComment #196
tic2000 CreditAttribution: tic2000 commentedper #192
Comment #198
RobLoachIt's ie_styles that's hunking....
Comment #200
tic2000 CreditAttribution: tic2000 commentedComment #201
tic2000 CreditAttribution: tic2000 commentedper #192
Comment #202
sunI thought this was already in? The major objections (aside from personal opinions) have been resolved, so what is holding this off?
I would have some smaller nitpicks to share (variable names, spacing, etc.), but these can be tackled in a follow-up patch/issue, and I definitely won't add them here.
Thanks for constantly re-rolling!
Comment #203
mcrittenden CreditAttribution: mcrittenden commentedSubscribe. This would be *awesome*.
Comment #204
moshe weitzman CreditAttribution: moshe weitzman commentedOnce again, I elaborated on some of the justification for this patch in #122.
webchick informs me that Dries needs to make a call on this one so lets mostly sit tight and let him do that. If we get to code sprint day in paris and this is still not in, i encourage someone to poke dries. i wont be there or else i would do so.
Comment #205
webchickYes, sorry, I did mention back in #101:
"... I'd like to get Dries's two cents since it's quite a fundamental shift from what we've been doing."
but since there are > 100 replies since then, I can see how that might've gotten missed. ;)
Comment #206
tic2000 CreditAttribution: tic2000 commentedI can keep re-rolling, but I can't stop 09.01 from coming (unfortunately)
Comment #207
ronald_istos CreditAttribution: ronald_istos commentedThis this is an excellent step forward - however applying patch on current head gives me a white screen. Nothing in the logs either.
Comment #208
EvanDonovan CreditAttribution: EvanDonovan commentedSubscribe. Sure hope this makes it...
Comment #209
tic2000 CreditAttribution: tic2000 commented@istos
after you apply the patch you have to truncate the cache tables in the database. or you can apply the patch and install after that.
Comment #210
ronald_istos CreditAttribution: ronald_istos commentedthanks tic2000 - problem was that I was actually copy - pasting a patch that had not fully downloaded in the browser - my #fail... Anyway - got it working and this is a thing of beauty.
I know I am new to the issue queues and there are probably much bigger and exciting things still waiting to get in Drupal 7 but as a day in day out Drupal dev I know this will save me time and make life all that much simpler. It will make for cleaner themes and easier to maintain themes and code.
So please tell me what it will take to get this in. I will be in Paris on Monday - perhaps I should dress up as the Druplicon and try and distract Dries long enough to get this through :-)
Comment #211
tic2000 CreditAttribution: tic2000 commented@istos
It takes Dries to say "yay" or "nay".
You can dress as the Druplicon anyway, just to show your Drupal love :P
Comment #212
pwolanin CreditAttribution: pwolanin commentedtagging
Comment #214
lilou CreditAttribution: lilou commentedHEAD is broken.
Comment #216
tic2000 CreditAttribution: tic2000 commentedComment #217
moshe weitzman CreditAttribution: moshe weitzman commentedplease wait for green before commit.
Comment #219
tic2000 CreditAttribution: tic2000 commentedComment #220
tic2000 CreditAttribution: tic2000 commentedThis will pass
Comment #221
lilou CreditAttribution: lilou commentedTestbot is happy.
Comment #222
Dries CreditAttribution: Dries commentedThis patch makes sense to me, but then, I'm not a designer. In other words from a technical point of view, I have no concerns with this patch. The concern I have is with the designer experience, but that is up to designers to advice us/me on.
Comment #223
webchickReport from the D4D group at Drupalcon code sprint:
<body>
absolutely needs to be somewhere where themers can access it and add classes or attributes to it. So I think what we need to do here is restructure slightly, and make html.tpl.php just the<html><head>...</head></html>
part and have page.tpl.php include<body>...</body>
. They did agree that it would be nice if all of the tpl.php files behaved the same, even though some of the use cases here are ideally done in preprocess. So I think once this is fixed, we're good to go.Comment #224
tic2000 CreditAttribution: tic2000 commentedI'd reroll a patch, but for some strange reason (meaning that I can't understand it)
drupal_get_js('footer')
doesn't work in pre/process_page functions, only in it's wrapper.If anyone can give me a hint on how to make it work I'll provide the patch (it's already done, just that I don't want to provide one with scope footer not working for javascript).
Comment #225
tic2000 CreditAttribution: tic2000 commentedI set to CNR for the test bot, but it's CNW
After further investiogation, in process_page drupal_get_js only returns the default javascript files, nothing else.
I attach the patch so people can test it and maybe suggest a solution
Comment #226
NaheemSays CreditAttribution: NaheemSays commentedSomething to be aware of with having the body tags in the page.tpl.php is that things such as the "magic" hidden regions of page_top and page_bottom will also need to be coded into page.tpl.php.
Comment #227
Zarabadoo CreditAttribution: Zarabadoo commentedWebchick was sure to bring that up while talking to the designers during the code sprint. To us, it is a non-issue.
Comment #228
mattyoung CreditAttribution: mattyoung commentedsubscribe
Comment #229
moshe weitzman CreditAttribution: moshe weitzman commentedThemers can edit html.tpl.php just as easily as page.tpl.php. I don't know if body belongs in one place or the other, but "themers can't or won't edit html.tpl.php" is not a compelling argument, IMO.
Comment #230
sunSo #220 is ready to fly then.
Comment #231
xmacinfoAgreed for #220. :-)
Comment #232
webchick@#229: One of the main points of separating these out from my view is so we don't need to do that horrific hack we have in D6 of splicing in the meta tag before the title, because we'll already set it up for them here. So while themers can edit html.tpl.php, we'd really rather they didn't.
One of the major arguments against this patch was from a themers' perspective, page.tpl.php being in one cohesive template file is very good, because it's something that's instantly recognizable as stuff they see in Dreamweaver, etc. Putting
<body> ... </body>
in page.tpl.php is a compromise that keeps that same feel because everything from the beginning of the body to the end falls under the viewport that themers need to be able to control.So what is the technical reason that
<body> ... </body>
can't be part of page.tpl.php? I don't quite follow. Without that, I'm suddenly a lot less enthusiastic about this patch.Comment #233
webchickComment #234
mcrittenden CreditAttribution: mcrittenden commentedIf this means that it will be considered "bad practice" to do something like change my doctype, then I don't think it's going to get much support.
Comment #235
webchick@mcrittenden: No, it's more an issue of the frequency with which things need to be edited. In an ideal world, the only reason you'd need to deal with html.tpl.php is to do something like switch your doctype headers. If someone really has a reason to put title before meta in their theme, then they are free to do so. But html.tpl.php will ship with sensible defaults that should work in the vast majority of situations.
Conversely, core has no way to provide sensible body classes that will work in the vast majority of situations because there are literally an unlimited number of ways "context" can be communicated to the theme layer, so changing body classes is something that is relatively common. Therefore, splitting the stuff between body into two separate files would create more work for themers and also some conceptual issues with separating the logic of what is conceptually one "piece" into multiple files.
Comment #236
PasqualleI have to agree, the body tag is tied to the page in Drupal.
"I hate IE because I can't move the body tag outside of page.tpl.php" Should I add this to the list, ihateie.com? :)
Comment #237
pwolanin CreditAttribution: pwolanin commentedif at all possible I'd like to move the title tag generation to http://api.drupal.org/api/function/drupal_add_html_head/7 so that there is no way to change the order in the tpl. But as far as this issue - I can see why having the body tags in the page.tpl would be more familiar. Though do we really need at all a tpl for the head versus a theme function?
Comment #238
tic2000 CreditAttribution: tic2000 commented@webchick
It's not that body can't be part page. Using render inside page.tpl.php means no javascript is processed till the page is rendered and that means we can't add javascript in preprocess or process functions for page.tpl.php (which means we can't add javascript to the footer scope) because it's not aware of the files yet and it adds only the default ones.
I can't add that in process for html because would add the javascript after the body tag.
DamZ suggested on irc that I do a drupal_get_js('footer') at the end of page tpl.php, but it's not an option imo. Another option suggested is to make a theme function for page_bottom, or a bottom region that would be processed after page.tpl.php
And since we discuss a themer POV. Today I had to change the header on a site that has 6 page files. As a developer I can do a header theme function/file and solve the problem. As a themer I have to edit each 6 files every time I want to change the header/footer. With this change I can do the "main" layout inside html.tpl.php and micro-organization inside page.tpl.php
Comment #239
xmacinfoThis is exactly my point of view since I am essentially a themer. I do often need to deal with multiple zzz.page.tpl.php files. So if I need to change something in the head, I need to to this is all page.tpl files.
But we must not look at this patch only from the theme POV. We must make sure that JavaScript won't break. :-)
I do agree, though, that body should be part of the page part.
Comment #240
EvanDonovan CreditAttribution: EvanDonovan commentedTo echo xmacinfo:
As someone with 20+ page.tpl.php files in the same theme (b/c different sections have different header images), I would much appreciate this change.
Comment #241
pwolanin CreditAttribution: pwolanin commentedFor performance, we shoudl *really* make this a theme function not a template - especially if we are able to change theme() to always pass a $variables array to functions. In fact with that change, hopefully we can revert several templates to functions (maybe using heredoc).
Comment #242
Nick Lewis CreditAttribution: Nick Lewis commented#220 failed to apply (some spacing issue in system.module) -- rerolled it to review.
Comment #243
Nick Lewis CreditAttribution: Nick Lewis commentedThis looks great to me! Basics all work:
-Changed text direction to right to left (that should be useful for some people)
-was able to cleanly get rid of the overlays and toolbars
-moved scripts to the bottom of the page.
Also appears to make some really interesting stuff possible... but exploring that is beyond the scope of this patch.
I think the bottom line is that it will make designing multiple page.tpl.php files much less of a headache, and keep the files smaller.
My biggest concern was altering body classes, but its non-issue: this was sufficient.
Comment #244
Nick Lewis CreditAttribution: Nick Lewis commented@webchick #232 -- is often used (at least by me) as a top level controller for switching styles. Its not something i employ often, but its useful if you want to ahve totally different designs page by page. e.g.
body#fatboy {
background-color:black;
color:#fffffff;
}
body#valleygirl {
background-color:pink;
color:#000000;
}
All the variables you could need to decide whether its #fatboy or #valleygirl are there (there's too many if anything). So the old way will work, but what's great about this new way is that you can control these sort of style switches from a central place without having to update -- say 10 -- page.tpl.php files [extreme -- but having so many html heads and body tags get unwieldy].
In any case this is great for people who know how cascading stylesheets actually work, and those who don't can jolly well make a container around their page.tpl.php files to get the same results! :-D
Now you ask "why can't body be a part of a page.tpl.php file -- and i'll respond: because the $page_top, $page_bottom regions are part of html.tpl.php. We can't put those outside of the body, and to put them within a page.tpl.php makes things all the harder! One more note in themer documentation: "oh... the admin overlay breaks unless you do print $page_top, print $page_bottom at the top and bottom of your page.tpl.php files.
Final note -- i was wondering what all this strange stuff was I was seeing in the HTML head that I would never have put there.
For example:
[head profile="http://ns.inria.fr/grddl/rdfa/"]... [/head]
WTF is that about?
oh...
http://rdfa.info/
I don't know if anyone noticed the html tag, but here it is:
Why is this important? To be perfectly honest -- I really really don't want to know... (though its pretty interesting to google those that i don't recognize)...
Comment #245
Dries CreditAttribution: Dries commentedI'll leave this patch up to webchick. Follow-up your gut on this one, Angie.
Comment #246
webchickMy gut, eh? :) Is that why I'm apparently having such a hard time communicating plain english? :)
If you guys insist on putting the body tag in html.tpl.php, then work with Zarabadoo and mortendk to figure out what their concerns are and how this patch addresses them, and get them to chime in here saying they're cool with the current patch, or at least can live with it.
Comment #247
moshe weitzman CreditAttribution: moshe weitzman commentedZarabadoo recently stated that he has no objections. At least thats what I understand form #227.
Where the body tag goes is important but secondary to the main benefit of this patch. Please reread #122 where I outlined the benefits (consistency of granular themeing). It would be preferable if someone opposed to this patch would state "I understand the consistency improvements introduced here but I think they are outweighed by x". To date, I have not heard that.
Comment #248
alexanderpas CreditAttribution: alexanderpas commentedI've reviewed this patch, and while initially against this, i'm completely for this patch after the review
- keep the body out of the page.tpl.php, as is simplyfies saying "all the content inside the body tags go into page.tpl.php and then you make the following adjustments." (peer-tutor speaking -- dropping people from plain HTML right into drupal ;) )
the only gripe I have is that we should devise an easy way for taxonomy, book and/or other modules to properly add additional classes to the body tag. (I think that belongs in another (followup) issue)
on a side note: after all patching to the theming layer is done, I would like to see a "Creating Drupal 7 Themes" in the theme guide section of the handbook. (I will help if possible.)
Comment #249
tic2000 CreditAttribution: tic2000 commented@alexanderpas
Adding classes to the body tag is exactly like it is before the patch with the minor change that the hook used now is hook_preprocess/process_html instead of hook_preprocess/process_page
Comment #250
webchick#227 was a reply to #226.
Comment #251
webchickBtw, I suppose I should respond to this too, so there is some context to why I am being so persnickety about this.
Themers will not have the context of reply #122 to know the reasoning behind this decision. The theme system needs to make sense to them from the outset, and not get in their way. The vast majority of replies here that have been in favour of this patch come from people I see on lots of other core issues. Those opinions are very important, but I also want to see representation of opinions I do NOT see on every core issue here, because those are the people who have to live with this decision, and will not have the nice background that explains logically why this is "a good thing."
I solicited two such opinions at Drupalcon Paris, since I know this patch is important to getting the render system working consistently and all of the other reasons outlined at #122, and I didn't want to see it held up for lack of reviews. Zarabadoo and Morten both gave very strong reasons for why this patch would be incredibly debilitating to them in its current state. They proposed what seems like a reasonable compromise to me, but this approach has not been followed.
Therefore, I'm asking that you sync up with them to give them information on why their approach won't work, determine what their concerns are, and provide information out how they can work around these concerns with the proposed patch. I want to see one of them be the one to mark this RTBC.
Comment #252
pwolanin CreditAttribution: pwolanin commentedwherever it ends up, we should allow for non-class attributes for the body tag also (to support RDFa, etc).
if we move the body tag to page.tpl.php, does this html part actually need to be a template instead of a simple theme function?
Comment #253
mcrittenden CreditAttribution: mcrittenden commentedI think/hope that I'd qualify for that :).
I am probably 80% themer 20% developer and I personally am a huge fan of this change. The one consistent argument I can find against it throughout this thread is that it splits up page.tpl.php which is confusing for new themers because now you have to use two different files to get the big picture. This argument makes no sense to me: I personally expected it to be this way when I began with Drupal, and was confused that it's all in a single file.
Because of that, my first reaction with Drupal's page.tpl.php was "uh, what if I need different ones for different pages?" and I finally resorted to hardcoding include (header/footer) statements into page.tpl.php to make up for this (just making different page.tpl.php's has never been a good solution for me because it completely goes against the DRY principles that Drupal stands for so I was willing to deal with the ugliness of include()'s in favor of that).
Look at the other CMS'es/frameworks out there. Django has template inheritance (which is essentially what we are doing here), MODx has header/footer snippets which get included, WP has header/footer.php, Rails has template inheritance, most PHP frameworks encourage you to write your own header/footer views and include those, etc. As early as first level web development courses, you learn about including headers and footers. My point here is not that we need to do what everybody else is doing, but rather that it's sort of strange to me to say that "splitting up a page would be confusing to people" when that's what's already going on everywhere you look.
So I'm a huge fan of this, with one concern: the one thing I haven't seen discussed thus far is whether there is the possibility to have multiple html.tpl.php's or not? Example use case: a themer needs to output a certain View as XML instead of HTML, so he/she obviously needs access to the for that specific page. How would that work? Sure, there's a better way to go about doing this, but remember our discussion is about themers, and themers should be able to stick to the theme. NOTE: Asking for override-able html.tpl.php's may seem like a contradiction since I just argued against the same behavior for page.tpl.php, but I'm personally ok with it for html.tpl.php since that will need to be overridden FAR less often.
So this patch has a HUGE +1 for me as long as html.tpl.php is still override-able as needed.
Comment #254
Pasqualle@mcrittenden: you can override the html.tpl.php template in your theme.
In the patch there is the same template suggestion for html as for the page template (like you can use html-node-1.tpl.php), but I am not sure if we really need that.. it was requested in comment #117->#126
And you can add own template suggestions in the *_preprocess_html() function, like
---
for separate header and footer templates (instead of includes) I would advise something like:
so then in the page.tpl.php template you would use
and yes, I also like it separated..
Don't we have some kind of region templates in D7 already? I've read it somewhere..
Comment #255
tic2000 CreditAttribution: tic2000 commentedI agree with @mcrittenden. I don't think a single file it's less confusing than 2. When I was learning HTML, theming (I had no course to attend too unfortunately) I used Dreamweaver too and I did use that for developing till early this year. The "advantage" even when I was theming was that it added the initial tags so I wouldn't bother. I was only confused when I saw the first php file for some forum and there were no html tags at all. Found out pretty fast there was a file for the header, one for the footer and learned how to use them and the advantage of using them. Up to this day I probably need to look up how to properly add a css file or js with just html tags, and not using some function like drupal_add_css/drupal_add_js. Why? Because I rarely have to statically add something in the head tag.
Since those first days as themer or later as developer I'm used to have the header and footer separated from the main content because I don't need to change them so often. Adding body inside page.tpl.php would not help me as a themer because I won't be able to do that.
I did try to move body inside page.tpl.php (patch in #225), but javascript added to footer won't simply work.
But I'm not only themer so I too want to hear those reason that require the body tag inside page.tpl.php. I still wait for the full review from JohnAlbin too, so I can either answer to them or patch a solution.
Even if this will go in with body inside page.tpl.php, the developer in me will help the themer in me and I will move body inside html.tpl.php in the themes I'll need to build (I do not publish themes on d.o)
Comment #256
pwolanin CreditAttribution: pwolanin commentedfixing bug in modules/system/page.tpl.php:
and also html.tpl.php did not apply the $attributes to the body along with the classes, and $head_title was *still* being printed before $head. Note, however, that applying $classes and $attributes to the body tag in html.tpl.php is a bit of a violation of the description of them as applying to the "top level template entity", which would be the html tag. I think I'm siding with the body tag in page.tpl.php crowd.
@moshe - I think you are mis-reading #227 - it seems to say that printing the extra regions is not an issue
Comment #258
pwolanin CreditAttribution: pwolanin commentedleft a stray line in the garland page.tpl.php
discussing with Nick Lewis in IRC - I think we could accomplish one of the major goals of the patch - letting drupal_add_js() work in page.tpl.php - by making the call to get the footer js as a #post_process
Comment #259
pwolanin CreditAttribution: pwolanin commentedok, thanks to the magic of a #post_render, we can get footer scope JS to render even if you call drupal_add_js() in page.tpl.php (as long as it's above that last render call).
So, this patch puts the body tags back in page.tpl.php which seems to be the preference of more themers, and which makes more sens to me wrt application of classes and attributes.
Comment #260
pwolanin CreditAttribution: pwolanin commentedfixing some of the doxygen
Note - I'd actually tend to favor removing the template suggestions now for the theme('html') and would rather see it be a theme function. But seems like those points were debated before, and this issue is way too long already.
Comment #262
Nick Lewis CreditAttribution: Nick Lewis commentedThis is a compromise. I've reviewed it, and would mark it RTBC, but want to give it a little more time. (measured in hours). The patch is different now.
Comment #263
pwolanin CreditAttribution: pwolanin commentedThis doesn't fail locally - and DBlog is not affected by the patch, so I think there's a testbot issue.
Comment #264
Nick Lewis CreditAttribution: Nick Lewis commentedIt is -- i did a retest and it came back green
Comment #265
alexanderpas CreditAttribution: alexanderpas commentedso, themers must now remember to add these lines each time when creating their page.tpl
also, seeing that documentation, it is just ugly
In addition to that, the location of those regions is pretty much fixed, because certain core) modules expect them to be on this place (toolbar for example.)
In my opinion, the upside of having
<body>
in the page.tpl.php doesn't compare to the downsides of having those additional requirements in page.tpl.phpif we move
<body>
to html.tpl.php basically the only required items left in page.tpl.php are things like<?php print render($page['content']); ?>
(which makes sense for new themers, since the output is... very visually ;) )If book.module (or context.module) for example had the ability to add more classes (which book or context you're in.) to the body tag, it all could be fixed in CSS, and there would be no reason to add
<body>
to page.tpl.php in my opinion (prove me wrong please!)I'm on crack. Are you, too?
Comment #266
tic2000 CreditAttribution: tic2000 commentedWhat are the upside of body inside page.tpl.php? I still fail to see that and nobody detailed it. Adding a class directly?
Book can use book_preprocess_html to add classes to the body tag if it's inside html.tpl.php as it should do if it needs to use book_preprocess_page with body inside page.tpl.php
Comment #267
SeanBannister CreditAttribution: SeanBannister commentedSub, Sorry didn't have a chance to read all #266 issues but I like where this is heading.
Comment #268
Nick Lewis CreditAttribution: Nick Lewis commentedtic2000 -- basically it comes down to webchick saying:
They've been MIA on IRC. pwolanin makes the point that it is a bit weird to handle those regions differently. I can also see why having a body tag in page.tpl.php adds immediate context for simple themers.
All in all though, i agree with you: having a body tag just adds one more way for people to get into trouble and ensures that we have to keep code that we really don't need in our page.tpl.php.
I see only loss, and no gains from doing it this way. And i personally fail to see how its any easier -- seems harder if anything.
However i prefer this to nothing too.
Comment #269
Nick Lewis CreditAttribution: Nick Lewis commentedalexanderpas:
Agreed, i just harassed mortendk to see if i can git a quick answer on this, because i think this compromise may be a mistake.
Comment #270
tic2000 CreditAttribution: tic2000 commentedpage_top and page_bottom can be moved in page.tpl.php and body tag remain inside html.tpl.php, so that is not an argument.
Comment #271
pwolanin CreditAttribution: pwolanin commented@tic2000 - supplying classes and attributes for theme('html') and then applying them to the body violates the documented behavior of applying those to the top-level elementis hte respective tempalte, so for that reason also I think having body in page.tpl.php is more consistent.
Comment #272
tic2000 CreditAttribution: tic2000 commented@pwolanin
Maybe I'm mistaken, but top level in the documentation means it's a top level in the array (like show_messages which is copyed from $variables['page'] to $variables), it has nothing to do with the html tags structure. Also it's top level for each template (html or page), not cross templates. And even if, you change the documentation, not the code.
I want to hear the themers reason why body makes more sense in page.tpl.php and not html.tpl.php. What makes themers life harder if it's the the other way around.
Comment #273
mcrittenden CreditAttribution: mcrittenden commentedI'm a themer, and it really couldn't matter less to me as long as I can get to it. IMO, there's no clear logical place for it to go (i.e., neither html.tpl.php or page.tpl.php makes more sense than the other), so I'd just put it wherever is easiest from a coding standpoint.
Comment #274
xmacinfoI am a themer too and I don't care if body is in html.tpl.php or page.tpl.php.
We must not stop this patch happening just because someone wants the body tag to be in one place or the other. :-)
Comment #275
mortendk CreditAttribution: mortendk commentedokay so heres the deal:
by not having the inside page.tpl.php my first question would be - where did they hide my body tag?
It simply feels wrong to remove the outer element of the body outside of the page, it dosnt look "right" - I know its a "feely" answer.
When we look at the page.tpl. We wanna see the whole tree (yes i know that i dont have the anymore) and not having to find a part of my markup up in another tpl.
What is the problem with having in page.tpl ?
as i see it makes much more sence to have you body and all the content one place
/mdk
Comment #276
alexanderpas CreditAttribution: alexanderpas commented@mortendk
those were exactly the concerns i had in #248 before reviewing the patch in #242.
(speaking as peer-tutor here)
First problem i had, was how to explain the split to my peers.
well, after reviewing the patch i noticed it is very simple:
1) take your plain HTML mockup (already valid XHTML Strict + all CSS)
2) paste everything that is between the body tags in page,tpl.php (between is an easier to understand when used with two similar tags ;) )
--- note that we can remove 7 steps here, (in comparison to D6,) since we don't have to change head and body itself anymore. ---
--- the old version of 2) read: "paste all html in page.tpl.php" and needed steps to replace properly ---
3) remove the dummy lipsum and replace it with
print render($page['content']);
--- (they notice the content is coming from drupal, instead of their lipsum, and hopes get up ;) ) ---
4-8) replaces the other basic regions (header, footer, 2 sidebars, help etc..) ;)
--- (they now see menu's being generated etc. (more encouragement)) ---
9) now we add the CSS files to the info files, and adapt the selectors to the generated html.
--- (they see CSS being applied as it is added & adapted (cheers ;) )) ---
and so on...
To the question where
<head>
is, (asked a lot more i think) the answer is "Drupal takes care of that, and you don't have to worry about it." (usually the next thing you get is "but i need it for this and that" and you can learn them a more drupalish way.)Comment #277
Nick Lewis CreditAttribution: Nick Lewis commentedSubmitting 220 for retest. MortenDK is a convert. Attached is some the theory involved in doing that. It shows how html.tpl.php can be used as a wrapper for common elements (such as branding) while allowing content regions to switch interchangeably. It more or less lets us make sites with complicated, varying layouts without losing the agility to quickly change common required elements. [think nytimes.com layout]
Really really ghetto proof in the form of stark attached.
Comment #278
alexanderpas CreditAttribution: alexanderpas commentedI guess this'll work better ;)
directly taken from #242 as #220 didn't apply due to spacing issue.
Comment #279
mortendk CreditAttribution: mortendk commentedyup i must accept the error of my ways ;)
and as I understood it nick it would still be possible to put the where you want to?
but it make sense to add baisc branding (header, logo all kinda branding)
------------------
header (html.tpl) <- branding etc
------------------
page.tpl
------------------
foota (html.tpl) <- web2.0ish footer etc
------------------
so wheb you wann change the whole layout from a 2 colum to a 4 columt you dont have to have multiple header / footer in your theme..
Comment #280
Nick Lewis CreditAttribution: Nick Lewis commentedThis is still hack territory -- and i may post some followup issues to make it less hackish -- but here's what we gain.
I ASSURE YOU that people will use this in such a manner.
We can make html.tpl.php that contains elements common to all page designs.
Where it reads
print $page;
we can define varying interchangeable layouts:two columns title hidden
three columns title hidden
A couple things to note:
1. MUCH less code for complex layouts.
2. Much more agility for making large changes with multi-page.tpl.php setups
3. if statements are practically unneeded anymore for layouts
4. combined with control over template suggestions, this is very very powerful
5. at present i actually have to pass page variables in manually to get stuff like site name.
Comment #281
Nick Lewis CreditAttribution: Nick Lewis commented#242 passes - some details relating to template variables should go into follow up issues imho.
Comment #282
xmacinfopwolanin did put back the body tags page.tpl.php in #259 and #260, why starting with #242 to create a new patch?
Comment #283
Pasquallere #280: you should not use html.tpl.php this way, because you will have all the same problems as with page.tpl.php in D6.. The best solution is to keep html.tpl.php unchanged.. just create your separate header and footer templates and print them in page.tpl.php
I am not sure what is the final decision where the
<body>
tag should be, but I do not really care. I just need<head>
outside the page template..Comment #284
alexanderpas CreditAttribution: alexanderpas commentedbecause those were in response of #246... which we have resolved now, so #242 (or #278) is what will be committed.
Comment #285
Nick Lewis CreditAttribution: Nick Lewis commented@Pasqualle -- the particular problem is complicated designs with multiple varied layouts. (think lots of page.tpl.php files)
Nearly any design, however will share some common junk. Using your suggestion we have:
header.php
footer.php
Ugly? sure.
Works? yes. Require each page.tpl.php file to have $wrapper_top, $wrapper_bottom yep.
Why do that when you can just have it in your html.tpl.php file however:
Done -- will never havy worry about my wrappers again. And I can actually see the top and bottom on one page. This is why we're putting body in html.tpl.php -- no one has to do this. But i sure will. beats
if () {
everywhere and 500 line page.tpl.php files.Its happy accidental feature introduced by your patch!
Comment #286
tic2000 CreditAttribution: tic2000 commentedre #283: #280 is one of the 2 reasons I think body should go in html.tpl.php. Yes, it won't work if you want to add js, but that is a decision you can make. With body inside page.tpl.php you can't make that decision and if you want to change that you have to do some of the changes the patch does in your theme preprocess/process functions which is a lot more than a themer should do. But as @Nick Lewis and @mortendk say, this will be used for branding more, not embeded views or other stuff that needs js, and if you need js for something that it's on all pages you can use the info file to add that js to all your pages.
The other reason is that you can "hide" page_top and page_bottom which can be confusing for people new to drupal. For a long time I didn't knew what $closure was in my page.tpl.php and many times forgot to add that to the page.
I think the decision now goes for body inside html.tpl.php
Comment #287
pwolanin CreditAttribution: pwolanin commentedplease- look at #256. There are bugs in the patch from #220/242.
The wrong region is rendered and $attributes are not applied to the body
Comment #288
xmacinfoOK. That makes sense. There is now an agreement to put body inside html.tpl.php.
I, too, forgot to include the $closure way too often in D6. :-)
Comment #289
webchickGood. :) Looks like we're all agreed then. Let's get a re-roll of #242 with pwolanin's changes and call this sucker fixed.
Thanks SO much, Nick and Morten!
Comment #290
Nick Lewis CreditAttribution: Nick Lewis commentedSecurity fix for title order, and wrong sidebar fixed as well. I think how attributes are dealt with should be in a follow up
Comment #292
Nick Lewis CreditAttribution: Nick Lewis commentedComment #294
Nick Lewis CreditAttribution: Nick Lewis commentedthis is the one i feel it
Comment #295
webchickCome on, bot!
Comment #296
Nick Lewis CreditAttribution: Nick Lewis commented[say wtihGesture="mr burns hands"]Excellent[/say]
Comment #297
Nick Lewis CreditAttribution: Nick Lewis commentedBTW -- are the status things defaulting to one status lower now?
Comment #298
whatdoesitwant CreditAttribution: whatdoesitwant commentedI am a newbie themer and I am in favour of this patch because it seems to:
- diminish mistakes
- reduce repetition
- clean up my workspace
I would like to have the body tags around my
blockcontent simply for context (coherence) and accessibility, though.But if that means the comment junk from #265 needs to be brought in as well, it's not worth it.
Still it would be difficult to explain drupal theming to other newbies like myself when you leave the body tag out of the main tpl.php. I always like to know my place in the hierarchy, just to get a grip on things. I'm guessing they do to.
Comment #299
mfer CreditAttribution: mfer commentedSetting status back to RTBC.
Comment #300
mfer CreditAttribution: mfer commented@whatdoesitwant - When you work in the block.tpl.php file or one of the views.tpl.php files you don't know when you are in the html hierarchy. This provides an html.tpl.php file that is the top level template. The page.tpl.php is where the content goes.
I think and hope we end up with some good documentation on this. We're going to need it.
Comment #301
alexanderpas CreditAttribution: alexanderpas commented@whatdoesitwant:
see also my post at #276 on how to explain it to others
Comment #302
catchA couple of years back I had a job in a university department which still had it's main site using Apache SSI includes. That site had a header.html and footer.html - and no tag in the content pages either - so agreed that this mirrors that sort of 'primitive CMS' (and all the examples of other CMSes which do something similar), which in this case we'd be moving closer to rather than further from.
Comment #303
webchickOk. I've looked through this and though I continue to share JohnAlbin and quicksketch's concerns about the mental mis-match presented by this patch for new themers, I think Pasqualle, Moshe, and others make a compelling case that we are simply trading one set of WTFs for another. This patch brings consistency to granular theming via the render system that works in all other .tpl.php files.
Committed to HEAD.
I want to sincerely thank the themers who came out of the woodwork to review this patch and help build consensus with "real world" examples.
Now. Let's make sure those follow-up docs are really good. :P
Comment #304
tic2000 CreditAttribution: tic2000 commentedOMG. I must be asleep :P
Now we need #522006: Conditional Styles in .info files, since drupal_add_css has it so themers won't need html.tpl.php just to add conditional styles.
Comment #305
Nick Lewis CreditAttribution: Nick Lewis commentedZOMYGOD webchick can we open a new issue for theme system docs? We're on page two already! Maybe a general themer doc issue if the render stuff isn't well documented yet?
Comment #306
alexanderpas CreditAttribution: alexanderpas commentedhere ya go: #579698: Fix theme update docs so they are correct on what is in page.tpl.php/html.tpl.php
also, putting back to fixed.
Comment #307
alexanderpas CreditAttribution: alexanderpas commentedI seem to be getting exceptions from page.tpl.php due to this patch over at #516150: Add fallback for main content block rendering, could someone please investigate?
Comment #308
mcrittenden CreditAttribution: mcrittenden commented@alexanderpas: if you could create a new issue and link to it from here, IMO that'd be good. We're already at #308 so I think we can call it quits here.
Comment #309
Anonymous (not verified) CreditAttribution: Anonymous commentedArriving VERY late in the discussion (like always I feel...) and very enthusiast about this work.
I was a little bit scared at first when I realized that having
html.tpl.php
andpage.tpl.php
might be problematic when you want to add conditional classes to the<body>
, depending on page variables. For example, how could I add a class to the<body>
only when there is$tabs
in the page ? As the<body>
is not in thepage.tpl.php
, I'm not sure how I could make these play together well... ?I know you could wrap your content into an extra
<div>
with the page classes, but what if I actually don't want to, because I don't want to add markup where I don't need to ? It feels like Drupal would force me to have extra markup, and IMHO, this is not a good thing.This is the two cents coming from a pure themer POV.
Comment #310
alexanderpas CreditAttribution: alexanderpas commented@#308:
fix is now included in: #516150: Add fallback for main content block rendering
@#309:
Please, post any questions and/or usecases you or other themers have that need documentation over here: #579698: Fix theme update docs so they are correct on what is in page.tpl.php/html.tpl.php -- that way we can ensure that the documentation rocks! (nice usecase BTW ;) )
Comment #311
mattyoung CreditAttribution: mattyoung commentedmay I ask what "granular theming" is?