Issue #3379885 by catch, Wim Leers: Use MessagesCommand in BigPipe to remove...

Issue #3379885 by catch, Wim Leers: Use MessagesCommand in BigPipe to remove special casing of the messages placeholder

core/modules/big_pipe/big_pipe.services.yml

@@ -11,7 +11,7 @@ services:
       - { name: placeholder_strategy, priority: 0 }
   big_pipe:
     class: Drupal\big_pipe\Render\BigPipe
-    arguments: ['@renderer', '@session', '@request_stack', '@http_kernel', '@event_dispatcher', '@config.factory']
+    arguments: ['@renderer', '@session', '@request_stack', '@http_kernel', '@event_dispatcher', '@config.factory', '@messenger']
   Drupal\big_pipe\Render\BigPipe: '@big_pipe'
   html_response.attachments_processor.big_pipe:
     public: false

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

@@ -5,10 +5,12 @@
 use Drupal\Component\Utility\Crypt;
 use Drupal\Component\Utility\Html;
 use Drupal\Core\Ajax\AjaxResponse;
+use Drupal\Core\Ajax\MessageCommand;
 use Drupal\Core\Ajax\ReplaceCommand;
 use Drupal\Core\Asset\AttachedAssets;
 use Drupal\Core\Asset\AttachedAssetsInterface;
 use Drupal\Core\Config\ConfigFactoryInterface;
+use Drupal\Core\Messenger\MessengerInterface;
 use Drupal\Core\Render\HtmlResponse;
 use Drupal\Core\Render\RendererInterface;
 use Symfony\Contracts\EventDispatcher\EventDispatcherInterface;
@@ -205,29 +207,17 @@ class BigPipe {
    */
   protected $configFactory;
 
-  /**
-   * Constructs a new BigPipe class.
-   *
-   * @param \Drupal\Core\Render\RendererInterface $renderer
-   *   The renderer.
-   * @param \Symfony\Component\HttpFoundation\Session\SessionInterface $session
-   *   The session.
-   * @param \Symfony\Component\HttpFoundation\RequestStack $request_stack
-   *   The request stack.
-   * @param \Symfony\Component\HttpKernel\HttpKernelInterface $http_kernel
-   *   The HTTP kernel.
-   * @param \Symfony\Contracts\EventDispatcher\EventDispatcherInterface $event_dispatcher
-   *   The event dispatcher.
-   * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
-   *   The config factory.
-   */
-  public function __construct(RendererInterface $renderer, SessionInterface $session, RequestStack $request_stack, HttpKernelInterface $http_kernel, EventDispatcherInterface $event_dispatcher, ConfigFactoryInterface $config_factory) {
+  public function __construct(RendererInterface $renderer, SessionInterface $session, RequestStack $request_stack, HttpKernelInterface $http_kernel, EventDispatcherInterface $event_dispatcher, ConfigFactoryInterface $config_factory, protected ?MessengerInterface $messenger = NULL) {
     $this->renderer = $renderer;
     $this->session = $session;
     $this->requestStack = $request_stack;
     $this->httpKernel = $http_kernel;
     $this->eventDispatcher = $event_dispatcher;
     $this->configFactory = $config_factory;
+    if (!isset($this->messenger)) {
+      @trigger_error('Calling ' . __CLASS__ . '::_construct() without the $messenger argument is deprecated in drupal:10.3.0 and it will be required in drupal:11.0.0. See https://www.drupal.org/node/3343754', E_USER_DEPRECATED);
+      $this->messenger = \Drupal::messenger();
+    }
   }
 
   /**
@@ -538,29 +528,16 @@ protected function sendPlaceholders(array $placeholders, array $placeholder_orde
 
     // Create a Fiber for each placeholder.
     $fibers = [];
-    $message_placeholder_id = NULL;
     foreach ($placeholder_order as $placeholder_id) {
       if (!isset($placeholders[$placeholder_id])) {
         continue;
       }
       $placeholder_render_array = $placeholders[$placeholder_id];
-
-      // Ensure the messages placeholder renders last, the render order of every
-      // other placeholder is safe to change.
-      // @see static::getPlaceholderOrder()
-      if (isset($placeholder_render_array['#lazy_builder']) && $placeholder_render_array['#lazy_builder'][0] === 'Drupal\Core\Render\Element\StatusMessages::renderMessages') {
-        $message_placeholder_id = $placeholder_id;
-      }
       $fibers[$placeholder_id] = new \Fiber(fn() => $this->renderPlaceholder($placeholder_id, $placeholder_render_array));
     }
     $iterations = 0;
     while (count($fibers) > 0) {
       foreach ($fibers as $placeholder_id => $fiber) {
-        // Keep skipping the messages placeholder until it's the only Fiber
-        // remaining. @todo https://www.drupal.org/project/drupal/issues/3379885
-        if (isset($message_placeholder_id) && $placeholder_id === $message_placeholder_id && count($fibers) > 1) {
-          continue;
-        }
         try {
           if (!$fiber->isStarted()) {
             $fiber->start();
@@ -594,6 +571,15 @@ protected function sendPlaceholders(array $placeholders, array $placeholder_orde
           $ajax_response->addCommand(new ReplaceCommand(sprintf('[data-big-pipe-placeholder-id="%s"]', $big_pipe_js_placeholder_id), $elements['#markup']));
           $ajax_response->setAttachments($elements['#attached']);
 
+          // Delete all messages that were generated during the rendering of this
+          // placeholder, to render them in a BigPipe-optimized way.
+          $messages = $this->messenger->deleteAll();
+          foreach ($messages as $type => $type_messages) {
+            foreach ($type_messages as $message) {
+              $ajax_response->addCommand(new MessageCommand($message, NULL, ['type' => $type], FALSE));
+            }
+          }
+
           // Push a fake request with the asset libraries loaded so far and
           // dispatch KernelEvents::RESPONSE event. This results in the
           // attachments for the AJAX response being processed by
@@ -736,8 +722,7 @@ protected function renderPlaceholder($placeholder, array $placeholder_render_arr
    *
    * @return array
    *   Indexed array; the order in which the BigPipe placeholders will start
-   *   execution. Placeholders begin execution in DOM order, except for the
-   *   messages placeholder which must always be executed last. Note that due to
+   *   execution. Placeholders begin execution in DOM order. Note that due to
    *   the Fibers implementation of BigPipe, although placeholders will start
    *   executing in DOM order, they may finish and render in any order. Values
    *   are the BigPipe placeholder IDs. Note that only unique placeholders are
@@ -751,35 +736,7 @@ protected function getPlaceholderOrder($html, $placeholders) {
     foreach ($xpath->query('//span[@data-big-pipe-placeholder-id]') as $node) {
       $placeholder_ids[] = Html::escape($node->getAttribute('data-big-pipe-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\Core\Messenger\MessengerInterface::addMessage()
-    // 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 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;
+    return array_unique($placeholder_ids);
   }
 
   /**

core/modules/big_pipe/tests/src/Functional/BigPipeTest.php

@@ -175,14 +175,12 @@ public function testBigPipe() {
       $cases['exception__lazy_builder']->bigPipePlaceholderId          => NULL,
       $cases['exception__embedded_response']->bigPipePlaceholderId     => NULL,
     ], [
-      0 => $cases['edge_case__html_non_lazy_builder']->bigPipePlaceholderId,
+      0 => $cases['html']->bigPipePlaceholderId,
+      1 => $cases['edge_case__html_non_lazy_builder']->bigPipePlaceholderId,
       // The suspended placeholder is replaced after the non-suspended
       // placeholder even though it appears first in the page.
       // @see Drupal\big_pipe\Render\BigPipe\Render::sendPlaceholders()
-      1 => $cases['edge_case__html_non_lazy_builder_suspend']->bigPipePlaceholderId,
-       // The 'html' case contains the 'status messages' placeholder, which is
-      // always rendered last.
-      2 => $cases['html']->bigPipePlaceholderId,
+      2 => $cases['edge_case__html_non_lazy_builder_suspend']->bigPipePlaceholderId,
     ]);
 
     $this->assertSession()->responseContains('</body>');

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

@@ -82,8 +82,7 @@ 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"';
-
+    $messages_markup = '<div class="messages messages--status" role="status"';
     $test_routes = [
       // Messages placeholder rendered first.
       'render_placeholder_message_test.first',
@@ -104,9 +103,9 @@ public function testMessages_2712935() {
       $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');
+      $assert->elementExists('css', 'div[aria-label="Status message"]');
+      $assert->responseContains('aria-label="Status message">P1');
+      $assert->responseContains('aria-label="Status message">P2');
 
       // Verify that we end with all messages printed, hence again zero queued.
       $this->drupalGet(Url::fromRoute('render_placeholder_message_test.queued'));

core/modules/big_pipe/tests/src/Unit/Render/FiberPlaceholderTest.php

@@ -7,6 +7,7 @@
 use Drupal\big_pipe\Render\BigPipe;
 use Drupal\big_pipe\Render\BigPipeResponse;
 use Drupal\Core\Config\ConfigFactoryInterface;
+use Drupal\Core\Messenger\MessengerInterface;
 use Drupal\Core\Render\ElementInfoManagerInterface;
 use Drupal\Core\Render\HtmlResponse;
 use Drupal\Core\Render\PlaceholderGeneratorInterface;
@@ -54,7 +55,7 @@ public function testLongPlaceholderFiberSuspendingLoop() {
           'languages:language_interface',
           'theme',
         ],
-      ]
+      ],
     );
 
     $session = $this->prophesize(SessionInterface::class);
@@ -67,6 +68,7 @@ public function testLongPlaceholderFiberSuspendingLoop() {
       $this->prophesize(HttpKernelInterface::class)->reveal(),
       $this->createMock(EventDispatcherInterface::class),
       $this->prophesize(ConfigFactoryInterface::class)->reveal(),
+      $this->prophesize(MessengerInterface::class)->reveal(),
     );
     $response = new BigPipeResponse(new HtmlResponse());
 

core/modules/big_pipe/tests/src/Unit/Render/ManyPlaceholderTest.php

@@ -7,6 +7,7 @@
 use Drupal\big_pipe\Render\BigPipe;
 use Drupal\big_pipe\Render\BigPipeResponse;
 use Drupal\Core\Config\ConfigFactoryInterface;
+use Drupal\Core\Messenger\MessengerInterface;
 use Drupal\Core\Render\HtmlResponse;
 use Drupal\Core\Render\RendererInterface;
 use Drupal\Tests\UnitTestCase;
@@ -34,7 +35,8 @@ public function testManyNoJsPlaceHolders() {
       $this->prophesize(RequestStack::class)->reveal(),
       $this->prophesize(HttpKernelInterface::class)->reveal(),
       $this->prophesize(EventDispatcherInterface::class)->reveal(),
-      $this->prophesize(ConfigFactoryInterface::class)->reveal()
+      $this->prophesize(ConfigFactoryInterface::class)->reveal(),
+      $this->prophesize(MessengerInterface::class)->reveal()
     );
     $response = new BigPipeResponse(new HtmlResponse());