Convert "messages" page element into blocks

Here's the first version of this. Just splitting off messages from the rest of the patch now, with a couple minor tweaks. I'll take care of block__preprocess_page and the post render changes in the next version. The post render version stuff looks sort of complicated, but I'll try to work it out.

1. Added block_preprocess_page to remove the messages variable from the page template if it exists. This avoids a conflict with the messages block.
2. webchick wanted to make sure that messages always showed up. So this also prevents the messages block from being disabled, and it also provides a fallback for Bartik and Seven in case the block module is disabled.
3. Added the post render cache for messages.

Trying a different way to remove the page messages variable.

I was unsetting the messages region rather than the messages variable.

Taking bets on if this fixes the fails or introduces a whole new slew of them.

Well that's unfortunate.

Just an experiment here. Checking what happens if I remove the block_preprocess_page function.

Found a typo in Bartik's page template that was causing a problem. The interdiff is against patch 7, not patch 10. I don't think this will fix everything, but it might fix some of the fails.

Would be pretty neat if patch 7-11 was the winner. If so, I'm celebrating with a slushie.

If I had to bet as to what's going on here, it's this:

There are a lot of tests that add in the block module but still use the testing profile.

The minimal and standard profiles add a messages block to the messages region, but the testing profile does not, because the testing profile does not have blocks.

So if the block module is enabled, but there's no messages block installed with the testing profile, block_preprocess_page will still remove the messages variable from the template.

What would be good is if somehow block_preprocess_page could detect if there was at least one messages block placed in a region. If so, then remove the messages variable. That presumably would help things.

However it doesn't entirely explain all the failures because when block_preprocess_page was removed, there were still 811 fails and 4 exceptions. I'm not clear why.

Maybe I just screwed up the way that the messages post rendering is working.

Found another dumb error I made. Ugh. Reworking the patching and testing it.

I fixed a typo in the services definition (yay), but when I tested this, messages weren't showing up (boo).

There's a very good chance I'm doing this wrong. If anybody would like to take a look, I'd much appreciate it.

timplunkett helped to rework the block_preprocess_page part. This now checks if a messages block is present before removing the messages variable. That should help.

Down to 10 test failures. Much, much better.

All the tests that are failing are running on the standard profile, where the messages block is installed, and presumably the messages variable is removed from the page.

So since messages aren't actually showing up in the block, my guess is that the post render cache thing I set up is the problem here. We probably need Wim to figure out what's going wrong. But if you are not Wim, and you want to take a look at this, and you see something that looks off, that's cool too!

Just a quick review ...

1. +++ b/core/modules/system/src/MessagesPostRenderCache.php
   @@ -0,0 +1,45 @@
   +  /**
   +   * The messages.
   +   * They are stored in a static property so they can appear in multiple blocks.
   +   *
   +   */
   +  protected static $messages;
   +
   ...
   +    if (!isset(static::$messages)) {
   +      static::$messages = drupal_get_messages();
   +    }
   

   Can't you just skip this by using clear_queue=FALSE ?

2. +++ b/core/modules/system/templates/install-page.html.twig
   @@ -30,7 +30,12 @@
   +      {{ page.messages }}
   

   Does it really make sense to introduce a new region and not reuse the content one?

While working on rerolling #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees — which was green in the middle of June (#1805054-118: Cache localized, access filtered, URL resolved, and rendered menu trees) — I noticed something similar: my #post_render_cache callbacks in renderable arrays for blocks are never invoked! Please check if this is the case here as well; just put a print 'YAY';exit; in the #post_render_cache callback. If the page is rendered incompletely and you see "YAY", then my suspicion is wrong — and I hope I'm wrong.

The reason:

In a nutshell, it's because the theme system/Twig invokes drupal_render() on a copy of the render array, and hence cache tags and #post_render_cache metadata doesn't bubble up as it should […]

There's an issue to fix this over at #2273277: Figure out a solution for the problematic interaction between the render system and the theme system when using #pre_render, and that also contains more info.

Important: this used to work just fine in mid June. So something may have changed block rendering-wise between June 16 and June 22 that causes this. But the overarching problem is #2273277, and that also just needs to be fixed.

Time to revive this. Now that post render callbacks should be working correctly, we should be able to get this working.

Indeed, #2273277: Figure out a solution for the problematic interaction between the render system and the theme system when using #pre_render landed, which makes it possible to push this one forward again. I'll provide reviews!

Attached a straight reroll of #18

Also, I wanted to mention some considerations now that we are moving twig files into classy in the Banana Phase 2:

New twig file:

• core/modules/system/templates/block--system-page-messages-block.html.twig - No CSS classes in here.

Core twig files modified:

• core/modules/system/templates/install-page.html.twig
• core/modules/system/templates/maintenance-page.html.twig
• core/modules/system/templates/page.html.twig

I put this here so that we can take a look down the road.

+++ b/core/includes/theme.inc
@@ -1800,6 +1800,12 @@ function drupal_common_theme() {
+    // @TODO change this to status_messages once the messages are gone from
+    // template_preprocess_page
+    'status_messages_block' => array(
+      'variables' => array('status_headings' => array(), 'message_list' => NULL),
+      'template' => 'status-messages',
+    ),

This @todo needs to be dealt with here. Any suggestions to make this work/go away?

#26: just remove it from template_preprocess_page() and page.html.twig?

OK, here is some work on this:

Fixed these two errors that were coming up on site installation:

• Error: Class 'Drupal\block\BlockBase' not found in /var/www/drupal8/core/modules/system/src/Plugin/Block/SystemPageMessagesBlock.php, line 23
• PHP Fatal error: Declaration of Drupal\system\Plugin\Block\SystemPageMessagesBlock::buildConfigurationForm() must be compatible with Drupal\Core\Plugin\PluginFormInterface::buildConfigurationForm(array $form, Drupal\Core\Form\FormStateInterface $form_state) in /var/www/drupal8/core/modules/system/src/Plugin/Block/SystemPageMessagesBlock.php on line 23

And fixed also this one when you tried visiting the site:

• Fatal error: Call to undefined method Drupal\Core\Theme\ThemeNegotiator::getActiveTheme() in /var/www/drupal8/core/modules/block/block.module on line 274

I then tried to work on that @todo, but messages are not showing up apparently... could use some guidance!

I'm making the changes necessary to get messages to work again, to unblock you. Will post patch tomorrow morning CET.

Fixed the patch, now messages show up, except that show_messages (just grep for that and you'll about 1.5 dozen matches) is not yet taken into account, that's an important thing to fix still.
That's patch 31.

1. NOT use #post_render_cache and keep \Drupal\system\Plugin\Block\SystemPageMessagesBlock::isCacheable() returning FALSE
2. OR keep using #post_render_cache and make \Drupal\system\Plugin\Block\SystemPageMessagesBlock::isCacheable() return TRUE

Right now, we're not caching it, and generating a render cache placeholder every request. That's pointless. #post_render_cache already ensures that something that is not cacheable becomes cacheable, by only storing a placeholder. Which makes the block perfectly cacheable.

But…actually, only option 2 allows for the messages block being placed multiple times. So we should go with option 2.

And when doing that, we can also optimize things a bit more: instead of rendering the messages N times (for N messages blocks), having N #post_render_cache callbacks executed, and having to replace the placeholder in the HTML N times… we can reduce all those Ns to just 1. After all, each messages block must contain exactly the same messages. So by ensuring that we generate exactly the same placeholder, we can enforce that more optimal behavior :)

No actual changes in the number of test failures/exceptions, because those are not caused by messages not appearing, but by other things being broken :)

Looking at the failing tests here... I see this warning all over the place on the failing tests:
mb_strtolower() expects parameter 1 to be string, object given

Line 299 on file Unicode.php function Drupal\Component\Utility\Unicode::strtolower()

Any clue what it is?

It's receiving an object instead of a string. Just add a

if (!is_string($parameter)) {
  debug(debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 10));
}

to figure out which calling code is responsible.

This should fix some of the failing tests

Thanks for picking this up @Wim Leers and nice catch @lauriii.

Few little items when reviewing the code for this:

1. +++ b/core/modules/system/templates/install-page.html.twig
   @@ -30,7 +30,10 @@
   -      {{ messages }}
   ...
   +      {% if messages %}
   +        {{ messages }}
   +      {% endif %}
   

   This is not necessary at all, won't change anything but not needed because it will just print nothing if empty. If you need to print messages just leave it in there, or move it if need be.

2. +++ b/core/modules/system/templates/maintenance-page.html.twig
   @@ -41,7 +41,11 @@
   -    {{ messages }}
   ...
   +    {% if messages %}
   +      {{ messages }}
   +    {% endif %}
   

   Same here.

3. +++ b/core/themes/bartik/templates/status-messages.html.twig
   @@ -0,0 +1,58 @@
   +{% set has_messages = 0 %}
   +{% for type, messages in message_list %}
   +  {% if messages|length > 0 %}
   +    {% set has_messages = 1 %}
   +  {% endif %}
   +{% endfor %}
   

   This variable prep logic should be in preprocess and send the variable to the template.

4. +++ b/core/themes/bartik/templates/status-messages.html.twig
   @@ -0,0 +1,58 @@
   +{% if has_messages %}
   +<div id="messages"><div class="section clearfix">
   +{% for type, messages in message_list %}
   

   Our indentation coding standards issues here. Each div should be nested in it's own line and we are indenting the after the if to be inline with twig coding standards.

5. +++ b/core/themes/bartik/templates/status-messages.html.twig
   @@ -0,0 +1,58 @@
   +        {{ messages.0 }}
   +      {% endif %}
   +    {% if type == 'error' %}
   +      </div>
   

   Indent is weird here. One needs to go forward or back.

6. +++ b/core/themes/bartik/templates/status-messages.html.twig
   @@ -0,0 +1,58 @@
   +</div></div> <!-- /.section, /#messages -->
   

   No need for the inline comment.

Those likely won't fix the problem but may help spot something that could inch it forward some more;)

This should fix some more tests.

Thanks @joelpittet for the review! I fixed points from #40.

Lets try this to fix most of the tests..

Hmm lets see now

Holy cross post batman, that's twice crossposted with testbot and laurriii...

Thanks a bunch @lauriii for tracking down those bugs again and fixing the nitpicks, here's a couple more and a question on that preprocess/template.

1. +++ b/core/includes/theme.inc
   @@ -1789,7 +1763,8 @@ function drupal_common_theme() {
   +      'variables' => array('status_headings' => array(), 'message_list' => NULL),
   

   Short array syntax.

2. +++ b/core/themes/bartik/bartik.theme
   @@ -110,6 +110,13 @@ function bartik_preprocess_menu(&$variables) {
   +function bartik_preprocess_status_messages(&$variables) {
   +  $variables['has_messages'] = !empty($variables['message_list']);
   

   Weird, I wonder why bartik only getting this? Shouldn't that be something core needs? And before, it was checking for the key messages is empty, could you do the equivalent in PHP?

3. +++ b/core/themes/seven/templates/install-page.html.twig
   @@ -27,7 +27,10 @@
   +    {% if messages %}
   +      {{ messages }}
   +    {% endif %}
   

   One more of these unnecessary if statements.

Lets see if its going to right direction

I also removed messages variable from templates because the variable is not set anymore so I couldn't figure out why it should be still there

Added 'block' into modules array and system_page_messages_block added. Not enough time to do more. At Drupal-sprint FI.

Added interdiff.

Ignoring #53/#54 because it's several kilobytes less in patch size and the interdiff is suspiciously large.

Straight reroll of #50 (had to fix 2 conflicts), then pushing this forward a bit.

(No worries, Scionar, we've all been there. It takes a while to get that patch stuff going well :))

Review. Minor reroll attached, which fixes whitespace nits + point 3 below.

1. +++ b/core/modules/block/block.module
   @@ -258,6 +258,37 @@ function template_preprocess_block(&$variables) {
   +function block_preprocess_page(&$variables) {
   

   I'd like us to get rid of this, in favor of the approach in #507488-137: Convert page elements (local tasks, actions) into blocks, if possible. That'd be consistent with these other "essential page elements".

2. +++ b/core/modules/block/src/BlockListBuilder.php
   @@ -296,6 +296,11 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    // Do not allow disabling the status messages block when it is present.
   +    if (isset($form['blocks']['system_page_messages']['region'])) {
   +      $form['blocks']['system_page_messages']['region']['#required'] = TRUE;
   +    }
   

   This doesn't actually work. E.g. for Bartik, this block is called bartik_messages.

   It also seems unnecessary, because we don't do this for the main content block either.

3. +++ b/core/modules/system/src/Plugin/Block/SystemPageMessagesBlock.php
   @@ -0,0 +1,129 @@
   +  protected $configFactory;
   

   This is never used, so we can significantly simplify SystemPageMessagesBlock. Done.

4. +++ b/core/themes/bartik/bartik.theme
   @@ -110,6 +110,13 @@ function bartik_preprocess_menu(&$variables) {
    /**
   + * Implements hook_preprocess_HOOK() for status-messages.html.twig.
   + */
   +function bartik_preprocess_status_messages(&$variables) {
   +  $variables['has_messages'] = !empty($variables['message_list']);
   +}
   

   Don't we define logic like this in the Twig templates themselves nowadays?

5. +++ b/core/themes/bartik/templates/maintenance-page.html.twig
   @@ -39,13 +39,7 @@
   -            {% if messages %}
   -              <div id="messages">
   -                <div class="section clearfix">
   -                  {{ messages }}
   -                </div>
   -              </div>
   -            {% endif %}
   +            {{ page.messages }}
   

   I don't think the maintenance page (or install page) can ever use blocks. They have regions, yes, but not blocks.
   So we'll need to make sure we fill the messages region with something even when the block module is disabled. Which is why I referred to that other issue earlier.

1. The included block schema was invalid. Easy fix; removes a few test failures.
2. The cache tags for the messages block is now present on pages, updated PageCacheTagsIntegrationTest

Fixed a few tests.

Funny one: BlockTestBase::setUp() was creating a messages block, but later during that function, all existing blocks are deleted… :) So it couldn't possibly work. This should help reduce the number of test failures by another bit.

Fixing everything like this feels wrong though. See #58.1 & #58.5. Doing that now.]

EDIT: oops, debug additions while gearing up for that next step made it into this patch + interdiff. This will fail majestically.

Reverted all changes in #63, except those in BlockTest(Base). The SimplePageVariant (used when block module is not active) now also renders messages. This is what is causing the majority of the test failures, and should thus help significantly in bringing this closer to zero test failures.

Note that we want to remove the drupalPlaceBlock() calls added in previous patches whenever we also had to enable the block module.

That leaves only one important matter to decide: when block module is enabled, do we require a messages block to be placed, or do we want to automatically provide a fallback? If we don't automatically provide a fallback (we currently don't, yet), then it's easy for a site builder to end up with a site that never shows messages (i.e. when the messages block is removed).

593 fails. Much better.

Most of the remaining ones are probably for the case where block module is being used, but the messages block is not being placed yet.

Can't we make WebTestBase::setUp() check which modules are being installed, and if the block module is also being installed, then place the messages block automatically?

do we require a messages block to be placed, or do we want to automatically provide a fallback?

Just thinking about that, won't we need a fallback to warn about placing a block? Could we possibly render messages via one of the core regions, like page.top if no block is placed, but provide a warning that the block should be placed?

I think we should do it the same way as for MainContentInterface and PageTitleInterface have a PageMessagesInterface.

That is then:

a) Consistent with what we have elsewhere
b) Provides the fallback
c) Still makes it possible to configure the block

Agreed.

Done. This should be green or very close to green.

Next reroll: removing all those drupalPlaceBlock() additions.

Now we can make the patch significantly smaller.

I think the thousands of test failures above show we have sufficient test coverage.

We will still have the failures caused by shortcut module's shortcut_preprocess_page().
LOL, no, that's in #507488: Convert page elements (local tasks, actions) into blocks. My bad.

Unfortunately still 30 fails (but that's >500 less than before), but fortunately #71 has exactly the same fails with a 28% smaller patch :) Hunting down those last 30.

1. +++ b/core/lib/Drupal/Core/Render/Element/StatusMessages.php
   @@ -0,0 +1,122 @@
   +    // Replace the placeholder.
   +    $callback = get_class() . '::renderMessages';
   +    $placeholder = $renderer->generateCachePlaceholder($callback, $context);
   +    $element['#markup'] = str_replace($placeholder, $markup, $element['#markup']);
   +    $element = $renderer->mergeBubbleableMetadata($element, $messages);
   

   We really really need to make this API easier to use. It is way too low-level.

   Not the fault of this issue though.

2. +++ b/core/modules/system/src/Plugin/Block/SystemPageMessagesBlock.php
   @@ -0,0 +1,69 @@
   +    $form['cache']['max_age']['#value'] = -1;
   +    $form['cache']['max_age']['#disabled'] = TRUE;
   

   Should use Cache::PERMANENT to make this explicit.

Except for the usage of hard-coded -1 and the failing tests this would be RTBC for me.

#76.1: indeed, out of scope here.

#76.2: fixed, great catch, I missed that :)

This reroll should fix the majority (if not all) of the remaining test failures. Summary of changes:

1. #theme => messages -> #type => messages
2. Messages are rendered via #post_render_cache and hence drupal_render_root() must be used. As should have been the case for all these AJAX command callbacks already anyway.

Interdiff was right, patch was wrong. Rerolled.

Needs a change record OR change record updates for the #theme => #type change and integration.

Done: https://www.drupal.org/node/2446615

Shouldn't there be a change record about the actual change here? Removing the availability of 'messages' for page templates and converting it into a block.

#83: good point, done: https://www.drupal.org/node/2446647 — mirrored after https://www.drupal.org/node/2410773

This fixes the 3 failures in BlockPageVariantTest and expands the test coverage.

Most of the remaining failures were due to #show_messages on the BareHtmlPageRenderer.

But the failure in SystemAuthorizeTest I spent hours debugging, thanks to this gem introduced in one of the patches above:

diff --git a/core/themes/seven/templates/maintenance-page.html.twig b/core/themes/seven/templates/maintenance-page.html.twig
index f2bc53b..1e6acef 100644
--- a/core/themes/seven/templates/maintenance-page.html.twig
+++ b/core/themes/seven/templates/maintenance-page.html.twig
@@ -27,8 +27,7 @@
     {% if title %}
       <h1>{{ title }}</h1>
     {% endif %}
-    {{ messages }}
-    {{ page.content }}
+    {{ page.messages }}
   </main>
 
   {% if page.page_bottom %}

… note how page.content is removed. ARGH!!!!!!

This still won't be green, but will be very close.

A bunch of installer-related test failures appeared in #87 because the installer was now actually trying to show messages again, but doing so failed because the hash salt isn't set yet during the installation.

This will hopefully be green.

+++ b/core/lib/Drupal/Core/Render/Element/StatusMessages.php
@@ -55,8 +55,16 @@ public static function generatePlaceholder(array $element) {
+    catch (\RuntimeException $e) {
+      // Status messages are also shown during the installer, at which time no
+      // hash salt is defined yet.
+      $hash_salt = Crypt::randomBytes(8);
+    }

Ugh, that is unfortunate.

Not the fault of this patch, though.

Interdiff looks good! Lets see once we are green :).

Has tests, has change record updates, the type messages here is fine and makes sense as you usually just place the block anyway.

Therefore:

RTBC! Great work!

And a bit of clean-up.

Most notable changes pointed out:

1. +++ b/core/modules/system/src/Plugin/Block/SystemPageMessagesBlock.php
   @@ -0,0 +1,70 @@
   + * Contains \Drupal\system\Plugin\Block\SystemPageMessagesBlock.
   ...
   + * Provides a block to display the page messages.
   ...
   + *   admin_label = @Translation("Page messages")
   ...
   +class SystemPageMessagesBlock extends BlockBase implements MessagesBlockPluginInterface {
   

   IMO the "page" part here should be omitted; this is just about "messages", period. There's nothing page-specific about them.

   The only reason the "page" prefix is here, is because it used to be in the page template.

2. +++ b/core/themes/bartik/bartik.theme
   @@ -110,6 +110,13 @@ function bartik_preprocess_menu(&$variables) {
   +function bartik_preprocess_status_messages(&$variables) {
   +  $variables['has_messages'] = !empty($variables['message_list']);
   +}
   

   Why not just use

   {% if message_list is not empty %}
   

   in Twig instead of creating this variable and then doing

   {% if has_messages %}
   

   ?

   A preprocess function is pointless here.

3. +++ b/core/themes/bartik/templates/status-messages.html.twig
   @@ -0,0 +1,53 @@
   +      {% for type, messages in message_list %}
   +        <div class="messages messages--{{ type }}" role="contentinfo" aria-label="{{ status_headings[type] }}">
   +          {% if type == 'error' %}
   +            <div role="alert">
   +          {% endif %}
   +            {% if status_headings[type] %}
   +              <h2 class="visually-hidden">{{ status_headings[type] }}</h2>
   +            {% endif %}
   +            {% if messages|length > 1 %}
   +              <ul class="messages__list">
   +                {% for message in messages %}
   +                  <li class="messages__item">{{ message }}</li>
   +                {% endfor %}
   +              </ul>
   +            {% else %}
   +              {{ messages.0 }}
   +            {% endif %}
   +          {% if type == 'error' %}
   +            </div>
   +          {% endif %}
   +        </div>
   +      {% endfor %}
   

   All this is copy/pasted directly from classy's template. So why not just inherit that? All Bartik wants is that wrapper.

   Another reason: Classy's template has been improved since, and this template was not updated accordingly. By using inheritance, we prevent the need to keep them in sync manually.

And a tiny additional bit for SimplePageVariant, to ensure messages are shown above.

I would love to RTBC this, but looking closer again and re-reviewing all code, there is two things:

a) 'display' needs to be brought back as parameter, else this is an API change of sorts. (can just put in context and pass through to #post_render_cache, so should be fine - ).

+ $plugin_id => $context,

but this will need to change to a key thats dependent on what is displayed else there will be only one item in post_render_cache as far as I have understood post_render_cache. (Might be wrong, but I know I had trouble with that notion when working my RenderCachePlaceholder class).

b) Template changes should quickly be verified by Cottser and the bartik changes by emmamaria. While the markup keeps the same the location changes and I know themers might care about this.

c) It might be nice (though probably - maybe not - out of scope) to add a configurable parameter to the block to select 'Status', Errors, etc.

That would actually be pretty cool to be able to place multiple blocks for that in different regions and probably a reason why a) display existed in the first place in the API.

And might be a good test case for a).

a)

Great catch.

+++ b/core/includes/theme.inc
@@ -523,25 +523,6 @@ function template_preprocess_datetime_wrapper(&$variables) {
- * @param array $variables
- *   An associative array containing:
- *   - display: (optional) May have a value of 'status' or 'error' when only
- *     displaying messages of that specific type.
- */
-function template_preprocess_status_messages(&$variables) {
-  $variables['message_list'] = drupal_get_messages($variables['display']);

So it's about bringing this back. It's not used by anything in core, but removing it would be an API change.

Done!

b) Agreed. Markup is identical before & after, but still, we want this to be verified.

c) That's indeed out of scope. Also a bad idea UX-wise — that should be available only to developers & themers. If we give users that choice, it's too easy to end up with a site that is broken. AFAIK it was always intended for developers & themers only. That's why it's only in the API and nowhere to be seen in the UI (in D8 nor D7/6/5).

As requested here is a general review of the Twig/template action. Only minor points.

1. +++ b/core/includes/theme.inc
   @@ -1789,7 +1762,8 @@ function drupal_common_theme() {
        'status_messages' => array(
   -      'variables' => array('display' => NULL),
   +      'variables' => ['status_headings' => [], 'message_list' => NULL],
   +      'template' => 'status-messages',
        ),
   

   +++ b/core/modules/system/system.module
   @@ -168,6 +168,10 @@ function system_theme() {
   +    'block__system_messages_block' => array(
   +      'base hook' => 'block',
   +      'template' => 'block--system-messages-block',
   +    ),
   

   These 'template' lines should (hopefully) no longer be needed :D https://www.drupal.org/node/2231673

2. +++ b/core/themes/bartik/templates/status-messages.html.twig
   @@ -0,0 +1,35 @@
   +{% extends "@classy/system/status-messages.html.twig" %}
   ...
   +{% block messages %}
   ...
   +{% endblock messages %}
   
   +++ b/core/themes/classy/templates/system/status-messages.html.twig
   @@ -25,6 +25,7 @@
   +{% block messages %}
   
   @@ -55,3 +56,4 @@
   +{% endblock messages %}
   

   General question: why extend Classy's status messages template when everything is being replaced?

3. +++ b/core/themes/bartik/templates/status-messages.html.twig
   @@ -0,0 +1,35 @@
   + * @ingroup themeable
   

   Anything but the default template from a module should not have @ingroup themeable - https://www.drupal.org/node/1823416#docblock

   Unfortunately I just found that all or nearly all Classy templates have this same issue :/

4. +++ b/core/themes/classy/templates/system/page.html.twig
   @@ -31,7 +31,6 @@
   - * - messages: Status and error messages. Should be displayed prominently.
   

   In Classy's page.html.twig the 'messages' variable documentation is removed but page.messages is not added to the regions documentation like in the other templates.

Edited to add better context and expand one of the code snippets.

1. Nice, done! :)
2. Bartik merely wants to add an additional wrapper around the markup that Classy generates. i.e.:

   1. +++ b/core/themes/bartik/templates/page.html.twig
      @@ -109,11 +109,7 @@
      -    {% if messages %}
      -      <div id="messages">
      -        <div class="section clearfix">{{ messages }}</div>
      -      </div>
      -    {% endif %}
      

      We used to have this…

   2. +++ b/core/themes/bartik/templates/status-messages.html.twig
      @@ -0,0 +1,35 @@
      +{% block messages %}
      +  {% if message_list is not empty %}
      +    <div id="messages">
      +      <div class="section clearfix">
      +        {{ parent() }}
      +      </div>
      +    </div>
      +  {% endif %}
      +{% endblock messages %}
      

      … and now it lives here.

3. That makes sense, since this template was originally a copy of Classy. Now removed.
4. Nice catch, fixed.

@Wim Leers - I totally missed the use of parent() - nicely done! I would be so happy to see that actually being used in core, or at least a core theme :)

#101: Glad you like it :) core/themes/bartik/templates/block--search-form-block.html.twig already uses it though, so this won't be the first :)

Did a quick review, and found one small nitpick:

1. +++ b/core/lib/Drupal/Core/Render/BareHtmlPageRenderer.php
   @@ -49,6 +49,12 @@ public function renderBarePage(array $content, $title, $page_theme_property, arr
   +    // @todo In Drupal 9, add a $show_messages function parameter.
   

   Can we please have an issue for this?

So happy to see this so close to getting in. Big thanks to all who worked to make that happen!

#103: AFAIK we don't file issues for D9 @todos? At least we don't for @deprecated stuff, which implies an @todo also. It's just a note when revising this in the future; hopefully the entire "bare HTML page renderer" concept can go away in D9. Since it's an API we'd rather get rid of, it feels unnecessary to file an issue for it.

Patch does not apply due to this commit #2349559: [meta] Discuss the organization of subfolders in Classy

Rerolled, I will now take a look at Bartik with this patch.

I think this line will be the culprit:

+{% extends "@classy/system/status-messages.html.twig" %}

it should be:

{% extends "status-messages.html.twig" %} per https://www.drupal.org/node/2381103

so that it automatically extends from bartik ...

But hmm, that is the same file, so won't work, so need to specify the right path where system-messages.html.twig now lives after classy move.

--

Do we need a:

+{% extends "base-theme:status-messages.html.twig" %}

or such directive in a follow-up?

The folder moved because of the Classy reorganization. This template file is extending a template with the same name, so the namespace and path to file has to be declared explicitly.

From a visual review of the patch, I noticed the messages block are in quite different places to where they originally were.

Admin pages

Messages printing at bottom of admin pages - in Seven and Bartik - this is a bug.

Seven - before patch

Seven - after patch

Bartik - before patch

Bartik - after patch

Frontend

Messages have moved from being just above content to below the title - which looks really different in Bartik

Bartik - before patch

Bartik - after patch

Classy - before patch

Classy - after patch

Re #110 don't want to derail but #2387069: {% extends "foo.html.twig" %} in Twig templates does not respect theme inheritance would be a good place to potentially add that follow-up mentioned as a related issue ;)

#113 is that on a new install, they look like they're not in the right regions?

@Jeff Burnz I thought it was a fresh install, on reflection after that review at midnight I may try again.

No I was right about the admin messages! On a fresh install the messages still print underneath the admin section as per #113.
 

%nbsp;
I dropped the database, cleared files and reinstalled Drupal with the patch applied.

Frontend messages work correctly, apologies for reporting them.

Admin pages:

• Seven didn't have a "Messages" block yet, so it was using the fallback.
• The Bartik problem you showed must be due to something being changed in the block layout because that cannot be reproduced with a fresh install.

UPDATE: emmamaria has now indicated there's indeed no problem on the frontend.

Frontend:

• Again, the Bartik problem you showed must be due to something being changed in the block layout because that cannot be reproduced with a fresh install.
• Classy isn't installed by default so what you're seeing here is the fallback behavior. This is expected. By placing a "Messages" block, the messages would appear in the original location again. Note that once #507488: Convert page elements (local tasks, actions) into blocks is solved, the "before" would again become reality, but because the title and local tasks are currently hardcoded in the page template, there is no way to make the default behavior match the "before" picture right now. (Unless we make SimplePageVariant and BlockPageVariant render the fallback messages in the messages region, but that should be avoided, since not every theme will have a messages region; we only require a content region.

The interdiff for #119 was correct, the patch was flawed (some other file snuck in).

> Seven didn't have a "Messages" block yet, so it was using the fallback.

But shouldn't that fallback end up in page.messages as well and hence be at the right position?

#122: No, see the deleted last bullet in #119.

Thanks for the explanation! :)

The patch is still not happy.
Tried applying and get this mid applying...

Checking patch core/modules/system/src/Plugin/Block/SystemMessagesBlock.php...
Checking patch core/modules/system/src/Tests/Cache/PageCacheTagsIntegrationTest.php...
error: while searching for:
      'config:block.block.bartik_powered',
      'config:block.block.bartik_main_menu',
      'config:block.block.bartik_account_menu',
      'block_plugin:system_breadcrumb_block',
      'block_plugin:system_main_block',
      'block_plugin:system_menu_block__account',

error: patch failed: core/modules/system/src/Tests/Cache/PageCacheTagsIntegrationTest.php:94
error: core/modules/system/src/Tests/Cache/PageCacheTagsIntegrationTest.php: patch does not apply
Checking patch core/modules/system/src/Tests/Theme/MessageTest.php...

#2449069: Remove default block_plugin cache tags, because they're useless caused that conflict. Straight reroll.

Issue summary is not up to date with current patch approach and we do need beta evaluation. We also need a change record for #type => 'status_messages'

1. +++ b/core/lib/Drupal/Core/Block/MessagesBlockPluginInterface.php
   @@ -0,0 +1,19 @@
   +interface MessagesBlockPluginInterface extends BlockPluginInterface { }
   

   I don't see any reason for adding this.
   Oh ok I see it's used in BlockPageVariant now $messages_block_displayed = TRUE; can we add docs related to that here some @see and explanation would be great.

2. +++ b/core/lib/Drupal/Core/Render/Element/StatusMessages.php
   @@ -0,0 +1,135 @@
   +class StatusMessages extends RenderElement {
   

   Can we add a @code block in the docs with some examples here?

3. +++ b/core/lib/Drupal/Core/Render/Element/StatusMessages.php
   @@ -0,0 +1,135 @@
   +      '#message_list' => drupal_get_messages($context['display']),
   

   Can we add a @todo here #2278383: Create an injectible service for drupal_set_message()?

4. +++ b/core/modules/file/src/Controller/FileWidgetAjaxController.php
   @@ -38,8 +38,8 @@ public function upload(Request $request) {
   +      return $response->addCommand(new ReplaceCommand(NULL, drupal_render_root($status_messages)));
   
   @@ -53,8 +53,8 @@ public function upload(Request $request) {
   +      return $response->addCommand(new ReplaceCommand(NULL, drupal_render_root($status_messages)));
   
   @@ -76,9 +76,9 @@ public function upload(Request $request) {
   +    $form['#prefix'] .= drupal_render_root($status_messages);
   +    $output = drupal_render_root($form);
   
   +++ b/core/modules/quickedit/src/QuickEditController.php
   @@ -204,7 +204,7 @@ public function fieldForm(EntityInterface $entity, $field_name, $langcode, $view
   +      $output = drupal_render_root($form);
   
   @@ -214,9 +214,9 @@ public function fieldForm(EntityInterface $entity, $field_name, $langcode, $view
   +        $response->addCommand(new FieldFormValidationErrorsCommand(drupal_render_root($status_messages)));
   
   +++ b/core/modules/system/form.api.php
   @@ -153,8 +153,8 @@ function callback_batch_finished($success, $results, $operations) {
   +  $command = new \Drupal\Core\Ajax\PrependCommand('#block-system-main .content', drupal_render_root($status_messages));
   
   +++ b/core/modules/views_ui/src/Form/Ajax/ViewsFormBase.php
   @@ -203,7 +203,7 @@ protected function ajaxFormWrapper($form_class, FormStateInterface &$form_state)
   +    $output = drupal_render_root($form);
   
   @@ -220,8 +220,8 @@ protected function ajaxFormWrapper($form_class, FormStateInterface &$form_state)
   +      if ($messages = drupal_render_root($status_messages)) {
   

   Let's not use deprecated functions.

https://www.drupal.org/node/2446615 is the CR, leaving the rest to Wim.

Patch applies cleanly.

Messages print in the same places as core right now with the patch applied - admin and frontend, which is great.

Review of the Bartik theme.

I have a few nitpicks

1. The location of the new messages region on the block layout page.
   In theory it should be below the menus and above Featured top, this is where it prints on the frontend.
    
   

 

2. I really do not like an empty region
   printing for messages when there are no messages being currently displayed.
    
   <div class="region region-messages"></div>
    
   This can cause visual problems for Bartik. For example if messages are put in the Featured Top region, the region has a background colour and padding. It will print those styles no matter if there are messages or not. As we are making messages a block and flexible in placement, we should make sure the block doesn't break anything visually. See below...
    
   
    
   Can this be changed?

    

3. One last thing, does the region have to be called Messages? We shouldn't create and name regions based on the content we happen to be putting in them at the time. I know Bartik currently does that a lot but that is for purposeful visual design. This is a bit different.