Posted by Jeff Burnz on June 15, 2011 at 10:11pm
9 followers
Issue Summary
As stated in title, lots to do here.
http://api.drupal.org/api/drupal/modules--system--maintenance-page.tpl.p...
As stated in title, lots to do here.
http://api.drupal.org/api/drupal/modules--system--maintenance-page.tpl.p...
Comments
#1
First patch, any thoughts?
#2
Thanks for the patch!
just an FYI that @rupl is working on #1077578: Convert page.tpl.php to HTML5, which is related.
#3
+++ b/modules/system/maintenance-page.tpl.phpundefined@@ -51,14 +49,14 @@
- <div id="sidebar-first" class="column sidebar">
+ <aside id="sidebar-first" class="column sidebar">
<?php print $sidebar_first; ?>
- </div> <!-- /sidebar-first -->
page template will use div for these wrappers, so lets be consistent with that.
+++ b/modules/system/maintenance-page.tpl.phpundefined@@ -66,25 +64,25 @@
<div id="footer-wrapper">
- <div id="footer">
+ <footer>
<?php if (!empty($footer)): print $footer; endif; ?>
- </div> <!-- /footer -->
+ </footer> <!-- /footer -->
We don't need the outer wrapper, and the footer tag should be inside the condition.
-19 days to next Drupal core point release.
#4
Should I remove the
<div id="content">? (There's already a<div id="main">and a<section>for the content, so that's still 3 layers of html tags as-is now)#5
I haven't looked at the what rupl ended up with in page template, but emulating that closely I think is pretty important, from a styling perspective so themers don't have to add back styling hooks available in page tpl, so it (the theme/design) will just "work" for the most part without any special casing required.
#6
here I mainly just rupl-fied this template file by moving the needed stuff from page.tpl.php into this tpl
feedback?
I'm a little concerned about moving around html tag hirearchy as I did. Does this tpl use a css file or does it use the same ones as all the others?
#7
edit: ignore this one (following along with the conversation on IRC re: page.tpl.php)
updated patch coming
#8
changed footer id to footer rather than #contentinfo
#9
Thanks for your work on this patch! Note that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."
Tagging as novice for the task of rerolling the Drupal 8.x patch.
If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.
#10
ok updated to fix /core path
#11
Per Jacine's request, I'm marking that I'd like to help review this patch
#12
Tagging this for the next sprint.
#13
Thanks for rerolling. :)
#14
I saw the following PHP notices when testing with 5.3.2. I was using Stark in maintenance mode while logged out of Drupal.
Notice: Undefined variable: html_attributes in include() (line 15 of .../d8h5/core/modules/system/maintenance-page.tpl.php).Do we need $html_attributes in the
<html>tag for this template? Does maintenance mode prohibit modules from outputting to this variable?Notice: Undefined index: sidebar_first in include() (line 59 of .../d8h5/core/modules/system/maintenance-page.tpl.php).Notice: Undefined index: sidebar_second in include() (line 73 of .../d8h5/core/modules/system/maintenance-page.tpl.php).
For the sidebar warnings, the notice disappears when I change
<?php if ($page['sidebar_first']): ?>to<?php if (!empty($page['sidebar_first'])): ?>, and do the same with $sidebar_second. Not sure why that syntax works in regular page.tpl.php, but it throws a notice here.+++ b/core/modules/system/maintenance-page.tpl.phpundefined@@ -11,10 +11,8 @@
+<!DOCTYPE html>
Remove the line break between the closing PHP tag and the DOCTYPE.
+++ b/core/modules/system/maintenance-page.tpl.phpundefined@@ -23,71 +21,68 @@
+ <section id="content-content" class="clearfix">
+ <?php print $content; ?>
+ </section> <!-- #content-content -->
Remove
<section>element surrounding$contentto match page.tpl.php.+++ b/core/modules/system/maintenance-page.tpl.phpundefined
@@ -23,71 +21,68 @@
+ </section> <!-- #content-content -->
+ </div> <!-- #content -->
+++ b/core/modules/system/maintenance-page.tpl.phpundefined
@@ -23,71 +21,68 @@
+ </div> <!-- #page -->
I know this is nitpicky, but please remove whitespace between closing tags and comments. Thanks and great work!!
#15
Not sure that as a reviewer I should be re-rolling a patch, but I did it for the sake of helping move things along. These are some things that I addressed:
Please double check that the end closing tag for #logo-title was indeed missing, as it could just be me missing something.
#16
The last submitted patch, maintenance-page.tpl_.php--convert-to-html5-1189822-14.patch, failed testing.
#17
rupl, as for the white space between closing tags and comments, we should be consistent with page.tpl.php that includes the space. Personally I would get rid of the space, but as long as we are consistent then I guess it doesn't matter :)
#18
#15: maintenance-page.tpl_.php--convert-to-html5-1189822-14.patch queued for re-testing.
#19
@rasskull are you looking at the current page.tpl.php in your development directory, or as it will be committed in that issue I linked to? I personally removed all of the whitespace-separated comments and slashes in the other issue. If you look at the patch in #32 you will see what I mean.
You are correct about the
#logo-titleclosing tag. Don't know how I missed that :) No other validation errors popped up when I ran it just now.#20
@rupl ahhh yes that could be an issue, I'll make sure I'm up to date - great thing about that pesky space :)
Since @chrispomeroy has been been doing such a great job of getting the patches rolling I'll step aside and let him roll the next with anything that looks fitting from my last patch.
#21
-removed $html_attributes
-changed if ($page to if (!empty(page
-removed break b/t php tag and doctype tag
-removed section element
-removed spaces between closing tags and comments
-removed the #content comment like in page.tpl.php (shrug)
-removed the / from comments
#22
ok this is the one - last one had other junk in it
#23
Great work, Chris!
Regarding the
$html_attributesnotice: Jacine pointed me to http://api.drupal.org/api/drupal/includes--theme.inc/function/template_p...I think we should add some properties to
$html_attributesin this preprocess, and then we can print more than<html>while avoiding the PHP notices. Even justlangwould suffice.#24
Yes, we do need lang. Lossing the lang and dir attributes is not good. Please bring them back :) No need for xml:lang though, see #1330922: Drupal 8 HTML 5 page template should use lang not xml:lang.
#25
#26
Hey, just wanted to let you guys know I looked into the idea of implementing this with a theme hook suggestion, and it's basically a no go for this template, so carry on.
And yes, the preprocess function for this template needs to be brought up to speed with what's currently going on in
template_preprocess_html()as much as possible for consistency.#27
I'm not sure I'm 100% following, but it sounds like $html_elements creates a php error because the variable isn't set but we still need to output language and language direction in
<html>.Take a look at this patch and see if that does it, if not I might be a little lost here on this issue.
#28
Yeah, well, Jacine IMHO was trying to explain that there should be a preprocess for this template (if not already), and that should follow the HTML attributes generated in template_preprocess_html(). Just adding $html_elements or whatever in the template will not make it work without actually defining it in a preprocess function like in template_preprocess_html(). The way you used the lang and dir attributes (putting aside that dir did not get a closing quote), the list of HTML attributes is not extensible like in the general HTML template, which makes it inconsistent both for developers and themers.
#29
I've taken the relevant bits from the preprocess and process functions and copied them to the maintenance versions of each function. So
$html_attributesand$body_attributesare now available to be altered in maintenance mode. There's nothing in$body_attributesfor now, but I did test it successfully before submitting patch.#30
Looks good to me for language support! Thanks!
#31
This looks really solid to me, I looked through it for awhile and didn't catch anything!
#32
To make this more consistent with page.tpl.php I did the following in this patch:
Some of the things like #logo-title removal and messages being moved were discussed with @jacine so if you think I'm crazy for doing it that's why :)
#33
Ask jacine what she wants, we can't copy the html attributes all around.
She has a plan for this...
#34
Hey guys...
So, first I was hoping to use theme hook suggestions here, and I think that's what @aspilicious is referring to in #33, but as I said in #26, that's probably not going to work out.
And yes, @ Gábor Hojtsy is correct in #28. That's exactly what I meant.
The patch in #32 looks good from a markup standpoint, minus one whitespace error.
+++ b/core/modules/system/maintenance-page.tpl.phpundefined@@ -10,84 +10,80 @@
+ ¶
There's some whitespace here.
However, there are massive problems with the preprocess functions behind this template. They are so incredibly inconsistent and there is no rhyme or reason for the differences at this point. There's even Drupal 6 code sitting in the template_preprocess_maintenance_page() function. This is why I wanted to be able to just use theme hook suggestions, but literally got scared away from it because of all the hacks surrounding maintenance mode. I was considering letting that slide and moving the remaining tasks to a separate issue, but it's really, really pissing me off right now.
I'm still not sure it's possible, but I think I'll take a crack at it, because there is just too wrong much to write right now, and will end up being way easier to code than try to explain.
So, standby...
#35
From #1183042-28: Add WAI-ARIA roles to Core blocks
"I believe role="main" should go in the page.tpl and maintenance-page.tpl because the page content's title is in those templates and is part of the "main" content."
With which I agree. Can we role this into this template conversion please?
#36
The patch here will follow page.tpl.php, and role="main" is already in there. :)
#37
Alright, so I actually went into the depths of hell with this patch and tried to make it work. I did have it working until it got to the update/install pages, where it would no longer select the proper theme. Going through the process though, I'm not happy with the way it would complicate template_preprocess_page(), so decided not to bother continuing on this quest. However, I now know which variables are intentionally left out (and unfortunately never documented). Those are:
These all require the menu system, which is either not built (install) or not available (maintenance mode, DB connection or fatal error).
Anyway, as I sit here ready to write this patch, I just can't bring myself to do it, because I hate this template so much. So, I'd like to ask why on earth are doing this, and should we continue doing this?
If we think about the 80% use case here, the only thing that is needed here is the stuff in the header, sidebar, $messages and content. There are 3 use cases for this template and only 2 regions are actually used:
These are all very minimal pages and personally I think this is a case for a very minimal template. It seems completely crazy to provide all the variables to this template, and try to maintain consistently from a markup standpoint with page.tpl.php, because they serve 2 completely different purposes.
Thoughts?
#38
Yes, we cannot really get the kind of data that is not available in maintenance, so I'm not sure we should sweat much about it. Let's build a consistent parallel template IMHO with the pieces we do have.
#39
I didn't expect all of the variables to be mirrored either. When we were discussing the attempt to match page.tpl.php up above, I assumed we meant that we would match it when possible and simply remove the non-functional bits. I agree that we can/should not duplicate all the preprocess functionality.
@Jacine, I am re-assigning this back to myself. You're no longer allowed to fret over this one :)
#40
@rupl ♥
#41
Ok I don't know what I'm doing. I just tried to figure this out and removed hunks that I didn't like ;).
I removed an entire hunk setting region variables, maybe that broke everything, dunno. Thats for the theming guys and girls.
#42
The last submitted patch, maintenance_template_try_because_I_was_forced_by_webchick.patch, failed testing.
#43
without whitespace issues...
#44
The last submitted patch, maintenance_template_try_because_I_was_forced_by_webchick_v2.patch, failed testing.
#45
/me shouldn't write patches this late
#46
The last submitted patch, maintenance_template_try_because_I_was_forced_by_webchick_v3.diff, failed testing.
#47
Eat this testbot
#48
The last submitted patch, maintenance_template_1189822_47.patch, failed testing.
#49
+++ b/core/includes/theme.incundefined@@ -2570,13 +2570,8 @@
+ foreach($region as $region) {
This should be
foreach($regions as $region). As written in the patch, it takes an empty var and assigns it to itself.+++ b/core/includes/theme.maintenance.incundefined
@@ -138,7 +138,7 @@
- return theme('maintenance_page', $variables);
+ return theme('maintenance', $variables);
+++ b/core/includes/theme.maintenance.incundefined
@@ -154,7 +154,7 @@
- return theme('maintenance_page', $variables);
+ return theme('maintenance', $variables);
These two sections should retain the
maintenance_pagehook because we're only changing tpl suggestions, not the hook itself.+++ b/core/includes/theme.incundefined@@ -2641,6 +2629,14 @@
+ $variables['html_attributes_array']['lang'] = $language;
When I viewed source, I saw
<html lang="en English 0 1 0 1". This is because $language is an object and the language code is stored within$language->language.--
For low level maintenance stuff like this we should be testing the entire install process, then let the bot run SimpleTest like normal. When I attempted to do a fresh install with this patch, it WSODed. After applying the changes I noted above, it seemed to work pretty well.
Finally, it only occurred to me just now that the installation process uses Seven's maintenance-page.tpl.php. Shall we update that template while we're at it, or should we wait until the other core themes receive HTML5 treatment? If we do not want to change it now, we're introducing the following PHP notice to many of the installation screens. This is due to our use of
$html_attributesin the new preprocess/tpl.Notice: Undefined property: stdClass::$dir in include() (line 4 of /Users/fkchris/Sites/d8h5/core/themes/seven/maintenance-page.tpl.php).#50
Ok, first thing was a typo...
I think I fixed Bartik and seven so it won't throw errors anymore.
Now I'm confused about one thing:
maintenance-page.tpl.php is renamed to maintenance.tpl.php but the preprocessing functions are still in maintenance_page. How does that system works?
#51
No we need to change the hook too. The point here is to have this make sense and be consistent. If we are changing the template there is no point in leaving the theme hook named something else. That would be very confusing.
@aspilicious preprocess functions are named based on the theme hook:
+++ b/core/includes/common.incundefined@@ -6600,7 +6600,7 @@
'maintenance_page' => array(
This is the theme hook and must be changed to 'maintenance'. Once that is done, the functions should be renamed to template_preprocess_maintenance() and template_process_maintenance() and they will Just Work ™. Make sense? If not, let me know and I'll assign this back to myself.
#52
@Jacine don't you dare reassign this to yourself!!
This is a simple matter of mismatched hooks. I opted to resolve them back to the original name, but renaming them all consistently is not hard, whichever name is preferred. I opted to maintain the old "maintenance_page" hook for compatibility, but these types of changes are what slush is for.
Here is a list of places where 'maintenance_page' currently occurs in core. Much of this will break when we change the hook.
d8h5 fkchris$ grep -rn 'maintenance_page' *
core/includes/batch.inc:193: $fallback = theme('maintenance_page', array('content' => $fallback, 'show_messages' => FALSE));
core/includes/common.inc:2508: print theme('maintenance_page', array('content' => filter_xss_admin(variable_get('maintenance_mode_message',
core/includes/common.inc:6601: 'maintenance_page' => array(
core/includes/errors.inc:253: print theme('maintenance_page', array('content' => t('The website encountered an unexpected error. Please try again later.')));
core/includes/theme.inc:2378: * template_preprocess_maintenance_page() to keep all of them consistent.
core/includes/theme.inc:2553: * This preprocessor will run its course when theme_maintenance_page() is
core/includes/theme.inc:2565:function template_preprocess_maintenance_page(&$variables) {
core/includes/theme.inc:2643: $variables['theme_hook_suggestion'] = 'maintenance_page__offline';
core/includes/theme.inc:2649: * This processor will run its course when theme_maintenance_page() is invoked.
core/includes/theme.inc:2653:function template_process_maintenance_page(&$variables) {
core/includes/theme.maintenance.inc:141: return theme('maintenance_page', $variables);
core/includes/theme.maintenance.inc:157: return theme('maintenance_page', $variables);
core/modules/overlay/overlay.module:461: * Implements hook_preprocess_maintenance_page().
core/modules/overlay/overlay.module:466: * @see overlay_preprocess_maintenance_page()
core/modules/overlay/overlay.module:468:function overlay_preprocess_maintenance_page(&$variables) {
core/modules/system/maintenance.tpl.php:11: * @see template_preprocess_maintenance_page()
core/themes/bartik/template.php:76: * Implements hook_preprocess_maintenance_page().
core/themes/bartik/template.php:78:function bartik_preprocess_maintenance_page(&$variables) {
core/themes/bartik/template.php:88:function bartik_process_maintenance_page(&$variables) {
core/themes/bartik/templates/maintenance-page.tpl.php:10: * @see template_preprocess_maintenance_page()
core/themes/bartik/templates/maintenance-page.tpl.php:11: * @see bartik_process_maintenance_page()
core/themes/seven/template.php:6:function seven_preprocess_maintenance_page(&$vars) {
#53
Not if you hazz a patch.
Untested so it could blow up site, just adding now because rupl posted the comment.
#54
Cool, cool. Thanks guys! So yeah... I could care less about backward compatibility. In the history of Drupal, I have never, ever been able to "upgrade" a theme. LOL. I'm much, much more interested in having templates that makes sense, and now is the time to make these kinds of changes. BUT... You bring up a good point considering the nature of this template. I'm not sure if this would have a direct affect people running update.php from Drupal 7 to Drupal 8. We would need to test that for sure, and if there is a problem, figure out how to fix it.
Setting to needs review so we can see what the testbot thinks of @aspilicious' patch.
#55
Didn't work because of the suffix. Added on purpose because I didn't think this patch is rdy for testing. But we'll see...
#56
Testresults ar GREEN :D, what are the next steps?
#57
@aspilicous Great! See the BUT in #54.
#58
Upgrading does not work. You get a white screen visiting core/update.php
#59
*jacine weeps loudly.*
Ok, thanks @aspilicous. I'm going to ask someone that knows about this stuff if there's any hope for this. At the very least, even if we cannot change the template hook (which would suck), the tests probably shouldn't be passing right now.
#60
This definitely needs an issue summary before I ask for help. Just noting that in case someone gets to it before I'm able to.
#61
includes/update.inc has code in a d8 bootstrap fix function, which currently only checks the schema. It would have code to fix bootstrap issues like this. Look at related d7 code at http://api.drupal.org/api/drupal/includes--update.inc/function/update_fi.... See the d8 code at http://api.drupal.org/api/drupal/core--includes--update.inc/function/upd... and/or http://api.drupal.org/api/drupal/core--includes--update.inc/function/upd....
#62
Here you go.
Upgrade works, someone needs to retest this manually :)
#63
Looks good in terms of language. :)
#64
This patch works well, but I think this should be split into two issues. So let's move the bulk of this patch over into #1360994: Change hook_maintenance_page to hook_maintenance and leave the HTML5 conversion to this issue.
Postponed on #1360994: Change hook_maintenance_page to hook_maintenance
#65
I don't think this is on the html5 sprint given it has that tag for "the next sprint" since Nov 1st and is not actively being worked on. Let me know if I steppd on toes inadvertently. Just wanting to clear the issues in current sprint list :)
#66
Well, we have talked about it every week in our sprint meetings, but it's held up on #1360994: Change hook_maintenance_page to hook_maintenance, which we need help with. Having an issue tagged sprint for a few months without it being finished is also not rare. :)
#67
Let's not hold this up on the hook name change anymore. It needs to get done.