Problem/Motivation

Followup from #2218039: Render the maintenance/install page like any other HTML page.

Bring file name patterns in line with core standards.

Address theme suggestions as well, for example, page__maintenance.

Proposed resolution

Use core's template suggestions and rename templates and related preprocess functions

User interface changes

None.

API changes

removal of theme keys maintenance_page & install_page

Template name changes.
maintenance_page => page--maintenance
install_page => page--maintenance--install
maintenance_page__offline => page__maintenance__offline

preprocess functions moved to system module
template_preprocess_maintenance_page() => system_preprocess_page__maintenance()
template_preprocess_install_page => system_preprocess_page__maintenance__install()

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because follow-up and just renames templates and increase TX
Issue priority Normal because nothing is broken
Unfrozen changes Unfrozen because it only changes template names and related preprocess functions
Disruption Disruptive for custom modules/themes because it will require a rename templates and preprocess functions if any

Original report by @jessebeach

Comments

jessebeach’s picture

Issue summary: View changes
cs_shadow’s picture

FileSize
1.46 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,222 pass(es), 491 fail(s), and 152 exception(s). View

Renamed the file and replaced occurrences of 'maintenance-page.html.twig' to 'page--maintenance.html.twig' in comments. Attaching a patch for the same.

cs_shadow’s picture

Status: Active » Needs review
sun’s picture

Status: Needs review » Postponed

Thanks! #2218039: Render the maintenance/install page like any other HTML page should land first though. Hopefully this patch will still apply then.

Status: Postponed » Needs work

The last submitted patch, 2: core-2231853-1.patch, failed testing.

sun’s picture

Title: Rename maintenance-page.html.twig to page--maintenance.html.twig » Rename maintenance-page template into page--maintenace + install-page into page--install
Component: install system » theme system
Status: Needs work » Needs review

We additionally need to change the template suggestions, so that the renamed page templates are actually picked up.

Let's merge the patch from #2231857: Rename install-page.html.twig to page--install.html.twig into this here + then figure out how to get the template suggestions to work.

sun’s picture

Or on a second thought, perhaps we don't actually need to futz with template suggestions...

It might be sufficient to just simply change the #theme property in Drupal\Core\Page\DefaultHtmlPageRenderer::renderPage():

-      // @todo Change into theme suggestions "page__$theme".
-      '#theme' => $theme . '_page',
+      '#theme' => "page__$theme",

Can you try that? :-)

cs_shadow’s picture

The tests fail because it expects the file named 'maintenance-page template' to be present. Any suggestions on how to solve this instead of manually changing all the tests?

sun’s picture

@cs_shadow: That is caused by the missing change I mentioned in #7.


Hm. Potential problem:

I wonder whether page--X could conflict with the regular page template suggestions?

For example, if you have a path that is /install, then the page template for that path would be page--install. (same for /maintenance)

Would it be acceptable if we'd use single dashes for these special unicorns? Or shall we ignore that potential problem?


On a second after-thought, a single dash ^^ does not work, because the page template theme suggestions only work with two dashes...

What we want to achieve is that the regular page.html.twig template is used, in case no page--install.html.twig template exists.

→ We actually have to obey to the pattern for theme suggestions, because otherwise, the fallback to 'page' would not work.


Lastly, speaking of fallbacks: The install page is actually a more specific incarnation of the maintenance page.

→ page__maintenance__install

...which causes theme template suggestions to be applied in this order:

→ page--maintenance--install.html.twig
→ page--maintenance.html.twig
→ page.html.twig

sun’s picture

After some back and forth, the parent issue has landed (again): #2218039: Render the maintenance/install page like any other HTML page

Let's move forward here! Any ideas/feedback on #9, anyone?

sun’s picture

Assigned: Unassigned » sun
FileSize
5.52 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,048 pass(es), 1 fail(s), and 0 exception(s). View

So attached patch performs the main changes...

  1. Renamed templates.
  2. Renamed preprocess functions, removed theme hooks.

...but doing so causes us to inherently run into the problem space of #939462: Specific preprocess functions for theme hook suggestions are not invoked

:-/

Status: Needs review » Needs work

The last submitted patch, 11: template.maintenance.11.patch, failed testing.

sun’s picture

I just stumbled over #2231505: Convert theme_field__node__title() to Twig, which implements a custom/cumbersome workaround in hook_theme() that could seemingly work here, too:

  'field__node__title' => array(
    'base hook' => 'field',
    'template' => 'field--node--title',
  ),
Cottser’s picture

Title: Rename maintenance-page template into page--maintenace + install-page into page--install » Rename maintenance-page template into page--maintenance + install-page into page--maintenance--install

That issue didn't implement it (just added the 'template' line) but yes that is totally valid as a workaround for #939462: Specific preprocess functions for theme hook suggestions are not invoked :)

The /maintenance template suggestion clash is still a bit concerning to me as a "gotcha" here.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,532 pass(es), 314 fail(s), and 363,175 exception(s). View
7.26 KB

Is this an acceptable interim step to #939462: Specific preprocess functions for theme hook suggestions are not invoked that's sufficient to unblock this issue?

Status: Needs review » Needs work

The last submitted patch, 15: template.maintenance.15.patch, failed testing.

effulgentsia’s picture

Status: Needs work » Needs review
FileSize
13.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,611 pass(es). View
1.23 KB

Status: Needs review » Needs work

The last submitted patch, 17: template.maintenance.17.patch, failed testing.

effulgentsia’s picture

17: template.maintenance.17.patch queued for re-testing.

effulgentsia’s picture

Status: Needs work » Needs review

Green, so back to needs review.

Just to clarify, in HEAD, *_preprocess_maintenance_page() ran instead of *_preprocess_page(), because it was a fully separate theme hook. With this patch, *_preprocess_page__maintenance() runs in addition to *_preprocess_page(). Is there any code within the preprocess functions that we want to adjust as a result of that?

sun’s picture

Based on my studies, the install + maintenance preprocess functions should work as-is (and actually better + more accurately) with that inheritance.

However, I think I have to object to the proposed stop-gap change. I'd be inclined to agree if it wasn't a major API change. Changing that parameter to be a $context looks clunky and is in no way guaranteed to be the final API. $context is hard enough to figure out for PHP/module developers already; I do not want to introduce a $context parameter in the theme space. Therefore, I'd like to push back on that; let's resolve the preprocess issue first.

With regard to temporary stop-gap workarounds, only the hack referenced in #13 (which apparently exists in HEAD already) would work for me.

effulgentsia’s picture

Changing that parameter to be a $context looks clunky and is in no way guaranteed to be the final API.

Let's finalize it here then (or a spin-off, but not one that needs to wait for all of #939462: Specific preprocess functions for theme hook suggestions are not invoked). Regardless of that issue's scope to run suggestion-specific preprocessors automatically, I think we'll still want a way to inform the base preprocess functions of what suggestion(s) are also involved, in the same way we currently inform the preprocessors of $hook. Similar to how in OOP, a base class can inspect get_class($this) if it really needs to. I thought $context would be preferable to $hook, $info, $suggestions, $suggestion all being individual parameters. Do you have a better suggestion (heh, no pun intended)?

sun’s picture

Let's finalize it here then […] Regardless of that issue's scope to run suggestion-specific preprocessors automatically, I think we'll still want a way to inform the base preprocess functions of what suggestion(s) are also involved…

How does that not duplicate the whole point of that issue to 100%?

We'd only repeat the whole discussion from scratch. And, by only looking at this isolated deeper aspect, I'm highly concerned that we'd only duct-tape without having a good understanding of whether the resulting change doesn't make the overall situation worse instead of better. The proposed clean-up of this issue here doesn't provide sufficient justification for such a risk.

What I could agree with would be an effort to restart that issue from a clean slate, because yes, that issue is very lengthy, complex, and cumbersome; it requires multiple hours to fully process (+ validate) all input and data in all comments, so as to fully understand all involved aspects and most importantly, expectations of themers (as well as module developers). I vaguely remember very well that just doing that analysis took me half a day back in 2012/2013 when I last studied it (which was before the extremely debatable hook_theme_suggestions() + _alter() was introduced; FWIW, a total performance drain, as some of the theme registry logic has been moved into the runtime layer and so that's re-executed from scratch for every. single. call. to. theme.).

In short, the theme system "very badly needs us." I guess it's that crew again. ;-)

Jalandhar’s picture

Issue tags: +Needs reroll

Patch #17, needs to be updated.

Jalandhar’s picture

Status: Needs review » Needs work
andypost’s picture

Issue tags: +frontend, +Twig
Related issues: +#2372581: Rename 'page' render element to 'body', page.html.twig to body.html.twig
pguillard’s picture

Status: Needs work » Needs review
FileSize
13.16 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 103,225 pass(es), 2 fail(s), and 6 exception(s). View

Just tried reroll #17 the best I can...

Status: Needs review » Needs work

The last submitted patch, 27: template.maintenance.27.patch, failed testing.

madhavvyas’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll

Patch re rolled for comment 27

madhavvyas’s picture

FileSize
26.88 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 68,440 pass(es), 21,774 fail(s), and 77,099 exception(s). View

Patch re rolled for comment 27

Status: Needs review » Needs work

The last submitted patch, 30: template-maintence-2231853-29.patch, failed testing.

Antti J. Salminen’s picture

Assigned: sun » Unassigned
Status: Needs work » Needs review
FileSize
23.25 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 111,814 pass(es). View
16.3 KB

Worked a bit on this, starting with pguillard's reroll from #27. The preprocess issue should be fixed now so we only need to declare the implementations in hook_theme. Still needs work (the suggestion hook for offline and possibly changes for the preprocess functions) but marking needs review to see how functional it is.

Status: Needs review » Needs work

The last submitted patch, 32: template-maintenance-2231853-32.patch, failed testing.

Antti J. Salminen’s picture

Status: Needs work » Needs review

Looks like the patch still applies and the tests pass now that #2565259: Some route serializations in update test database dumps are broken is in.

Antti J. Salminen’s picture

andypost’s picture

FileSize
3.79 KB
12.12 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 112,563 pass(es), 1 fail(s), and 0 exception(s). View

rename classy template and clean-ups

  1. +++ b/core/includes/theme.inc
    @@ -1669,6 +1666,12 @@ function drupal_common_theme() {
    +    'page__maintenance' => array(
    +      'base hook' => 'page',
    +    ),
    +    'page__maintenance__install' => array(
    +      'base hook' => 'page',
    +    ),
    

    is this still needed? suppose not after #939462: Specific preprocess functions for theme hook suggestions are not invoked

  2. +++ b/core/lib/Drupal/Core/Render/BareHtmlPageRendererInterface.php
    @@ -38,9 +38,9 @@
    + * - Install (hook_preprocess__page__maintenance__install(), page--maintenance--install.html.twig).
    
    +++ b/core/modules/system/templates/page--maintenance--install.html.twig
    @@ -0,0 +1,55 @@
    + * @see template_preprocess_page__maintenance__install()
    

    suppose hook_preprocess_page__m...

Status: Needs review » Needs work

The last submitted patch, 37: 2231853-template-maintenance-37.patch, failed testing.

The last submitted patch, 37: 2231853-template-maintenance-37.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
10.87 KB
21.96 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,206 pass(es). View

fix css, and move template preprocess functions to system
Also merged system_theme_suggestions_page() and system_theme_suggestions_maintenance_page()

EDIT

+++ b/core/includes/theme.inc
@@ -1390,49 +1390,6 @@ function theme_get_suggestions($args, $base, $delimiter = '__') {
-function template_preprocess_page__maintenance(&$variables) {
...
-function template_preprocess_page__maintenance__install(&$variables) {

+++ b/core/modules/system/system.module
@@ -253,39 +253,71 @@ function system_theme_suggestions_html(array $variables) {
+function system_preprocess_page__maintenance(&$variables) {
...
+function system_preprocess_page__maintenance__install(&$variables) {

without theme-key this is not invoked so moved to preprocess

lauriii’s picture

Issue tags: +Needs beta evaluation
andypost’s picture

Issue summary: View changes
Issue tags: -Needs beta evaluation

Updated IS and added beta eval

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

I tested manually update.php, maintenance mode and installer and everything seems to be fine with those. I discussed with @andypost and @davidhernandez that renaming CSS files even though the CSS files have changed is not necessary because the current names makes more sense than anything else we could quickly figure out.

davidhernandez’s picture

Issue tags: +Needs change record

I think changing somewhat major template names could do with a change record.

andypost’s picture

lauriii’s picture

Status: Reviewed & tested by the community » Needs work

CR looks good.

+++ b/core/modules/system/system.module
@@ -253,39 +253,71 @@ function system_theme_suggestions_html(array $variables) {
+function system_preprocess_page__maintenance(&$variables) {
...
+function system_preprocess_page__maintenance__install(&$variables) {

Sorry I didn't see this before but these should be still prefixed with template_. Also the @see is pointing for template prefixed version :)

andypost’s picture

To make them work as template_ we need to keep theme keys
not sure it makes sense and this preprocess adds a system library

lauriii’s picture

I'm sorry but what do you mean by theme keys?

andypost’s picture

+++ b/core/includes/theme.inc
@@ -1656,12 +1656,6 @@ function drupal_common_theme() {
-    'page__maintenance' => array(
-      'base hook' => 'page',
-    ),
-    'page__maintenance__install' => array(
-      'base hook' => 'page',
-    ),

this ones that are removed in #37

lauriii’s picture

I filed #2567693: Specific preprocess functions not picked up for template_ prefix for template_ prefixed preprocess functions not being picked up. We can use the hook_preprocess_* functions but we should add @todo's to fix them after the bug has been fixed. Also the documentation should be pointing on the right preprocess functions.

davidhernandez’s picture

@lauriii

Also the documentation should be pointing on the right preprocess functions.

Which lines of documentation are you referring to?

lauriii’s picture

I thought I saw some wrong preprocess functions being mentioned in the docblocks but couldn't find that at least now

Status: Needs work » Needs review
pguillard’s picture

FileSize
21.82 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,932 pass(es), 9 fail(s), and 9 exception(s). View

#40 rerolled, as it was not applying anymore

Status: Needs review » Needs work

The last submitted patch, 54: 2231853-template-maintenance-54.patch, failed testing.

The last submitted patch, 54: 2231853-template-maintenance-54.patch, failed testing.

Status: Needs work » Needs review
andypost’s picture

looks that should go to 8.1

joelpittet’s picture

Version: 8.0.x-dev » 8.1.x-dev

agreed

hass’s picture

Isn't this not a BC break that need to wait until D9? Themes that implemented it in 8.0 will break in 8.1.

joelpittet’s picture

If we can't put in a BC layer then yes, D9. Would like to give the opportunity if it's feasible.

andypost’s picture

any idea about BC shim here?
we can't change stable theme anymore til 9.x...

so that needs general idea how to move forward

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Status: Needs review » Needs work

The last submitted patch, 54: 2231853-template-maintenance-54.patch, failed testing.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.