@catch at #2702001-12: Inject config factory for BigPipe (including in exception handlers):

Can we open a follow-up, or link to the existing issue if there is one, to something which would allow the message to be printed in the same request? For example could we show error messages in the footer as a final fallback?

@Wim Leers' reply at #13:

You mean a follow-up to #2678568: Ensure good UX & DX even when A) rendering of placeholder fails, B) some response event subscriber fails, not to this, right? This is solely code clean-up. What you describe sounds feasible, but I'm not sure if it's desirable. It'd mean appending HTML to the HTML just before the closing </body> tag. And that HTML may simply end up not being visible depending on the theme. I think it's good enough as it is.

And @catch's reply to that at #14:

Well I still think the config setting for error display leaking into bigpipe is a bit smelly, I'm not aware of anywhere else we have to deal with that.

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

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?

catch’s picture

Apart from error messages, won't this affect any drupal_set_message() call which might happen during rendering of a placeholder?

drupal_set_message() just adds things to session, the real issue seems to me that we have a race condition where drupal_get_messages() can be run considerably earlier on a big pipe request compared to the rest of rendering, when compared to a regular request.

With the js implementation of BigPipe it pught to be feasible to handle messages last - so that the maximum can be pulled from session.

Non-js I think we're stuck with footer rendering. #77245: Provide a common API for displaying JavaScript messages is dealing with a similar problem.

I don't think it's an issue with the error handler as such.

catch’s picture

Title: Consider allowing errors thrown while rendering BigPipe placeholders to be printed in the footer, before </body> » Messages are rendered much earlier with BigPipe, meaning those set within placeholders only appear on the next request

Re-titling to reflect what I think the problem is.

Wim Leers’s picture

Status: Active » Needs review
FileSize
771 bytes

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.

Status: Needs review » Needs work

The last submitted patch, 5: 2712935-5.patch, failed testing.

Wim Leers’s picture

Title: Messages are rendered much earlier with BigPipe, meaning those set within placeholders only appear on the next request » Messages are not rendered last, meaning those messages set within placeholders only appear on the next request
Component: big_pipe.module » render system
Category: Task » Bug report
Priority: Normal » Major

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

Assigned: Unassigned » Wim Leers
Status: Needs work » Needs review

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.

dawehner’s picture

One thing I was always wondering, why are drupal messages not something which can also be bubbled.
Let me explain what I mean with that. For example each individual block currently can attach its required libraries. Those are then attached to the HTML and are processed by the browser. Messages could be seen sort of similar. 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.

Wim Leers’s picture

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.

Wim Leers’s picture

Issue tags: +Needs tests
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.

catch’s picture

#attached['messages'] would be good, but not sure it can replace direct writing to $_SESSION - we'll still have the error handler etc. wanting to set messages and that shouldn't rely on the rendering system (or at least makes things complicated if it does). Also there's always the possibility that a message gets set after messages get rendered (via the error handler) and $_SESSION means that they least get rendered on the next request.

Fabianx’s picture

I also thought about bubbling messages via #attached message, but the problem is that we don't want to cache the messages, e.g. an error thrown by a block that is cached.

But maybe we do, if the message itself would always have max-age = 0, so it would never be cached in the first place, but then it would be auto-placeholdered for no reason except that it has a message, which is uhm ...

However that would not solve anything here as the problem is that every placeholder could still set a message.

Therefore:

The messages block must come last (also in core, probably should just change the order in the subscriber) and there is nothing we can do for no-js big pipe if any of the placeholders show a message.

dawehner’s picture

However that would not solve anything here as the problem is that every placeholder could still set a message.

Well, the question is whether we can treat messages similar to libraries, which are rendered specially by ajax.js, by aka. making it possible to load them dynamically. Messages could be similar in that sense .

Wim Leers’s picture

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

Wim Leers’s picture

#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.)

Wim Leers’s picture

The last submitted patch, 16: status_messages_placeholder-2712935-16.patch, failed testing.

The last submitted patch, 16: status_messages_placeholder-2712935-16.patch, failed testing.

The last submitted patch, 17: status_messages_placeholder-2712935-17.patch, failed testing.

The last submitted patch, 17: status_messages_placeholder-2712935-17.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 18: status_messages_placeholder-2712935-18.patch, failed testing.

Wim Leers’s picture

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.

The last submitted patch, 17: status_messages_placeholder-2712935-17.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review

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.

Wim Leers’s picture

Wim Leers’s picture

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

Wim Leers’s picture

FileSize
2.65 KB

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

Wim Leers’s picture

The last submitted patch, 27: status_messages_placeholder-2712935-27.patch, failed testing.

The last submitted patch, 28: status_messages_placeholder-2712935-28.patch, failed testing.

The last submitted patch, 27: status_messages_placeholder-2712935-27.patch, failed testing.

The last submitted patch, 28: status_messages_placeholder-2712935-28.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 30: status_messages_placeholder-2712935-30.patch, failed testing.

The last submitted patch, 30: status_messages_placeholder-2712935-30.patch, failed testing.

Wim Leers’s picture

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

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs work » Needs review
FileSize
3.5 KB
22.26 KB

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.

Fabianx’s picture

Quick drive-by review:

+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -653,7 +653,39 @@ protected function replacePlaceholders(array &$elements) {
+    $possible_message_placeholders = [
+      '<drupal-render-placeholder callback="Drupal\Core\Render\Element\StatusMessages::renderMessages" arguments="0" token="a8c34b5e"></drupal-render-placeholder>',
+      '<drupal-render-placeholder callback="Drupal\Core\Render\Element\StatusMessages::renderMessages" arguments="0=status" token="a26f536e"></drupal-render-placeholder>',
+      '<drupal-render-placeholder callback="Drupal\Core\Render\Element\StatusMessages::renderMessages" arguments="0=warning" token="67320bbd"></drupal-render-placeholder>',
+      '<drupal-render-placeholder callback="Drupal\Core\Render\Element\StatusMessages::renderMessages" arguments="0=error" token="35b549cb"></drupal-render-placeholder>',
+    ];

I don't really like that.

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

I don't think we should hardcode the combinations here.

That feels really fragile.

Wim Leers’s picture

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.

catch’s picture

I think we need profiling to justify the hard-coding, but I don't see a way around hardcoding without doing the generation.

Fabianx’s picture

Status: Needs review » Needs work

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

Like 'pink-message' or 'debug' and I have seen such custom message types in the wild.

We might choose to disallow that and the documentation clearly states that is the case (only supported: status, error, warning), but it is still a little BC break for assumed functionality.

But if we don't want to go #placeholder_options: Why not #placeholder_id or #placeholder_name?

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

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

Wim Leers’s picture

Status: Needs work » Needs review

#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.

Fabianx’s picture

Uhm, no what I meant is an ID that is independent of the placeholder name:

    $element['messages_placeholder'] = [
       '#lazy_builder' => [get_class() . '::renderMessages', [$element['#display']]],
       '#placeholder_name' => StatusMessages::PLACEHOLDER_NAME,
       '#create_placeholder' => TRUE,
     ];
  if (isset($placeholder['#placeholder_name']) && $placeholder['#placeholder_name'] === StatusMessages::PLACEHOLDER_NAME) {
    // Move last ...
  }

If not, then examining the lazy builder should be done - IMHO.

Wim Leers’s picture

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 :)

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/big_pipe/tests/src/FunctionalJavascript/BigPipeRegressionTest.php
@@ -27,13 +28,7 @@ class BigPipeRegressionTest extends JavascriptTestBase {
-    'node',
-    'comment',
...
-    'history',
-    'editor',
-    'ckeditor',
-    'filter',

nit - Those are unrelated changes, but I guess they are okay.

RTBC! Thanks for trying it out!

The last submitted patch, 45: status_messages_placeholder-2712935-45.patch, failed testing.

alexpott’s picture

+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -665,25 +665,18 @@ protected function replacePlaceholders(array &$elements) {
+    $message_placeholders = [];
+    foreach ($elements['#attached']['placeholders'] as $placeholder => $placeholder_element) {
+      if (isset($placeholder_element['#lazy_builder']) && $placeholder_element['#lazy_builder'][0] === 'Drupal\Core\Render\Element\StatusMessages::renderMessages') {
+        $message_placeholders[] = $placeholder;
+      }
+    }
...
     $placeholders = array_keys($elements['#attached']['placeholders']);
     $ordered_placeholders = array_merge(
-      array_diff($placeholders, $possible_message_placeholders),
-      array_intersect($placeholders, $possible_message_placeholders)
+      array_diff($placeholders, $message_placeholders),
+      array_intersect($placeholders, $message_placeholders)
     );

I think this could be neater as an array_filter() or you could even do the whole thing in one go and just move the message ones to the end. Like this...

    $placeholders = array_keys($elements['#attached']['placeholders']);
    foreach (placeholders as $key => $placeholder_element) {
      if (isset($placeholder_element['#lazy_builder']) && $placeholder_element['#lazy_builder'][0] === 'Drupal\Core\Render\Element\StatusMessages::renderMessages') {
        unset($placeholders($key));
        $placeholders[] = $placeholder_element;
      }
    }
alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Or maybe even do this...

    // Delay rendering message placeholders.
    $message_placeholders = [];
    foreach ($elements['#attached']['placeholders'] as $placeholder_element) {
      if (isset($placeholder_element['#lazy_builder']) && $placeholder_element['#lazy_builder'][0] === 'Drupal\Core\Render\Element\StatusMessages::renderMessages') {
        $message_placeholders[] = $placeholder_element;
      }
      else {
        $elements = $this->renderPlaceholder($placeholder_element, $elements);
      }
    }

    // Render message placeholders now everything else is rendered.
    foreach ($message_placeholders as $placeholder_element) {
      $elements = $this->renderPlaceholder($placeholder_element, $elements);
    }

For even less array messing around.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.74 KB
21.73 KB

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Fabianx’s picture

Issue tags: +Needs reroll
Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Issue tags: -Needs reroll

Rerolling…

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs work » Reviewed & tested by the community
FileSize
22.88 KB

Straight reroll.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 54: status_messages_placeholder-2712935-54.patch, failed testing.

The last submitted patch, 54: status_messages_placeholder-2712935-54.patch, failed testing.

Wim Leers’s picture

The last submitted patch, 54: status_messages_placeholder-2712935-54.patch, failed testing.

The last submitted patch, 54: status_messages_placeholder-2712935-54.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 57: status_messages_placeholder-2712935-57.patch, failed testing.

The last submitted patch, 57: status_messages_placeholder-2712935-57.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 57: status_messages_placeholder-2712935-57.patch, failed testing.

The last submitted patch, 57: status_messages_placeholder-2712935-57.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Too. Many. Random. Fails.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 81e51d9f27abf58da5be431b0a4590342aa1880c to 8.2.x and ec674c0 to 8.1.x. Thanks!

  • alexpott committed 81e51d9 on 8.2.x
    Issue #2712935 by Wim Leers, Fabianx, catch, alexpott: Messages are not...

  • alexpott committed ec674c0 on 8.1.x
    Issue #2712935 by Wim Leers, Fabianx, catch, alexpott: Messages are not...
Wim Leers’s picture

  • alexpott committed 81e51d9 on 8.3.x
    Issue #2712935 by Wim Leers, Fabianx, catch, alexpott: Messages are not...

  • alexpott committed 81e51d9 on 8.3.x
    Issue #2712935 by Wim Leers, Fabianx, catch, alexpott: Messages are not...

Status: Fixed » Closed (fixed)

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

Berdir’s picture

Somehow this doesn't seem to be fully fixed yet. We can still have test fails in simplenews related to this problem and can reproduce manually. See also #2773333: Form validation errors, status messages on form submission are shown after page refresh when form is rendered in block, which was originally opened days before this was committed but people still see this problem there as well.