summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorNathaniel Catchpole2016-07-04 10:40:04 (GMT)
committerNathaniel Catchpole2016-07-04 10:40:04 (GMT)
commit5e86905f1b4344a65ef4998a79d7a716f3b1a97e (patch)
treec5e42f168c13d83d261bce43987b86f24265e0e8
parent3eb2def4460c25cff93940a63315d54b1a9153cc (diff)
Issue #2698811 by danmuzyka, Wim Leers, jhedstrom, Fabianx: BigPipe unnecessarily renders and sends multiple-occurrence placeholders multiple times when using JS — can cause JS errors
-rw-r--r--core/modules/big_pipe/src/Render/BigPipe.php29
-rw-r--r--core/modules/big_pipe/src/Tests/BigPipeTest.php36
-rw-r--r--core/modules/big_pipe/tests/modules/big_pipe_test/big_pipe_test.routing.yml9
-rw-r--r--core/modules/big_pipe/tests/modules/big_pipe_test/src/BigPipeTestController.php51
-rw-r--r--core/modules/big_pipe/tests/src/FunctionalJavascript/BigPipeRegressionTest.php117
-rw-r--r--core/modules/big_pipe/tests/themes/big_pipe_test_theme/big_pipe_test_theme.info.yml5
-rw-r--r--core/modules/big_pipe/tests/themes/big_pipe_test_theme/templates/field--comment.html.twig13
7 files changed, 258 insertions, 2 deletions
diff --git a/core/modules/big_pipe/src/Render/BigPipe.php b/core/modules/big_pipe/src/Render/BigPipe.php
index 3e67737..5e3cc12 100644
--- a/core/modules/big_pipe/src/Render/BigPipe.php
+++ b/core/modules/big_pipe/src/Render/BigPipe.php
@@ -226,6 +226,13 @@ class BigPipe implements BigPipeInterface {
$preg_placeholder_strings = array_map($prepare_for_preg_split, array_keys($no_js_placeholders));
$fragments = preg_split('/' . implode('|', $preg_placeholder_strings) . '/', $html, NULL, PREG_SPLIT_NO_EMPTY | PREG_SPLIT_DELIM_CAPTURE);
+ // Determine how many occurrences there are of each no-JS placeholder.
+ $placeholder_occurrences = array_count_values(array_intersect($fragments, array_keys($no_js_placeholders)));
+
+ // Set up a variable to store the content of placeholders that have multiple
+ // occurrences.
+ $multi_occurrence_placeholders_content = [];
+
foreach ($fragments as $fragment) {
// If the fragment isn't one of the no-JS placeholders, it is the HTML in
// between placeholders and it must be printed & flushed immediately. The
@@ -236,6 +243,15 @@ class BigPipe implements BigPipeInterface {
continue;
}
+ // If there are multiple occurrences of this particular placeholder, and
+ // this is the second occurrence, we can skip all calculations and just
+ // send the same content.
+ if ($placeholder_occurrences[$fragment] > 1 && isset($multi_occurrence_placeholders_content[$fragment])) {
+ print $multi_occurrence_placeholders_content[$fragment];
+ flush();
+ continue;
+ }
+
$placeholder = $fragment;
assert('isset($no_js_placeholders[$placeholder])');
$token = Crypt::randomBytesBase64(55);
@@ -310,6 +326,13 @@ class BigPipe implements BigPipeInterface {
// they can be sent in ::sendPreBody().
$cumulative_assets->setAlreadyLoadedLibraries(array_merge($cumulative_assets->getAlreadyLoadedLibraries(), $html_response->getAttachments()['library']));
$cumulative_assets->setSettings($html_response->getAttachments()['drupalSettings']);
+
+ // If there are multiple occurrences of this particular placeholder, track
+ // the content that was sent, so we can skip all calculations for the next
+ // occurrence.
+ if ($placeholder_occurrences[$fragment] > 1) {
+ $multi_occurrence_placeholders_content[$fragment] = $html_response->getContent();
+ }
}
}
@@ -508,7 +531,9 @@ EOF;
*
* @return array
* Indexed array; the order in which the BigPipe placeholders must be sent.
- * Values are the BigPipe placeholder IDs.
+ * Values are the BigPipe placeholder IDs. Note that only unique
+ * placeholders are kept: if the same placeholder occurs multiple times, we
+ * only keep the first occurrence.
*/
protected function getPlaceholderOrder($html) {
$fragments = explode('<div data-big-pipe-placeholder-id="', $html);
@@ -521,7 +546,7 @@ EOF;
$order[] = $placeholder;
}
- return $order;
+ return array_unique($order);
}
}
diff --git a/core/modules/big_pipe/src/Tests/BigPipeTest.php b/core/modules/big_pipe/src/Tests/BigPipeTest.php
index f947c46..370b815 100644
--- a/core/modules/big_pipe/src/Tests/BigPipeTest.php
+++ b/core/modules/big_pipe/src/Tests/BigPipeTest.php
@@ -269,6 +269,42 @@ class BigPipeTest extends WebTestBase {
unlink(\Drupal::root() . '/' . $this->siteDirectory . '/error.log');
}
+ /**
+ * Tests BigPipe with a multi-occurrence placeholder.
+ */
+ public function testBigPipeMultiOccurrencePlaceholders() {
+ $this->drupalLogin($this->rootUser);
+ $this->assertSessionCookieExists(TRUE);
+ $this->assertBigPipeNoJsCookieExists(FALSE);
+
+ // By not calling performMetaRefresh() here, we simulate JavaScript being
+ // enabled, because as far as the BigPipe module is concerned, JavaScript is
+ // enabled in the browser as long as the BigPipe no-JS cookie is *not* set.
+ // @see setUp()
+ // @see performMetaRefresh()
+
+ $this->drupalGet(Url::fromRoute('big_pipe_test_multi_occurrence'));
+ $big_pipe_placeholder_id = 'callback=Drupal%5CCore%5CRender%5CElement%5CStatusMessages%3A%3ArenderMessages&amp;args[0]&amp;token=a8c34b5e';
+ $expected_placeholder_replacement = '<script type="application/vnd.drupal-ajax" data-big-pipe-replacement-for-placeholder-with-id="' . $big_pipe_placeholder_id . '">';
+ $this->assertRaw('The count is 1.');
+ $this->assertNoRaw('The count is 2.');
+ $this->assertNoRaw('The count is 3.');
+ $raw_content = $this->getRawContent();
+ $this->assertTrue(substr_count($raw_content, $expected_placeholder_replacement) == 1, 'Only one placeholder replacement was found for the duplicate #lazy_builder arrays.');
+
+ // By calling performMetaRefresh() here, we simulate JavaScript being
+ // disabled, because as far as the BigPipe module is concerned, it is
+ // enabled in the browser when the BigPipe no-JS cookie is set.
+ // @see setUp()
+ // @see performMetaRefresh()
+ $this->performMetaRefresh();
+ $this->assertBigPipeNoJsCookieExists(TRUE);
+ $this->drupalGet(Url::fromRoute('big_pipe_test_multi_occurrence'));
+ $this->assertRaw('The count is 1.');
+ $this->assertNoRaw('The count is 2.');
+ $this->assertNoRaw('The count is 3.');
+ }
+
protected function assertBigPipeResponseHeadersPresent() {
$this->pass('Verifying BigPipe response headers…', 'Debug');
$this->assertTrue(FALSE !== strpos($this->drupalGetHeader('Cache-Control'), 'private'), 'Cache-Control header set to "private".');
diff --git a/core/modules/big_pipe/tests/modules/big_pipe_test/big_pipe_test.routing.yml b/core/modules/big_pipe/tests/modules/big_pipe_test/big_pipe_test.routing.yml
index 4979406..7105060 100644
--- a/core/modules/big_pipe/tests/modules/big_pipe_test/big_pipe_test.routing.yml
+++ b/core/modules/big_pipe/tests/modules/big_pipe_test/big_pipe_test.routing.yml
@@ -15,3 +15,12 @@ no_big_pipe:
_no_big_pipe: TRUE
requirements:
_access: 'TRUE'
+
+big_pipe_test_multi_occurrence:
+ path: '/big_pipe_test_multi_occurrence'
+ defaults:
+ _controller: '\Drupal\big_pipe_test\BigPipeTestController::multiOccurrence'
+ _title: 'BigPipe test multiple occurrences of the same placeholder'
+ requirements:
+ _access: 'TRUE'
+
diff --git a/core/modules/big_pipe/tests/modules/big_pipe_test/src/BigPipeTestController.php b/core/modules/big_pipe/tests/modules/big_pipe_test/src/BigPipeTestController.php
index 450a464..30594a5 100644
--- a/core/modules/big_pipe/tests/modules/big_pipe_test/src/BigPipeTestController.php
+++ b/core/modules/big_pipe/tests/modules/big_pipe_test/src/BigPipeTestController.php
@@ -53,6 +53,30 @@ class BigPipeTestController {
}
/**
+ * A page with multiple occurrences of the same placeholder.
+ *
+ * @see \Drupal\big_pipe\Tests\BigPipeTest::testBigPipeMultipleOccurrencePlaceholders()
+ *
+ * @return array
+ */
+ public function multiOccurrence() {
+ return [
+ 'item1' => [
+ '#lazy_builder' => [static::class . '::counter', []],
+ '#create_placeholder' => TRUE,
+ ],
+ 'item2' => [
+ '#lazy_builder' => [static::class . '::counter', []],
+ '#create_placeholder' => TRUE,
+ ],
+ 'item3' => [
+ '#lazy_builder' => [static::class . '::counter', []],
+ '#create_placeholder' => TRUE,
+ ],
+ ];
+ }
+
+ /**
* #lazy_builder callback; builds <time> markup with current time.
*
* Note: does not actually use current time, that would complicate testing.
@@ -98,4 +122,31 @@ class BigPipeTestController {
return ['#plain_text' => BigPipeTestSubscriber::CONTENT_TRIGGER_EXCEPTION];
}
+ /**
+ * #lazy_builder callback; returns the current count.
+ *
+ * @see \Drupal\big_pipe\Tests\BigPipeTest::testBigPipeMultipleOccurrencePlaceholders()
+ *
+ * @return array
+ * The render array.
+ */
+ public static function counter() {
+ // Lazy builders are not allowed to build their own state like this function
+ // does, but in this case we're intentionally doing that for testing
+ // purposes: so we can ensure that each lazy builder is only ever called
+ // once with the same parameters.
+ static $count;
+
+ if (!isset($count)) {
+ $count = 0;
+ }
+
+ $count++;
+
+ return [
+ '#markup' => BigPipeMarkup::create("<p>The count is $count.</p>"),
+ '#cache' => ['max-age' => 0],
+ ];
+ }
+
}
diff --git a/core/modules/big_pipe/tests/src/FunctionalJavascript/BigPipeRegressionTest.php b/core/modules/big_pipe/tests/src/FunctionalJavascript/BigPipeRegressionTest.php
new file mode 100644
index 0000000..ab28073
--- /dev/null
+++ b/core/modules/big_pipe/tests/src/FunctionalJavascript/BigPipeRegressionTest.php
@@ -0,0 +1,117 @@
+<?php
+
+namespace Drupal\Tests\big_pipe\FunctionalJavascript;
+
+use Drupal\comment\CommentInterface;
+use Drupal\comment\Entity\Comment;
+use Drupal\comment\Plugin\Field\FieldType\CommentItemInterface;
+use Drupal\comment\Tests\CommentTestTrait;
+use Drupal\editor\Entity\Editor;
+use Drupal\filter\Entity\FilterFormat;
+use Drupal\FunctionalJavascriptTests\JavascriptTestBase;
+use Drupal\simpletest\ContentTypeCreationTrait;
+use Drupal\simpletest\NodeCreationTrait;
+
+/**
+ * BigPipe regression tests.
+ *
+ * @group big_pipe
+ */
+class BigPipeRegressionTest extends JavascriptTestBase {
+
+ use CommentTestTrait;
+ use ContentTypeCreationTrait;
+ use NodeCreationTrait;
+
+ /**
+ * {@inheritdoc}
+ */
+ public static $modules = [
+ 'node',
+ 'comment',
+ 'big_pipe',
+ 'history',
+ 'editor',
+ 'ckeditor',
+ 'filter',
+ ];
+
+ /**
+ * {@inheritdoc}
+ */
+ public function setUp() {
+ parent::setUp();
+
+ // Use the big_pipe_test_theme theme.
+ $this->container->get('theme_installer')->install(['big_pipe_test_theme']);
+ $this->container->get('config.factory')->getEditable('system.theme')->set('default', 'big_pipe_test_theme')->save();
+ }
+
+ /**
+ * Ensure comment form works with history and big_pipe modules.
+ *
+ * @see https://www.drupal.org/node/2698811
+ */
+ public function testCommentForm_2698811() {
+ // Ensure an `article` node type exists.
+ $this->createContentType(['type' => 'article']);
+ $this->addDefaultCommentField('node', 'article');
+
+ // Enable CKEditor.
+ $format = $this->randomMachineName();
+ FilterFormat::create([
+ 'format' => $format,
+ 'name' => $this->randomString(),
+ 'weight' => 1,
+ 'filters' => [],
+ ])->save();
+ $settings['toolbar']['rows'] = [
+ [
+ [
+ 'name' => 'Links',
+ 'items' => [
+ 'DrupalLink',
+ 'DrupalUnlink',
+ ],
+ ],
+ ],
+ ];
+ $editor = Editor::create([
+ 'format' => $format,
+ 'editor' => 'ckeditor',
+ ]);
+ $editor->setSettings($settings);
+ $editor->save();
+
+ $admin_user = $this->drupalCreateUser([
+ 'access comments',
+ 'post comments',
+ 'use text format ' . $format,
+ ]);
+ $this->drupalLogin($admin_user);
+
+ $node = $this->createNode([
+ 'type' => 'article',
+ 'comment' => CommentItemInterface::OPEN,
+ ]);
+ // Create some comments.
+ foreach (range(1, 5) as $i) {
+ $comment = Comment::create([
+ 'status' => CommentInterface::PUBLISHED,
+ 'field_name' => 'comment',
+ 'entity_type' => 'node',
+ 'entity_id' => $node->id(),
+ ]);
+ $comment->save();
+ }
+ $this->drupalGet($node->toUrl()->toString());
+ // Confirm that CKEditor loaded.
+ $javascript = <<<JS
+ (function(){
+ return Object.keys(CKEDITOR.instances).length > 0;
+ }());
+JS;
+ $this->assertJsCondition($javascript);
+ }
+
+}
diff --git a/core/modules/big_pipe/tests/themes/big_pipe_test_theme/big_pipe_test_theme.info.yml b/core/modules/big_pipe/tests/themes/big_pipe_test_theme/big_pipe_test_theme.info.yml
new file mode 100644
index 0000000..2b3c842
--- /dev/null
+++ b/core/modules/big_pipe/tests/themes/big_pipe_test_theme/big_pipe_test_theme.info.yml
@@ -0,0 +1,5 @@
+name: 'BigPipe test theme'
+type: theme
+description: 'Theme for testing BigPipe edge cases.'
+version: VERSION
+core: 8.x
diff --git a/core/modules/big_pipe/tests/themes/big_pipe_test_theme/templates/field--comment.html.twig b/core/modules/big_pipe/tests/themes/big_pipe_test_theme/templates/field--comment.html.twig
new file mode 100644
index 0000000..a26fa78
--- /dev/null
+++ b/core/modules/big_pipe/tests/themes/big_pipe_test_theme/templates/field--comment.html.twig
@@ -0,0 +1,13 @@
+{#
+/**
+ * @file
+ * Test that comments still work with the form above instead of below.
+ *
+ * @see \Drupal\Tests\big_pipe\FunctionalJavascript\BigPipeRegressionTest::testCommentForm_2698811()
+ */
+#}
+<section{{ attributes }}>
+ {{ comment_form }}
+
+ {{ comments }}
+</section>