Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.As stated in title, lots to do here.
http://api.drupal.org/api/drupal/modules--system--maintenance-page.tpl.p...
Comments
Comment #1
chrispomeroy CreditAttribution: chrispomeroy commentedFirst patch, any thoughts?
Comment #2
JacineThanks for the patch!
just an FYI that @rupl is working on #1077578: [Followup] Convert bartiks page.tpl.php to HTML5, which is related.
Comment #3
Jeff Burnz CreditAttribution: Jeff Burnz commentedpage template will use div for these wrappers, so lets be consistent with that.
We don't need the outer wrapper, and the footer tag should be inside the condition.
-19 days to next Drupal core point release.
Comment #4
chrispomeroy CreditAttribution: chrispomeroy commentedShould 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)Comment #5
Jeff Burnz CreditAttribution: Jeff Burnz commentedI 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.
Comment #6
chrispomeroy CreditAttribution: chrispomeroy commentedhere 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?
Comment #7
chrispomeroy CreditAttribution: chrispomeroy commentededit: ignore this one (following along with the conversation on IRC re: page.tpl.php)
updated patch coming
Comment #8
chrispomeroy CreditAttribution: chrispomeroy commentedchanged footer id to footer rather than #contentinfo
Comment #9
xjmThanks 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.
Comment #10
chrispomeroy CreditAttribution: chrispomeroy commentedok updated to fix /core path
Comment #11
rasskull CreditAttribution: rasskull commentedPer Jacine's request, I'm marking that I'd like to help review this patch
Comment #12
JacineTagging this for the next sprint.
Comment #13
xjmThanks for rerolling. :)
Comment #14
ruplI saw the following PHP notices when testing with 5.3.2. I was using Stark in maintenance mode while logged out of Drupal.
Do we need $html_attributes in the
<html>tag for this template? Does maintenance mode prohibit modules from outputting to this variable?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.Remove the line break between the closing PHP tag and the DOCTYPE.
Remove
<section>element surrounding$contentto match page.tpl.php.I know this is nitpicky, but please remove whitespace between closing tags and comments. Thanks and great work!!
Comment #15
rasskull CreditAttribution: rasskull commentedNot 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.
Comment #17
rasskull CreditAttribution: rasskull commentedrupl, 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 :)
Comment #18
rasskull CreditAttribution: rasskull commented#15: maintenance-page.tpl_.php--convert-to-html5-1189822-14.patch queued for re-testing.
Comment #19
rupl@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.Comment #20
rasskull CreditAttribution: rasskull commented@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.
Comment #21
chrispomeroy CreditAttribution: chrispomeroy commented-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
Comment #22
chrispomeroy CreditAttribution: chrispomeroy commentedok this is the one - last one had other junk in it
Comment #23
ruplGreat 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.Comment #24
Gábor HojtsyYes, 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.
Comment #25
Gábor HojtsyComment #26
JacineHey, 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.Comment #27
chrispomeroy CreditAttribution: chrispomeroy commentedI'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.
Comment #28
Gábor HojtsyYeah, 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.
Comment #29
ruplI'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.Comment #30
Gábor HojtsyLooks good to me for language support! Thanks!
Comment #31
rasskull CreditAttribution: rasskull commentedThis looks really solid to me, I looked through it for awhile and didn't catch anything!
Comment #32
rasskull CreditAttribution: rasskull commentedTo 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 :)
Comment #33
aspilicious CreditAttribution: aspilicious commentedAsk jacine what she wants, we can't copy the html attributes all around.
She has a plan for this...
Comment #34
JacineHey 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.
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...
Comment #35
Everett Zufelt CreditAttribution: Everett Zufelt commentedFrom #1183042-28: Regression: 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?
Comment #36
JacineThe patch here will follow page.tpl.php, and role="main" is already in there. :)
Comment #37
JacineAlright, 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?
Comment #38
Gábor HojtsyYes, 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.
Comment #39
ruplI 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 :)
Comment #40
Jacine@rupl ♥
Comment #41
aspilicious CreditAttribution: aspilicious commentedOk 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.
Comment #43
aspilicious CreditAttribution: aspilicious commentedwithout whitespace issues...
Comment #45
aspilicious CreditAttribution: aspilicious commented/me shouldn't write patches this late
Comment #47
aspilicious CreditAttribution: aspilicious commentedEat this testbot
Comment #49
ruplThis should be
foreach($regions as $region). As written in the patch, it takes an empty var and assigns it to itself.These two sections should retain the
maintenance_pagehook because we're only changing tpl suggestions, not the hook itself.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.Comment #50
aspilicious CreditAttribution: aspilicious commentedOk, 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?
Comment #51
JacineNo 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:
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.
Comment #52
rupl@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.
Comment #53
aspilicious CreditAttribution: aspilicious commentedNot if you hazz a patch.
Untested so it could blow up site, just adding now because rupl posted the comment.
Comment #54
JacineCool, 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.
Comment #55
aspilicious CreditAttribution: aspilicious commentedDidn't work because of the suffix. Added on purpose because I didn't think this patch is rdy for testing. But we'll see...
Comment #56
aspilicious CreditAttribution: aspilicious commentedTestresults ar GREEN :D, what are the next steps?
Comment #57
Jacine@aspilicous Great! See the BUT in #54.
Comment #58
aspilicious CreditAttribution: aspilicious commentedUpgrading does not work. You get a white screen visiting core/update.php
Comment #59
Jacine*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.
Comment #60
JacineThis definitely needs an issue summary before I ask for help. Just noting that in case someone gets to it before I'm able to.
Comment #61
Gábor Hojtsyincludes/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....
Comment #62
aspilicious CreditAttribution: aspilicious commentedHere you go.
Upgrade works, someone needs to retest this manually :)
Comment #63
Gábor HojtsyLooks good in terms of language. :)
Comment #64
ruplThis 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
Comment #65
Gábor HojtsyI 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 :)
Comment #66
JacineWell, 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. :)
Comment #67
JacineLet's not hold this up on the hook name change anymore. It needs to get done.
Comment #68
RobLoach#62: maintenance_template_1189822_62.patch queued for re-testing.
Comment #70
Gábor HojtsyAnybody want to pick this up again?
Comment #71
star-szrUpdating the theme hook seems way out of scope for an HTML5 conversion - that really ballooned the scope of this patch.
Instead of rerolling I would recommend rolling a new patch making the minimal changes necessary to make maintenance-page.html.twig HTML5. It may be as simple as updating the DOCTYPE.
Just a note - this may conflict with #1337554: Develop and use separate branding for the installer but maybe we can get it in sooner :)
Comment #72
star-szrOh, and see also #2011578: Markup for Stark's page.html.twig + maintenance-page.html.twig, maybe that will get in first if it has enough momentum behind it (read: reviews and patches).
Comment #73
jair CreditAttribution: jair commentedComment #74
star-szrmaintenance-page.html.twig is now HTML5: #2011578: Markup for Stark's page.html.twig + maintenance-page.html.twig.
Do we still want to rename maintenance-page to maintenance? That could still happen here, but for that task it would be easier to start anew than reroll the latest patch.
I looked through the patch and there is still some other small cleanup from this patch that could be done (in other issues):
Most of this still seems valid but should be handled in another issue as cleanup:
#2073621: Remove unneeded variables from template_preprocess_maintenance_page()
This is already happening in #2067915: Restore html attributes to maintenance-page.html.twig.
Comment #75
sun#2218039: Render the maintenance/install page like any other HTML page (or a follow-up issue to it) will rename the templates to page-maintenance + page-install, because they are just variants (page-specific suggestions) of page.
Comment #76
mgiffordLooks like we can close this issue now.
Comment #77
star-szrYup, I reviewed the old patches here back when I commented #74 and @sun provided another issue for the template renaming. Let's put this to bed.