From 816c106e6ee91ce13d5ae575ad61e740f52d4887 Mon Sep 17 00:00:00 2001
From: catch <6915-catch@users.noreply.drupalcode.org>
Date: Mon, 21 Oct 2024 11:51:26 +0100
Subject: [PATCH] =?UTF-8?q?Issue=20#2578855=20by=20ga=C3=ABlg,=20wim=20lee?=
=?UTF-8?q?rs,=20andyf,=20prudloff,=20johnwebdev,=20duaelfr,=20jonathansha?=
=?UTF-8?q?w,=20catch,=20quietone,=20longwave,=20berdir,=20dawehner:=20For?=
=?UTF-8?q?m=20tokens=20are=20now=20rendered=20lazily,=20allow=20forms=20t?=
=?UTF-8?q?o=20opt=20in=20to=20be=20cacheable?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
---
.../Core/Entity/Entity/EntityFormDisplay.php | 5 ++
core/lib/Drupal/Core/Form/FormBuilder.php | 13 +++-
.../src/Controller/ContactController.php | 6 ++
core/modules/node/src/NodeForm.php | 4 ++
.../Tests/Core/Form/FormBuilderTest.php | 64 ++++++++++++++-----
5 files changed, 73 insertions(+), 19 deletions(-)
diff --git a/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
index 8c591c9a4179..c292c53ff6b2 100644
--- a/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
+++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
@@ -206,6 +206,11 @@ public function buildForm(FieldableEntityInterface $entity, array &$form, FormSt
// Associate the cache tags for the form display.
$this->renderer->addCacheableDependency($form, $this);
+ // The form might not have the correct cacheability metadata, so make it
+ // uncacheable by default.
+ // @todo Remove this in https://www.drupal.org/node/3395524.
+ $form['#cache']['max-age'] = 0;
+
// Add a process callback so we can assign weights and hide extra fields.
$form['#process'][] = [$this, 'processForm'];
}
diff --git a/core/lib/Drupal/Core/Form/FormBuilder.php b/core/lib/Drupal/Core/Form/FormBuilder.php
index f31346a37877..be0a22f456f8 100644
--- a/core/lib/Drupal/Core/Form/FormBuilder.php
+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -770,10 +770,17 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
],
],
],
- '#cache' => [
- 'max-age' => 0,
- ],
];
+
+ // If a form hasn't explicitly opted in to caching by setting max-age at
+ // the top level, then make it uncacheable in case it doesn't have the
+ // correct cacheability metadata.
+ // @todo Remove this in the next major version, after the deprecation
+ // process from https://www.drupal.org/project/drupal/issues/3395157
+ // has ended.
+ if (!isset($form['#cache']['max-age'])) {
+ $form['form_token']['#cache']['max-age'] = 0;
+ }
}
}
diff --git a/core/modules/contact/src/Controller/ContactController.php b/core/modules/contact/src/Controller/ContactController.php
index 709ae33245d4..ac512b448581 100644
--- a/core/modules/contact/src/Controller/ContactController.php
+++ b/core/modules/contact/src/Controller/ContactController.php
@@ -81,6 +81,12 @@ public function contactSitePage(?ContactFormInterface $contact_form = NULL) {
$form['#title'] = $contact_form->label();
$form['#cache']['contexts'][] = 'user.permissions';
$this->renderer->addCacheableDependency($form, $config);
+
+ // The form might not have the correct cacheability metadata, so make it
+ // uncacheable by default.
+ // @todo Remove this in https://www.drupal.org/node/3395506.
+ $form['#cache']['max-age'] = 0;
+
return $form;
}
diff --git a/core/modules/node/src/NodeForm.php b/core/modules/node/src/NodeForm.php
index 31c97b3cb4c8..c3893136360e 100644
--- a/core/modules/node/src/NodeForm.php
+++ b/core/modules/node/src/NodeForm.php
@@ -92,6 +92,10 @@ public function form(array $form, FormStateInterface $form_state) {
// parent::form().
$store = $this->tempStoreFactory->get('node_preview');
+ // Because of the temp store integration, this is not cacheable.
+ // @todo add the correct cache contexts in https://www.drupal.org/project/drupal/issues/3397987
+ $form['#cache']['max-age'] = 0;
+
// Attempt to load from preview when the uuid is present unless we are
// rebuilding the form.
$request_uuid = \Drupal::request()->query->get('uuid');
diff --git a/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
index 3137681d18e6..7b99fc5a2c12 100644
--- a/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
+++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
@@ -7,6 +7,7 @@
use Drupal\Component\Utility\Html;
use Drupal\Component\Utility\NestedArray;
use Drupal\Core\Access\AccessResult;
+use Drupal\Core\Cache\Cache;
use Drupal\Core\Cache\Context\CacheContextsManager;
use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
use Drupal\Core\Form\EnforcedResponseException;
@@ -909,7 +910,7 @@ public static function providerTestInvalidToken() {
*
* @dataProvider providerTestFormTokenCacheability
*/
- public function testFormTokenCacheability($token, $is_authenticated, $expected_form_cacheability, $expected_token_cacheability, $method): void {
+ public function testFormTokenCacheability($token, $is_authenticated, $method, $opted_in_for_cache): void {
$user = $this->prophesize(AccountProxyInterface::class);
$user->isAuthenticated()
->willReturn($is_authenticated);
@@ -924,6 +925,10 @@ public function testFormTokenCacheability($token, $is_authenticated, $expected_f
$form['#token'] = $token;
}
+ if ($opted_in_for_cache) {
+ $form['#cache']['max-age'] = Cache::PERMANENT;
+ }
+
$form_arg = $this->createMock('Drupal\Core\Form\FormInterface');
$form_arg->expects($this->once())
->method('getFormId')
@@ -934,19 +939,45 @@ public function testFormTokenCacheability($token, $is_authenticated, $expected_f
$form_state = new FormState();
$built_form = $this->formBuilder->buildForm($form_arg, $form_state);
- if (!isset($expected_form_cacheability) || ($method == 'get' && !is_string($token))) {
+
+ // FormBuilder does not set a form token when:
+ // - #token is set to FALSE.
+ // - #method is set to 'GET' and #token is not a string. This means the GET
+ // form did not get a form token by default, and the form did not
+ // explicitly opt in.
+ if ($token === FALSE || ($method == 'get' && !is_string($token))) {
$this->assertEquals($built_form['#cache'], ['tags' => ['CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:form']]);
- }
- else {
- $this->assertTrue(isset($built_form['#cache']));
- $this->assertEquals($expected_form_cacheability, $built_form['#cache']);
- }
- if (!isset($expected_token_cacheability)) {
$this->assertFalse(isset($built_form['form_token']));
}
+ // Otherwise, a form token is set, but only if the user is logged in. It is
+ // impossible (and unnecessary) to set a form token if the user is not
+ // logged in, because there is no session, and hence no CSRF token.
else {
- $this->assertTrue(isset($built_form['form_token']));
- $this->assertEquals($expected_token_cacheability, $built_form['form_token']['#cache']);
+ // For forms that are eligible for form tokens, a cache context must be
+ // set that indicates the form token only exists for logged in users.
+ $this->assertTrue(isset($built_form['#cache']));
+ $expected_cacheability_metadata = [
+ 'tags' => ['CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:form'],
+ 'contexts' => ['user.roles:authenticated'],
+ ];
+ if ($opted_in_for_cache) {
+ $expected_cacheability_metadata['max-age'] = Cache::PERMANENT;
+ }
+ $this->assertEquals($expected_cacheability_metadata, $built_form['#cache']);
+ // Finally, verify that a form token is generated when appropriate, with
+ // the expected cacheability metadata (or lack thereof).
+ if (!$is_authenticated) {
+ $this->assertFalse(isset($built_form['form_token']));
+ }
+ else {
+ $this->assertTrue(isset($built_form['form_token']));
+ if ($opted_in_for_cache) {
+ $this->assertFalse(isset($built_form['form_token']['#cache']));
+ }
+ else {
+ $this->assertEquals(['max-age' => 0], $built_form['form_token']['#cache']);
+ }
+ }
}
}
@@ -957,12 +988,13 @@ public function testFormTokenCacheability($token, $is_authenticated, $expected_f
*/
public static function providerTestFormTokenCacheability() {
return [
- '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'], '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'], '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'], 'tags' => ['CACHE_MISS_IF_UNCACHEABLE_HTTP_METHOD:form']], ['max-age' => 0], 'get'],
+ 'token:none,authenticated:true' => [NULL, TRUE, 'post', FALSE],
+ 'token:none,authenticated:true,opted_in_for_cache' => [NULL, TRUE, 'post', TRUE],
+ 'token:none,authenticated:false' => [NULL, FALSE, 'post', FALSE],
+ 'token:false,authenticated:false' => [FALSE, FALSE, 'post', FALSE],
+ 'token:false,authenticated:true' => [FALSE, TRUE, 'post', FALSE],
+ 'token:none,authenticated:false,method:get' => [NULL, FALSE, 'get', FALSE],
+ 'token:test_form_id,authenticated:false,method:get' => ['test_form_id', TRUE, 'get', FALSE],
];
}
--
GitLab