Issue #3395776 by catch, kristiaanvandeneynde, Wim Leers, Fabianx, larowlan: Make POST requests render cacheable

(cherry picked from commit ffa4daa3fb)

core/lib/Drupal/Core/Form/FormBuilder.php

@@ -348,10 +348,9 @@ class FormBuilder implements FormBuilderInterface, FormValidatorInterface, FormS
     // throwing an exception.
     // @see Drupal\Core\EventSubscriber\EnforcedFormResponseSubscriber
     //
-    // @todo Exceptions should not be used for code flow control. However, the
+    // @todo Exceptions should not be used for code flow control. In order to
-    //   Form API does not integrate with the HTTP Kernel based architecture of
+    //   resolve this issue properly it is necessary to completely separate form
-    //   Drupal 8. In order to resolve this issue properly it is necessary to
+    //   submission from rendering.
-    //   completely separate form submission from rendering.
     //   @see https://www.drupal.org/node/2367555
     if ($response instanceof Response) {
       throw new EnforcedResponseException($response);
@@ -831,6 +830,10 @@ class FormBuilder implements FormBuilderInterface, FormValidatorInterface, FormS
       }
     }
 
+    // Add the 'CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:form' cache tag to
+    // identify this render array as a form to the render cache.
+    $form['#cache']['tags'][] = 'CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:form';
+
     // Invoke hook_form_alter(), hook_form_BASE_FORM_ID_alter(), and
     // hook_form_FORM_ID_alter() implementations.
     $hooks = ['form'];

core/lib/Drupal/Core/Render/PlaceholderingRenderCache.php

@@ -96,10 +96,6 @@ 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()->isMethodCacheable()) {
-      return FALSE;
-    }
 
     // When rendering placeholders, special case auto-placeholdered elements:
     // avoid retrieving them from cache again, or rendering them again.
@@ -130,7 +126,9 @@ 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.
+    // Writes to the render cache are disabled on uncacheable HTTP requests, to
+    // prevent very low hit rate items from being written. If we're not writing
+    // to the cache, there's also no benefit to placeholdering either.
     if (!$this->requestStack->getCurrentRequest()->isMethodCacheable()) {
       return FALSE;
     }

core/lib/Drupal/Core/Render/RenderCache.php

@@ -59,16 +59,17 @@ class RenderCache implements RenderCacheInterface {
    * {@inheritdoc}
    */
   public function get(array $elements) {
-    // Form submissions rely on the form being built during the POST request,
+    if (!$this->isElementCacheable($elements)) {
-    // and render caching of forms prevents this from happening.
-    // @todo remove the isMethodCacheable() check when
-    //   https://www.drupal.org/node/2367555 lands.
-    if (!$this->requestStack->getCurrentRequest()->isMethodCacheable() || !$this->isElementCacheable($elements)) {
       return FALSE;
     }
 
     $bin = isset($elements['#cache']['bin']) ? $elements['#cache']['bin'] : 'render';
     if (($cache_bin = $this->cacheFactory->get($bin)) && $cache = $cache_bin->get($elements['#cache']['keys'], CacheableMetadata::createFromRenderArray($elements))) {
+      if (!$this->requestStack->getCurrentRequest()->isMethodCacheable()) {
+        if (!empty(array_filter($cache->tags, fn (string $tag) => str_starts_with($tag, 'CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:')))) {
+          return FALSE;
+        }
+      }
       return $cache->data;
     }
     return FALSE;
@@ -78,10 +79,9 @@ class RenderCache implements RenderCacheInterface {
    * {@inheritdoc}
    */
   public function set(array &$elements, array $pre_bubbling_elements) {
-    // Form submissions rely on the form being built during the POST request,
+    // Avoid setting cache items on POST requests, this ensures that cache items
-    // and render caching of forms prevents this from happening.
+    // with a very low hit rate won't enter the cache. All render elements
-    // @todo remove the isMethodCacheable() check when
+    // except forms will still be retrieved from cache when available.
-    //   https://www.drupal.org/node/2367555 lands.
     if (!$this->requestStack->getCurrentRequest()->isMethodCacheable() || !$this->isElementCacheable($elements)) {
       return FALSE;
     }

core/lib/Drupal/Core/Render/Renderer.php

@@ -394,9 +394,11 @@ 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.
+    // placeholder), replace the current element with a placeholder. On
-    // @todo remove the isMethodCacheable() check when
+    // uncacheable requests, always skip placeholdering - if a form is inside
-    //   https://www.drupal.org/node/2367555 lands.
+    // a placeholder, which is likely, we want to render it as soon as possible,
+    // so that form submission and redirection can take over before any more
+    // content is rendered.
     if (isset($elements['#create_placeholder']) && $elements['#create_placeholder'] === TRUE && $this->requestStack->getCurrentRequest()->isMethodCacheable()) {
       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

@@ -108,7 +108,11 @@ class BigPipeStrategy implements PlaceholderStrategyInterface {
   public function processPlaceholders(array $placeholders) {
     $request = $this->requestStack->getCurrentRequest();
 
-    // @todo remove this check when https://www.drupal.org/node/2367555 lands.
+    // Prevent placeholders from being processed by BigPipe on uncacheable
+    // request methods. For example, a form rendered inside a placeholder will
+    // be rendered as soon as possible before any headers are sent, so that it
+    // can be detected, submitted, and redirected immediately.
+    // @todo https://www.drupal.org/node/2367555
     if (!$request->isMethodCacheable()) {
       return [];
     }

core/modules/block/tests/src/Functional/Views/DisplayBlockTest.php

@@ -290,7 +290,7 @@ class DisplayBlockTest extends ViewTestBase {
     $this->drupalGet('');
     $this->assertSession()->elementNotExists('xpath', $block_title_xpath);
 
-    $this->assertCacheTags(array_merge($block->getCacheTags(), ['block_view', 'config:block_list', 'config:system.site', 'config:views.view.test_view_block', 'http_response', 'rendered']));
+    $this->assertCacheTags(array_merge($block->getCacheTags(), ['block_view', 'config:block_list', 'config:system.site', 'config:views.view.test_view_block', 'http_response', 'CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:form', 'rendered']));
   }
 
   /**

core/modules/block_content/tests/src/Functional/BlockContentCacheTagsTest.php

@@ -10,7 +10,6 @@ use Drupal\Core\Cache\Cache;
 use Drupal\Core\Cache\CacheableMetadata;
 use Drupal\Core\Entity\EntityInterface;
 use Drupal\Tests\system\Functional\Entity\EntityCacheTagsTestBase;
-use Symfony\Component\HttpFoundation\Request;
 
 /**
  * Tests the Content Block entity's cache tags.
@@ -81,14 +80,7 @@ class BlockContentCacheTagsTest extends EntityCacheTagsTestBase {
     $build = $this->container->get('entity_type.manager')->getViewBuilder('block')->view($block, 'block');
 
     // Render the block.
-    // @todo The request stack manipulation won't be necessary once
-    //   https://www.drupal.org/node/2367555 is fixed and the
-    //   corresponding $request->isMethodCacheable() checks are removed from
-    //   Drupal\Core\Render\Renderer.
-    $request_stack = $this->container->get('request_stack');
-    $request_stack->push(new Request());
     $this->container->get('renderer')->renderRoot($build);
-    $request_stack->pop();
 
     // Expected keys, contexts, and tags for the block.
     // @see \Drupal\block\BlockViewBuilder::viewMultiple()

core/modules/comment/tests/src/Kernel/CommentDefaultFormatterCacheTagsTest.php

@@ -84,6 +84,7 @@ class CommentDefaultFormatterCacheTagsTest extends EntityKernelTestBase {
     $renderer->renderRoot($build);
     $expected_cache_tags = [
       'entity_test_view',
+      'CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:form',
       'entity_test:' . $commented_entity->id(),
       'config:core.entity_form_display.comment.comment.default',
       'config:field.field.comment.comment.comment_body',
@@ -131,6 +132,7 @@ class CommentDefaultFormatterCacheTagsTest extends EntityKernelTestBase {
       'comment:' . $comment->id(),
       'config:filter.format.plain_text',
       'user_view',
+      'CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:form',
       'user:' . $user->id(),
       'config:core.entity_form_display.comment.comment.default',
       'config:field.field.comment.comment.comment_body',

core/modules/node/node.module

@@ -1036,9 +1036,8 @@ function node_query_node_access_alter(AlterableInterface $query) {
 
   // Bubble the 'user.node_grants:$op' cache context to the current render
   // context.
-  $request = \Drupal::requestStack()->getCurrentRequest();
   $renderer = \Drupal::service('renderer');
-  if ($request->isMethodCacheable() && $renderer->hasRenderContext()) {
+  if ($renderer->hasRenderContext()) {
     $build = ['#cache' => ['contexts' => ['user.node_grants:' . $op]]];
     $renderer->render($build);
   }

core/modules/page_cache/tests/src/Functional/PageCacheTagsIntegrationTest.php

@@ -160,6 +160,7 @@ class PageCacheTagsIntegrationTest extends BrowserTestBase {
       'config:block.block.olivero_primary_admin_actions',
       'config:block.block.olivero_page_title',
       'node_view',
+      'CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:form',
       'node:' . $node_1->id(),
       'user:' . $author_1->id(),
       'config:filter.format.basic_html',
@@ -199,6 +200,7 @@ class PageCacheTagsIntegrationTest extends BrowserTestBase {
       'config:block.block.olivero_primary_admin_actions',
       'config:block.block.olivero_page_title',
       'node_view',
+      'CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:form',
       'node:' . $node_2->id(),
       'user:' . $author_2->id(),
       'config:filter.format.full_html',

core/modules/search/tests/src/Functional/SearchPageCacheTagsTest.php

@@ -185,6 +185,7 @@ class SearchPageCacheTagsTest extends BrowserTestBase {
       'search_index',
       'search_index:node_search',
       'http_response',
+      'CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:form',
       'rendered',
       'node_list',
     ];

core/modules/shortcut/tests/src/Functional/ShortcutCacheTagsTest.php

@@ -109,6 +109,7 @@ class ShortcutCacheTagsTest extends EntityCacheTagsTestBase {
       'config:shortcut.set.default',
       'config:system.menu.admin',
       'config:system.theme',
+      'CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:form',
       'rendered',
     ];
     $this->assertCacheTags($expected_cache_tags);

core/profiles/standard/tests/src/FunctionalJavascript/StandardPerformanceTest.php

@@ -32,7 +32,11 @@ class StandardPerformanceTest extends PerformanceTestBase {
    */
   protected function setUp(): void {
     parent::setUp();
-
+    // Create a node to be shown on the front page.
+    $this->drupalCreateNode([
+      'type' => 'article',
+      'promote' => NodeInterface::PROMOTED,
+    ]);
     // Grant the anonymous user the permission to look at user profiles.
     user_role_grant_permissions('anonymous', ['access user profiles']);
   }
@@ -41,11 +45,6 @@ class StandardPerformanceTest extends PerformanceTestBase {
    * Tests performance for anonymous users.
    */
   public function testAnonymous() {
-    // Create two nodes to be shown on the front page.
-    $this->drupalCreateNode([
-      'type' => 'article',
-      'promote' => NodeInterface::PROMOTED,
-    ]);
     // Request a page that we're not otherwise explicitly testing to warm some
     // caches.
     $this->drupalGet('search');
@@ -260,7 +259,6 @@ class StandardPerformanceTest extends PerformanceTestBase {
       'SELECT "name", "value" FROM "key_value" WHERE "name" IN ( "twig_extension_hash_prefix" ) AND "collection" = "state"',
       'SELECT "name", "value" FROM "key_value" WHERE "name" IN ( "theme:stark" ) AND "collection" = "config.entity.key_store.block"',
       'SELECT "config"."name" AS "name" FROM "config" "config" WHERE ("collection" = "") AND ("name" LIKE "search.page.%" ESCAPE ' . "'\\\\'" . ') ORDER BY "collection" ASC, "name" ASC',
-      'SELECT "menu_tree"."id" AS "id" FROM "menu_tree" "menu_tree" WHERE ("menu_name" = "account") AND ("expanded" = 1) AND ("has_children" = 1) AND ("enabled" = 1) AND ("parent" IN ("")) AND ("id" NOT IN (""))',
       'SELECT COUNT(*) AS "expression" FROM (SELECT 1 AS "expression" FROM "flood" "f" WHERE ("event" = "user.failed_login_ip") AND ("identifier" = "CLIENT_IP") AND ("timestamp" > "TIMESTAMP")) "subquery"',
       'SELECT "base_table"."uid" AS "uid", "base_table"."uid" AS "base_table_uid" FROM "users" "base_table" INNER JOIN "users_field_data" "users_field_data" ON "users_field_data"."uid" = "base_table"."uid" WHERE ("users_field_data"."name" IN ("ACCOUNT_NAME")) AND ("users_field_data"."default_langcode" IN (1))',
       'SELECT "base"."uid" AS "uid", "base"."uuid" AS "uuid", "base"."langcode" AS "langcode" FROM "users" "base" WHERE "base"."uid" IN (2)',
@@ -286,12 +284,12 @@ class StandardPerformanceTest extends PerformanceTestBase {
     ];
     $recorded_queries = $performance_data->getQueries();
     $this->assertSame($expected_queries, $recorded_queries);
-    $this->assertSame(28, $performance_data->getQueryCount());
+    $this->assertSame(27, $performance_data->getQueryCount());
-    $this->assertSame(85, $performance_data->getCacheGetCount());
+    $this->assertSame(108, $performance_data->getCacheGetCount());
     $this->assertSame(1, $performance_data->getCacheSetCount());
     $this->assertSame(1, $performance_data->getCacheDeleteCount());
     $this->assertSame(1, $performance_data->getCacheTagChecksumCount());
-    $this->assertSame(31, $performance_data->getCacheTagIsValidCount());
+    $this->assertSame(44, $performance_data->getCacheTagIsValidCount());
     $this->assertSame(0, $performance_data->getCacheTagInvalidationCount());
   }
 

core/tests/Drupal/KernelTests/Core/Render/Element/ActionsTest.php

@@ -65,7 +65,7 @@ class ActionsTest extends KernelTestBase implements FormInterface {
     $result = \Drupal::formBuilder()->getForm($this);
     \Drupal::service('renderer')->renderRoot($result);
     $this->assertEquals(['system/base', 'core/drupal.dropbutton'], $result['#attached']['library']);
-    $this->assertEquals(['foo'], $result['#cache']['tags']);
+    $this->assertEquals(['CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:form', 'foo'], $result['#cache']['tags']);
   }
 
 }

core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php

@@ -930,7 +930,7 @@ class FormBuilderTest extends FormTestBase {
     $form_state = new FormState();
     $built_form = $this->formBuilder->buildForm($form_arg, $form_state);
     if (!isset($expected_form_cacheability) || ($method == 'get' && !is_string($token))) {
-      $this->assertFalse(isset($built_form['#cache']));
+      $this->assertEquals($built_form['#cache'], ['tags' => ['CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:form']]);
     }
     else {
       $this->assertTrue(isset($built_form['#cache']));
@@ -952,12 +952,12 @@ class FormBuilderTest extends FormTestBase {
    */
   public static function providerTestFormTokenCacheability() {
     return [
-      'token:none,authenticated:true' => [NULL, TRUE, ['contexts' => ['user.roles:authenticated']], ['max-age' => 0], 'post'],
+      'token:none,authenticated:true' => [NULL, TRUE, ['contexts' => ['user.roles:authenticated'], 'tags' => ['CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:form']], ['max-age' => 0], 'post'],
-      'token:none,authenticated:false' => [NULL, FALSE, ['contexts' => ['user.roles:authenticated']], NULL, 'post'],
+      'token:none,authenticated:false' => [NULL, FALSE, ['contexts' => ['user.roles:authenticated'], 'tags' => ['CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:form']], NULL, 'post'],
       'token:false,authenticated:false' => [FALSE, FALSE, NULL, NULL, 'post'],
       'token:false,authenticated:true' => [FALSE, TRUE, NULL, NULL, 'post'],
-      'token:none,authenticated:false,method:get' => [NULL, FALSE, ['contexts' => ['user.roles:authenticated']], NULL, 'get'],
+      'token:none,authenticated:false,method:get' => [NULL, FALSE, ['contexts' => ['user.roles:authenticated'], 'tags' => ['CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:form']], NULL, 'get'],
-      'token:test_form_id,authenticated:false,method:get' => ['test_form_id', TRUE, ['contexts' => ['user.roles:authenticated']], ['max-age' => 0], 'get'],
+      'token:test_form_id,authenticated:false,method:get' => ['test_form_id', TRUE, ['contexts' => ['user.roles:authenticated'], 'tags' => ['CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:form']], ['max-age' => 0], 'get'],
     ];
   }
 

core/tests/Drupal/Tests/Core/Render/RendererBubblingTest.php

@@ -35,7 +35,7 @@ class RendererBubblingTest extends RendererTestBase {
    */
   public function testBubblingWithoutPreRender() {
     $this->setUpRequest();
-    $this->setupMemoryCache();
+    $this->setUpMemoryCache();
 
     $this->cacheContextsManager->expects($this->any())
       ->method('convertTokensToKeys')
@@ -135,7 +135,7 @@ class RendererBubblingTest extends RendererTestBase {
    */
   public function testContextBubblingEdgeCases(array $element, array $expected_top_level_contexts, $expected_cache_item) {
     $this->setUpRequest();
-    $this->setupMemoryCache();
+    $this->setUpMemoryCache();
     $this->cacheContextsManager->expects($this->any())
       ->method('convertTokensToKeys')
       ->willReturnArgument(0);
@@ -316,7 +316,7 @@ class RendererBubblingTest extends RendererTestBase {
     $current_user_role = &$this->currentUserRole;
 
     $this->setUpRequest();
-    $this->setupMemoryCache();
+    $this->setUpMemoryCache();
 
     $test_element = [
       '#cache' => [
@@ -445,7 +445,7 @@ class RendererBubblingTest extends RendererTestBase {
    */
   public function testBubblingWithPrerender($test_element) {
     $this->setUpRequest();
-    $this->setupMemoryCache();
+    $this->setUpMemoryCache();
 
     // Mock the State service.
     $memory_state = new State(new KeyValueMemoryFactory());
@@ -528,7 +528,7 @@ class RendererBubblingTest extends RendererTestBase {
    */
   public function testOverWriteCacheKeys() {
     $this->setUpRequest();
-    $this->setupMemoryCache();
+    $this->setUpMemoryCache();
 
     // Ensure a logic exception
     $data = [

core/tests/Drupal/Tests/Core/Render/RendererDebugTest.php

@@ -26,7 +26,7 @@ class RendererDebugTest extends RendererTestBase {
    */
   public function testDebugOutput() {
     $this->setUpRequest();
-    $this->setupMemoryCache();
+    $this->setUpMemoryCache();
 
     $element = [
       '#cache' => [

core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php

@@ -564,7 +564,7 @@ class RendererPlaceholdersTest extends RendererTestBase {
    */
   public function testUncacheableParent(array $element, array $args, array $expected_placeholder_render_array, array|false $placeholder_cache_keys, array $bubbled_cache_contexts, array $bubbled_cache_tags, array $placeholder_expected_render_cache_array): void {
     if ($placeholder_cache_keys) {
-      $this->setupMemoryCache();
+      $this->setUpMemoryCache();
     }
     else {
       $this->setUpUnusedCache();
@@ -727,7 +727,7 @@ class RendererPlaceholdersTest extends RendererTestBase {
    * @dataProvider providerPlaceholders
    */
   public function testCacheableParentWithPostRequest(array $test_element, array $args): void {
-    $this->setUpUnusedCache();
+    $this->setUpMemoryCache();
 
     // Verify behavior when handling a non-GET request, e.g. a POST request:
     // also in that case, placeholders must be replaced.
@@ -762,8 +762,13 @@ class RendererPlaceholdersTest extends RendererTestBase {
    *
    * @dataProvider providerPlaceholders
    */
-  public function testPlaceholderingDisabledForPostRequests(array $test_element, array $args): void {
+  public function testPlaceholderingDisabledForPostRequests(array $test_element, array $args, array $expected_placeholder_render_array, array|false $placeholder_cache_keys): void {
+    if ($placeholder_cache_keys && !empty($test_element['placeholder']['#cache']['keys'])) {
+      $this->setUpMemoryCache();
+    }
+    else {
       $this->setUpUnusedCache();
+    }
     $this->setUpRequest('POST');
 
     $element = $test_element;

core/tests/Drupal/Tests/Core/Render/RendererTest.php

@@ -832,7 +832,7 @@ class RendererTest extends RendererTestBase {
    */
   public function testRenderCache($child_access, $expected_tags) {
     $this->setUpRequest();
-    $this->setupMemoryCache();
+    $this->setUpMemoryCache();
 
     // Create an empty element.
     $test_element = [
@@ -882,7 +882,7 @@ class RendererTest extends RendererTestBase {
    */
   public function testRenderCacheMaxAge($max_age, $is_render_cached, $render_cache_item_expire) {
     $this->setUpRequest();
-    $this->setupMemoryCache();
+    $this->setUpMemoryCache();
 
     $element = [
       '#cache' => [
@@ -927,7 +927,7 @@ class RendererTest extends RendererTestBase {
    */
   public function testRenderCacheProperties(array $expected_results) {
     $this->setUpRequest();
-    $this->setupMemoryCache();
+    $this->setUpMemoryCache();
 
     $element = $original = [
       '#cache' => [

core/tests/Drupal/Tests/Core/Render/RendererTestBase.php

@@ -232,7 +232,7 @@ abstract class RendererTestBase extends UnitTestCase {
   /**
    * Sets up a memory-based render cache back-end.
    */
-  protected function setupMemoryCache() {
+  protected function setUpMemoryCache() {
     $this->memoryCache = $this->memoryCache ?: new VariationCache($this->requestStack, new MemoryBackend(new Time($this->requestStack)), $this->cacheContextsManager);
 
     $this->cacheFactory->expects($this->atLeastOnce())