Issue #2712935 by Wim Leers, Fabianx, catch, alexpott: Messages are not...

Issue #2712935 by Wim Leers, Fabianx, catch, alexpott: Messages are not rendered last, meaning those messages set within placeholders only appear on the next request

core/lib/Drupal/Core/Render/Renderer.php

@@ -636,8 +636,33 @@ protected function replacePlaceholders(array &$elements) {
       return FALSE;
     }
 
-    foreach (array_keys($elements['#attached']['placeholders']) as $placeholder) {
-      $elements = $this->renderPlaceholder($placeholder, $elements);
+    // The 'status messages' placeholder needs to be special cased, because it
+    // depends on global state that can be modified when other placeholders are
+    // being rendered: any code can add messages to render.
+    // This violates the principle that each lazy builder must be able to render
+    // itself in isolation, and therefore in any order. However, we cannot
+    // change the way drupal_set_message() works in the Drupal 8 cycle. So we
+    // have to accommodate its special needs.
+    // Allowing placeholders to be rendered in a particular order (in this case:
+    // last) would violate this isolation principle. Thus a monopoly is granted
+    // to this one special case, with this hard-coded solution.
+    // @see \Drupal\Core\Render\Element\StatusMessages
+    // @see https://www.drupal.org/node/2712935#comment-11368923
+
+    // First render all placeholders except 'status messages' placeholders.
+    $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;
+      }
+      else {
+        $elements = $this->renderPlaceholder($placeholder, $elements);
+      }
+    }
+
+    // Then render 'status messages' placeholders.
+    foreach ($message_placeholders as $message_placeholder) {
+      $elements = $this->renderPlaceholder($message_placeholder, $elements);
     }
 
     return TRUE;

core/modules/big_pipe/src/Render/BigPipe.php

@@ -134,7 +134,7 @@ public function sendContent($content, array $attachments) {
     $pre_body = implode('', $parts);
 
     $this->sendPreBody($pre_body, $nojs_placeholders, $cumulative_assets);
-    $this->sendPlaceholders($placeholders, $this->getPlaceholderOrder($pre_body), $cumulative_assets);
+    $this->sendPlaceholders($placeholders, $this->getPlaceholderOrder($pre_body, $placeholders), $cumulative_assets);
     $this->sendPostBody($post_body);
 
     // Close the session again.
@@ -534,6 +534,9 @@ protected function renderPlaceholder($placeholder, array $placeholder_render_arr
    *
    * @param string $html
    *   HTML markup.
+   * @param array $placeholders
+   *   Associative array; the BigPipe placeholders. Keys are the BigPipe
+   *   placeholder IDs.
    *
    * @return array
    *   Indexed array; the order in which the BigPipe placeholders must be sent.
@@ -541,18 +544,45 @@ protected function renderPlaceholder($placeholder, array $placeholder_render_arr
    *   placeholders are kept: if the same placeholder occurs multiple times, we
    *   only keep the first occurrence.
    */
-  protected function getPlaceholderOrder($html) {
+  protected function getPlaceholderOrder($html, $placeholders) {
     $fragments = explode('<div data-big-pipe-placeholder-id="', $html);
     array_shift($fragments);
-    $order = [];
+    $placeholder_ids = [];
 
     foreach ($fragments as $fragment) {
       $t = explode('"></div>', $fragment, 2);
-      $placeholder = $t[0];
-      $order[] = $placeholder;
+      $placeholder_id = $t[0];
+      $placeholder_ids[] = $placeholder_id;
+    }
+    $placeholder_ids = array_unique($placeholder_ids);
+
+    // The 'status messages' placeholder needs to be special cased, because it
+    // depends on global state that can be modified when other placeholders are
+    // being rendered: any code can add messages to render.
+    // This violates the principle that each lazy builder must be able to render
+    // itself in isolation, and therefore in any order. However, we cannot
+    // change the way drupal_set_message() works in the Drupal 8 cycle. So we
+    // have to accommodate its special needs.
+    // Allowing placeholders to be rendered in a particular order (in this case:
+    // last) would violate this isolation principle. Thus a monopoly is granted
+    // to this one special case, with this hard-coded solution.
+    // @see \Drupal\Core\Render\Element\StatusMessages
+    // @see \Drupal\Core\Render\Renderer::replacePlaceholders()
+    // @see https://www.drupal.org/node/2712935#comment-11368923
+    $message_placeholder_ids = [];
+    foreach ($placeholders as $placeholder_id => $placeholder_element) {
+      if (isset($placeholder_element['#lazy_builder']) && $placeholder_element['#lazy_builder'][0] === 'Drupal\Core\Render\Element\StatusMessages::renderMessages') {
+        $message_placeholder_ids[] = $placeholder_id;
+      }
     }
 
-    return array_unique($order);
+    // Return placeholder IDs in DOM order, but with the 'status messages'
+    // placeholders at the end, if they are present.
+    $ordered_placeholder_ids = array_merge(
+      array_diff($placeholder_ids, $message_placeholder_ids),
+      array_intersect($placeholder_ids, $message_placeholder_ids)
+    );
+    return $ordered_placeholder_ids;
   }
 
 }

core/modules/big_pipe/src/Tests/BigPipeTest.php

@@ -170,6 +170,11 @@ public function testBigPipe() {
       $cases['edge_case__html_non_lazy_builder']->bigPipePlaceholderId => Json::encode($cases['edge_case__html_non_lazy_builder']->embeddedAjaxResponseCommands),
       $cases['exception__lazy_builder']->bigPipePlaceholderId          => NULL,
       $cases['exception__embedded_response']->bigPipePlaceholderId     => NULL,
+    ], [
+      0 => $cases['edge_case__html_non_lazy_builder']->bigPipePlaceholderId,
+      // The 'html' case contains the 'status messages' placeholder, which is
+      // always rendered last.
+      1 => $cases['html']->bigPipePlaceholderId,
     ]);
 
     $this->assertRaw('</body>', 'Closing body tag present.');
@@ -336,8 +341,11 @@ protected function assertBigPipeNoJsPlaceholders(array $expected_big_pipe_nojs_p
    *
    * @param array $expected_big_pipe_placeholders
    *   Keys: BigPipe placeholder IDs. Values: expected AJAX response.
+   * @param array $expected_big_pipe_placeholder_stream_order
+   *   Keys: BigPipe placeholder IDs. Values: expected AJAX response. Keys are
+   *   defined in the order that they are expected to be rendered & streamed.
    */
-  protected function assertBigPipePlaceholders(array $expected_big_pipe_placeholders) {
+  protected function assertBigPipePlaceholders(array $expected_big_pipe_placeholders, array $expected_big_pipe_placeholder_stream_order) {
     $this->pass('Verifying BigPipe placeholders & replacements…', 'Debug');
     $this->assertSetsEqual(array_keys($expected_big_pipe_placeholders), explode(' ', $this->drupalGetHeader('BigPipe-Test-Placeholders')));
     $placeholder_positions = [];
@@ -365,8 +373,12 @@ protected function assertBigPipePlaceholders(array $expected_big_pipe_placeholde
     ksort($placeholder_positions, SORT_NUMERIC);
     $this->assertEqual(array_keys($expected_big_pipe_placeholders), array_values($placeholder_positions));
     $this->assertEqual(count($expected_big_pipe_placeholders), preg_match_all('/' . preg_quote('<div data-big-pipe-placeholder-id="', '/') . '/', $this->getRawContent()));
-    $expected_big_pipe_placeholders_with_replacements = array_filter($expected_big_pipe_placeholders);
-    $this->assertEqual(array_keys($expected_big_pipe_placeholders_with_replacements), array_values($placeholder_replacement_positions));
+    $expected_big_pipe_placeholders_with_replacements = [];
+    foreach ($expected_big_pipe_placeholder_stream_order as $big_pipe_placeholder_id) {
+      $expected_big_pipe_placeholders_with_replacements[$big_pipe_placeholder_id] = $expected_big_pipe_placeholders[$big_pipe_placeholder_id];
+    }
+    $this->assertEqual($expected_big_pipe_placeholders_with_replacements, array_filter($expected_big_pipe_placeholders));
+    $this->assertSetsEqual(array_keys($expected_big_pipe_placeholders_with_replacements), array_values($placeholder_replacement_positions));
     $this->assertEqual(count($expected_big_pipe_placeholders_with_replacements), preg_match_all('/' . preg_quote('<script type="application/vnd.drupal-ajax" data-big-pipe-replacement-for-placeholder-with-id="', '/') . '/', $this->getRawContent()));
 
     $this->pass('Verifying BigPipe start/stop signals…', 'Debug');

core/modules/big_pipe/tests/src/FunctionalJavascript/BigPipeRegressionTest.php

@@ -30,14 +30,8 @@ class BigPipeRegressionTest extends JavascriptTestBase {
    * {@inheritdoc}
    */
   public static $modules = [
-    'node',
-    'comment',
     'big_pipe',
     'big_pipe_regression_test',
-    'history',
-    'editor',
-    'ckeditor',
-    'filter',
   ];
 
   /**
@@ -57,6 +51,8 @@ public function setUp() {
    * @see https://www.drupal.org/node/2698811
    */
   public function testCommentForm_2698811() {
+    $this->assertTrue($this->container->get('module_installer')->install(['comment', 'history', 'ckeditor'], TRUE), 'Installed modules.');
+
     // Ensure an `article` node type exists.
     $this->createContentType(['type' => 'article']);
     $this->addDefaultCommentField('node', 'article');
@@ -124,6 +120,8 @@ public function testCommentForm_2698811() {
    * @see https://www.drupal.org/node/2678662
    */
   public function testMultipleClosingBodies_2678662() {
+    $this->assertTrue($this->container->get('module_installer')->install(['render_placeholder_message_test'], TRUE), 'Installed modules.');
+
     $this->drupalLogin($this->drupalCreateUser());
     $this->drupalGet(Url::fromRoute('big_pipe_regression_test.2678662'));
 
@@ -138,9 +136,52 @@ public function testMultipleClosingBodies_2678662() {
 
     // Besides verifying there is no JavaScript syntax error, also verify the
     // HTML structure.
-    $this->assertSession()->responseContains(BigPipe::STOP_SIGNAL . "\n\n\n</body></html>", 'The BigPipe stop signal is present just before the closing </body> and </html> tags.');
+    $this->assertSession()
+      ->responseContains(BigPipe::STOP_SIGNAL . "\n\n\n</body></html>", 'The BigPipe stop signal is present just before the closing </body> and </html> tags.');
     $js_code_until_closing_body_tag = substr(BigPipeRegressionTestController::MARKER_2678662, 0, strpos(BigPipeRegressionTestController::MARKER_2678662, '</body>'));
-    $this->assertSession()->responseNotContains($js_code_until_closing_body_tag . "\n" . BigPipe::START_SIGNAL, 'The BigPipe start signal does NOT start at the closing </body> tag string in an inline script.');
+    $this->assertSession()
+      ->responseNotContains($js_code_until_closing_body_tag . "\n" . BigPipe::START_SIGNAL, 'The BigPipe start signal does NOT start at the closing </body> tag string in an inline script.');
+  }
+
+  /**
+   * Ensure messages set in placeholders always appear.
+   *
+   * @see https://www.drupal.org/node/2712935
+   */
+  public function testMessages_2712935() {
+    $this->assertTrue($this->container->get('module_installer')->install(['render_placeholder_message_test'], TRUE), 'Installed modules.');
+
+    $this->drupalLogin($this->drupalCreateUser());
+    $messages_markup = '<div role="contentinfo" aria-label="Status message"';
+
+    $test_routes = [
+      // Messages placeholder rendered first.
+      'render_placeholder_message_test.first',
+      // Messages placeholder rendered after one, before another.
+      'render_placeholder_message_test.middle',
+      // Messages placeholder rendered last.
+      'render_placeholder_message_test.last',
+    ];
+
+    $assert = $this->assertSession();
+    foreach ($test_routes as $route) {
+      // Verify that we start off with zero messages queued.
+      $this->drupalGet(Url::fromRoute('render_placeholder_message_test.queued'));
+      $assert->responseNotContains($messages_markup);
+
+      // Verify the test case at this route behaves as expected.
+      $this->drupalGet(Url::fromRoute($route));
+      $assert->elementContains('css', 'p.logged-message:nth-of-type(1)', 'Message: P1');
+      $assert->elementContains('css', 'p.logged-message:nth-of-type(2)', 'Message: P2');
+      $assert->responseContains($messages_markup);
+      $assert->elementExists('css', 'div[aria-label="Status message"] ul');
+      $assert->elementContains('css', 'div[aria-label="Status message"] ul li:nth-of-type(1)', 'P1');
+      $assert->elementContains('css', 'div[aria-label="Status message"] ul li:nth-of-type(2)', 'P2');
+
+      // Verify that we end with all messages printed, hence again zero queued.
+      $this->drupalGet(Url::fromRoute('render_placeholder_message_test.queued'));
+      $assert->responseNotContains($messages_markup);
+    }
   }
 
 }

core/modules/system/tests/modules/render_placeholder_message_test/render_placeholder_message_test.info.yml

+name: 'Placeholder setting a message test'
+type: module
+description: 'Support module for PlaceholderMessageTest.'
+package: Testing
+version: VERSION
+core: 8.x

core/modules/system/tests/modules/render_placeholder_message_test/render_placeholder_message_test.routing.yml

+render_placeholder_message_test.first:
+  path: '/render_placeholder_message_test/first'
+  defaults:
+    _controller: '\Drupal\render_placeholder_message_test\RenderPlaceholderMessageTestController::messagesPlaceholderFirst'
+  requirements:
+    _access: 'TRUE'
+render_placeholder_message_test.middle:
+  path: '/render_placeholder_message_test/middle'
+  defaults:
+    _controller: '\Drupal\render_placeholder_message_test\RenderPlaceholderMessageTestController::messagesPlaceholderMiddle'
+  requirements:
+    _access: 'TRUE'
+render_placeholder_message_test.last:
+  path: '/render_placeholder_message_test/last'
+  defaults:
+    _controller: '\Drupal\render_placeholder_message_test\RenderPlaceholderMessageTestController::messagesPlaceholderLast'
+  requirements:
+    _access: 'TRUE'
+render_placeholder_message_test.queued:
+  path: '/render_placeholder_message_test/queued'
+  defaults:
+    _controller: '\Drupal\render_placeholder_message_test\RenderPlaceholderMessageTestController::queuedMessages'
+  requirements:
+    _access: 'TRUE'

core/modules/system/tests/modules/render_placeholder_message_test/src/RenderPlaceholderMessageTestController.php

+<?php
+
+namespace Drupal\render_placeholder_message_test;
+use Drupal\Core\Render\RenderContext;
+use Symfony\Component\DependencyInjection\ContainerAwareInterface;
+use Symfony\Component\DependencyInjection\ContainerAwareTrait;
+
+class RenderPlaceholderMessageTestController implements ContainerAwareInterface {
+
+  use ContainerAwareTrait;
+
+  /**
+   * @return array
+   */
+  public function messagesPlaceholderFirst() {
+    return $this->build([
+      '<drupal-render-placeholder callback="Drupal\Core\Render\Element\StatusMessages::renderMessages" arguments="0" token="a8c34b5e"></drupal-render-placeholder>',
+      '<drupal-render-placeholder callback="\Drupal\render_placeholder_message_test\RenderPlaceholderMessageTestController::setAndLogMessage" arguments="0=P1" token="0546ada3"></drupal-render-placeholder>',
+      '<drupal-render-placeholder callback="\Drupal\render_placeholder_message_test\RenderPlaceholderMessageTestController::setAndLogMessage" arguments="0=P2" token="83d2df0d"></drupal-render-placeholder>',
+    ]);
+  }
+
+  /**
+   * @return array
+   */
+  public function messagesPlaceholderMiddle() {
+    return $this->build([
+      '<drupal-render-placeholder callback="\Drupal\render_placeholder_message_test\RenderPlaceholderMessageTestController::setAndLogMessage" arguments="0=P1" token="0546ada3"></drupal-render-placeholder>',
+      '<drupal-render-placeholder callback="Drupal\Core\Render\Element\StatusMessages::renderMessages" arguments="0" token="a8c34b5e"></drupal-render-placeholder>',
+      '<drupal-render-placeholder callback="\Drupal\render_placeholder_message_test\RenderPlaceholderMessageTestController::setAndLogMessage" arguments="0=P2" token="83d2df0d"></drupal-render-placeholder>',
+    ]);
+  }
+
+  /**
+   * @return array
+   */
+  public function messagesPlaceholderLast() {
+    return $this->build([
+      '<drupal-render-placeholder callback="\Drupal\render_placeholder_message_test\RenderPlaceholderMessageTestController::setAndLogMessage" arguments="0=P1" token="0546ada3"></drupal-render-placeholder>',
+      '<drupal-render-placeholder callback="\Drupal\render_placeholder_message_test\RenderPlaceholderMessageTestController::setAndLogMessage" arguments="0=P2" token="83d2df0d"></drupal-render-placeholder>',
+      '<drupal-render-placeholder callback="Drupal\Core\Render\Element\StatusMessages::renderMessages" arguments="0" token="a8c34b5e"></drupal-render-placeholder>',
+    ]);
+  }
+
+  /**
+   * @return array
+   */
+  public function queuedMessages() {
+    return ['#type' => 'status_messages'];
+  }
+
+  /**
+   * @return array
+   */
+  protected function build(array $placeholder_order) {
+    $build = [];
+    $build['messages'] = ['#type' => 'status_messages'];
+    $build['p1'] = [
+      '#lazy_builder' => ['\Drupal\render_placeholder_message_test\RenderPlaceholderMessageTestController::setAndLogMessage', ['P1']],
+      '#create_placeholder' => TRUE,
+    ];
+    $build['p2'] = [
+      '#lazy_builder' => ['\Drupal\render_placeholder_message_test\RenderPlaceholderMessageTestController::setAndLogMessage', ['P2']],
+      '#create_placeholder' => TRUE,
+    ];
+
+    /** @var \Drupal\Core\Render\RendererInterface $renderer */
+    $renderer = $this->container->get('renderer');
+    $renderer->executeInRenderContext(new RenderContext(), function () use (&$build, $renderer) {
+      return $renderer->render($build, FALSE);
+    });
+
+    $reordered = [];
+    foreach ($placeholder_order as $placeholder) {
+      $reordered[$placeholder] = $build['#attached']['placeholders'][$placeholder];
+    }
+    $build['#attached']['placeholders'] = $reordered;
+
+    return $build;
+  }
+
+  /**
+   * #lazy_builder callback; sets and prints a message.
+   *
+   * @param string $message
+   *   The message to send.
+   *
+   * @return array
+   *   A renderable array containing the message.
+   */
+  public static function setAndLogMessage($message) {
+    // Set message.
+    drupal_set_message($message);
+
+    // Print which message is expected.
+    return ['#markup' => '<p class="logged-message">Message: ' . $message . '</p>'];
+  }
+
+}

core/modules/system/tests/src/Functional/Render/PlaceholderMessageTest.php

+<?php
+
+namespace Drupal\Tests\system\Functional\Render;
+
+use Drupal\Core\Url;
+use Drupal\Tests\BrowserTestBase;
+
+/**
+ * Functional test verifying that messages set in placeholders always appear.
+ *
+ * @group Render
+ */
+class PlaceholderMessageTest extends BrowserTestBase {
+
+  /**
+   * Modules to enable.
+   *
+   * @var array
+   */
+  public static $modules = ['render_placeholder_message_test'];
+
+  /**
+   * Test rendering of message placeholder.
+   */
+  public function testMessagePlaceholder() {
+    $messages_markup = '<div role="contentinfo" aria-label="Status message"';
+
+    $test_routes = [
+      // Messages placeholder rendered first.
+      'render_placeholder_message_test.first',
+      // Messages placeholder rendered after one, before another.
+      'render_placeholder_message_test.middle',
+      // Messages placeholder rendered last.
+      'render_placeholder_message_test.last',
+    ];
+
+    $assert = $this->assertSession();
+    foreach ($test_routes as $route) {
+      // Verify that we start off with zero messages queued.
+      $this->drupalGet(Url::fromRoute('render_placeholder_message_test.queued'));
+      $assert->responseNotContains($messages_markup);
+
+      // Verify the test case at this route behaves as expected.
+      $this->drupalGet(Url::fromRoute($route));
+      $assert->elementContains('css', 'p.logged-message:nth-of-type(1)', 'Message: P1');
+      $assert->elementContains('css', 'p.logged-message:nth-of-type(2)', 'Message: P2');
+      $assert->responseContains($messages_markup);
+      $assert->elementExists('css', 'div[aria-label="Status message"] ul');
+      $assert->elementContains('css', 'div[aria-label="Status message"] ul li:nth-of-type(1)', 'P1');
+      $assert->elementContains('css', 'div[aria-label="Status message"] ul li:nth-of-type(2)', 'P2');
+
+      // Verify that we end with all messages printed, hence again zero queued.
+      $this->drupalGet(Url::fromRoute('render_placeholder_message_test.queued'));
+      $assert->responseNotContains($messages_markup);
+    }
+  }
+
+}