Messages are not rendered last, meaning those messages set within placeholders only appear on the next request

So, to reply to #14: Sure, that's smelly. I dislike it too. But the reason is simple: this is a streamed response, with calculations happening during the streaming. Which means it's too late to allow the default error handler's behavior, which uses drupal_set_message(). That doesn't work, because messages may already have been sent.

Perhaps the better solution then is for BigPipe to override the default error handler on BigPipe responses, to not use drupal_set_message(), but use what you proposed.

I don't know why I didn't think of that before, that seems the perfect solution, doesn't it?

Triaged by @Fabianx and I at DrupalCon New Orleans.

We see basically two options:

1. Hardcode a special casing of the messages placeholder… which feels awful
2. Introduce #placeholder_options in the render system, which then allows a weight to be set, which the BigPipe placeholder strategy (and other placeholder strategies) can then take into account. #placeholder_options would then allow scalar values.

Wim is not a fan of option 2 because it opens the door to lots of undocumented pieces of metadata that have big consequences, but he also doesn't see any better way.

Option 2 is what Fabianx proposed from the very beginning, but Wim was against because there were no real-world use cases yet, and now there is one. Another possible use case is the ability to specify a name/ID that would be part of the placeholder ID, to simplify debugging.

Per #2702609-42: Disable placeholdering (and BigPipe) on unsafe requests to help find forms as fast as possible (to allow EnforcedResponses to work), I think this is actually a bug in the render system.

16:55:53 <WimLeers> berdir: oh, validation message… that sounds like https://www.drupal.org/node/2712935
16:55:55 <Druplicon> https://www.drupal.org/node/2712935 => Messages are rendered much earlier with BigPipe, meaning those set within placeholders only appear on the next request #2712935: Messages are not rendered last, meaning those messages set within placeholders only appear on the next request => 6 comments, 1 IRC mention
16:55:58 <WimLeers> berdir: I'd bet that's it
16:56:29 <berdir> WimLeers: could be, will check. but it's an anonymous user that shouldn't have a session..
16:56:55 <WimLeers> berdir: so then can't you reproduce it _without_ BigPipe? I bet you can
16:57:05 <berdir> WimLeers: the module isn't even enabled in that test..
16:57:07 <WimLeers> berdir: in which case this may very well be a generic problem with placeholders.
16:57:10 <WimLeers> berdir: yep exactly
16:57:11 <WimLeers> catch: ^^
16:58:55 <WimLeers> catch: Fabianx-screen: berdir is seeing his validation error working in that the form item is marked red, but the error message is missing. My theory is that this is caused by the fact that we order placeholders in no particular order by default and in DOM order in BigPipe. But the 'messages' placeholder should always be rendered last, otherwise messages may
16:58:55 <WimLeers> be missing. So this would probably be a major bug for the render system
16:58:58 <WimLeers> alexpott: ^^
16:59:50 <alexpott> WimLeers: missing messages is a major bug
16:59:58 <WimLeers> y

I started looking into this again because I'm starting to work on this.

I'm still deeply concerned about #placeholder_options (see #5.2). It can easily become a dumping ground. From experience with weights in the module system (to adjust the hook order), asset system (to adjust asset order in D7), event subscribers (priorities to adjust event subscriber order), we already know that it's a brittle system. That's why the asset system in D8 started relying solely on dependencies. Doing something similar here doesn't make sense though: the "messages" placeholder would simply have to be run after anything else. But it wouldn't know which those other things are. So we can't do something like that.

Furthermore, assigning weights to placeholders is antithetical to one of the key design aspects of placeholders: that they are independently renderable. By providing weights, we open the door for placeholders other than messages to start relying on global state, because you (a placeholder developer) can then control the order in which placeholders are rendered. This removes control from placeholder strategies, which should be the ones in charge of determining the placeholder execution order.

Which leads me to the key point: the entire reason we have this problem is because messages rely on global state. And no, I also don't see a way how we can avoid that.

So I think that in this case, it's appropriate to:

1. not open the door for more functionality relying on global state
2. hence grant messages a monopoly on that
3. hence for now special-case messages (see #5.1), because that turns out to be the lesser of two evils
4. then when another justified need for global state arises, we can reconsider adding weights to placeholders, because then we'll have two use cases. Hopefully that day will never come.

Thoughts? Especially @catch.

Yep, #attached[messages]. I'd swear I've written about exactly that before.

That still requires it to be processed last, of course. I don't see a way around that.

When you could create a new message as part of rendering some placeholder, the attachments could contain that message. By that the messages block could be rendered early and then just appended over time. This of course would need some customiziation in ajax.js, but that sounds not totally impossible.

Yes, but this would not work when JS is turned off. Which means we'd need different rendering of messages based on whether the current request comes from a JS-enabled browser.

14:22:33 <WimLeers> catch: https://www.drupal.org/node/2712935#comment-11368923 is pretty much blocked on you
14:22:35 <Druplicon> https://www.drupal.org/node/2712935 => Messages are not rendered last, meaning those messages set within placeholders only appear on the next request #2712935: Messages are not rendered last, meaning those messages set within placeholders only appear on the next request => 11 comments, 5 IRC mentions
14:23:01 <WimLeers> catch: just need some high-level feedback for now — there's no patch yet.
14:29:43 <catch> WimLeers: fine with special casing messages, didn't like the weight idea, not sure about #attached
14:30:07 <catch> WimLeers: one handed, does that help?
14:31:52 <WimLeers> catch: yes!
14:31:56 <WimLeers> catch: #attached is long-term anyway
14:32:17 <WimLeers> catch: and other placeholders can add #attached[messages], so messages would sitll need to be rendered last anyway
14:32:25 <WimLeers> catch: so, thanks for the quick +1!
14:34:50 <catch> WimLeers: I thought I'd posted to say I didn't like the weight idea, but clearly didn't, sorry.
14:36:22 <WimLeers> catch: np!

So, I have the "go" for the direction outlined in #9.

Step 1: regression tests, for both the render system and BigPipe.

First, here's a test to prove we can reliably measure the current (wrong) behavior.

#16 should come back green, because it's asserting the wrong behavior. This then, is asserting the right behavior. And hence it should come back red.

(Note that the Needs tests issue tag must remain, because this patch only contains render system test coverage, not big_pipe.module.)

And here's the fix for render system.

Next: big_pipe.module test coverage.

Now that's interesting! In #16 + #17 + #18, the 8.2 test results always have 127 additional failures compared to the 8.1 test. That suggests Drupal 8.2 HEAD is broken. And https://www.drupal.org/node/3060/qa confirms that.

EDIT: catch reverted the sole 8.2.x commit of today, hopefully that fixed it. Queued all 3 patches for re-testing against 8.2.x.

Ah, now #16 is green on 8.2, #17 is red, #18 is green. This now perfectly matches the 8.1.x results and the expectations.

Expect the big_pipe.module test coverage tomorrow.

And here's test coverage for big_pipe.module. This should fail.

And here's the fix for BigPipe. Effectively the same as the fix for the render system. This should pass.

The #28 interdiff is wrong. This is the right one.

Fixed nits.

This change caused a fail in the existing BigPipe test coverage. Working on a fix.

And here's the fix. BigPipeTest::testBigPipe() was assuming that placeholders would be streamed in the order that they appeared in the DOM. This is a correct assumption in the current codebase, but this patch changes that. So we need a minor adjustment to the test.

This is now ready for final review.

Can't we at least allow an attribute with an "ID" or "name" for the placeholder so we can easier identify it.

But that would still mean requiring + allowing #placeholder_options. Which is problematic, for reasons outlined above.

That feels really fragile.

It's not. The accompanying assert() statements would fail before a patch could be committed to Drupal core that changes this.

The only alternative I can think of is to modify \Drupal\Core\Render\Element\StatusMessages so that it generates its own custom placeholder. But you'd then still end up with a set of hardcoded strings.

#41: true. But why even do that if we can easily hardcode it and have assertions to prove correctness. It'd be entirely pointless. This is not a premature optimization because it doesn't add complexity.

#42:

However I can extend the status codes and I can override the block to allow new status codes.

No you can't. From drupal_set_message()'s docblock:

 * @param string $type
 *   (optional) The message's type. Defaults to 'status'. These values are
 *   supported:
 *   - 'status'
 *   - 'warning'
 *   - 'error'

it is still a little BC break for assumed functionality.

It's not a BC break. I've never ever seen anybody assume they can pass custom $types to drupal_set_message().

An optional name or ID is for other reasons pretty important - especially as seen here for identifying the things.

I don't see why it's important. Even if we did that, then that #placeholder_name would have to use a different name for the four possible different arguments. So we still wouldn't be able to hardcode it to a single thing. So you'd still end hardcoding four permutations:

1. <drupal-render-placeholder name="messages-all" token="TOKEN1"></drupal-render-placeholder>
2. <drupal-render-placeholder name="messages-status" token="TOKEN2"></drupal-render-placeholder>
3. <drupal-render-placeholder name="messages-warning" token="TOKEN3"></drupal-render-placeholder>
4. <drupal-render-placeholder name="messages-error" token="TOKEN4"></drupal-render-placeholder>

The only alternative I can think of is to modify \Drupal\Core\Render\Element\StatusMessages so that it generates its own custom placeholder. But you'd then still end up with a set of hardcoded strings.

So that'd look like this:

diff --git a/core/lib/Drupal/Core/Render/Element/StatusMessages.php b/core/lib/Drupal/Core/Render/Element/StatusMessages.php
index f026aa5..545e29b 100644
--- a/core/lib/Drupal/Core/Render/Element/StatusMessages.php
+++ b/core/lib/Drupal/Core/Render/Element/StatusMessages.php
@@ -2,6 +2,8 @@
 
 namespace Drupal\Core\Render\Element;
 
+use Drupal\Core\Render\Markup;
+
 /**
  * Provides a messages element.
  *
@@ -45,9 +47,10 @@ public function getInfo() {
    *   The updated renderable array containing the placeholder.
    */
   public static function generatePlaceholder(array $element) {
-    $element['messages_placeholder'] = [
+    $placeholder_markup = '<drupal-status-messages display="' . ($element['#display'] === NULL ? 'all' : $element['#display']) . '" token="' . hash('crc32b', serialize($element)) . '""></drupal-status-messages>';
+    $element['messages_placeholder']['#markup'] = Markup::create($placeholder_markup);
+    $element['messages_placeholder']['#attached']['placeholders'][$placeholder_markup] = [
       '#lazy_builder' => [get_class() . '::renderMessages', [$element['#display']]],
-      '#create_placeholder' => TRUE,
     ];
 
     return $element;

… but that'd still mean you're hardcoding four permutations:

1. <drupal-status-messages display="all" token="TOKEN1"></drupal-status-messages>
2. <drupal-status-messages display="status" token="TOKEN2"></drupal-status-messages>
3. <drupal-status-messages display="warning" token="TOKEN3"></drupal-status-messages>
4. <drupal-status-messages display="error" token="TOKEN4"></drupal-status-messages>

An alternative would be to look into the #lazy_builder itself and match the LazyBuilder string. That too is less fragile.

True, and I could live with that if catch +1s. But it makes the code more complex, less greppable for IMO no good reason.

True, and I could live with that if catch +1s. But it makes the code more complex, less greppable for IMO no good reason.

I wrote this, but now I see I was wrong. My apologies!

The code is not really more complex, nor less greppable. The complexity is similar, if not less.

Thanks for insisting, @Fabianx! You were right :)

Good call, #49 looks much simpler indeed. Done.

Rerolling…

Straight reroll.

Bad reroll, my bad. Merged one thing wrong. Interdiff included.

Unrelated fail :/ Drupal\config\Tests\ConfigImportAllTest. Retesting. Back to RTBC.

Too. Many. Random. Fails.

Yay! :)

Unblocked #2765385: Three minor bugs in BigPipe test coverage.

And finally created #2775587: Mark BigPipe as beta-level stability (instead of alpha).