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)

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

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.');
       }

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 [];
     }
 

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 absent' => [$placeholders, 'GET', FALSE, FALSE, FALSE, []],
-      '_no_big_pipe absent, no session, no-JS cookie present' => [$placeholders, FALSE, FALSE, TRUE, []],
+      '_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, TRUE, FALSE, FALSE, []],
+      '_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, TRUE, FALSE, TRUE, []],
+      '_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, TRUE, TRUE, FALSE, []],
+      '_no_big_pipe present, session, no-JS cookie absent' => [$placeholders, 'GET', TRUE, TRUE, FALSE, []],
-      '_no_big_pipe present, session, no-JS cookie present' => [$placeholders, TRUE, TRUE, TRUE, []],
+      '_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, FALSE, TRUE, FALSE, [
+      '_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, []],
     ];
   }
 

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 {
@@ -781,6 +784,40 @@ class RendererPlaceholdersTest extends RendererTestBase {
     $this->assertPlaceholderRenderCache(FALSE, [], []);
   }
 
+  /**
+   * @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.
    *