Commit 5e86905f authored by catch's avatar catch

Issue #2698811 by danmuzyka, Wim Leers, jhedstrom, Fabianx: BigPipe...

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
parent 3eb2def4
......@@ -226,6 +226,13 @@ protected function sendNoJsPlaceholders($html, $no_js_placeholders, AttachedAsse
$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 @@ protected function sendNoJsPlaceholders($html, $no_js_placeholders, AttachedAsse
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 @@ protected function sendNoJsPlaceholders($html, $no_js_placeholders, AttachedAsse
// 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 @@ protected function renderPlaceholder($placeholder, array $placeholder_render_arr
*
* @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 @@ protected function getPlaceholderOrder($html) {
$order[] = $placeholder;
}
return $order;
return array_unique($order);
}
}
......@@ -269,6 +269,42 @@ public function testBigPipeNoJs() {
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".');
......
......@@ -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'
......@@ -52,6 +52,30 @@ public static function nope() {
return ['#markup' => '<p>Nope.</p>'];
}
/**
* 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.
*
......@@ -98,4 +122,31 @@ public static function responseException() {
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],
];
}
}
<?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);
}
}
name: 'BigPipe test theme'
type: theme
description: 'Theme for testing BigPipe edge cases.'
version: VERSION
core: 8.x
{#
/**
* @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>
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment