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.
Files: 
CommentFileSizeAuthor
#294 294-html_template.patch33 KBNick Lewis
Passed: 12948 passes, 0 fails, 0 exceptions
[ View ]
#292 1701-html_template.patch31.28 KBNick Lewis
Failed: 12940 passes, 4 fails, 12 exceptions
[ View ]
#290 html_template_no_281.patch32.97 KBNick Lewis
Failed: 12954 passes, 12 fails, 0 exceptions
[ View ]
#278 html_template-242-again.patch32.97 KBalexanderpas
Passed: 12944 passes, 0 fails, 0 exceptions
[ View ]
#277 stark.zip20.1 KBNick Lewis
#260 html_template-469242-260.patch30.78 KBpwolanin
Passed: 12950 passes, 0 fails, 0 exceptions
[ View ]
#259 html_template-469242-259.patch30.79 KBpwolanin
Passed: 12920 passes, 0 fails, 0 exceptions
[ View ]
#258 html_template-469242-258.patch31.63 KBpwolanin
Passed: 12937 passes, 0 fails, 0 exceptions
[ View ]
#256 html_template-469242-256.patch31.47 KBpwolanin
Failed: 12928 passes, 173 fails, 3489 exceptions
[ View ]
#242 220-html_template.patch32.97 KBNick Lewis
Passed: 12962 passes, 0 fails, 0 exceptions
[ View ]
#225 469242-225-html_template.patch29.46 KBtic2000
Passed: 12928 passes, 0 fails, 0 exceptions
[ View ]
#220 469242-220-html_template.patch32.94 KBtic2000
Passed: 12952 passes, 0 fails, 0 exceptions
[ View ]
#219 469242-219-html_template.patch32.26 KBtic2000
Failed: 12786 passes, 0 fails, 1 exception
[ View ]
#216 469242-215-html_template.patch31.78 KBtic2000
Failed: 12778 passes, 1 fail, 2 exceptions
[ View ]
#200 469242-200-html_template.patch32.51 KBtic2000
Failed: Failed to apply patch.
[ View ]
#198 469242.patch32.39 KBRobLoach
Failed: 12362 passes, 0 fails, 10 exceptions
[ View ]
#195 469242-194-html_template.patch33.28 KBtic2000
Failed: Failed to apply patch.
[ View ]
#191 469242-191-html_template.patch32.87 KBtic2000
Failed: Failed to apply patch.
[ View ]
#186 469242-186-html_template.patch32.92 KBtic2000
Failed: Failed to apply patch.
[ View ]
#182 469242-182-html_template.patch31.19 KBtic2000
Failed: 12124 passes, 4 fails, 12 exceptions
[ View ]
#178 pass-element-changes-to-wrapper.txt3.11 KBtic2000
#175 469242-175-html_template.patch30.29 KBtic2000
Failed: Failed to apply patch.
[ View ]
#173 469242-173-html_template.patch29.41 KBtic2000
Failed: 12071 passes, 0 fails, 84 exceptions
[ View ]
#169 469242-169-html_template.patch29.57 KBtic2000
Failed: Failed to apply patch.
[ View ]
#168 469242-168-html_template.patch29.7 KBtic2000
Failed: Failed to apply patch.
[ View ]
#164 469242-164-html_template.patch29.59 KBtic2000
Failed: Failed to run tests.
[ View ]
#158 469242-158-html_template.patch29.16 KBtic2000
Failed: Failed to apply patch.
[ View ]
#156 469242-139-html_template.patch26.47 KBtic2000
Failed: Failed to run tests.
[ View ]
#152 469242-139-html_template.patch26.47 KBtic2000
Failed: Failed to run tests.
[ View ]
#140 469242-139-html_template.patch26.47 KBtic2000
Failed: Failed to run tests.
[ View ]
#138 469242-138-html_template.patch26.33 KBtic2000
Failed: Failed to run tests.
[ View ]
#102 469242-102-html_template.patch26.25 KBtic2000
Failed: Failed to apply patch.
[ View ]
#101 footer.php.txt650 byteswebchick
#101 header.php.txt2.59 KBwebchick
#99 469242-90-html_template.patch26.25 KBmoshe weitzman
Failed: Failed to apply patch.
[ View ]
#94 head.tpl_.diff25.31 KBmoshe weitzman
Failed: Failed to apply patch.
[ View ]
#90 469242-90-html_template.patch26.25 KBtic2000
Failed: Failed to apply patch.
[ View ]
#87 469242-87-html_template.patch26.22 KBtic2000
Failed: 11691 passes, 0 fails, 88 exceptions
[ View ]
#64 469242-64-html_template.patch27.34 KBtic2000
Failed: Failed to apply patch.
[ View ]
#63 469242-63-html_template.patch21.84 KBtic2000
Failed: Failed to apply patch.
[ View ]
#61 469242-61-html_template.patch21.82 KBtic2000
Failed: 11659 passes, 5 fails, 0 exceptions
[ View ]
#53 469242-53-html_template.patch22.26 KBtic2000
Failed: Failed to run tests.
[ View ]
#48 469242-48-html_template.patch22.49 KBtic2000
Failed: Failed to run tests.
[ View ]
#41 469242-41-html_template.patch21.72 KBPasqualle
Failed: Failed to run tests.
[ View ]
#33 469242-reroll-on-steroids.patch31.08 KBtic2000
Failed: Failed to run tests.
[ View ]
#29 469242-29-html_template.patch21.74 KBPasqualle
Failed: 11562 passes, 1 fail, 0 exceptions
[ View ]
#27 469242-reroll-on-steroids.patch29.99 KBtic2000
Failed: 11563 passes, 0 fails, 228 exceptions
[ View ]
#21 469242-21-html_template.patch27.4 KBPasqualle
Failed: Failed to run tests.
[ View ]
#19 469242-19-html_template.patch26.5 KBPasqualle
Failed: 11531 passes, 36 fails, 2535 exceptions
[ View ]
#17 469242-17-html_template.patch19.22 KBPasqualle
Failed: Failed to apply patch.
[ View ]
#16 469242-16-html_template.patch19.73 KBPasqualle
Failed: Failed to apply patch.
[ View ]
#14 head.tpl_.diff20.41 KBmoshe weitzman
Failed: Failed to apply patch.
[ View ]
#9 head.tpl_.diff20.43 KBmoshe weitzman
Failed: Failed to apply patch.
[ View ]
#5 469242-html_template.patch14.96 KBPasqualle
Failed: Failed to apply patch.
[ View ]

Comments

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

that would not fix this problem

<head>
  <?php print $all_header_variables ?>
  ...
</head>
<body>
  <?php drupal_add_css(...) ?>  // no effect
  <?php drupal_add_js(...) ?>  // no effect
  ...
</body>

the page needs to be processed before printing <head>

EDIT: but yes, you are right, it is partially related to that issue..

I have not said that it was solving the problem. I said it was related, and it is.

sorry, little misunderstanding from my part..

StatusFileSize
new14.96 KB
Failed: Failed to apply patch.
[ View ]

problems:
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 .class

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

Status:Active» Needs review

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

I don't get why those changes are needed:

-body.two-sidebars #squeeze {
+.two-sidebars #squeeze {

Even though the templates are split in two, the structure of the rendered HTML stays the same, right ?

StatusFileSize
new20.43 KB
Failed: Failed to apply patch.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch failed testing.

@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.class

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

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

+          <?php print $page['header']; ?>
+          <?php render($page['content']); ?>
+            <?php print render($page['content']) ?>

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

Status:Needs work» Needs review
StatusFileSize
new20.41 KB
Failed: Failed to apply patch.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new19.73 KB
Failed: Failed to apply patch.
[ View ]

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

StatusFileSize
new19.22 KB
Failed: Failed to apply patch.
[ View ]

with unix style line ends this time
fixed a missing "body " for 1 css selector in garland's style.css

I still not understand why do we have to fix two different issues in one patch

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

StatusFileSize
new26.5 KB
Failed: 11531 passes, 36 fails, 2535 exceptions
[ View ]

patch rerolled with render()

Status:Needs review» Needs work

The last submitted patch failed testing.

StatusFileSize
new27.4 KB
Failed: Failed to run tests.
[ View ]

fixed garland header and the maintenance page

Status:Needs work» Needs review

The patch moves from (simplified):

<body class="<?php print $classes; ?>">
  <div id="page-top-region"><?php print $page_top; ?></div>
  <div id="page">...</div>

to
<body>
  <div id="page-top-region"><?php print $page_top; ?></div>
  <div id="page" class="<?php print $classes; ?>">...</div>

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 :

-body.drag {
+#id-of-the-new-wrapping-div.drag {

I know IE6 doesn't parse .foo.bar selectors, I don't remember if #foo.bar is OK...

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

Status:Needs review» Needs work

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

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

Status:Needs work» Needs review
StatusFileSize
new29.99 KB
Failed: 11563 passes, 0 fails, 228 exceptions
[ View ]

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

Status:Needs review» Needs work

The last submitted patch failed testing.

StatusFileSize
new21.74 KB
Failed: 11562 passes, 1 fail, 0 exceptions
[ View ]

Ok, 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.

Status:Needs work» Needs review

Really like the look of this, only skimmed through the patch but as long as we keep body.class the rest looks nice.

Status:Needs review» Needs work

The last submitted patch failed testing.

StatusFileSize
new31.08 KB
Failed: Failed to run tests.
[ View ]

@Pasqualle the patch in #27 solves the class problem.

New patch, this should pass the tests.

It may solve the problem but it looks really ugly:

body #body

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

Status:Needs work» Needs review

You can replace that with only #body if you really want to.

Re #29 test fail:
the CascadingStylesheetsTestCase->testRenderInlineFullPage() in common.test seems to be wrong.

it searches for the string

<style type="text/css">body{padding:0px;}</style>

but the actual html is now:

<style type="text/css">body{padding:0px;}<!--[if lt IE 7]><link type="text/css" rel="stylesheet" media="all" href="/themes/garland/fix-ie.css" /><![endif]--></style>

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.

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

hmm, the same code is used in the devel.module

drupal_add_css('<!--[if IE]>
    <link href="' . $path .'/devel_themer_ie_fix.css" rel="stylesheet" type="text/css" media="screen" />
<![endif]-->', array('type' => 'inline'));

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

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

StatusFileSize
new21.72 KB
Failed: Failed to run tests.
[ View ]

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

Status:Needs review» Needs work

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?

no, 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..

Well, then move things in hook_preprocess_html, if they don't break anything (at least for toolbar that needs to be done)

Great 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

<html>
<head>
  ...
    <?php print $styles ?>
    <?php print $scripts ?>
  ...
</head>
<?php print $page ?>
</html>

and page.tpl.php
<body class="<?php print $classes; ?>">
  <div id="page-wrapper"><div id="page">
  ...
  </div>
</body>

@Frando
I think because $closure has to be in html.tpl.php

StatusFileSize
new22.49 KB
Failed: Failed to run tests.
[ View ]

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

Status:Needs work» Needs review

We need a review, don't we?

Status:Needs review» Needs work

this is wrong:

-function toolbar_page_alter(&$page) {
+function toolbar_html_alter(&$page) {

you wanted to change toolbar_preprocess_page(), but that would not be necessary either, if we move

drupal_static('page_classes_array', $variables['classes_array']);

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?

Ah, true :). And true if we move to process also

<?php
'#theme_wrapper' => 'html',
?>

OMG... Subscribing.

Status:Needs work» Needs review
StatusFileSize
new22.26 KB
Failed: Failed to run tests.
[ View ]

A new version.
Moved drupal_static() to template_process_page()

subscribe

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

We can ask that in the future, but for now, to core modules we should do it in the patch.

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

Issue tags:+Needs themer review

Adding tag so we get a review from themers

Status:Needs review» Needs work

The last submitted patch failed testing.

render() needs to be changed to print render()
http://drupal.org/cvs?commit=236980

Status:Needs work» Needs review
StatusFileSize
new21.82 KB
Failed: 11659 passes, 5 fails, 0 exceptions
[ View ]

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

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new21.84 KB
Failed: Failed to apply patch.
[ View ]

I missed some prints.

StatusFileSize
new27.34 KB
Failed: Failed to apply patch.
[ View ]

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

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

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

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

to find out where the head is rendered

why would a themer care about where the html head is rendered?

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

Because he want to add IE specific styles for example, like Garland does.

Regarding IE-specific styles like Garland, should we consider moving parts of Conditional Styles into core?

Could we refer to html.tpl.php in page.tpl.php comment header and vice versa?

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

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

if ($browser = 'IE' and $browser_version < 7) {
  drupal_add_css('myIEfix.css');
}

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?

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

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

OK, I understand somewhat what is being requested here, but I am against this move for these reasons:

  1. tpl.php files are meant to output the results. Period. No function calls or logic should go in these.
  2. The only advantage this offers is being able to add css/js files in page.tpl.php, which conflicts with the above.
  3. It creates more confusion by creating yet another tpl file.
  4. drupal_add_css and drupal_add_js can easily be called in your preprocess_page, conditionaly, to fullfill your specific need.
  5. If you need to add js or css to the whole site, you can just do it in the info file.

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

@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') or theme('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.

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

Status:Needs review» Needs work

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

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

  1. PHP in tpls should be limited to print, t(), and if/elseif/else/endif. And with the new granular theming patch, we add show(), hide(), and render() to that list.
  2. More complex display logic should go in a theme’s preprocess or process function.
  3. Business logic (any logic that should remain on the site even if the theme is switched) should go in a module. <-- this is probably where Pasqualle’s embedded view should be.

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.

Status:Needs work» Closed (won't fix)

Then this is a won't fix.

grrr... I disagree with this being a wontfix.

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.

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:

Making things easier for programmers is not a good trade for making things harder for new themers.

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.

Status:Closed (won't fix)» Needs work

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

There definitely is a real problem here, this issue should not have been marked won't fix after JohnAlbin's comment, which explicitly said:

So, I would mark this “won’t fix” except for Moshe’s comment in #7; page.tpl is inconsistent with regards to render().

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.

last patch looks good to me. just some old API docs lying around mention

drupal_static('page_classes_array', $variables['classes_array']);
in template_preprocess_page(). we don't do that anymore.

this is really close. lets not lose it.

StatusFileSize
new26.22 KB
Failed: 11691 passes, 0 fails, 88 exceptions
[ View ]

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

Status:Needs work» Needs review

CNR of course.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new26.25 KB
Failed: Failed to apply patch.
[ View ]

This one should pass, but...

@tic2000: those are good examples, you will do it also in D7, I promise..

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

  <?php if ($page['page_top']): ?>
    <div id="page-top-region" class="region clearfix">
      <?php print render($page['page_top']); ?>
     </div>
   <?php endif; ?>

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

Category:feature» bug
StatusFileSize
new25.31 KB
Failed: Failed to apply patch.
[ View ]

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

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

page-domain-community-front.tpl.php
print insert_view('blog_feeds');
-----
page-user.tpl.php
print insert_view('user_quicklinks');
----
page-nodetype-story.tpl.php
print insert_view('user_summary');
---
page-contact.tpl.php
print insert_view('team_contacts')
---

vs
template.php
MYTHEME_preprocess_page() {
  if ($domain == 'community && $variables['is_front']) {
    $variables['view_blog_feed'] = insert_view('blog_feed'];
  }
  if (arg(0) == 'user') {
    $variables['view_user_quicklinks'] = insert_view('user_quicklinks');
  }
  if ($vars['node'] && arg(2) != 'edit') {
    $variables['view_user_summary'] = insert_view('user_summary');
  }
  if (arg(0) == 'node' && arg(1) == 42 && arg(2) != 'edit') {
    $variables['view_team_contacts'] = insert_view('team_contacts');
  }
}
+ all previous templates, just use variables instead of functions

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.

Status:Needs review» Needs work

The last submitted patch failed testing.

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

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

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new26.25 KB
Failed: Failed to apply patch.
[ View ]

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

Nice patch.

+++ modules/system/html.tpl.php 1 Jan 1970 00:00:00 -0000
...
+ * @see template_preprocess_html()
+ * @see template_process()
+ * @see page.tpl.php
+ */
+?>
+
+<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML+RDFa 1.0//EN"
+  "http://www.w3.org/MarkUp/DTD/xhtml-rdfa-1.dtd">

There was no leading white-space previously in page.tpl.php, can we keep it that way?

StatusFileSize
new2.59 KB
new650 bytes

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

-          <?php if ($highlight): ?><div id="highlight"><?php print $highlight ?></div><?php endif; ?>
+          <?php if ($page['highlight']): ?><div id="highlight"><?php print render($page['highlight']); ?></div><?php endif; ?>

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.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new26.25 KB
Failed: Failed to apply patch.
[ View ]

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

Status:Needs review» Reviewed & tested by the community

Only an empty line removed and test bot is happy so back to RTBC

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.

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

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.

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

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

Status:Reviewed & tested by the community» Needs review

Sounds like this still needs some more discussion.

Status:Needs review» Reviewed & tested by the community

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

cross posted.

(chances are this will need an executive decision)

Status:Reviewed & tested by the community» Needs review

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

Status:Needs review» Reviewed & tested by the community

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

Status:Reviewed & tested by the community» Needs review

Sorry... didn't mean to change the status.

@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".

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

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

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

I'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.

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

Status:Needs review» Reviewed & tested by the community

@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

@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($elements['#theme'], $elements);
    theme($elements['#theme_wrapper'], $elements);

----> theme () equals drupal_add_css() and drupal_add_js()

Status:Reviewed & tested by the community» Needs review

@webchick Indeed, we need more discussion.

@quicksketch:

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.

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:

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: “Making things easier for programmers is not a good trade for making things harder for new themers.”

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:

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

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:

Um. "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.

@Pasqualle:

@webchick: would you or anyone else revert #455844: Allow more granular theming of drupal_render'ed elements. The page.tpl.php is broken and the only solution is to "Move outside page.tpl.php". If we can use a render() in node.tpl.php, then page.tpl.php is broken.

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-hungry explode(). So, that's an alternative solution to breaking up the page.tpl.

Status:Needs review» Reviewed & tested by the community

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

Status:Reviewed & tested by the community» Needs review

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

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.

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

Status:Needs review» Reviewed & tested by the community

cross posted

Status:Reviewed & tested by the community» Needs review

Actually, I think Moshe crossposted with me. I intended to mark it "needs review" while we continue to discuss.

That sounds like a (very) big gotcha to me - "you can do this in all the other 'tpl.php' files, but not this one."

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.

Let 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

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.

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

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.

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.

I 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"..

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

Status:Needs review» Needs work

While we debate how this whole issue should work out I did notice a problem....

<?php print $page ?>

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.

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

A huge +1 for that idea, Crell. I think it could (at the very least) make the division much clearer.

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

I'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.

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

Just wanted to randomly throw in that we now have a field called "Body" by default in every Drupal install.

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

OK, 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)

Status:Needs work» Needs review
StatusFileSize
new26.33 KB
Failed: Failed to run tests.
[ View ]

OK, 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.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new26.47 KB
Failed: Failed to run tests.
[ View ]

Forgot to change a $left to $page['left']

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review

And we try again.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch failed testing.

I give up. The bot is not in a good mood.

Status:Needs work» Needs review

StatusFileSize
new26.47 KB
Failed: Failed to run tests.
[ View ]

One more try.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new26.47 KB
Failed: Failed to run tests.
[ View ]

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new29.16 KB
Failed: Failed to apply patch.
[ View ]

Forgot we have a new theme now.

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

If we do that do we also rename hook_page_alter to hook_body_alter?

Ideally, I'd prefer to see a single $page construct, with the HTML template added as a #theme_wrapper on the page object.

@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

    'page' => array(
      'arguments' => array('page' => NULL),
      'template' => 'body',
    ),
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
    'body' => array(
      'arguments' => array('page' => NULL),
      'template' => 'body',
    ),
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?

IMO, 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.

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

StatusFileSize
new29.59 KB
Failed: Failed to run tests.
[ View ]

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review

Here we go again :)

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new29.7 KB
Failed: Failed to apply patch.
[ View ]

New try.

StatusFileSize
new29.57 KB
Failed: Failed to apply patch.
[ View ]

Status:Needs review» Reviewed & tested by the community

Thanks for the endless re-rolls, tic2000. RTBC.

Status:Reviewed & tested by the community» Needs work

I'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() to template_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.

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

Status:Needs work» Needs review
StatusFileSize
new29.41 KB
Failed: 12071 passes, 0 fails, 84 exceptions
[ View ]

This patch takes care of the regression and of the "empty" problem. Let see what the bot things.
Waiting for the full review.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new30.29 KB
Failed: Failed to apply patch.
[ View ]

Now it should come back green.

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

I found a way to allow passing changes from the wrapped theme to the wrapping theme (if the themes are templates)

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

StatusFileSize
new3.11 KB

@Pasqualle
I have a patch created.
I will actually attach it as text file for those who want to take a look.

To quote moshe "at some point the cure is worst than the disease" and I agree with him.

Status:Needs review» Needs work

The last submitted patch failed testing.

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

Status:Needs work» Needs review
StatusFileSize
new31.19 KB
Failed: 12124 passes, 4 fails, 12 exceptions
[ View ]

Reroll. One addition to the patch is that now it adds suggestion for HTML template files.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new32.92 KB
Failed: Failed to apply patch.
[ View ]

I don't like green :)

I love green!

Status:Needs review» Reviewed & tested by the community

once again, rtcb

Status:Reviewed & tested by the community» Needs work

The last submitted patch failed testing.

Ugh. Any chance you can re-roll, tic2000?

Status:Needs work» Needs review
StatusFileSize
new32.87 KB
Failed: Failed to apply patch.
[ View ]

Status:Needs review» Reviewed & tested by the community

thanks tic2000. still rtbc.

Status:Reviewed & tested by the community» Needs work

The last submitted patch failed testing.

Oh noes. Bot says "failed Failed: Failed to apply patch."

Status:Needs work» Needs review
StatusFileSize
new33.28 KB
Failed: Failed to apply patch.
[ View ]

Status:Needs review» Reviewed & tested by the community

per #192

Status:Reviewed & tested by the community» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new32.39 KB
Failed: 12362 passes, 0 fails, 10 exceptions
[ View ]

It's ie_styles that's hunking....

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new32.51 KB
Failed: Failed to apply patch.
[ View ]

Status:Needs review» Reviewed & tested by the community

per #192

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

Subscribe. This would be *awesome*.

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

Yes, 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. ;)

I can keep re-rolling, but I can't stop 09.01 from coming (unfortunately)

This this is an excellent step forward - however applying patch on current head gives me a white screen. Nothing in the logs either.

Subscribe. Sure hope this makes it...

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

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

@istos
It takes Dries to say "yay" or "nay".
You can dress as the Druplicon anyway, just to show your Drupal love :P

Issue tags:+API change

tagging

Status:Reviewed & tested by the community» Needs work

The last submitted patch failed testing.

Status:Needs work» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new31.78 KB
Failed: 12778 passes, 1 fail, 2 exceptions
[ View ]

Status:Needs review» Reviewed & tested by the community

please wait for green before commit.

Status:Reviewed & tested by the community» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new32.26 KB
Failed: 12786 passes, 0 fails, 1 exception
[ View ]

StatusFileSize
new32.94 KB
Passed: 12952 passes, 0 fails, 0 exceptions
[ View ]

This will pass

Status:Needs review» Reviewed & tested by the community

Testbot is happy.

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

Status:Reviewed & tested by the community» Needs work

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

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

Status:Needs work» Needs review
StatusFileSize
new29.46 KB
Passed: 12928 passes, 0 fails, 0 exceptions
[ View ]

I 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

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

Webchick was sure to bring that up while talking to the designers during the code sprint. To us, it is a non-issue.

subscribe

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

Status:Needs review» Reviewed & tested by the community

So #220 is ready to fly then.

Agreed for #220. :-)

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

Status:Reviewed & tested by the community» Needs review

So while themers can edit html.tpl.php, we'd really rather they didn't.

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

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

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

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

@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

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[.]

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

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

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

StatusFileSize
new32.97 KB
Passed: 12962 passes, 0 fails, 0 exceptions
[ View ]

#220 failed to apply (some spacing issue in system.module) -- rerolled it to review.

Status:Needs review» Reviewed & tested by the community

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

<?php
function stark_preprocess_html(&$vars) {
 
$vars['classes_array'][] = 'wombat';
}
?>

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

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML+RDFa 1.0//EN"
  "http://www.w3.org/MarkUp/DTD/xhtml-rdfa-1.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" dir="ltr"
  xmlns:admin="http://webns.net/mvcb/"
  xmlns:content="http://purl.org/rss/1.0/modules/content/"
  xmlns:dc="http://purl.org/dc/elements/1.1/"
  xmlns:dcterms="http://purl.org/dc/terms/"
  xmlns:foaf="http://xmlns.com/foaf/0.1/"
  xmlns:owl="http://www.w3.org/2002/07/owl#"
  xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
  xmlns:rdfs="http://www.w3.org/2000/01/rdf-schema#"
  xmlns:rss="http://purl.org/rss/1.0/"
  xmlns:sioc="http://rdfs.org/sioc/ns#"
  xmlns:xsd="http://www.w3.org/2001/XMLSchema">

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

I'll leave this patch up to webchick. Follow-up your gut on this one, Angie.

Status:Reviewed & tested by the community» Needs review

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

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

Status:Needs review» Reviewed & tested by the community

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

@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

Status:Reviewed & tested by the community» Needs review

#227 was a reply to #226.

Btw, I suppose I should respond to this too, so there is some context to why I am being so persnickety about this.

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.

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.

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

I also want to see representation of opinions I do NOT see on every core issue here

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

@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

$variables['template_files'][] = "myspecialtemplate.tpl.php";

---

for separate header and footer templates (instead of includes) I would advise something like:

// footer.tpl.php and header.tpl.php template creation in the template.php file
/**
* Implementation of hook_theme(). //The only hook which can be used in themes..
*/
function MYTHEMENAME_theme() {
  $items['header'] = array(
    'template' => 'header',
  );
  $items['footer'] = array(
    'template' => 'footer',
  );
  return $items;
}
/**
* Preprocess the page template.
*/
function MYTHEMENAME_preprocess_page(&$vars) {
  $vars['myheader'] = theme('header');
  $vars['myfooter'] = theme('footer');
  // Reconstruct CSS and JS variables. This is needed in D6, hopefully this will just work in D7 after this patch..
  $vars['css'] = drupal_add_css();
  $vars['styles'] = drupal_get_css();
  $vars['scripts'] = drupal_get_js();
}
//and of course (if neeeded) you have the preprocess functions for the new templates
function MYTHEMENAME_preprocess_header(&$vars) {
  ..
}
function MYTHEMENAME_preprocess_footer(&$vars) {
  ..
}

so then in the page.tpl.php template you would use
..
<?php print $myheader ?>
..
<?php print $myfooter ?>
..

and yes, I also like it separated..

Don't we have some kind of region templates in D7 already? I've read it somewhere..

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

StatusFileSize
new31.47 KB
Failed: 12928 passes, 173 fails, 3489 exceptions
[ View ]

fixing bug in modules/system/page.tpl.php:

-          <?php print $sidebar_second; ?>
+          <?php print render($page['sidebar_first']); ?>

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

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new31.63 KB
Passed: 12937 passes, 0 fails, 0 exceptions
[ View ]

left 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

StatusFileSize
new30.79 KB
Passed: 12920 passes, 0 fails, 0 exceptions
[ View ]

ok, 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.

StatusFileSize
new30.78 KB
Passed: 12950 passes, 0 fails, 0 exceptions
[ View ]

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

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review

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

This doesn't fail locally - and DBlog is not affected by the patch, so I think there's a testbot issue.

It is -- i did a retest and it came back green

+++ modules/system/page.tpl.php
@@ -76,41 +43,30 @@
+<body class="<?php print $classes; ?>"<?php print $attributes; ?>>
+<?php print render($page['page_top']); ?>
+++ modules/system/page.tpl.php
@@ -196,12 +152,17 @@
+<?php
// Any calls to drupal_add_js() must be made prior to rendering this last region.
+  print render($page['page_bottom']);
+
?>

<body>

so, 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.php

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

What 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

Sub, Sorry didn't have a chance to read all #266 issues but I like where this is heading.

tic2000 -- basically it comes down to webchick saying:

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.

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.

alexanderpas:

In my opinion, the upside of having in the page.tpl.php doesn't compare to the downsides of having those additional requirements in page.tpl.php

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.

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

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

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

I want to hear the themers reason why body makes more sense in page.tpl.php and not html.tpl.php.

I'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.

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

okay 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

@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

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

StatusFileSize
new20.1 KB

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

StatusFileSize
new32.97 KB
Passed: 12944 passes, 0 fails, 0 exceptions
[ View ]

I guess this'll work better ;)

directly taken from #242 as #220 didn't apply due to spacing issue.

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

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

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML+RDFa 1.0//EN"
  "http://www.w3.org/MarkUp/DTD/xhtml-rdfa-1.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="<?php print $language->language; ?>" dir="<?php print $language->dir; ?>"
  <?php print $rdf_namespaces; ?>>
<head profile="<?php print $grddl_profile; ?>">
  <title><?php print $head_title; ?></title>
  <?php print $head; ?>
  <?php print $styles; ?>
  <?php print $scripts; ?>
</head>
<body class="<?php print $classes; ?>">
  <?php print $page_top; ?>
  <div id="header">
    <h1 id="site-name">
      <a href="<?php print $front_page; ?>" title="<?php print t('Home'); ?>" rel="home"><span><?php print $site_name; ?></span></a>
    </h1>
    <div id="site-slogan"><?php print $site_slogan; ?></div>
  </div>
  <div id="navigation">
    <?php print theme('links', $main_menu, array('id' => 'main-menu', 'class' => array('links', 'clearfix')), t('Main menu')); ?>
  </div>
  <div id="content">
  <?php print $page; ?>
  </div>
  <?php print $page_bottom; ?>
</body>
</html>

Where it reads
<?php
print $page;
?>
we can define varying interchangeable layouts:
two columns title hidden
<h1 class="title" id="page-title"><?php print $title; ?></h1>
<div id="content-area" class="region">
  <?php print render($page['content']); ?>
</div>
<div id="sidebar-second" class="column sidebar">
    <?php print render($page['sidebar_first']); ?>
</div>

three columns title hidden
<div id="content-area" class="region">
  <?php print render($page['content']); ?>
</div>
<div id="sidebar-second" class="column sidebar">
    <?php print render($page['sidebar_first']); ?>
</div>
<div id="sidebar-first" class="column sidebar">
   <?php print render($page['sidebar_first']); ?>
</div>

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.

Status:Needs review» Reviewed & tested by the community

#242 passes - some details relating to template variables should go into follow up issues imho.

pwolanin did put back the body tags page.tpl.php in #259 and #260, why starting with #242 to create a new patch?

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

because those were in response of #246... which we have resolved now, so #242 (or #278) is what will be committed.

@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

<div id="wrapper-container">
    <div id="wrapper">
       <div id="wrapper-wrapper">

footer.php
       </div>
  </div>
</div>

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:

<body class="<?php print $classes; ?>">
<?php render($page_top);?>
<div id="wrapper-container">
    <div id="wrapper">
       <div id="wrapper-wrapper">
      <?php render($page);?>
       </div>
  </div>
</div>
<?php render($page_bottom);?>

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
<?php
if () {
?>
everywhere and 500 line page.tpl.php files.

Its happy accidental feature introduced by your patch!

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

Status:Reviewed & tested by the community» Needs work

please- 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

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

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

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new32.97 KB
Failed: 12954 passes, 12 fails, 0 exceptions
[ View ]

Security fix for title order, and wrong sidebar fixed as well. I think how attributes are dealt with should be in a follow up

Status:Reviewed & tested by the community» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new31.28 KB
Failed: 12940 passes, 4 fails, 12 exceptions
[ View ]

Status:Needs review» Needs work

The last submitted patch failed testing.

StatusFileSize
new33 KB
Passed: 12948 passes, 0 fails, 0 exceptions
[ View ]

this is the one i feel it

Status:Needs work» Reviewed & tested by the community

Come on, bot!

Status:Reviewed & tested by the community» Needs review

[say wtihGesture="mr burns hands"]Excellent[/say]

BTW -- are the status things defaulting to one status lower now?

Status:Reviewed & tested by the community» Needs review

I 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 block content 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.

Status:Needs review» Reviewed & tested by the community

Setting status back to RTBC.

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

Pages