Coming from #1189822: Convert maintenance-page.html.twig to HTML5.

Problem/Motivation

template_preprocess_maintenance_page() initializes unneeded and unused variables. Before, maintenance-page.html.twig was meant to be consistent with both page.html.twig and html.html.twig but maintenance-page is a completely separate template and more similar to html.html.twig, lacking a large number of features/variables available in page.html.twig.

Discussion from #1189822: Convert maintenance-page.html.twig to HTML5:

@Jacine in #37:

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.

@rupl in #39:

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.

Proposed resolution

Remove the initialization/setting of unused variables from template_preprocess_maintenance_page(). Starting point:

+++ 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']              = '';

Remaining tasks

Create patch.

User interface changes

n/a

API changes

Some blank variables will no longer be available in maintenance-page templates.

#1189822: Convert maintenance-page.html.twig to HTML5
#2067915: Restore html attributes to maintenance-page.html.twig
#2011578: Markup for Stark's page.html.twig + maintenance-page.html.twig

Files: 
CommentFileSizeAuthor
#19 remove-unused-variables-maintenance-1189822-19.patch3.5 KBm1n0
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,037 pass(es). View
#17 remove-unused-variables-maintenance-1189822-16.patch3.59 KBm1n0
PASSED: [[SimpleTest]]: [MySQL] 63,684 pass(es). View
#13 remove-unused-variables-maintenance-1189822-13.patch1.36 KBCyberschorsch
PASSED: [[SimpleTest]]: [MySQL] 58,849 pass(es). View
#11 remove-unused-variables-maintenance-1189822-11.patch1.36 KBCyberschorsch
PASSED: [[SimpleTest]]: [MySQL] 58,499 pass(es). View
#8 remove-unused-variables-maintenance-1189822-8.patch1.46 KBCyberschorsch
PASSED: [[SimpleTest]]: [MySQL] 58,843 pass(es). View
#5 remove-unused-variables-maintenance-1189822.patch1.35 KBCyberschorsch
PASSED: [[SimpleTest]]: [MySQL] 58,639 pass(es). View

Comments

Cottser’s picture

Issue tags: +Novice, +theme system cleanup

Tags.

sunnydeveloper’s picture

Assigned: Unassigned » sunnydeveloper

I'm going to take this one as part of a creating a resource to teach 'contribution to an open project', specifically Drupal (so we may be about a week if that's OK)

joelpittet’s picture

Assigned: sunnydeveloper » Unassigned
Issue tags: +prague2013

Unassigning, @sunnydeveloper you are welcome to grab this again, just freeing it up for the sprint tomorrow.

Cyberschorsch’s picture

Assigned: Unassigned » Cyberschorsch

Taking it as part of sprint at DC Prague

Cyberschorsch’s picture

Status: Active » Needs review
FileSize
1.35 KB
PASSED: [[SimpleTest]]: [MySQL] 58,639 pass(es). View

I removed unused variables as suggested.

joelpittet’s picture

Assigned: Cyberschorsch » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice, -prague2013

Thank you @Cyberschorsch for the patch @Cottser for finding this.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Quick fix

Good catch. No longer applies, though.

Cyberschorsch’s picture

FileSize
1.46 KB
PASSED: [[SimpleTest]]: [MySQL] 58,843 pass(es). View

Rerolled the patch :)

Cyberschorsch’s picture

Status: Needs work » Needs review
Cottser’s picture

Status: Needs review » Needs work

Thanks for your work on this @Cyberschorsch!

+++ b/core/includes/theme.inc
@@ -2898,20 +2898,13 @@ function template_preprocess_maintenance_page(&$variables) {
-  $variables['site_name']         = (theme_get_setting('features.name') ? String::checkPlain($site_name) : '');
+  $variables['site_name']         = (theme_get_setting('features.name') ? check_plain($site_name) : '');

We should be leaving site_name alone, the latest patch is rolling back a change made in another issue which we don't want to do. Here is the issue: #2089351: Convert all calls to check_plain() in core to Drupal\Component\Utility\String::checkPlain() in core/includes found via git blame.

Cyberschorsch’s picture

Status: Needs work » Needs review
FileSize
1.36 KB
PASSED: [[SimpleTest]]: [MySQL] 58,499 pass(es). View

Ah missed that one! I rerolled the patch and left site name alone

Cottser’s picture

Status: Needs review » Needs work

Wow that was quick :) we do want to get rid of main_menu and secondary_menu, though… and interdiffs are very helpful to see the changes made between patches by the way!

Cyberschorsch’s picture

Status: Needs work » Needs review
FileSize
1.36 KB
PASSED: [[SimpleTest]]: [MySQL] 58,849 pass(es). View

Alrighty, now with removed menus :)
Regarding the interdiff, I tried and failed since the first patch does not apply to the current HEAD and I am not sure where to go from there so I left that one out for now...

areke’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Ok, then this looks good. Thank you!

Cottser’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
Related issues: +#2090967: Extend page.html.twig with html.html.twig

@Cyberschorsch - yes, interdiffs don't usually make sense when the patch doesn't apply anymore :) thanks for sticking with this one.

I'm reading the template docs again (largely unchanged from the PHPTemplate version) and I think all our maintenance-page template docs should be updated while we are making this change. The system maintenance-page.html.twig says this:

 * All the available variables are mirrored in html.html.twig and
 * page.html.twig.
 * Some may be blank but they are provided for consistency.

While Bartik and Seven's templates have this line:

 * All of the available variables are mirrored in page.html.twig.

I don't think any of these are accurate, the variables should mirror those of html.html.twig if anything, so I think for now all three templates can have a line like this:

 * All of the available variables are mirrored in html.html.twig.

And as a bonus there is also this bit in template_preprocess_page() that we could remove:

 * Any changes to variables in this preprocessor should also be changed inside
 * template_preprocess_maintenance_page() to keep all of them consistent.

Also I wanted to mention that there was discussion about the inclusion of these blank variables in #1189822: Convert maintenance-page.html.twig to HTML5 that I agree with and still applies to the current task, added quotes from that issue to the issue summary.

m1n0’s picture

I rerolled the patch with above mentioned documentation changes.

m1n0’s picture

Status: Needs work » Needs review
FileSize
3.59 KB
PASSED: [[SimpleTest]]: [MySQL] 63,684 pass(es). View

I rerolled the patch with above mentioned documentation changes.

Cottser’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll, +Novice, +Twig

This needs a reroll now.

m1n0’s picture

FileSize
3.5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,037 pass(es). View

Reroll done.

m1n0’s picture

Status: Needs work » Needs review
Sutharsan’s picture

Issue tags: -Needs reroll

Tag 'Needs reroll' removed.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice

Double checked their lack of use and they are still not used nor needed, back to RTBC.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.