summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorNathaniel Catchpole2016-04-28 14:02:12 (GMT)
committerNathaniel Catchpole2016-04-28 14:04:17 (GMT)
commit0fcef5bd60f62ebdd4a10a2ad7035c22010c876c (patch)
treef0006add13143a9a6814f0aa7344301c51a1619f
parent3a6cac15da1489473b890bbcf2164c36d52e2023 (diff)
Issue #2702609 by Wim Leers, Berdir, Fabianx: Disable placeholdering (and BigPipe) on unsafe requests to help find forms as fast as possible (to allow EnforcedResponses to work)
-rw-r--r--core/lib/Drupal/Core/Render/PlaceholderingRenderCache.php10
-rw-r--r--core/lib/Drupal/Core/Render/Renderer.php4
-rw-r--r--core/modules/big_pipe/src/Render/Placeholder/BigPipeStrategy.php9
-rw-r--r--core/modules/big_pipe/tests/src/Unit/Render/Placeholder/BigPipeStrategyTest.php23
-rw-r--r--core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php37
5 files changed, 71 insertions, 12 deletions
diff --git a/core/lib/Drupal/Core/Render/PlaceholderingRenderCache.php b/core/lib/Drupal/Core/Render/PlaceholderingRenderCache.php
index 98b58fc..4256283 100644
--- a/core/lib/Drupal/Core/Render/PlaceholderingRenderCache.php
+++ b/core/lib/Drupal/Core/Render/PlaceholderingRenderCache.php
@@ -92,6 +92,11 @@ class PlaceholderingRenderCache extends RenderCache {
* {@inheritdoc}
*/
public function get(array $elements) {
+ // @todo remove this check when https://www.drupal.org/node/2367555 lands.
+ if (!$this->requestStack->getCurrentRequest()->isMethodSafe()) {
+ return FALSE;
+ }
+
// When rendering placeholders, special case auto-placeholdered elements:
// avoid retrieving them from cache again, or rendering them again.
if (isset($elements['#create_placeholder']) && $elements['#create_placeholder'] === FALSE) {
@@ -121,6 +126,11 @@ class PlaceholderingRenderCache extends RenderCache {
public function set(array &$elements, array $pre_bubbling_elements) {
$result = parent::set($elements, $pre_bubbling_elements);
+ // @todo remove this check when https://www.drupal.org/node/2367555 lands.
+ if (!$this->requestStack->getCurrentRequest()->isMethodSafe()) {
+ return FALSE;
+ }
+
if ($this->placeholderGenerator->canCreatePlaceholder($pre_bubbling_elements) && $this->placeholderGenerator->shouldAutomaticallyPlaceholder($elements)) {
// Overwrite $elements with a placeholder. The Renderer (which called this
// method) will update the context with the bubbleable metadata of the
diff --git a/core/lib/Drupal/Core/Render/Renderer.php b/core/lib/Drupal/Core/Render/Renderer.php
index 3c13dfd..09a1bf1 100644
--- a/core/lib/Drupal/Core/Render/Renderer.php
+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -338,7 +338,9 @@ class Renderer implements RendererInterface {
// If instructed to create a placeholder, and a #lazy_builder callback is
// present (without such a callback, it would be impossible to replace the
// placeholder), replace the current element with a placeholder.
- if (isset($elements['#create_placeholder']) && $elements['#create_placeholder'] === TRUE) {
+ // @todo remove the isMethodSafe() check when
+ // https://www.drupal.org/node/2367555 lands.
+ if (isset($elements['#create_placeholder']) && $elements['#create_placeholder'] === TRUE && $this->requestStack->getCurrentRequest()->isMethodSafe()) {
if (!isset($elements['#lazy_builder'])) {
throw new \LogicException('When #create_placeholder is set, a #lazy_builder callback must be present as well.');
}
diff --git a/core/modules/big_pipe/src/Render/Placeholder/BigPipeStrategy.php b/core/modules/big_pipe/src/Render/Placeholder/BigPipeStrategy.php
index 0edf3ba..ae5cf1c 100644
--- a/core/modules/big_pipe/src/Render/Placeholder/BigPipeStrategy.php
+++ b/core/modules/big_pipe/src/Render/Placeholder/BigPipeStrategy.php
@@ -105,12 +105,19 @@ class BigPipeStrategy implements PlaceholderStrategyInterface {
* {@inheritdoc}
*/
public function processPlaceholders(array $placeholders) {
+ $request = $this->requestStack->getCurrentRequest();
+
+ // @todo remove this check when https://www.drupal.org/node/2367555 lands.
+ if (!$request->isMethodSafe()) {
+ return [];
+ }
+
// Routes can opt out from using the BigPipe HTML delivery technique.
if ($this->routeMatch->getRouteObject()->getOption('_no_big_pipe')) {
return [];
}
- if (!$this->sessionConfiguration->hasSession($this->requestStack->getCurrentRequest())) {
+ if (!$this->sessionConfiguration->hasSession($request)) {
return [];
}
diff --git a/core/modules/big_pipe/tests/src/Unit/Render/Placeholder/BigPipeStrategyTest.php b/core/modules/big_pipe/tests/src/Unit/Render/Placeholder/BigPipeStrategyTest.php
index 651f434..9806fbf 100644
--- a/core/modules/big_pipe/tests/src/Unit/Render/Placeholder/BigPipeStrategyTest.php
+++ b/core/modules/big_pipe/tests/src/Unit/Render/Placeholder/BigPipeStrategyTest.php
@@ -22,8 +22,9 @@ class BigPipeStrategyTest extends UnitTestCase {
*
* @dataProvider placeholdersProvider
*/
- public function testProcessPlaceholders(array $placeholders, $route_match_has_no_big_pipe_option, $request_has_session, $request_has_big_pipe_nojs_cookie, array $expected_big_pipe_placeholders) {
+ public function testProcessPlaceholders(array $placeholders, $method, $route_match_has_no_big_pipe_option, $request_has_session, $request_has_big_pipe_nojs_cookie, array $expected_big_pipe_placeholders) {
$request = new Request();
+ $request->setMethod($method);
if ($request_has_big_pipe_nojs_cookie) {
$request->cookies->set(BigPipeStrategy::NOJS_COOKIE, 1);
}
@@ -45,7 +46,7 @@ class BigPipeStrategyTest extends UnitTestCase {
$big_pipe_strategy = new BigPipeStrategy($session_configuration->reveal(), $request_stack->reveal(), $route_match->reveal());
$processed_placeholders = $big_pipe_strategy->processPlaceholders($placeholders);
- if (!$route_match_has_no_big_pipe_option && $request_has_session) {
+ if ($request->isMethodSafe() && !$route_match_has_no_big_pipe_option && $request_has_session) {
$this->assertSameSize($expected_big_pipe_placeholders, $processed_placeholders, 'BigPipe is able to deliver all placeholders.');
foreach (array_keys($placeholders) as $placeholder) {
$this->assertSame($expected_big_pipe_placeholders[$placeholder], $processed_placeholders[$placeholder], "Verifying how BigPipeStrategy handles the placeholder '$placeholder'");
@@ -75,13 +76,13 @@ class BigPipeStrategyTest extends UnitTestCase {
];
return [
- '_no_big_pipe absent, no session, no-JS cookie absent' => [$placeholders, FALSE, FALSE, FALSE, []],
- '_no_big_pipe absent, no session, no-JS cookie present' => [$placeholders, FALSE, FALSE, TRUE, []],
- '_no_big_pipe present, no session, no-JS cookie absent' => [$placeholders, TRUE, FALSE, FALSE, []],
- '_no_big_pipe present, no session, no-JS cookie present' => [$placeholders, TRUE, FALSE, TRUE, []],
- '_no_big_pipe present, session, no-JS cookie absent' => [$placeholders, TRUE, TRUE, FALSE, []],
- '_no_big_pipe present, session, no-JS cookie present' => [$placeholders, TRUE, TRUE, TRUE, []],
- '_no_big_pipe absent, session, no-JS cookie absent: (JS-powered) BigPipe placeholder used for HTML placeholders' => [$placeholders, FALSE, TRUE, FALSE, [
+ '_no_big_pipe absent, no session, no-JS cookie absent' => [$placeholders, 'GET', FALSE, FALSE, FALSE, []],
+ '_no_big_pipe absent, no session, no-JS cookie present' => [$placeholders, 'GET', FALSE, FALSE, TRUE, []],
+ '_no_big_pipe present, no session, no-JS cookie absent' => [$placeholders, 'GET', TRUE, FALSE, FALSE, []],
+ '_no_big_pipe present, no session, no-JS cookie present' => [$placeholders, 'GET', TRUE, FALSE, TRUE, []],
+ '_no_big_pipe present, session, no-JS cookie absent' => [$placeholders, 'GET', TRUE, TRUE, FALSE, []],
+ '_no_big_pipe present, session, no-JS cookie present' => [$placeholders, 'GET', TRUE, TRUE, TRUE, []],
+ '_no_big_pipe absent, session, no-JS cookie absent: (JS-powered) BigPipe placeholder used for HTML placeholders' => [$placeholders, 'GET', FALSE, TRUE, FALSE, [
$cases['html']->placeholder => $cases['html']->bigPipePlaceholderRenderArray,
$cases['html_attribute_value']->placeholder => $cases['html_attribute_value']->bigPipeNoJsPlaceholderRenderArray,
$cases['html_attribute_value_subset']->placeholder => $cases['html_attribute_value_subset']->bigPipeNoJsPlaceholderRenderArray,
@@ -90,7 +91,8 @@ class BigPipeStrategyTest extends UnitTestCase {
$cases['exception__lazy_builder']->placeholder => $cases['exception__lazy_builder']->bigPipePlaceholderRenderArray,
$cases['exception__embedded_response']->placeholder => $cases['exception__embedded_response']->bigPipePlaceholderRenderArray,
]],
- '_no_big_pipe absent, session, no-JS cookie present: no-JS BigPipe placeholder used for HTML placeholders' => [$placeholders, FALSE, TRUE, TRUE, [
+ '_no_big_pipe absent, session, no-JS cookie absent: (JS-powered) BigPipe placeholder used for HTML placeholders — but unsafe method' => [$placeholders, 'POST', FALSE, TRUE, FALSE, []],
+ '_no_big_pipe absent, session, no-JS cookie present: no-JS BigPipe placeholder used for HTML placeholders' => [$placeholders, 'GET', FALSE, TRUE, TRUE, [
$cases['html']->placeholder => $cases['html']->bigPipeNoJsPlaceholderRenderArray,
$cases['html_attribute_value']->placeholder => $cases['html_attribute_value']->bigPipeNoJsPlaceholderRenderArray,
$cases['html_attribute_value_subset']->placeholder => $cases['html_attribute_value_subset']->bigPipeNoJsPlaceholderRenderArray,
@@ -99,6 +101,7 @@ class BigPipeStrategyTest extends UnitTestCase {
$cases['exception__lazy_builder']->placeholder => $cases['exception__lazy_builder']->bigPipeNoJsPlaceholderRenderArray,
$cases['exception__embedded_response']->placeholder => $cases['exception__embedded_response']->bigPipeNoJsPlaceholderRenderArray,
]],
+ '_no_big_pipe absent, session, no-JS cookie present: no-JS BigPipe placeholder used for HTML placeholders — but unsafe method' => [$placeholders, 'POST', FALSE, TRUE, TRUE, []],
];
}
diff --git a/core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php b/core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php
index 5fd49a7..4f5585c 100644
--- a/core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php
+++ b/core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php
@@ -10,9 +10,12 @@ namespace Drupal\Tests\Core\Render;
use Drupal\Component\Utility\Html;
use Drupal\Core\Cache\Cache;
use Drupal\Core\Render\Markup;
+use Drupal\Core\Render\RenderContext;
/**
* @coversDefaultClass \Drupal\Core\Render\Renderer
+ * @covers \Drupal\Core\Render\RenderCache
+ * @covers \Drupal\Core\Render\PlaceholderingRenderCache
* @group Render
*/
class RendererPlaceholdersTest extends RendererTestBase {
@@ -782,6 +785,40 @@ class RendererPlaceholdersTest extends RendererTestBase {
}
/**
+ * @covers ::render
+ * @covers ::doRender
+ * @covers \Drupal\Core\Render\RenderCache::get
+ * @covers \Drupal\Core\Render\PlaceholderingRenderCache::get
+ * @covers \Drupal\Core\Render\PlaceholderingRenderCache::set
+ * @covers ::replacePlaceholders
+ *
+ * @dataProvider providerPlaceholders
+ */
+ public function testPlaceholderingDisabledForPostRequests($test_element, $args) {
+ $this->setUpUnusedCache();
+ $this->setUpRequest('POST');
+
+ $element = $test_element;
+
+ // Render without replacing placeholders, to allow this test to see which
+ // #attached[placeholders] there are, if any.
+ $this->renderer->executeInRenderContext(new RenderContext(), function () use (&$element) {
+ return $this->renderer->render($element);
+ });
+ // Only test cases where the placeholders have been specified manually are
+ // allowed to have placeholders. This means that of the different situations
+ // listed in providerPlaceholders(), only type B can have attached
+ // placeholders. Everything else, whether:
+ // 1. manual placeholdering
+ // 2. automatic placeholdering via already-present cacheability metadata
+ // 3. automatic placeholdering via bubbled cacheability metadata
+ // All three of those should NOT result in placeholders.
+ if (!isset($test_element['#attached']['placeholders'])) {
+ $this->assertFalse(isset($element['#attached']['placeholders']), 'No placeholders created.');
+ }
+ }
+
+ /**
* Tests a placeholder that adds another placeholder.
*
* E.g. when rendering a node in a placeholder the rendering of that node