summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorAlex Pott2016-07-29 12:17:46 (GMT)
committerAlex Pott2016-07-29 12:17:46 (GMT)
commit81e51d9f27abf58da5be431b0a4590342aa1880c (patch)
treea322a4bfddf16c24acf9fd0844a37ce4a02b1b7c
parenta1bc0a47696a0e0da75c95888718b2c125c1ef34 (diff)
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
-rw-r--r--core/lib/Drupal/Core/Render/Renderer.php29
-rw-r--r--core/modules/big_pipe/src/Render/BigPipe.php42
-rw-r--r--core/modules/big_pipe/src/Tests/BigPipeTest.php18
-rw-r--r--core/modules/big_pipe/tests/src/FunctionalJavascript/BigPipeRegressionTest.php57
-rw-r--r--core/modules/system/tests/modules/render_placeholder_message_test/render_placeholder_message_test.info.yml6
-rw-r--r--core/modules/system/tests/modules/render_placeholder_message_test/render_placeholder_message_test.routing.yml24
-rw-r--r--core/modules/system/tests/modules/render_placeholder_message_test/src/RenderPlaceholderMessageTestController.php99
-rw-r--r--core/modules/system/tests/src/Functional/Render/PlaceholderMessageTest.php58
8 files changed, 314 insertions, 19 deletions
diff --git a/core/lib/Drupal/Core/Render/Renderer.php b/core/lib/Drupal/Core/Render/Renderer.php
index 09a1bf1..0bf1d63 100644
--- a/core/lib/Drupal/Core/Render/Renderer.php
+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -636,8 +636,33 @@ class Renderer implements RendererInterface {
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 9145208..1894aa9 100644
--- a/core/modules/big_pipe/src/Render/BigPipe.php
+++ b/core/modules/big_pipe/src/Render/BigPipe.php
@@ -134,7 +134,7 @@ class BigPipe implements BigPipeInterface {
$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 @@ EOF;
*
* @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 @@ EOF;
* 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;
}
}
diff --git a/core/modules/big_pipe/src/Tests/BigPipeTest.php b/core/modules/big_pipe/src/Tests/BigPipeTest.php
index 370b815..1a1bc3d 100644
--- a/core/modules/big_pipe/src/Tests/BigPipeTest.php
+++ b/core/modules/big_pipe/src/Tests/BigPipeTest.php
@@ -170,6 +170,11 @@ class BigPipeTest extends WebTestBase {
$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 @@ class BigPipeTest extends WebTestBase {
*
* @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 @@ class BigPipeTest extends WebTestBase {
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');
diff --git a/core/modules/big_pipe/tests/src/FunctionalJavascript/BigPipeRegressionTest.php b/core/modules/big_pipe/tests/src/FunctionalJavascript/BigPipeRegressionTest.php
index 3ce9907..a99d5fc 100644
--- a/core/modules/big_pipe/tests/src/FunctionalJavascript/BigPipeRegressionTest.php
+++ b/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 @@ class BigPipeRegressionTest extends JavascriptTestBase {
* @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 @@ JS;
* @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 @@ JS;
// 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);
+ }
}
}
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 0000000..532695b
--- /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 0000000..6734cf3
--- /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 0000000..f02f0b6
--- /dev/null
+++ b/core/modules/system/tests/modules/render_placeholder_message_test/src/RenderPlaceholderMessageTestController.php
@@ -0,0 +1,99 @@
+<?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>'];
+ }
+
+}
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 0000000..185ca49
--- /dev/null
+++ b/core/modules/system/tests/src/Functional/Render/PlaceholderMessageTest.php
@@ -0,0 +1,58 @@
+<?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);
+ }
+ }
+
+}