template_preprocess_maintenance_page() simply concatenates '#content' onto the content region:

  // Get all region content set with drupal_add_region_content().
  foreach (array_keys($regions) as $region) {
    // Assign region to a region variable.
    $region_content = drupal_get_region_content($region);
    isset($variables[$region]) ? $variables[$region] .= $region_content : $variables[$region] = $region_content;
  }

This means that '#content' can never be a render array and forces us to always build content for maintenance pages as strings instead of using the Render API.

This issue blocks multiple cleanups for the render API in sections of the site using maintenance pages.

Blocked issues

- #2072639: Convert update_info_page() from a string concatenation function to a render array builder
- #2060401: Remove useless "early render" in authorize.php
- #2060413: remove useless "early render" in update_selection_page()
- #2060441: Convert update_results_page() to return a renderable array rather than early render
- #2088957: title in template_preprocess_maintaince_page should be on 'content' not 'page'

Review Bonus

+2 https://drupal.org/node/855726#comment-8002615
+2 https://drupal.org/node/415710#comment-8002631
-3 tagged in #62

Total: 1

CommentFileSizeAuthor
#64 2072647-64.patch7.67 KBherom
#61 2072647-61.patch7.77 KBherom
#59 2072647-59.patch48.69 KBherom
#55 2072647-55.patch7.33 KBLinL
#44 2072647-44.patch7.47 KBthedavidmeister
#44 2072647-39-44-interdiff.txt2.81 KBthedavidmeister
#39 2072647-39.patch7.86 KBthedavidmeister
#39 2072647-37-39-interdiff.txt1.79 KBthedavidmeister
#37 2072647-37.patch7.84 KBthedavidmeister
#29 2072647-29.patch7.79 KBthedavidmeister
#28 2072647-28.patch7.85 KBthedavidmeister
#25 2072647-failing-test.patch4.02 KBthedavidmeister
#24 2072647-24.patch6.16 KBthedavidmeister
#15 interdiff.txt819 bytesjwilson3
#15 2072647-15.patch2.14 KBjwilson3
#12 interdiff.txt616 bytesjwilson3
#11 2072647-11.patch2.17 KBjwilson3
#9 2072647-8.patch2.26 KBjwilson3
#6 2072647-6.patch657 bytesthedavidmeister
#1 2072647-1.patch693 bytesthedavidmeister
#75 interdiff.txt855 bytesstar-szr
#75 2072647-74.patch7.3 KBstar-szr
#75 2072647-74-testonly.patch5.77 KBstar-szr
#73 2072647-72.patch7.65 KBherom
#73 interdiff-2072647-69-72.txt2.17 KBherom
#69 interdiff-2072647-66-69.txt733 bytesherom
#69 2072647-69.patch7.66 KBherom
#66 2072647-66.patch7.66 KBherom
#59 interdiff-2072647-55-59.txt1.75 KBherom
#48 2072647-48.patch7.28 KBthedavidmeister
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
Status: Active » Needs review
FileSize
2072647-1.patch693 bytes

patch.

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
Issue summary: View changes

Updated issue summary.

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
Issue tags: +theme system cleanup

Status: Needs review » Needs work
Issue tags: -theme system cleanup

The last submitted patch, 2072647-1.patch, failed testing.

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
Status: Needs work » Needs review
Issue tags: +theme system cleanup

#1: 2072647-1.patch queued for re-testing.

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented

discussing in IRC, potentially special-casing 'content' and accepting strings or render arrays there:

  // Get all region content set with drupal_add_region_content().
  $regions = $theme_data[$theme]->info['regions'];
  foreach (array_keys($regions) as $region) {
    // Assign region to a region variable.
    $region_content = drupal_get_region_content($region);
    if ($region === 'content') {
      $passed_content = is_string($variables['content']) ? array('#markup' => $variables['content']) : $variables['content'];
      $variables['content'] = array(
        'passed_content' => $passed_content,
        'region_content' => array('#markup' => $region_content),
      );
    }
    else {
      $variables[$region] = array('#markup' => $region_content);
    }
  }
thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
FileSize
2072647-6.patch657 bytes

Here is a better patch. Discussed this with @alexpott and @catch in IRC.

I have not figured out how to do tests cleanly yet.

jwilson3’s picture

jwilson3
Primary language English
Location Ecuador
CreditAttribution: jwilson3 commented
Issue tags: +Need tests

Its passing, so does this need tests then?

jwilson3’s picture

jwilson3
Primary language English
Location Ecuador
CreditAttribution: jwilson3 commented

This is loosely related to an issue i was working on, and could affect it #713462: Content added via drupal_add_region_content() is not added to pages if it were to get in.

Could we make template_preprocess_maintenance_page() work the same way template_preprocess_page() currently does to build up a render array inside $variables['page']['region_name'] ?

jwilson3’s picture

jwilson3
Primary language English
Location Ecuador
CreditAttribution: jwilson3 commented
FileSize
2072647-8.patch2.26 KB

I was thinking something like this (if it appears I'm hijacking your issue, I'm not! just tryin to help!

This patch also rearranges a couple lines of code so that when compared side-by-side with template_preprocess_page() after applying it, the similarities are more evident.

It also has the added benefit that if you apply the patch from #713462-19: Content added via drupal_add_region_content() is not added to pages, the for loop code is exactly the same between preprocess_page and preprocess_maintenance_page :)

Status: Needs review » Needs work

The last submitted patch, 2072647-8.patch, failed testing.

jwilson3’s picture

jwilson3
Primary language English
Location Ecuador
CreditAttribution: jwilson3 commented
Status: Needs work » Needs review
FileSize
2072647-11.patch2.17 KB

Bah. sorry.

jwilson3’s picture

jwilson3
Primary language English
Location Ecuador
CreditAttribution: jwilson3 commented
FileSize
interdiff.txt616 bytes

interdiff between #8-#11.

Status: Needs review » Needs work

The last submitted patch, 2072647-11.patch, failed testing.

jwilson3’s picture

jwilson3
Primary language English
Location Ecuador
CreditAttribution: jwilson3 commented 
+++ b/core/includes/theme.inc
@@ -2849,11 +2846,14 @@ function template_preprocess_maintenance_page(&$variables) {
+    if (!isset($variables['page'][$region_key])) {
+      $variables['page'][$region_key] = array();
+    }
+    // Append region content set with drupal_add_region_content() as markup.
+    if ($region_content = drupal_get_region_content($region_key)) {
+      $variables['page'][$region_key][]['#markup'] = $region_content;
+    }

So it appears that introducing everything underneath $variables['page'] (as is done in preprocesS_page) pretty much breaks everything :P

Perhaps just removing ['page'] will fix this.

jwilson3’s picture

jwilson3
Primary language English
Location Ecuador
CreditAttribution: jwilson3 commented
FileSize
interdiff.txt819 bytes
2072647-15.patch2.14 KB
jwilson3’s picture

jwilson3
Primary language English
Location Ecuador
CreditAttribution: jwilson3 commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 2072647-15.patch, failed testing.

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented

ah, sorry. I missed all this! great that someone else is taking interest :)

I'm not sure about trying to make 'maintenance_page' work more like 'page' here. I think that's super great in theory, but should live as a separate issue under #2025629: [meta] Ensure that all #theme/#theme_wrappers hooks provided by core are associated with a sensible, re-usable element #type, especially if the newly proposed approach isn't coming back green.

I was hoping the scope of this issue would be as small as possible, as fixing it unblocks at least 4 other issues.

I've also discussed the approach that I took with @alexpott and @catch in IRC and got a preliminary "go ahead", AFAIK it's literally just waiting on some tests as at #6.

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented

I took the liberty of opening #2086619: #theme 'maintenance_page' should be based on #type 'page'.

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented

FWIW, I did manage to install Drupal with the patch in #15, so I'm resubmitting for testing. If it's actually green, then maybe we should keep going with that approach here.

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
Status: Needs work » Needs review

#15: 2072647-15.patch queued for re-testing.

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
Assigned: Unassigned » thedavidmeister

oh green this time, yay! I'm working on tests.

jwilson3’s picture

jwilson3
Primary language English
Location Ecuador
CreditAttribution: jwilson3 commented

Saaaawweeeet!

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
FileSize
2072647-24.patch6.16 KB

This is not finished, the tests work but need some refactoring still.

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
FileSize
2072647-failing-test.patch4.02 KB

just the failing test.

Status: Needs review » Needs work

The last submitted patch, 2072647-failing-test.patch, failed testing.

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented

great, things that pass locally but not on d.o...

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
Status: Needs work » Needs review
FileSize
2072647-28.patch7.85 KB

green?

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
FileSize
2072647-29.patch7.79 KB

Uses DOMDocument instead of regex

Status: Needs review » Needs work
Issue tags: -theme system cleanup, -Need tests

The last submitted patch, 2072647-29.patch, failed testing.

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
Status: Needs work » Needs review

#29: 2072647-29.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 2072647-29.patch, failed testing.

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
Status: Needs work » Needs review

#29: 2072647-29.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +theme system cleanup, +Need tests

The last submitted patch, 2072647-29.patch, failed testing.

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented

sorry for the noise. getting different errors each time, and they don't show up on local.

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
Status: Needs work » Needs review

#29: 2072647-29.patch queued for re-testing.

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
FileSize
2072647-37.patch7.84 KB

nicer test names.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented 
  1. +++ b/core/modules/system/lib/Drupal/system/Tests/Common/RenderElementTypesTest.php
@@ -7,7 +7,9 @@
+use DOMDocument;

@@ -34,7 +36,22 @@ public static function getInfo() {
+      $value = new DOMDocument();
...
+      $expected = new DOMDocument();

    new \DOMDocument(); 

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/Common/RenderElementTypesTest.php
@@ -113,4 +130,115 @@ function testHtmlTag() {
+      $head_title = array('name' => check_plain($site_name));

    String::checkPlain 

  3. +++ b/core/modules/system/lib/Drupal/system/Tests/Common/RenderElementTypesTest.php
@@ -113,4 +130,115 @@ function testHtmlTag() {
+    drupal_add_html_head_link(array('rel' => 'shortcut icon', 'href' => drupal_strip_dangerous_protocols($favicon), 'type' => $type));

    Url::stripDangerousProtocols

Otherwise, I think this looks good!

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
FileSize
2072647-39.patch7.86 KB
2072647-37-39-interdiff.txt1.79 KB

here we go.

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented

I've opened:

- #2089327: Remove / deprecate calls to drupal_strip_dangerous_protocols() use \Drupal\Component\Utility\Url::stripDangerousProtocols()
- #2089331: [meta] Replace calls to check_plain() with Drupal\Component\Utility\String::checkPlain()

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
Assigned: thedavidmeister » Unassigned

unassigning from myself so someone can review

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
Issue summary: View changes

Updated issue summary.

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented

Related: #2088957: title in template_preprocess_maintaince_page should be on 'content' not 'page'

Currently, '#title' for 'page' does not work for maintenance pages. The functionality *looks* like it would work, but there's no way to pass 'page' into the preprocess in reality.

This could be fixed if 'content' was a render array that could have a #title attribute.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented 
  1. +++ b/core/includes/theme.inc
@@ -2850,11 +2847,14 @@ function template_preprocess_maintenance_page(&$variables) {
+  foreach (system_region_list($GLOBALS['theme']) as $region_key => $region_name) {

    $GLOBALS i don't like it. Do we have any alternative to this yet? 

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/Common/RenderElementTypesTest.php
@@ -113,4 +130,115 @@ function testHtmlTag() {
+    $site_config = \Drupal::config('system.site');

    I think we can leave it as per #2089337: Decide if tests are allowed to use \Drupal::something() instead of $this->container->get() 

  3. +++ b/core/modules/system/lib/Drupal/system/Tests/Common/RenderElementTypesTest.php
@@ -113,4 +130,115 @@ function testHtmlTag() {
+        $head_title['slogan'] = strip_tags(filter_xss_admin($site_slogan));

    We can use Xss::filterAdmin instead of filter_xss_admin. 

  4. +++ b/core/modules/system/lib/Drupal/system/Tests/Common/RenderElementTypesTest.php
@@ -113,4 +130,115 @@ function testHtmlTag() {
+    $expected = '<!DOCTYPE html>';
+    $expected .= '<html>';
+    $expected .= '<head>';
+    $expected .= '!head';
+    $expected .= '<title>!head_title</title>';
+    $expected .= '!styles';
+    $expected .= '!scripts';
+    $expected .= '</head>';
+    $expected .= '<body class="!attributes.class">';
+    $expected .= '<div class="l-container">';
+    $expected .= '<header role="banner">';
+    $expected .= '<a href="!front_page" title="Home" rel="home">';
+    $expected .= '<img src="!logo" alt="Home"/>';
+    $expected .= '</a>';
+    $expected .= '<div class="name-and-slogan">';
+    $expected .= '<h1 class="site-name">';
+    $expected .= '<a href="!front_page" title="Home" rel="home">!site_name</a>';
+    $expected .= '</h1>';
+    $expected .= '</div>';
+    $expected .= '</header>';
+    $expected .= '<main role="main">';
+    $expected .= '!title';
+    $expected .= '!content';
+    $expected .= '</main>';
+    $expected .= '</div>';
+    $expected .= '</body>';
+    $expected .= '</html>';

    This should be heredoc string. 

  5. +++ b/core/modules/system/lib/Drupal/system/Tests/Common/RenderElementTypesTest.php
@@ -113,4 +130,115 @@ function testHtmlTag() {
+      '!front_page' => url(),

    Can we use here?

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
FileSize
2072647-44.patch7.47 KB
2072647-39-44-interdiff.txt2.81 KB

1. The only reason that $GLOBALS appears in this patch is because it's more inline with what template_preprocess_page() does. It looks like work on getting at the active theme without globals is happening still #1954892: Replace 'theme callback' and hook_custom_theme() with a clean theme negotiation system.

2. So, nothing to do here then?

3. sure. updated in patch.

4. also updated.

5. we need to use url() to keep the tests portable, or they won't always match the output of template_preprocess_maintenance_page()

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented

I opened #2089433: Remove uses of deprecated XSS filter functions

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Need tests

Thank you @thedavidmeister for fixes. Code looks great. It has tests now. Nice clean up. So RTBC.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Title: #theme 'maintenance_page' should support render arrays in #content » #theme \'maintenance_page\' should support render arrays in #content
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
Status: Needs work » Needs review
FileSize
2072647-48.patch7.28 KB

reroll

Status: Needs review » Needs work
Issue tags: -Needs reroll, -theme system cleanup

The last submitted patch, 2072647-48.patch, failed testing.

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
Status: Needs work » Needs review

#48: 2072647-48.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 2072647-48.patch, failed testing.

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
Status: Needs work » Needs review
Issue tags: +Needs reroll, +theme system cleanup

#48: 2072647-48.patch queued for re-testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Title: #theme \'maintenance_page\' should support render arrays in #content » #theme 'maintenance_page' should support render arrays in #content
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

Back to RTBC.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

LinL’s picture

LinL CreditAttribution: LinL commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2072647-55.patch7.33 KB

Reroll

Status: Needs review » Needs work

The last submitted patch, 2072647-55.patch, failed testing.

jwilson3’s picture

jwilson3
Primary language English
Location Ecuador
CreditAttribution: jwilson3 commented

that's odd that the patch failed the two tests it added.

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented

indeed, was the markup of the maintenance page changed recently?

herom’s picture

herom CreditAttribution: herom commented
Status: Needs work » Needs review
FileSize
interdiff-2072647-55-59.txt1.75 KB
2072647-59.patch48.69 KB

update patch.
@thedavidmeister, this happened: #2067915: Restore html attributes to maintenance-page.html.twig

jwilson3’s picture

jwilson3
Primary language English
Location Ecuador
CreditAttribution: jwilson3 commented
Status: Needs review » Needs work 
diff --git a/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Block/AggregatorCategoryBlock.php b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Block/AggregatorCategoryBlock.php
index 618a3c2..eccd463 100644
--- a/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Block/AggregatorCategoryBlock.php
+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Block/AggregatorCategoryBlock.php
@@ -13,7 +13,6 @@
 use Drupal\Core\Annotation\Translation;
 use Drupal\Core\Database\Connection;
 use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
-use Drupal\Core\Session\AccountInterface;
 use Symfony\Component\DependencyInjection\ContainerInterface;
 
 /**
@@ -84,11 +83,11 @@ public function defaultConfiguration() {
   }
 
   /**
-   * {@inheritdoc}
+   * Overrides \Drupal\block\BlockBase::access().
    */
-  public function access(AccountInterface $account) {
+  public function access() {
     // Only grant access to users with the 'access news feeds' permission.
-    return $account->hasPermission('access news feeds');
+    return user_access('access news feeds');
   }
 
   /**
diff --git a/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Block/AggregatorFeedBlock.php b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Block/AggregatorFeedBlock.php
index b5272e9..4fc69fe 100644
--- a/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Block/AggregatorFeedBlock.php
+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Block/AggregatorFeedBlock.php
@@ -13,7 +13,6 @@
 use Drupal\Core\Database\Connection;
 use Drupal\Core\Entity\EntityStorageControllerInterface;
 use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
-use Drupal\Core\Session\AccountInterface;
 use Symfony\Component\DependencyInjection\ContainerInterface;

The interdiff looks fine, but the patch itself is unbelievable because it messes with a *ton* of unrelated stuff. You can tell just by comparing file sizes: it jumped from 7KB to 48KB between #55 and #59 :P

herom’s picture

herom CreditAttribution: herom commented
Status: Needs work » Needs review
FileSize
2072647-61.patch7.77 KB

oh! The patch wasn't rebased on HEAD, but diffed against HEAD.
fixing.

herom’s picture

herom CreditAttribution: herom commented
Issue summary: View changes

Updated issue summary.

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
Issue summary: View changes

Updated issue summary.

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
Issue tags: +Core Review Bonus

Tagging for #2094585: [policy, no patch] Core review bonus.

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
Issue summary: View changes

Updated issue summary.

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
Issue summary: View changes

Updated issue summary.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs reroll

Tagging for reroll.

herom’s picture

herom CreditAttribution: herom commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2072647-64.patch7.67 KB

rebased (auto-merged by git)

jwilson3’s picture

jwilson3
Primary language English
Location Ecuador
CreditAttribution: jwilson3 commented

Looks good to me. In addition to unblocking all the other render api cleanups, this is the ground work for fixing a long time theming wtf. Themers will finally have an easier time converting page.(tpl.php|twig) to maintenance-page.(tpl.php|twig).

My only thought is that we need better test coverage of drupal_add_region_content(), but I believe this test coverage doesnt necessarily block the changes here, and could be added as a part of #713462: Content added via drupal_add_region_content() is not added to pages.

herom’s picture

herom CreditAttribution: herom commented
FileSize
2072647-66.patch7.66 KB

patch didn't apply.
another reroll (again, auto-merged by git).

Status: Needs review » Needs work

The last submitted patch, 66: 2072647-66.patch, failed testing.

herom’s picture

herom CreditAttribution: herom commented
Assigned: Unassigned » herom
herom’s picture

herom CreditAttribution: herom commented
Assigned: herom » Unassigned
Status: Needs work » Needs review
FileSize
2072647-69.patch7.66 KB
interdiff-2072647-66-69.txt733 bytes

the order of loading css files was modified in #2008270: Remove drupal_add_css() from template_preprocess_maintenance_page() — use #attached. system.theme.css was given a higher weight than the rest (CSS_SKIN - 10).

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented

69: 2072647-69.patch queued for re-testing.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
CreditAttribution: ianthomas_uk commented
Priority: Normal » Critical

Critical as this blocks #1830588: [META] remove drupal_set_title() and drupal_get_title()

Status: Needs review » Needs work

The last submitted patch, 69: 2072647-69.patch, failed testing.

herom’s picture

herom CreditAttribution: herom commented
Status: Needs work » Needs review
FileSize
interdiff-2072647-69-72.txt2.17 KB
2072647-72.patch7.65 KB

two things happened:
#2168113: Add leading underscore and other discouragement to drupal_add_css() and drupal_add_js(), #1862202: Objectify the language system

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Assigned: Unassigned » star-szr

Thanks @herom and @ianthomas_uk, I'm going to review this today.

star-szr’s picture

star-szr
he/him
Primary language English
CreditAttribution: star-szr commented
Assigned: star-szr » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: -Core Review Bonus
FileSize
2072647-74-testonly.patch5.77 KB
2072647-74.patch7.3 KB
interdiff.txt855 bytes

Looks great!

  1. +++ b/core/includes/theme.inc
@@ -2350,11 +2350,8 @@ function theme_get_suggestions($args, $base, $delimiter = '__') {
+  $site_config = \Drupal::config('system.site');

@@ -2391,7 +2391,6 @@ function template_preprocess_maintenance_page(&$variables) {
-  $site_config = \Drupal::config('system.site');

    Minor but moving $site_config up doesn't seem to have any purpose and IMO is harder to follow.

    Since that change is so small, rolling it in here.

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/Common/RenderElementTypesTest.php
@@ -34,7 +39,22 @@ public static function getInfo() {
+      // We don't care about whitespace for the sake of comparing markup.
+      $value = new \DOMDocument();
+      $value->preserveWhiteSpace = FALSE;
+      $value->loadXML(drupal_render($element['value']));

    Nice, could this approach be taken with other render tests?

I'm also uploading a test-only patch again since it's been quite a while and we can't see the results of the last one.

Passing the core review bonus tag on to #1987398: field.module - Convert theme_ functions to Twig.

The last submitted patch, 75: 2072647-74-testonly.patch, failed testing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented
Status: Fixed » Reviewed & tested by the community

Really?

What is the hash for the commit that was pushed? I just fetched/pulled and I can't see any of the changes from #75.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Yes you're right, that was imaginary apparently.

Now properly committed/pushed to 8.x.

thedavidmeister’s picture

thedavidmeister CreditAttribution: thedavidmeister commented

Thanks!

Status: Fixed » Closed (fixed)

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

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Note that I'm removing the code and the test coverage in #2218039: Render the maintenance/install page like any other HTML page