Form validation errors, status messages on form submission are shown after page refresh when form is rendered in block

HI Renuka,

are you coding something like below :

$form = \Drupal::formBuilder()->getForm(Drupal\user\Form\UserLoginForm::class);

and later simply assigning to variable $variables['foo_form'] = $form;

+1

I'm also having this issue on my 8.1.8 site. I have also checked on a vanilla install of Drupal Core (8.1.9-dev) by placing the User login form block in the left sidebar and deliberately entering the username/password incorrectly. The status message does not display until the page is refreshed.

Is it possible that the messages for the page are being collated and rendered before the validation error message/s from forms in blocks have been set? Any advice, patches, or suggestions for where/how to patch would be greatly appreciated.

After discussing this issue with other, more prominent contributors to Drupal, I have re-written this issue description using the recommended issue template.

I have also bumped the priority of this issue to 'Major', because this bug is present in Drupal core, and it interferes with normal site visitors' use of the site. There is no current workaround that I am aware of.

Reproduced on simplytest.me on 8.3.x, too.

I think actually this is a critical UX regression of not seeing a message for the form.

Steps to reproduce again in bullet form:

• - Login as admin
• - Place user login block in first sidebar
• - Logout
• - Login with invalid credentials
• - No message shown

Ugh, this sounds like a bug in the render system, potentially related to placeholders.

By the way, great catch! Thanks for reporting this!

I stay by it being a critical regression and potentially a release blocker.

It amazingly still works in 8.0.x, but fails in 8.1.x and 8.3.x. I did not test 8.2.x.

Yes, see test history on https://www.drupal.org/pift-ci-job/450541, it apparently broke between april and mai. We're currently trying to figure out which change that was exactly.

I suspect this is an unintended (and unwanted) side-effect of either:

1. #2702609: Disable placeholdering (and BigPipe) on unsafe requests to help find forms as fast as possible (to allow EnforcedResponses to work)
2. #2712935: Messages are not rendered last, meaning those messages set within placeholders only appear on the next request

I suspect it is:

#2702609: Disable placeholdering (and BigPipe) on unsafe requests to help find forms as fast as possible (to allow EnforcedResponses to work)

because we disable placeholdering, it means that the messages block might be already displayed when the other block is executed.

I think we must only disable auto-placeholdering and/or use a real placeholder for the messages block (e.g. #attached placeholders) so it gets replaced as late as possible in the page request in any case.

Confirmed, if I place the messages block in the footer, the messages do show up.

it means that the messages block might be already displayed when the other block is executed.

… but didn't #2712935: Messages are not rendered last, meaning those messages set within placeholders only appear on the next request fix exactly that?

Can you explain how this somehow still renders messages before rendering other blocks? Because upon re-reading the #2712935: Messages are not rendered last, meaning those messages set within placeholders only appear on the next request patch, that should be impossible AFAICT.

diff --git a/core/lib/Drupal/Core/Render/Element/StatusMessages.php b/core/lib/Drupal/Core/Render/Element/StatusMessages.php
index f026aa5..345fb8b 100644
--- a/core/lib/Drupal/Core/Render/Element/StatusMessages.php
+++ b/core/lib/Drupal/Core/Render/Element/StatusMessages.php
@@ -45,12 +45,13 @@ public function getInfo() {
    *   The updated renderable array containing the placeholder.
    */
   public static function generatePlaceholder(array $element) {
-    $element['messages_placeholder'] = [
+    $element = [
       '#lazy_builder' => [get_class() . '::renderMessages', [$element['#display']]],
-      '#create_placeholder' => TRUE,
     ];
 
-    return $element;
+    // Directly create a placeholder as we need this to be placeholder
+    // regardless if this is a POST or GET request.
+    return \Drupal::service('render_placeholder_generator')->createPlaceholder($element);
   }
 
   /**

Untested, but that should be all that is needed to fix this once and for all.

Messages are just very special.

#14:

The reason is that we disable _all_ placeholdering for POST requests, so the other fix with the order never even is run.

So suddenly the order of the blocks matter again:

    // If instructed to create a placeholder, and a #lazy_builder callback is
    // present (without such a callback, it would be impossible to replace the
    // placeholder), replace the current element with a placeholder.
    // @todo remove the isMethodSafe() check when
    //       https://www.drupal.org/node/2367555 lands.
    if (isset($elements['#create_placeholder']) && $elements['#create_placeholder'] === TRUE && $this->requestStack->getCurrentRequest()->isMethodSafe()) {
      if (!isset($elements['#lazy_builder'])) {
        throw new \LogicException('When #create_placeholder is set, a #lazy_builder callback must be present as well.');
      }
      $elements = $this->placeholderGenerator->createPlaceholder($elements);
    }

is the relevant snippet.

Maybe we should also just do the opt-out of placeholdering for POST requests for non-explicit placeholders (e.g. auto-placeholders) instead of for all ...

Gahhhhhhh. An edge case of an edge case, all because of EnforcedResponses, which we had to do this for in #2702609: Disable placeholdering (and BigPipe) on unsafe requests to help find forms as fast as possible (to allow EnforcedResponses to work).

Thanks everyone for escalating this so quickly!

I have attached a patch that uses Fabianx's suggestion. I had to allow render_placeholder_generator to be a public service for this to work. I'm not sure what unintended consequences this may have.

The patch attached appears to fix this issue.

Thanks, @Juterpillar! :)

We definitely still need tests for this. Tests that reproduce this problem. Looking at the steps to reproduce in the issue summary, that should be easy.

Wim,

Julia has left for a 3 week break. Personally, I've not had the opportunity to play with D8 yet but this sounds like a good chance. If I can find the time I'll have a look at writing the tests for this some time this week.

Awesome, thanks a lot! I'm happy to provide reviews/tips to help you get this patch to RTBC!

I hope you're not too mad about me stealing away the issue :)

Here are tests using the user login block, where the existing user login block tests are. Not the perfect place to test a regression in the render system, we could also move it somewhere else, but I personally could live with them being there ;)

I've also verified with the failing simplenews tests, they pass again now with the patch applied.

I hope you're not too mad about me stealing away the issue :)

I don't think anyone is :) Thanks for jumping in!

So, basically… this patch is special-casing the messages placeholder even more. It's overriding the default behavior at:

    // If instructed to create a placeholder, and a #lazy_builder callback is
    // present (without such a callback, it would be impossible to replace the
    // placeholder), replace the current element with a placeholder.
    // @todo remove the isMethodSafe() check when
    //       https://www.drupal.org/node/2367555 lands.
    if (isset($elements['#create_placeholder']) && $elements['#create_placeholder'] === TRUE && $this->requestStack->getCurrentRequest()->isMethodSafe()) {
      …
      $elements = $this->placeholderGenerator->createPlaceholder($elements);
    }

… and is instead always calling that service. It's essentially omitting the final condition in the if-test.

In other words, once #2367555: Deprecate EnforcedResponseException support lands, this work-around will also become unnecessary and should be reverted. Hence we should duplicate that same @todo to the changes we're making in StatusMessages and to the fact that we're making a private service public.

+1 to adding a @todo

The current issue title of that issue implies to me that we can not actually remove this yet, as it just says to deprecate it, we'd still have to support it until it is actually removed? Also, making a service public is fine, but making it private again could break some other code so it would be an API change on its own.

Anyway, added the @todo's, if it doesn't happen in that issue the in the follow-up to actually remove that. And we can decide there about the service, on purpose used "Consider".

RTBC - Nice work!

+++ b/core/lib/Drupal/Core/Render/Element/StatusMessages.php
@@ -45,12 +45,15 @@ public function getInfo() {
+    // Directly create a placeholder as we need this to be placeholder

s/to be placeholder/to be placeholdered/

- fixed on commit to 8.3.x/8.2.x, thanks!

#29: thanks for adding that. Even if it doesn't get changed during the 8.x cycle, it should definitely be fixed in 9.x. Having a @todo there is just a valuable reminder, because this sort of change would easily be forgotten otherwise.

Great #29 is working perfectly. I was facing this issue from a long time.

Many thanks !!