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.
Related Issues
#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
Comments
Comment #1
star-szrTags.
Comment #2
sunnydeveloper CreditAttribution: sunnydeveloper commentedI'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
joelpittetUnassigning, @sunnydeveloper you are welcome to grab this again, just freeing it up for the sprint tomorrow.
Comment #4
CyberschorschTaking it as part of sprint at DC Prague
Comment #5
CyberschorschI removed unused variables as suggested.
Comment #6
joelpittetThank you @Cyberschorsch for the patch @Cottser for finding this.
Comment #7
webchickGood catch. No longer applies, though.
Comment #8
CyberschorschRerolled the patch :)
Comment #9
CyberschorschComment #10
star-szrThanks for your work on this @Cyberschorsch!
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
CyberschorschAh missed that one! I rerolled the patch and left site name alone
Comment #12
star-szrWow 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
CyberschorschAlrighty, 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 commentedOk, then this looks good. Thank you!
Comment #15
star-szr@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:
While Bartik and Seven's templates have this line:
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:
And as a bonus there is also this bit in template_preprocess_page() that we could remove:
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.
Comment #16
m1n0 CreditAttribution: m1n0 commentedI rerolled the patch with above mentioned documentation changes.
Comment #17
m1n0 CreditAttribution: m1n0 commentedI rerolled the patch with above mentioned documentation changes.
Comment #18
star-szrThis needs a reroll now.
Comment #19
m1n0 CreditAttribution: m1n0 commentedReroll done.
Comment #20
m1n0 CreditAttribution: m1n0 commentedComment #21
Sutharsan CreditAttribution: Sutharsan commentedTag 'Needs reroll' removed.
Comment #22
joelpittetDouble checked their lack of use and they are still not used nor needed, back to RTBC.
Comment #23
catchCommitted/pushed to 8.x, thanks!