Files: 
CommentFileSizeAuthor
#62 maintenance_template_1189822_62.patch27.22 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch maintenance_template_1189822_62.patch. Unable to apply patch. See the log in the details link for more information. View
#55 maintenance_template_1189822_52.patch26.57 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 34,146 pass(es). View
#53 maintenance_template_1189822_52-D8.patch26.57 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch maintenance_template_1189822_52-D8.patch. Unable to apply patch. See the log in the details link for more information. View
#47 maintenance_template_1189822_47.patch10.96 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#45 maintenance_template_try_because_I_was_forced_by_webchick_v3.diff8.88 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] 34,059 pass(es), 0 fail(s), and 824 exception(es). View
#43 maintenance_template_try_because_I_was_forced_by_webchick_v2.patch8.6 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/maintenance-page.tpl.php. View
#41 maintenance_template_try_because_I_was_forced_by_webchick.patch8.65 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/maintenance-page.tpl.php. View
#32 1189822-maintenance-page-tpl-32.patch6.7 KBrasskull
PASSED: [[SimpleTest]]: [MySQL] 33,839 pass(es). View
#29 1189822-maintenance-page-tpl-29.patch6.52 KBrupl
PASSED: [[SimpleTest]]: [MySQL] 33,801 pass(es). View
#27 maintenance-page.tpl_.php--convert-to-html5-1189822-27.patch4.99 KBchrispomeroy
PASSED: [[SimpleTest]]: [MySQL] 33,800 pass(es). View
#21 maintenance-page.tpl_.php--convert-to-html5-1189822-21.patch9.64 KBchrispomeroy
PASSED: [[SimpleTest]]: [MySQL] 33,752 pass(es). View
#22 maintenance-page.tpl_.php--convert-to-html5-1189822-22.patch4.91 KBchrispomeroy
PASSED: [[SimpleTest]]: [MySQL] 33,754 pass(es). View
#15 maintenance-page.tpl_.php--convert-to-html5-1189822-14.patch5.03 KBrasskull
PASSED: [[SimpleTest]]: [MySQL] 33,759 pass(es). View
#10 maintenance-page.tpl_.php--convert-to-html5-1189822-10.patch4.83 KBchrispomeroy
PASSED: [[SimpleTest]]: [MySQL] 33,755 pass(es). View
#8 maintenance-page.tpl_.php--convert-to-html5-1189822-8.patch4.82 KBchrispomeroy
PASSED: [[SimpleTest]]: [MySQL] 33,761 pass(es). View
#7 maintenance-page.tpl_.php--convert-to-html5-1189822-7.patch4.83 KBchrispomeroy
PASSED: [[SimpleTest]]: [MySQL] 33,774 pass(es). View
#6 maintenance-page.tpl_.php--convert-to-html5-1189822-6.patch4.82 KBchrispomeroy
PASSED: [[SimpleTest]]: [MySQL] 33,761 pass(es). View
#4 maintenance-page.tpl_.php-html5-1189822-4.patch3 KBchrispomeroy
PASSED: [[SimpleTest]]: [MySQL] 33,622 pass(es). View
#1 maintenance-page.tpl_.php-html5-1189822-1.patch2.56 KBchrispomeroy
PASSED: [[SimpleTest]]: [MySQL] 33,453 pass(es). View

Comments

chrispomeroy’s picture

Status: Active » Needs review
FileSize
2.56 KB
PASSED: [[SimpleTest]]: [MySQL] 33,453 pass(es). View

First patch, any thoughts?

Jacine’s picture

Thanks for the patch!

just an FYI that @rupl is working on #1077578: [Followup] Convert bartiks page.tpl.php to HTML5, which is related.

Jeff Burnz’s picture

Status: Needs review » Needs work
+++ 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.

chrispomeroy’s picture

Status: Needs work » Needs review
FileSize
3 KB
PASSED: [[SimpleTest]]: [MySQL] 33,622 pass(es). View
  • switched #sidebar-first and #sidebar-second back to divs
  • removed outer footer wrapper
  • put footer tag inside php if condition
  • removed #main-squeeze wrapper
  • switched close tag comments from /id-name to #id-name like rupl did

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)

Jeff Burnz’s picture

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.

chrispomeroy’s picture

FileSize
4.82 KB
PASSED: [[SimpleTest]]: [MySQL] 33,761 pass(es). View

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?

chrispomeroy’s picture

FileSize
4.83 KB
PASSED: [[SimpleTest]]: [MySQL] 33,774 pass(es). View

edit: ignore this one (following along with the conversation on IRC re: page.tpl.php)
updated patch coming

chrispomeroy’s picture

FileSize
4.82 KB
PASSED: [[SimpleTest]]: [MySQL] 33,761 pass(es). View

changed footer id to footer rather than #contentinfo

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Novice

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.

chrispomeroy’s picture

Status: Needs work » Needs review
FileSize
4.83 KB
PASSED: [[SimpleTest]]: [MySQL] 33,755 pass(es). View

ok updated to fix /core path

rasskull’s picture

Per Jacine's request, I'm marking that I'd like to help review this patch

Jacine’s picture

Issue tags: +sprint

Tagging this for the next sprint.

xjm’s picture

Issue tags: -Novice

Thanks for rerolling. :)

rupl’s picture

Status: Needs review » Needs work

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 $content to 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!!

rasskull’s picture

FileSize
5.03 KB
PASSED: [[SimpleTest]]: [MySQL] 33,759 pass(es). View

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:

  • added an ending div tag to the #logo-title element - I might be losing my mind but it looked like it was missing!
  • adjusted indenting on sub elements inside header to make it uniform
  • Made end element inline comments consistent with page.tpl.php (single space after end element, slash in front of selector)

Please double check that the end closing tag for #logo-title was indeed missing, as it could just be me missing something.

Status: Needs review » Needs work

The last submitted patch, maintenance-page.tpl_.php--convert-to-html5-1189822-14.patch, failed testing.

rasskull’s picture

I know this is nitpicky, but please remove whitespace between closing tags and comments. Thanks and great work!!

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

rasskull’s picture

Status: Needs work » Needs review
rupl’s picture

@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-title closing tag. Don't know how I missed that :) No other validation errors popped up when I ran it just now.

rasskull’s picture

Status: Needs work » Needs review

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

chrispomeroy’s picture

FileSize
9.64 KB
PASSED: [[SimpleTest]]: [MySQL] 33,752 pass(es). View

-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

chrispomeroy’s picture

FileSize
4.91 KB
PASSED: [[SimpleTest]]: [MySQL] 33,754 pass(es). View

ok this is the one - last one had other junk in it

rupl’s picture

Great work, Chris!

Regarding the $html_attributes notice: 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_attributes in this preprocess, and then we can print more than <html> while avoiding the PHP notices. Even just lang would suffice.

Gábor Hojtsy’s picture

Issue tags: +D8MI

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.

Gábor Hojtsy’s picture

Status: Needs review » Needs work
Jacine’s picture

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.

chrispomeroy’s picture

Status: Needs work » Needs review
FileSize
4.99 KB
PASSED: [[SimpleTest]]: [MySQL] 33,800 pass(es). View

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.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

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.

rupl’s picture

Status: Needs work » Needs review
FileSize
6.52 KB
PASSED: [[SimpleTest]]: [MySQL] 33,801 pass(es). View

I've taken the relevant bits from the preprocess and process functions and copied them to the maintenance versions of each function. So $html_attributes and $body_attributes are now available to be altered in maintenance mode. There's nothing in $body_attributes for now, but I did test it successfully before submitting patch.

Gábor Hojtsy’s picture

Looks good to me for language support! Thanks!

rasskull’s picture

Status: Needs review » Reviewed & tested by the community

This looks really solid to me, I looked through it for awhile and didn't catch anything!

rasskull’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
6.7 KB
PASSED: [[SimpleTest]]: [MySQL] 33,839 pass(es). View

To make this more consistent with page.tpl.php I did the following in this patch:

  • the #logo-title div was removed
  • moved the messages group outside of #main
  • added title prefix and suffix (not 100% sure these should be in here or not)
  • put the 'column' class on #content
  • moved sidebar first group under #main as it is on page.tpl.php

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

aspilicious’s picture

Status: Needs review » Needs work

Ask jacine what she wants, we can't copy the html attributes all around.
She has a plan for this...

Jacine’s picture

Assigned: Unassigned » Jacine

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

Everett Zufelt’s picture

From #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?

Jacine’s picture

The patch here will follow page.tpl.php, and role="main" is already in there. :)

Jacine’s picture

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:

  1. $main_menu
  2. $secondary_menu
  3. $tabs
  4. $action_links
  5. $node
  6. $breadcrumb

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:

  1. Installation pages (sidebar_first_content)
  2. Update page (sidebar_first, content)
  3. Maintenance page (content)

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?

Gábor Hojtsy’s picture

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.

rupl’s picture

Assigned: Jacine » rupl

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

Jacine’s picture

@rupl ♥

aspilicious’s picture

Status: Needs work » Needs review
FileSize
8.65 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/maintenance-page.tpl.php. View

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.

Status: Needs review » Needs work

The last submitted patch, maintenance_template_try_because_I_was_forced_by_webchick.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
8.6 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/maintenance-page.tpl.php. View

without whitespace issues...

Status: Needs review » Needs work
aspilicious’s picture

Status: Needs work » Needs review
FileSize
8.88 KB
FAILED: [[SimpleTest]]: [MySQL] 34,059 pass(es), 0 fail(s), and 824 exception(es). View

/me shouldn't write patches this late

Status: Needs review » Needs work
aspilicious’s picture

Status: Needs work » Needs review
FileSize
10.96 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Eat this testbot

Status: Needs review » Needs work

The last submitted patch, maintenance_template_1189822_47.patch, failed testing.

rupl’s picture

+++ 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_page hook 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_attributes in 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).
aspilicious’s picture

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?

Jacine’s picture

These two sections should retain the maintenance_page hook because we're only changing tpl suggestions, not the hook itself.

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.

rupl’s picture

@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) {
aspilicious’s picture

FileSize
26.57 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch maintenance_template_1189822_52-D8.patch. Unable to apply patch. See the log in the details link for more information. View

Not if you hazz a patch.

Untested so it could blow up site, just adding now because rupl posted the comment.

Jacine’s picture

Status: Needs work » Needs review

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.

aspilicious’s picture

FileSize
26.57 KB
PASSED: [[SimpleTest]]: [MySQL] 34,146 pass(es). View

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

aspilicious’s picture

Testresults ar GREEN :D, what are the next steps?

Jacine’s picture

Issue tags: +Needs manual testing

@aspilicous Great! See the BUT in #54.

aspilicious’s picture

Upgrading does not work. You get a white screen visiting core/update.php

Jacine’s picture

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

Jacine’s picture

Status: Needs review » Needs work
Issue tags: -Needs manual testing

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.

Gábor Hojtsy’s picture

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

aspilicious’s picture

Status: Needs work » Needs review
FileSize
27.22 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch maintenance_template_1189822_62.patch. Unable to apply patch. See the log in the details link for more information. View

Here you go.

Upgrade works, someone needs to retest this manually :)

Gábor Hojtsy’s picture

Looks good in terms of language. :)

rupl’s picture

Status: Needs review » Postponed

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

Gábor Hojtsy’s picture

Issue tags: -sprint

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

Jacine’s picture

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

Jacine’s picture

Status: Postponed » Needs review

Let's not hold this up on the hook name change anymore. It needs to get done.

RobLoach’s picture

Issue tags: -html5, -D8MI

Status: Needs review » Needs work
Issue tags: +html5, +D8MI

The last submitted patch, maintenance_template_1189822_62.patch, failed testing.

Gábor Hojtsy’s picture

Anybody want to pick this up again?

Cottser’s picture

Title: Convert maintenance-page.tpl.php to HTML5 » Convert maintenance-page.html.twig to HTML5
Assigned: rupl » Unassigned
Issue tags: +Twig

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

Cottser’s picture

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

jair’s picture

Issue tags: +Needs reroll
Cottser’s picture

Issue tags: -Needs reroll

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

  1. +++ b/core/includes/theme.inc
    @@ -2610,20 +2605,13 @@
    -  $variables['base_path']         = base_path();
       $variables['front_page']        = url();
    -  $variables['breadcrumb']        = '';
    -  $variables['feed_icons']        = '';
       $variables['help']              = '';
       $variables['language']          = $language;
    -  $variables['language']->dir     = $language->direction ? 'rtl' : 'ltr';
       $variables['logo']              = theme_get_setting('logo');
       $variables['messages']          = $variables['show_messages'] ? theme('status_messages') : '';
    -  $variables['main_menu']         = array();
    -  $variables['secondary_menu']    = array();
       $variables['site_name']         = (theme_get_setting('toggle_name') ? variable_get('site_name', 'Drupal') : '');
       $variables['site_slogan']       = (theme_get_setting('toggle_slogan') ? variable_get('site_slogan', '') : '');
    -  $variables['tabs']              = '';
    

    Most of this still seems valid but should be handled in another issue as cleanup:
    #2073621: Remove unneeded variables from template_preprocess_maintenance_page()

  2. +++ b/core/includes/theme.inc
    @@ -2641,20 +2629,32 @@
    +  // HTML element attributes.
    +  $variables['html_attributes_array']['lang'] = $language->language;
    +  $variables['html_attributes_array']['dir'] = $language->direction ? 'rtl' : 'ltr';
    +
    

    This is already happening in #2067915: Restore html attributes to maintenance-page.html.twig.

sun’s picture

Issue summary: View changes

Do we still want to rename maintenance-page to maintenance?

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

mgifford’s picture

Looks like we can close this issue now.

Cottser’s picture

Status: Needs work » Closed (works as designed)

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