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

Files: 
CommentFileSizeAuthor
#75 interdiff.txt855 bytesCottser
#75 2072647-74.patch7.3 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 64,223 pass(es).
[ View ]
#75 2072647-74-testonly.patch5.77 KBCottser
FAILED: [[SimpleTest]]: [MySQL] 64,170 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#73 2072647-72.patch7.65 KBherom
PASSED: [[SimpleTest]]: [MySQL] 64,171 pass(es).
[ View ]
#73 interdiff-2072647-69-72.txt2.17 KBherom
#69 interdiff-2072647-66-69.txt733 bytesherom
#69 2072647-69.patch7.66 KBherom
FAILED: [[SimpleTest]]: [MySQL] 64,250 pass(es), 1 fail(s), and 2 exception(s).
[ View ]
#66 2072647-66.patch7.66 KBherom
FAILED: [[SimpleTest]]: [MySQL] 59,754 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#59 interdiff-2072647-55-59.txt1.75 KBherom
#48 2072647-48.patch7.28 KBthedavidmeister
PASSED: [[SimpleTest]]: [MySQL] 59,243 pass(es).
[ View ]

Comments

thedavidmeister’s picture

Status:Active» Needs review
StatusFileSize
new693 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,304 pass(es).
[ View ]

patch.

thedavidmeister’s picture

Issue summary:View changes

Updated issue summary.

thedavidmeister’s picture

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

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

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

thedavidmeister’s picture

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

<?php
 
// 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

StatusFileSize
new657 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,481 pass(es).
[ View ]

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

Issue tags:+Need tests

Its passing, so does this need tests then?

jwilson3’s picture

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

StatusFileSize
new2.26 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/includes/theme.inc.
[ View ]

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

Status:Needs work» Needs review
StatusFileSize
new2.17 KB
FAILED: [[SimpleTest]]: [MySQL] 59,205 pass(es), 0 fail(s), and 2,825 exception(s).
[ View ]

Bah. sorry.

jwilson3’s picture

StatusFileSize
new616 bytes

interdiff between #8-#11.

Status:Needs review» Needs work

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

jwilson3’s picture

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

StatusFileSize
new819 bytes
new2.14 KB
PASSED: [[SimpleTest]]: [MySQL] 59,066 pass(es).
[ View ]
jwilson3’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

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

thedavidmeister’s picture

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’s picture

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

Status:Needs work» Needs review

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

thedavidmeister’s picture

Assigned:Unassigned» thedavidmeister

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

jwilson3’s picture

Saaaawweeeet!

thedavidmeister’s picture

StatusFileSize
new6.16 KB
FAILED: [[SimpleTest]]: [MySQL] 59,578 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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

thedavidmeister’s picture

StatusFileSize
new4.02 KB
FAILED: [[SimpleTest]]: [MySQL] 58,971 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

just the failing test.

Status:Needs review» Needs work

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

thedavidmeister’s picture

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

thedavidmeister’s picture

Status:Needs work» Needs review
StatusFileSize
new7.85 KB
PASSED: [[SimpleTest]]: [MySQL] 58,809 pass(es).
[ View ]

green?

thedavidmeister’s picture

StatusFileSize
new7.79 KB
PASSED: [[SimpleTest]]: [MySQL] 58,759 pass(es).
[ View ]

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

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

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

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

thedavidmeister’s picture

Status:Needs work» Needs review

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

thedavidmeister’s picture

StatusFileSize
new7.84 KB
PASSED: [[SimpleTest]]: [MySQL] 59,215 pass(es).
[ View ]

nicer test names.

tim.plunkett’s picture

  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

StatusFileSize
new7.86 KB
PASSED: [[SimpleTest]]: [MySQL] 58,818 pass(es).
[ View ]
new1.79 KB

here we go.

thedavidmeister’s picture

Assigned:thedavidmeister» Unassigned

unassigning from myself so someone can review

thedavidmeister’s picture

Issue summary:View changes

Updated issue summary.

thedavidmeister’s picture

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

  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

StatusFileSize
new7.47 KB
PASSED: [[SimpleTest]]: [MySQL] 59,149 pass(es).
[ View ]
new2.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

jibran’s picture

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

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

Status:Needs work» Needs review
StatusFileSize
new7.28 KB
PASSED: [[SimpleTest]]: [MySQL] 59,243 pass(es).
[ View ]

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

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

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

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

jibran’s picture

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

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

Patch no longer applies.

LinL’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new7.33 KB
FAILED: [[SimpleTest]]: [MySQL] 58,303 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Reroll

Status:Needs review» Needs work

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

jwilson3’s picture

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

thedavidmeister’s picture

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

herom’s picture

Status:Needs work» Needs review
StatusFileSize
new1.75 KB
new48.69 KB
PASSED: [[SimpleTest]]: [MySQL] 59,435 pass(es).
[ View ]

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

jwilson3’s picture

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

Status:Needs work» Needs review
StatusFileSize
new7.77 KB
PASSED: [[SimpleTest]]: [MySQL] 59,539 pass(es).
[ View ]

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

herom’s picture

Issue summary:View changes

Updated issue summary.

thedavidmeister’s picture

Issue summary:View changes

Updated issue summary.

thedavidmeister’s picture

thedavidmeister’s picture

Issue summary:View changes

Updated issue summary.

thedavidmeister’s picture

Issue summary:View changes

Updated issue summary.

Cottser’s picture

Issue summary:View changes
Status:Needs review» Needs work
Issue tags:+Needs reroll

Tagging for reroll.

herom’s picture

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new7.67 KB
PASSED: [[SimpleTest]]: [MySQL] 59,045 pass(es).
[ View ]

rebased (auto-merged by git)

jwilson3’s picture

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

StatusFileSize
new7.66 KB
FAILED: [[SimpleTest]]: [MySQL] 59,754 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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

Assigned:Unassigned» herom
herom’s picture

Assigned:herom» Unassigned
Status:Needs work» Needs review
StatusFileSize
new7.66 KB
FAILED: [[SimpleTest]]: [MySQL] 64,250 pass(es), 1 fail(s), and 2 exception(s).
[ View ]
new733 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

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

ianthomas_uk’s picture

Priority:Normal» Critical

Status:Needs review» Needs work

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

herom’s picture

Status:Needs work» Needs review
StatusFileSize
new2.17 KB
new7.65 KB
PASSED: [[SimpleTest]]: [MySQL] 64,171 pass(es).
[ View ]
Cottser’s picture

Assigned:Unassigned» Cottser

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

Cottser’s picture

Assigned:Cottser» Unassigned
Status:Needs review» Reviewed & tested by the community
Issue tags:-Core Review Bonus
StatusFileSize
new5.77 KB
FAILED: [[SimpleTest]]: [MySQL] 64,170 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new7.3 KB
PASSED: [[SimpleTest]]: [MySQL] 64,223 pass(es).
[ View ]
new855 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

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

thedavidmeister’s picture

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

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

Thanks!

Status:Fixed» Closed (fixed)

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

sun’s picture

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