During the course of #1189822: Convert maintenance-page.html.twig to HTML5, we decided we wanted to convert hook_maintenance_page() to hook_maintenance(). The reason is because maintenance and installation modes cannot run the entire codebase and lack the ability to use the full suite of theme templates, making 'maintenance_page' a misnomer that can lead to confusion, since it serves the role of both html.tpl.php and page.tpl.php.

Jacine said:

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

rupl adds:

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

Gábor Hojtsy adds:

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 posted a patch that properly upgrades. We can continue this issue by using that patch and simply renaming maintenance-page.tpl.php to maintanence.tpl.php instead of introducing markup changes.

CommentFileSizeAuthor
#18 1360994-maintenace-18.patch26.78 KBeffulgentsia
#12 1360994-maintenace-12.patch26.08 KBaspilicious
#10 1360994-maintenace-10.patch26.09 KBaspilicious
#4 1360994-maintenace-4.patch27.8 KBaspilicious
#1 1360994-maintenace.patch27.8 KBaspilicious
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Active » Needs review
FileSize
1360994-maintenace.patch27.8 KB

Lets try this

Jacine’s picture

Jacine
she/her
Location New York
CreditAttribution: Jacine commented
Issue tags: +html5, +sprint

Tagging so we can track this.

rupl’s picture

rupl
he/him
Primary language English
CreditAttribution: rupl commented
Status: Needs review » Reviewed & tested by the community

This patch is solid. Installed perfectly, all three themes render maintenance page properly. No warnings or notices anywhere. Markup changes will be submitted in other issue.

Since we did much of the debugging for this patch in #1189822: Convert maintenance-page.html.twig to HTML5, I'm marking this deceptively short issue RTBC.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
FileSize
1360994-maintenace-4.patch27.8 KB

There is space after "rebuilt" in the docs that didn't belong there. I removed it. Nothing functional changed.

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented

I'm in favor of this patch.

Of course, I'm in favor of re-merging html.tpl.php and page.tpl.php, so I'm a bit of a radical. ;-)

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs review

I'm not sure if we're not overloading the meaning of page here.

theme_maintenance_page() to me means "the maintenance page" which is what we're theming, it does not necessarily imply anything about html.php vs. page.tpl.php

theme_maintenance() doesn't tell me anything about what I'm going to be theming - it could be a maintenance message rather than a page.

Happy to be convinced otherwise but not sure there's enough justification for the re-naming here yet.

Jacine’s picture

Jacine
she/her
Location New York
CreditAttribution: Jacine commented

@catch, IMO the issue is that using the term "page" and mirroring all the crap in the page hook preprocess/process functions is completely misleading, not to mention a waste of code. It leads one to believe that the function of this page is related to page.tpl.php. It's not and it can never be. Most of the regions don't and won't ever have anything in them, and none of the menu related variables will ever work there. Maybe that made some sort of sense in D6, although I'd argue it unnecessarily confused matters.

The point is that this template needs to stop masquerading as something it's not. I tried in #1189822-37: Convert maintenance-page.html.twig to HTML5, which resulted in 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:

  • Installation pages (sidebar_first, content)
  • Maintenance page (content)
  • Update page (sidebar_first, 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.

I was totally confused going through the process of trying to figure out what will work and what wont (a) because it wasn't documented and (b) because the term "page" is loaded and implies a relation to page.tpl.php. So, that's why I think this change makes sense.

As a side note, I also agree with @JohnAlbin with the fact that splitting html.tpl.php and page.tpl.php was a bad move.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Issue tags: -html5, -sprint

#4: 1360994-maintenace-4.patch queued for re-testing.

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

The last submitted patch, 1360994-maintenace-4.patch, failed testing.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs work » Needs review
FileSize
1360994-maintenace-10.patch26.09 KB

rerolled

Status: Needs review » Needs work

The last submitted patch, 1360994-maintenace-10.patch, failed testing.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs work » Needs review
FileSize
1360994-maintenace-12.patch26.08 KB

There is a new cache api! And it's easier to use!

rupl’s picture

rupl
he/him
Primary language English
CreditAttribution: rupl commented

From #6:

theme_maintenance_page() to me means "the maintenance page" which is what we're theming, it does not necessarily imply anything about html.php vs. page.tpl.php

To echo #7, I disagree, and most of my years spent in Drupal have been in theme layer. As a themer, when I see _page at the end of a theme function, it implies a direct relation to a page.tpl.php and its preprocess function. This will certainly apply to newer devs, desperate to establish patterns while they go spelunking into Drupal's theme API.

This very confusion is the reason we created this issue. When I started work on #1189822: Convert maintenance-page.html.twig to HTML5, it took some digging to find the right functions to alter, and we could not directly port the default page.tpl.php because only a small subset of $variables were available in maintenance mode.

Would hook_maintenance_template() be a more palatable choice, albeit redundant? I'm like hook_maintenance(), but I wanted to throw another suggestion out there. I think this is a small convention we could change that would indicate the deviation from the regular theming system.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

Damnit just tried an D7 upgrade. Afterwards I get a Bartik maintenance theme (when setting in "maintenance mode" and not the black and whitish theme. Is this the correct behaviour?

Jacine’s picture

Jacine
she/her
Location New York
CreditAttribution: Jacine commented
Status: Needs review » Needs work

The upgrade should use the Seven theme. :/ What happened?

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

I was also confused as per catch's #6, but Jacine nicely summarizes in #7 why equating this one-off weird template to page.tpl.php is a WTF.

What is *probably* happening here is the theme registry still contains reference to the old function/template name, so the theme registry needs to be rebuilt. The update_prepare_d8_bootstrap() is where we should do any "pre-update" tasks, so it's possible it just needs a call to theme_registry_rebuild() or whatever it is.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

If the upgrade path hasn't been fixed by next week (Feb 28), I will give it a shot.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented
Status: Needs work » Needs review
FileSize
1360994-maintenace-18.patch26.78 KB

Just a reroll. CNR for a bot. Looking into upgrade path...

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented
Status: Needs review » Needs work

Re #14: it's not just on upgrade, it's on fresh install too. The patch removes themes/bartik/css/maintenance-page.css and doesn't replace it with a maintenance.css file, even though there is a drupal_add_css() call to the non-existent maintenance.css file. Can someone who knows what the contents of maintenance.css should be add it to the patch?

Re #15: The update works fine and is in the Seven theme. #14 is just about what happens after an upgraded site is set to maintenance mode.

Re #16: From what I can tell, the upgrade path works correctly, just an issue of a missing CSS file.

Re #6:

theme_maintenance() doesn't tell me anything about what I'm going to be theming - it could be a maintenance message rather than a page.

I agree. I'd prefer a better name. But I can understand the desire to remove "_page" as the suffix, and I don't have an alternate suggestion for a new suffix at this time. Ultimately though, I'm happy with whatever front-end developers think makes sense, since they're the ones who need to work with it, so given the number of +1s here, I think we should proceed, and open a follow-up if anyone has ideas for a slightly less vague name.

However, we also currently have 'install_page' and 'update_page'. How would you all feel about removing these entirely (they seem like useless wrappers around 'maintenance'), or else convert them to 'maintenance__install' and 'maintenance__update' (double underscore intentional, since these seem to be nothing more than what we normally do with theme hook suggestions). I'm ok with making that a follow-up rather than holding up this issue.

Jacine’s picture

Jacine
she/her
Location New York
CreditAttribution: Jacine commented

However, we also currently have 'install_page' and 'update_page'. How would you all feel about removing these entirely (they seem like useless wrappers around 'maintenance'), or else convert them to 'maintenance__install' and 'maintenance__update' (double underscore intentional, since these seem to be nothing more than what we normally do with theme hook suggestions). I'm ok with making that a follow-up rather than holding up this issue

The previous patches in this issue included theme hook suggestions. Not sure what's going on the CSS files for the themes, but Bartik's CSS was in the first patch.

I agree. I'd prefer a better name. But I can understand the desire to remove "_page" as the suffix, and I don't have an alternate suggestion for a new suffix at this time. Ultimately though, I'm happy with whatever front-end developers think makes sense, since they're the ones who need to work with it, so given the number of +1s here, I think we should proceed, and open a follow-up if anyone has ideas for a slightly less vague name.

TBH, I'm starting to realize that this is just a waste of time. If everyone minus themers disagrees with this, then let's just forget about it. Sure, it would be great if we could actually use page__maintenance.tpl.php. That's what I wanted to do originally, and I spent hours writing the patch to do it, but ultimately failed miserably, because the preprocess functions for page contain too many things that depend on a database and would turn the code into a worse mess than it already is.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented

If it were possible to change to page__maintenance, how would you feel about making it actually parallel page, and also have a html__maintenance to parallel html? I know you don't like the page/html split, but so long as we have it, do you think it would be an improvement to have maintenance follow a consistent pattern, and discuss the pros/cons of recombining page and html (for both regular and maintenance) in a separate issue?

Jacine’s picture

Jacine
she/her
Location New York
CreditAttribution: Jacine commented

I don't see the point of implementing a html__maintenance template. If the html + page separation is actually supposed to be useful, and all we need to change markup-wise is the page template's contents then what's the point of using html__maintenance? Sounds like additional maintenance, confusion and overhead to me.

But if you are just talking about the ability to do so (defining the theme hook suggestion), yes, this is what I tried initiative in #1189822: Convert maintenance-page.html.twig to HTML5. It destroyed the page preprocess function and make it 200% more complicated, which is just not worth it. It also has the potential to get screwed in contrib when themes/modules make menu-related variables in their own preprocess functions.

All of this and the fact that the use cases for the maintenance pages do not mirror the page template for any good reason is why I think this should be a separate hook without any association to page, but no one seems to agree with that other than themers. While I appreciate the help, I'm not exactly interested in arguing about it or pushing it through. The world will not end if we leave this WTF be, and I've already wasted a good 12 hours already between trying to patch the issue in the previous issue and constantly defending my reasoning in comments, so I'm ready to accept defeat here and move on. :)

JohnAlbin’s picture

JohnAlbin
he/they/他
Primary language English
Location Taipei, Taiwan
CreditAttribution: JohnAlbin commented

Using page__maintenance and html__maintenance is a no go because of how ridiculously complicated it would make the preprocess functions. Some stuff needs to be done when the database is available and some doesn't.

theme_maintenance_page() to me means "the maintenance page" which is what we're theming, it does not necessarily imply anything about html.php vs. page.tpl.php

That’s because you're not a themer. The "page" part of the "maintenance_page" name jumps out at a themer and makes a themer think of the first template they ever modify, page.tpl.php.

The problem comes when you think that maintenance_page.tpl.php will follow the same structure as page.tpl.php. It doesn't. It's an amalgamation of html.tpl.php and page.tpl.php.

Leaving the theme hook as maintenance_page will continue this confusion.

theme_maintenance() doesn't tell me anything about what I'm going to be theming - it could be a maintenance message rather than a page.

Ok. This is a fair point. What if, instead of renaming to theme_maintenance, we lean into the expectations of the themer. Let's split the maintenance_page.tpl.php into 2 parts by adding a maintenance_html.tpl.php.

Jacine’s picture

Jacine
she/her
Location New York
CreditAttribution: Jacine commented

Let's split the maintenance_page.tpl.php into 2 parts by adding a maintenance_html.tpl.php.

This still leaves us back at square one (same confusion), but also with another useless version of html.tpl.php to maintain. :/

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented

Not to get into name bikeshedding, but "theme_maintenance_mode" might clear up confusion. I understand that this is used for reasons other than maintenance mode (e.g. the installer/upgrader) but I think the name implies "more limited." Maybe.

I'm also not a themer, and am happy to be told to go screw myself. :D

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented
Assigned: Unassigned » effulgentsia

I started work on an attempt to make maintenance-page.tpl.php more consistent with page.tpl.php and capable of reusing the same html.tpl.php used for non-maintenance pages. Nothing to share yet, but will post here when I have something.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented
Assigned: effulgentsia » Unassigned
Status: Needs work » Closed (works as designed)

As per #20 and later, closing this as "by design", and posted what I hope makes sense for themers in #1472542: Make theme('maintenance_page') consistent with theme('page').