Rename maintenance-page template into page--maintenance + install-page into page--maintenance--install

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

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

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.

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

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?

@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

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?

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

:-/

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',
  ),

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.

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?

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

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?

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.

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

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

Patch #17, needs to be updated.

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

Patch re rolled for comment 27

Patch re rolled for comment 27

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.

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.

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

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

Updated IS and added beta eval

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.

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

CR https://www.drupal.org/node/2567567

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

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

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

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

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.

@lauriii

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

Which lines of documentation are you referring to?

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

#40 rerolled, as it was not applying anymore

looks that should go to 8.1

agreed

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.

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

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

As for BC
- keep old theme function & preprocess
- maybe use theme suggestion to provide ordered list with new theme key before old implementation
- keep fallback for old template ( cos some distros use custom maintenance page

Here is a re-rolled patch. (Worked on the reroll only)

Now it needs changes after https://www.drupal.org/node/2960810

@pguillard - Thank you for the reroll. It is adviced that once you reroll the patch you remove the "Needs reroll" tag.

Here's a re-roll + fix of test (checksum) and claro parts

EDIT other tests fails because no title (H1 tag) displayed)

Looks that 10.x material

Just a reroll. There was nothing complicated.

Yes, I didn't run the checks.

fix css for claro, but language selection still has label hidden (that's why InstallerTest fails)

fix CS

Looks Claro needs more fixes and not clear about default config test