From 81e51d9f27abf58da5be431b0a4590342aa1880c Mon Sep 17 00:00:00 2001 From: Alex Pott Date: Fri, 29 Jul 2016 13:17:46 +0100 Subject: [PATCH] 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 | 29 +++++- core/modules/big_pipe/src/Render/BigPipe.php | 42 ++++++-- .../big_pipe/src/Tests/BigPipeTest.php | 18 +++- .../BigPipeRegressionTest.php | 57 +++++++++-- .../render_placeholder_message_test.info.yml | 6 ++ ...ender_placeholder_message_test.routing.yml | 24 +++++ ...RenderPlaceholderMessageTestController.php | 99 +++++++++++++++++++ .../Render/PlaceholderMessageTest.php | 58 +++++++++++ 8 files changed, 314 insertions(+), 19 deletions(-) create mode 100644 core/modules/system/tests/modules/render_placeholder_message_test/render_placeholder_message_test.info.yml create mode 100644 core/modules/system/tests/modules/render_placeholder_message_test/render_placeholder_message_test.routing.yml create mode 100644 core/modules/system/tests/modules/render_placeholder_message_test/src/RenderPlaceholderMessageTestController.php create mode 100644 core/modules/system/tests/src/Functional/Render/PlaceholderMessageTest.php diff --git a/core/lib/Drupal/Core/Render/Renderer.php b/core/lib/Drupal/Core/Render/Renderer.php index 09a1bf1032..0bf1d63195 100644 --- a/core/lib/Drupal/Core/Render/Renderer.php +++ b/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; diff --git a/core/modules/big_pipe/src/Render/BigPipe.php b/core/modules/big_pipe/src/Render/BigPipe.php index 91452085b0..1894aa96aa 100644 --- a/core/modules/big_pipe/src/Render/BigPipe.php +++ b/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('
', $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; } } diff --git a/core/modules/big_pipe/src/Tests/BigPipeTest.php b/core/modules/big_pipe/src/Tests/BigPipeTest.php index 370b815dfb..1a1bc3d3c2 100644 --- a/core/modules/big_pipe/src/Tests/BigPipeTest.php +++ b/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('', '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('
", 'The BigPipe stop signal is present just before the closing and tags.'); + $this->assertSession() + ->responseContains(BigPipe::STOP_SIGNAL . "\n\n\n", 'The BigPipe stop signal is present just before the closing and tags.'); $js_code_until_closing_body_tag = substr(BigPipeRegressionTestController::MARKER_2678662, 0, strpos(BigPipeRegressionTestController::MARKER_2678662, '')); - $this->assertSession()->responseNotContains($js_code_until_closing_body_tag . "\n" . BigPipe::START_SIGNAL, 'The BigPipe start signal does NOT start at the closing 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 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 = '
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); + } } } diff --git a/core/modules/system/tests/modules/render_placeholder_message_test/render_placeholder_message_test.info.yml b/core/modules/system/tests/modules/render_placeholder_message_test/render_placeholder_message_test.info.yml new file mode 100644 index 0000000000..532695bc0a --- /dev/null +++ b/core/modules/system/tests/modules/render_placeholder_message_test/render_placeholder_message_test.info.yml @@ -0,0 +1,6 @@ +name: 'Placeholder setting a message test' +type: module +description: 'Support module for PlaceholderMessageTest.' +package: Testing +version: VERSION +core: 8.x diff --git a/core/modules/system/tests/modules/render_placeholder_message_test/render_placeholder_message_test.routing.yml b/core/modules/system/tests/modules/render_placeholder_message_test/render_placeholder_message_test.routing.yml new file mode 100644 index 0000000000..6734cf33e9 --- /dev/null +++ b/core/modules/system/tests/modules/render_placeholder_message_test/render_placeholder_message_test.routing.yml @@ -0,0 +1,24 @@ +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' diff --git a/core/modules/system/tests/modules/render_placeholder_message_test/src/RenderPlaceholderMessageTestController.php b/core/modules/system/tests/modules/render_placeholder_message_test/src/RenderPlaceholderMessageTestController.php new file mode 100644 index 0000000000..f02f0b63f4 --- /dev/null +++ b/core/modules/system/tests/modules/render_placeholder_message_test/src/RenderPlaceholderMessageTestController.php @@ -0,0 +1,99 @@ +build([ + '', + '', + '', + ]); + } + + /** + * @return array + */ + public function messagesPlaceholderMiddle() { + return $this->build([ + '', + '', + '', + ]); + } + + /** + * @return array + */ + public function messagesPlaceholderLast() { + return $this->build([ + '', + '', + '', + ]); + } + + /** + * @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' => '

Message: ' . $message . '

']; + } + +} diff --git a/core/modules/system/tests/src/Functional/Render/PlaceholderMessageTest.php b/core/modules/system/tests/src/Functional/Render/PlaceholderMessageTest.php new file mode 100644 index 0000000000..185ca4985a --- /dev/null +++ b/core/modules/system/tests/src/Functional/Render/PlaceholderMessageTest.php @@ -0,0 +1,58 @@ +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); + } + } + +} -- GitLab