Remove unneeded variables from template_preprocess_maintenance_page() [#2073621]

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

Comment	File	Size	Author
#19	remove-unused-variables-maintenance-1189822-19.patch	3.5 KB	m1n0
#19
#17	remove-unused-variables-maintenance-1189822-16.patch	3.59 KB	m1n0
#17
#13	remove-unused-variables-maintenance-1189822-13.patch	1.36 KB	Cyberschorsch
#13
#11	remove-unused-variables-maintenance-1189822-11.patch	1.36 KB	Cyberschorsch
#11
#8	remove-unused-variables-maintenance-1189822-8.patch	1.46 KB	Cyberschorsch
#8
#5	remove-unused-variables-maintenance-1189822.patch	1.35 KB	Cyberschorsch
#5

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

star-szr

he/him

English

CreditAttribution: star-szr commented 25 August 2013 at 12:48

Issue tags:

+Novice, +theme system cleanup

Tags.

Comment #2

sunnydeveloper CreditAttribution: sunnydeveloper commented 31 August 2013 at 15:28

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)

Comment #3

joelpittet

English

Vancouver

CreditAttribution: joelpittet commented 26 September 2013 at 17:14

Assigned:	sunnydeveloper	» Unassigned
Issue tags:		+prague2013

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

Comment #4

Cyberschorsch

German

CreditAttribution: Cyberschorsch commented 27 September 2013 at 11:34

Assigned:

Unassigned

» Cyberschorsch

Taking it as part of sprint at DC Prague

Comment #5

Cyberschorsch

German

CreditAttribution: Cyberschorsch commented 27 September 2013 at 11:43

Status:

Active

» Needs review

File	Size
remove-unused-variables-maintenance-1189822.patch	1.35 KB

I removed unused variables as suggested.

Comment #6

joelpittet

English

Vancouver

CreditAttribution: joelpittet commented 1 October 2013 at 18:15

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.

Comment #7

webchick

she/they

English

Vancouver 🇨🇦

CreditAttribution: webchick commented 9 October 2013 at 05:27

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

Good catch. No longer applies, though.

Comment #8

Cyberschorsch

German

CreditAttribution: Cyberschorsch commented 9 October 2013 at 05:59

File	Size
remove-unused-variables-maintenance-1189822-8.patch	1.46 KB

Rerolled the patch :)

Comment #9

Cyberschorsch

German

CreditAttribution: Cyberschorsch commented 9 October 2013 at 06:00

Status:

Needs work

» Needs review

Comment #10

star-szr

he/him

English

CreditAttribution: star-szr commented 9 October 2013 at 11:41

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.

Comment #11

Cyberschorsch

German

CreditAttribution: Cyberschorsch commented 9 October 2013 at 12:16

Status:

Needs work

» Needs review

File	Size
remove-unused-variables-maintenance-1189822-11.patch	1.36 KB

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

Comment #12

star-szr

he/him

English

CreditAttribution: star-szr commented 9 October 2013 at 12:25

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!

Comment #13

Cyberschorsch

German

CreditAttribution: Cyberschorsch commented 9 October 2013 at 12:48

Status:

Needs work

» Needs review

File	Size
remove-unused-variables-maintenance-1189822-13.patch	1.36 KB

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

Comment #14

areke CreditAttribution: areke commented 6 December 2013 at 03:24

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

Ok, then this looks good. Thank you!

Comment #15

star-szr

he/him

English

CreditAttribution: star-szr commented 6 December 2013 at 15:22

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.