Problem/Motivation

  1. #2559011: Ensure form tokens are marked max-age=0 made sure the form token (a CSRF token) was only set for authenticated users, and ensured it specified max-age=0.
  2. #2571995: GET forms shouldn't have CSRF tokens by default refined this to make sure that GET forms don't get a form token by default, i.e. only POST forms get this.
  3. #2463567: Push CSRF tokens for forms to placeholders + #lazy_builder then moves the rendering of the form token into a #lazy_builder callback, which means the rendered form can actually be cached, because the form token is rendered later, and therefore the rendered form is not always by definition bound to the current user/session, which is what made it uncacheable. But it keeps the max-age=0 that point 1 introduced, because removing that merits further discussion.
  4. This issue is about removing the max-age=0 that point 1 introduced, and having that further discussion.

#2552873-18: node/1 flamegraphs also points out how #2571909: CommentForm selects using the user formatted name caused a very big performance regression. #2571909 made the comment form no longer personalized per user, so we thought we made the form cacheable. But we forgot about the form token setting max-age=0, which then makes the full node display uncacheable!

Proposed resolution

Allow forms to opt-in to be cacheable but still default to uncacheable for BC. Then, #3395157: Deprecate not giving cacheability metadata to forms.

Remaining tasks

Move the #access boolean bug fix to a new related issue
Create follow-ups for cacheability metadata fixes (see #100)
Maybe create issues for future major versions of core, according to #101 : 3. and 4.

User interface changes

None.

API changes

None, in the scope of this issue.

Data model changes

None.

CommentFileSizeAuthor
#118 interdiff-2578855-117-118.txt609 bytesprudloff
#118 drupal-2578855-118.patch35.63 KBprudloff
#117 drupal-2578855-117.patch35.24 KBprudloff
#114 drupal-2578855-114-9.5.x.patch35.53 KBAlbert Volkman
#112 2578855-nr-bot.txt158 bytesneeds-review-queue-bot
#108 drupal-2578855-108-9.4.3.patch35.5 KBGaëlG
#107 drupal-2578855-102-9.3.12.patch37.63 KBGaëlG
#107 drupal-2578855-102-9.5.x.patch37.63 KBGaëlG
#103 drupal-2578855-103.patch36.65 KBQuentin Massez
#102 drupal-2578855-102.patch37.59 KBprudloff
#99 drupal-2578855-99.patch37.52 KBprudloff
#96 drupal-2578855-96.patch37.41 KBprudloff
#95 interdiff_92-95.txt2.27 KBnikitagupta
#95 2578855-95.patch37.27 KBnikitagupta
#92 2578855-92.patch37.35 KBQuentin Massez
#89 2578855-89.patch36.77 KBGaëlG
#88 2578855-88.patch36.81 KBprudloff
#87 2578855-87.patch37.23 KBGaëlG
#83 2578855-83.patch36.86 KBDuaelFr
#83 interdiff-2578855-81-83.txt9.18 KBDuaelFr
#81 interdiff_75-81.txt2.3 KBjohnwebdev
#81 2578855-81.patch37.22 KBjohnwebdev
#76 interdiff_74-75.txt1.72 KBjohnwebdev
#76 2578855-75.patch38.13 KBjohnwebdev
#74 2578855-74.patch37.47 KBjohnwebdev
#74 interdiff_71-74.txt2.45 KBjohnwebdev
#71 interdiff_67-71.txt585 bytesjohnwebdev
#71 2578855-71.patch37.26 KBjohnwebdev
#68 interdiff_65-67.txt2.52 KBjohnwebdev
#67 2578855-67.patch36.95 KBjohnwebdev
#65 cacheable_forms-2578855-65.patch33.77 KBAndyF
#65 interdiff_63-65.txt3.42 KBAndyF
#39 interdiff.txt5.52 KBWim Leers
#39 cacheable_forms-2578855-39.patch29.34 KBWim Leers
#36 interdiff.txt849 bytesWim Leers
#36 cacheable_forms-2578855-36.patch27.89 KBWim Leers
#35 cacheable_forms-2578855-35.patch28.68 KBWim Leers
#22 interdiff.txt2.4 KBWim Leers
#22 cacheable_forms-2578855-22.patch28.82 KBWim Leers
#17 interdiff.txt1.44 KBWim Leers
#17 cacheable_forms-2578855-17.patch26.5 KBWim Leers
#15 interdiff.txt11.69 KBWim Leers
#15 cacheable_forms-2578855-15.patch25.11 KBWim Leers
#12 interdiff.txt5.9 KBWim Leers
#12 cacheable_forms-2578855-12.patch13.49 KBWim Leers
#9 interdiff.txt4.22 KBWim Leers
#9 cacheable_forms-2578855-9.patch7.74 KBWim Leers
#4 interdiff.txt2.96 KBWim Leers
#4 cacheable_forms-2578855-4.patch3.61 KBWim Leers
#3 cacheable_forms-2578855-3.patch681 bytesWim Leers
#40 cacheable_forms-2578855-40.patch29.34 KBWim Leers
#40 interdiff.txt2.08 KBWim Leers
#43 cacheable_forms-2578855-42.patch30.14 KBWim Leers
#43 interdiff.txt2.68 KBWim Leers
#45 cacheable_forms-2578855-45.patch32.11 KBWim Leers
#45 interdiff.txt2.68 KBWim Leers
#61 reroll_diff_45-61.txt34.77 KBAndyF
#61 cacheable_forms-2578855-61.patch32.4 KBAndyF
#63 interdiff_61-63.txt3.7 KBAndyF
#63 cacheable_forms-2578855-63.patch30.91 KBAndyF

Issue fork drupal-2578855

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

#2552873-18: node/1 flamegraphs also points out how #2571909: CommentForm selects using the user formatted name caused a very big performance regression. #2571909 made the comment form no longer personalized per user, so we thought we made the form cacheable. But we forgot about the form token setting max-age=0, which then makes the full node display uncacheable!

Wim Leers’s picture

Status: Active » Needs review
FileSize
681 bytes

So this is basically what we want to do. I know some unit tests break. Let's see what else breaks.

Wim Leers’s picture

The "contact" forms end up being cached incorrectly due to missing cacheability metadata. Here's a fix for that.

In fixing that, discovered that ContactAuthenticatedUserTest is only passing because it only does assertions that verify certain things are absent. But it's a 404 response, so everything is absent.

The last submitted patch, 3: cacheable_forms-2578855-3.patch, failed testing.

The last submitted patch, 3: cacheable_forms-2578855-3.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 4: cacheable_forms-2578855-4.patch, failed testing.

The last submitted patch, 4: cacheable_forms-2578855-4.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
7.74 KB
4.22 KB

Here we go, unit tests updated. The unit test became approximately two orders of magnitude easier to understand.

Status: Needs review » Needs work

The last submitted patch, 9: cacheable_forms-2578855-9.patch, failed testing.

The last submitted patch, 9: cacheable_forms-2578855-9.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
13.49 KB
5.9 KB

A bunch of fixes, still working on the rest.

Status: Needs review » Needs work

The last submitted patch, 12: cacheable_forms-2578855-12.patch, failed testing.

The last submitted patch, 12: cacheable_forms-2578855-12.patch, failed testing.

Wim Leers’s picture

By fixing the missing language_select-related cacheability metadata, I uncovered a few bits of unpleasantness. The biggest one of those is also the first real surprise: turns out #2487600: #access should support AccessResultInterface objects or better has to always use it was not 100% correct/complete; the cacheability metadata for an access result on #access is lost, but only when A) access is denied (i.e. an early return occurs) and B) it is a child of an element being rendered through a template.
This reroll fixes that and adds missing test coverage.

The way I discovered this, is thanks to the language_select field depending on the ContentLanguageSettings config entity, of which one exists per entity bundle. The settings stored in that config entity determine if the widget to select a language shows up at all, and which values are available. And due to missing cacheability metadata, that is exactly the information that was missing.
Turns out that there is both a field access hook (language_entity_field_access()) and a plain form alter (language_form_alter()) cause #access to be set to forbid access. I think we only need to keep one of them, but fixing that is out of scope for this issue. What did need to be fixed here, was their cacheability metadata: they both depended on the corresponding contentLanguageSettings config entity, but they omitted this cacheability metadata.
Finally, to make matters worse, ContentLanguageSettings::loadByEntityTypeBundle() creates ephemeral instances of this config entity (i.e. ones that aren't saved), which has some default values. This is fine until you deal with cacheability metadata: then you need to be notified of changes, and in this case, we aren't, because only when the language settings are first customized, the config entity is actually saved, which means only changes after that point will cause the proper invalidations. I've added a work-around for this. It can be made obsolete later, but this is a perfectly fine, perfectly acceptable work-around.
For all of this, I've also added test coverage.

Status: Needs review » Needs work

The last submitted patch, 15: cacheable_forms-2578855-15.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
26.5 KB
1.44 KB

This fixes the node form related test failures. This was quite simple: max-age=0 is the appropriate choice, because the node form tightly integrates with TempStore. For analogous reasons, it never makes sense to cache the node preview (nor does that make sense at all) — this already was the case, this interdiff just slightly refines that.

moshe weitzman’s picture

because the node form tightly integrates with TempStore.

Does the mere viewing of the form interact with tempstore? Or does that happen once you submit the form?

Wim Leers’s picture

Does the mere viewing of the form interact with tempstore? Or does that happen once you submit the form?

Yes, the mere viewing: it retrieves data from TempStore, and for TempStore we don't have cache tags, so we can't know when it has changed, hence the need for max-age=0.

moshe weitzman’s picture

Thanks for clarifying. This pattern is pretty bad for any form aspires to show up on a non-admin page. I wouldnt know where to document such a thing.

Status: Needs review » Needs work

The last submitted patch, 17: cacheable_forms-2578855-17.patch, failed testing.

Wim Leers’s picture

This fixes the Search form test failures, but that will really only be fixable once #2464409: Search results should bubble rendered entity cache tags and set list cache tags lands, so I've added a @todo for that.

Status: Needs review » Needs work

The last submitted patch, 22: cacheable_forms-2578855-22.patch, failed testing.

The last submitted patch, 15: cacheable_forms-2578855-15.patch, failed testing.

The last submitted patch, 17: cacheable_forms-2578855-17.patch, failed testing.

The last submitted patch, 22: cacheable_forms-2578855-22.patch, failed testing.

Wim Leers’s picture

Issue tags: +rc target triage, +Performance

I suspect this should now be done in Drupal 8.1, but given the fact that this can yield an enormous speedup on /node/X pages (i.e. pages where the comment form is visible), it probably should at least be triaged to determine whether it should be an RC target or not.

moshe weitzman’s picture

Yeah, this meets the Performance Critical criteria listed at #1744302: [meta] Resolve known performance regressions in Drupal 8. See recent node/1 flame graphs. Furthermore, popular contrib/custom forms can appear on almost all pages (e.g. fivestar widget).

Berdir’s picture

It is a big performance improvement but it's also a rather big behavior change. I don't really see how we'd do this exactly without some sort of BC break? Unless we make it configurable, but then we'd essentially require a module that displays a form in e.g. a block to deal with both possible values of that option?

Maybe we can allow forms to opt-in, possibly by setting max-age to a non-zero value themself?

I know we had exactly this kind of discussion before and I know that Wim really doesn't like opt-in performance improvements, but in the previous discussions, we weren't in the RC phase yet.

catch’s picture

Maybe we can allow forms to opt-in, possibly by setting max-age to a non-zero value themself?

That makes sense to me, I don't think we can change the form behaviour across the board at the moment.

Even with making the behaviour opt-in there's a bit of a change for form_alters/widgets that they'll be expected to provide correct cacheability metadata whereas at the moment they might not and could still work by accident, but that seems OK to me considering the performance gain.

moshe weitzman’s picture

I'll argue that we worked on criteria for Performance Critical for a reason. We are allowed to break BC for Criticals. If we can get a (tentative) green light here, I'm hoping that @wimleers will clear up the final fails.

xjm’s picture

Issue summary: View changes

Linking the discussion of the rc-target-ness of this in the summary.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
    @@ -171,7 +171,7 @@ public function buildForm(FieldableEntityInterface $entity, array &$form, FormSt
    -        $form[$name]['#access'] = $items->access('edit');
    +        $form[$name]['#access'] = $items->access('edit', NULL, TRUE);
    

    I'm confused, didn't we threw an exception when #access was not an object?

  2. +++ b/core/modules/editor/src/Element.php
    @@ -53,6 +53,7 @@ function preRenderTextFormat(array $element) {
    +    $element['#cache']['tags'][] = 'config:editor_list';
    
    +++ b/core/modules/filter/src/Element/TextFormat.php
    @@ -210,6 +210,8 @@ public static function processFormat(&$element, FormStateInterface $form_state,
     
    +    $element['#cache']['tags'][] = 'config:filter_format_list';
    +
    
    +++ b/core/modules/language/language.module
    @@ -163,6 +163,7 @@ function language_process_language_select($element) {
    +    $element['#cache']['tags'][] = 'config:language_content_settings_list';
    

    Don't we want to use \Drupal::entityManager()->getDefinition('editor')->getListCacheTags()

  3. +++ b/core/modules/node/src/Controller/NodePreviewController.php
    @@ -26,6 +26,7 @@ public function view(EntityInterface $node_preview, $view_mode_id = 'full', $lan
         unset($build['#cache']);
    +    $build['#cache']['max-age'] = 0;
    

    Do we actually need both lines, it feels a bit redundant. At least it could be merged into one line, if needed

  4. +++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
    @@ -518,6 +518,11 @@ public function searchFormAlter(array &$form, FormStateInterface $form_state) {
    +        'contexts' => [
    +          'url.query_args',
    +        ],
    

    I guess its not worth to specify the exact gazillion possible query parameters here ...

  5. +++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
    @@ -845,19 +845,32 @@ public function testFormTokenCacheability($token, $is_authenticated, $expected_f
    +    // 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.
    

    Just a general question, you could have an anonymous user session, at least Drupal supports that, right? Do we need to take that into account somewhere?

catch’s picture

@Moshe is #30 not a tentative green light?

Wim Leers’s picture

FileSize
28.68 KB

First, a straight reroll of #22.

(Exact same changes, the context has just become slightly smaller, hence slightly different patch size.)

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
27.89 KB
849 bytes

#22 said

This fixes the Search form test failures, but that will really only be fixable once #2464409: Search results should bubble rendered entity cache tags and set list cache tags lands, so I've added a @todo for that

That issue has since landed. So, removed that work-around.

This change in #22 fixed 5 fails that existed in #17. Let's see how many return. (I tested the first two, and neither returned.)

Status: Needs review » Needs work

The last submitted patch, 36: cacheable_forms-2578855-36.patch, failed testing.

Wim Leers’s picture

So, comparing #36 to #22 (sadly #35 didn't run tests because I stupidly forgot to mark it NR):

  • Still 5 fails in Aggregator
  • Only 1 instead of 2 fails in Comment
  • Still 3 fails in Field
  • 2 fails that have reappeared because of #36's revert.
  • Still 1 fail in Taxonomy

So, Comment's cacheability metadata was improved in the last weeks. And Search will still need some fixing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
5.52 KB
29.34 KB

#29:

I know we had exactly this kind of discussion before and I know that Wim really doesn't like opt-in performance improvements, but in the previous discussions, we weren't in the RC phase yet.

Indeed, at this time, it must be opt-in. Even a year ago, it would have to have been opt-in, because forms are everywhere.

#33:

  1. No, #2487600: #access should support AccessResultInterface objects or better has to always use it made it possible to use AccessResultInterface objects. Requiring them to always be AccessResultInterface objects would have caught this problem months ago… :(
  2. That'd be better, yes. Done.
  3. Agreed, done.
  4. Future optimization :)
  5. Just a general question, you could have an anonymous user session, at least Drupal supports that, right?

    Yes, of course.

    But … Drupal has never set a form token for anonymous users! This is copied from FormBuilder when it was created in #2112807: Move the form builder functions in form.inc to a form service, where it was just copied from form.inc, >2 years ago (revision 82eb2924cfcc40dcedd981e4462176a6d9256f0f):

        // Add a token, based on either #token or form_id, to any form displayed to
        // authenticated users. This ensures that any submitted form was actually
        // requested previously by the user and protects against cross site request
        // forgeries.
        // This does not apply to programmatically submitted forms. Furthermore,
        // since tokens are session-bound and forms displayed to anonymous users are
        // very likely cached, we cannot assign a token for them.
        // During installation, there is no $user yet.
        if ($user && $user->isAuthenticated() && !$form_state['programmed']) {
          // Form constructors may explicitly set #token to FALSE when cross site
          // request forgery is irrelevant to the form, such as search forms.
          if (isset($form['#token']) && $form['#token'] === FALSE) {
            unset($form['#token']);
          }
          // Otherwise, generate a public token based on the form id.
          else {
            $form['#token'] = $form_id;
            $form['form_token'] = array(
              '#id' => Html::getUniqueId('edit-' . $form_id . '-form-token'),
              '#type' => 'token',
              '#default_value' => $this->csrfToken->get($form['#token']),
              // Form processing and validation requires this value, so ensure the
              // submitted form value appears literally, regardless of custom #tree
              // and #parents being set elsewhere.
              '#parents' => array('form_token'),
            );
          }
        }
    

    And in Drupal 7's drupal_prepare_form():

    if (!empty($user->uid) && !$form_state['programmed']) {
    
Wim Leers’s picture

This fixes the last fail in Comment.

The last submitted patch, 39: cacheable_forms-2578855-39.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 40: cacheable_forms-2578855-40.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
30.14 KB
2.68 KB

This makes cacheable forms opt-in for the first time!

It opts in the following two forms:

  1. CommentForm
  2. MessageForm (for /contact)

Next: removing as many of the changes as possible, to make this patch as simple as possible. Now that we don't aim to make all forms cacheable, we can defer many of the bugfixes in this patch to follow-up issues against 8.1.

Wim Leers’s picture

Ugh, #40's interdiff was correct, but the patch didn't contain it, it was identical to #39… Sorry about that.

Wim Leers’s picture

Ugh, it's worse. #43 contains the patch matching the #40 interdiff. So at least we'll know the effect of just that.

The attachments to this comment are associated with the text written in #43 (even though this interdiff is identical to that in #43).

The last submitted patch, 43: cacheable_forms-2578855-42.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 45: cacheable_forms-2578855-45.patch, failed testing.

xjm’s picture

Issue tags: -rc target triage

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

nielsaers’s picture

Does anyone have a status update on this issue?

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Mile23’s picture

Version: 8.4.x-dev » 8.5.x-dev
Issue tags: +Needs change record

I'm not sure if this is an issue or not still, because there's not a lot of good documentation to tell me what to expect in terms of form caching.

So here's a bump. :-)

Wim Leers’s picture

Forms aren't cacheable yet, so this is still an issue.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

DuaelFr’s picture

This would be a huge game changer!
@Wim Leers, do you still plan to work on this? <3

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

AndyF’s picture

Status: Needs work » Needs review
FileSize
34.77 KB
32.4 KB

Here's a reroll of #45, thanks!

Status: Needs review » Needs work

The last submitted patch, 61: cacheable_forms-2578855-61.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

AndyF’s picture

Status: Needs work » Needs review
FileSize
30.91 KB
3.7 KB

So we're seeing one or two extra failures (: New patch removes use of \Drupal::entityManager().

The last submitted patch, 63: cacheable_forms-2578855-63.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

AndyF’s picture

  • Fix a couple of tests calling assertCacheContext() without the corresponding test trait.
  • Update FormBuilder::handleInputElement() to support #access being an object. (I think there was just one bit, at the end of a looong line, where it hadn't already been updated. I kinda wanted to shorten the line as part of the patch but didn't want to add too much extra noise.)

Thanks!

Status: Needs review » Needs work

The last submitted patch, 65: cacheable_forms-2578855-65.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

johnwebdev’s picture

This should fix some more failing tests.

johnwebdev’s picture

FileSize
2.52 KB
+++ b/core/modules/field/tests/modules/field_test_boolean_access_denied/field_test_boolean_access_denied.module
@@ -14,5 +14,5 @@
+  return AccessResult::forbiddenIf($field_definition->getName() === \Drupal::state()->get('field.test_boolean_field_access_field'))->addCacheTags(['field_test_boolean_field_access_field']);

+++ b/core/modules/field/tests/src/Functional/Boolean/BooleanFieldTest.php
@@ -240,6 +242,8 @@ public function testFormAccess() {
+    Cache::invalidateTags(['field_test_boolean_field_access_field']);

I'm not entirely sure this was the right fix. I might have misinterpreted #15.

jonathanshaw’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 67: 2578855-67.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

johnwebdev’s picture

Status: Needs work » Needs review
FileSize
37.26 KB
585 bytes

So the reasons why we get:

Behat\Mink\Exception\ExpectationException: The text "config:language_content_settings_list" was not found anywhere in the "X-Drupal-Cache-Tags" response header.

is that it actually does not exists anymore! It was removed in #2572125: Content translation local tasks are not getting displayed due to caching in favour of using the "rendered" cache tag instead. I am wondering if that should be changed? But for now, I'm adding back the cache tag to the definition. Perhaps we will do this in a separate issue, unless we decide that having "rendered" on the forms are okay, and if so I will revert this change and change the tests to look for rendered instead.

johnwebdev’s picture

Assigned: Wim Leers » Unassigned
Issue tags: +Needs issue summary update

Unassigning Wim. Adding needs issue summary update.

Status: Needs review » Needs work

The last submitted patch, 71: 2578855-71.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

johnwebdev’s picture

Status: Needs work » Needs review
FileSize
2.45 KB
37.47 KB

Turns out that the entity forms gets cacheable, because EntityFormDisplay associates its cacheable metadata on ::buildForm to the $form element which is Cache::PERMANENT by default.

This patch changes the original idea by using the top level #cache, to instead introduce an arbitrary key #form_cacheable, this feels like a more safe way to ensure BC. This would also allow you to explicitly tell if a form is intended to be cacheable or not.

Status: Needs review » Needs work

The last submitted patch, 74: 2578855-74.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

johnwebdev’s picture

Status: Needs work » Needs review
FileSize
38.13 KB
1.72 KB

Should be green now!

johnwebdev’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update
Wim Leers’s picture

This patch changes the original idea by using the top level #cache, to instead introduce an arbitrary key #form_cacheable, this feels like a more safe way to ensure BC. This would also allow you to explicitly tell if a form is intended to be cacheable or not.

Interesting. This is both deeply concerning but potentially also necessarily pragmatic.

Can you explain some more detail why we cannot use #cache, why we must introduce a new #form_cacheable property? This has DX impact that we'll be carrying forward for many years, so it's probably the most critical thing to assess in this patch.


  1. +++ b/core/modules/ckeditor/tests/src/Functional/CKEditorLoadingTest.php
    @@ -132,6 +133,7 @@ public function testLoading() {
    +    Cache::invalidateTags(['rendered']);
    

    Why this cache tag?

  2. +++ b/core/modules/comment/tests/src/Kernel/CommentDefaultFormatterCacheTagsTest.php
    @@ -147,10 +149,10 @@ public function testCacheTags() {
    +    // non-zero max-age despite the lazy-built form.
    

    Wouldn't "thanks to the comment form being cacheable" be more accurate than "despite the lazy-built form"?

  3. +++ b/core/modules/contact/tests/src/Functional/ContactSitewideTest.php
    @@ -51,15 +53,25 @@ protected function setUp() {
    +    $this->assertResponse(200);
    

    Why assert this?

  4. +++ b/core/modules/contact/tests/src/Functional/ContactSitewideTest.php
    @@ -51,15 +53,25 @@ protected function setUp() {
    +    $this->assertCacheContext('user');
    

    I think this should then also assert that this particular response is not cached by Dynamic Page Cache.

johnwebdev’s picture

Can you explain some more detail why we cannot use #cache, why we must introduce a new #form_cacheable property

So, I discovered that the entity forms became cached (#74), although we never did any changes to explicitly make them so.

Entity forms already associates cache metadata on the top level form element! (EntityFormDisplay::195). To me, it seems counter-intuitive to change exisiting behaviour to ensure backwards compatibility when the end goal is the direct opposite - we want the forms to be cacheable.

If contrib/custom forms associates cacheable metadata on the top level form element (perhaps unlikely), those would potentially become cacheable which means we cannot guarantee BC with this approach.

With the #form_cacheable property; we can also provide a clear path forward. Fix cacheable metadata issues, work towards having all forms cacheable and then at some point; flip the switch.

Will address the code review later.

AndyF’s picture

johnwebdev’s picture

Addresses #78

1. Removed.
2. Fixed.
3. Removed.
4. Fixed.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

DuaelFr’s picture

  • Rerolled the patch for 9.1.x.
  • Cleaned up some unused use statements.
  • Replaced deprecated assertions introduced by this patch (assertRaw, assertIdentical, assertCacheTag) by their non-deprecated equivalent.

Status: Needs review » Needs work

The last submitted patch, 83: 2578855-83.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

AndyF’s picture

Status: Needs work » Needs review

I think the previous test failure had been down to the intermittent failure from #3066447: [random test failure] Random failures building media library form after uploading image (WidgetUploadTest): moving to NR.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

GaëlG’s picture

This one is for 8.9.6.

prudloff’s picture

Here is a reroll for Drupal 9.1.4.

(I can't generate the interdiff because it fails for some reason with Error applying patch1 to reconstructed file.)

GaëlG’s picture

And this one is for 9.1.5.

moshe weitzman’s picture

I'm ok with #form_cacheable key - seems like a small inelegance compared to this sitting in the queue for years.

Is there anything preventing the search form and login form from being #form_cacheable? Those appear on every page of many sites.

Berdir’s picture

Login form is rarely displayed for authenticated users :)

Search form might already have some tricks to not have a form_id/token as it is a GET form so might also already be cacheable, not sure? Views exposed filters have that too.

Quentin Massez’s picture

Here is a reroll for 9.1.7

renatog’s picture

Status: Needs review » Needs work

Please put a full stop at the end of these comments below:

// Log in as a different user and confirm that

And

// form token by default, and this form did not explicitly opt in)

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

nikitagupta’s picture

Status: Needs work » Needs review
FileSize
37.27 KB
2.27 KB
prudloff’s picture

Here is a reroll for Drupal 9.2.0.

longwave’s picture

+++ b/core/modules/comment/src/CommentForm.php
@@ -88,6 +88,9 @@ public function __construct(EntityRepositoryInterface $entity_repository, Accoun
+    $form['#form_cacheable']['max-age'] = TRUE;

+++ b/core/modules/contact/src/MessageForm.php
@@ -99,6 +99,9 @@ public static function create(ContainerInterface $container) {
+    $form['#form_cacheable'] = TRUE;

The first instance here looks wrong, #form_cacheable should just be a boolean flag and not an array?

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

prudloff’s picture

A reroll for Drupal 9.3.0.

jonathanshaw’s picture

This issue has stalled partly because of scope creep.

#74 / #79 offers a strong BC approach, let's use it. But let's use also the strong BC opt-in quality it offers to reduce the scope as close to #3 as possible.

We can do this by:
- opening follow-up issues for every core form we want to opt in, instead of opting them in here and fixing all their caching bugs here.
- open a new issue for the separate bug identified in #15.

jonathanshaw’s picture

I've come to think that #74 / 79 is mistaken.

We don't need #form_cacheable, we can do this with max-age as suggested in #29 / 30. e.g.

+ // By default, make the form uncacheable. Allow forms to opt in to be
+ // cached by explicitly specifying the #cache['max-age] key at the top level
+ // of the form.
+ if (!isset($form['#cache']['max-age'])) {
+ $form['form_token']['#cache']['max-age'] = 0;
+ }
+ }

In reply to #79:

Can you explain some more detail why we cannot use #cache, why we must introduce a new #form_cacheable property

So, I discovered that the entity forms became cached (#74), although we never did any changes to explicitly make them so.

Entity forms already associates cache metadata on the top level form element! (EntityFormDisplay::195). To me, it seems counter-intuitive to change exisiting behaviour to ensure backwards compatibility when the end goal is the direct opposite - we want the forms to be cacheable.

It's counter-intuitive but unproblematic. The entity form display cache data is currently unused, all we need to do is add a temporary explicit max-age=0 to it with a @todo to remove that in a follow-up issue. This is less problematic than YetAnotherMysteriousRenderArrayKey.

If contrib/custom forms associates cacheable metadata on the top level form element (perhaps unlikely), those would potentially become cacheable which means we cannot guarantee BC with this approach.

@catch mentioned this in #30 and decided it was an acceptable trade-off. Isn't it rather unlikely that someone was explicitly setting a non-zero max-age on a form that shouldn't be cached?

With the #form_cacheable property; we can also provide a clear path forward. Fix cacheable metadata issues, work towards having all forms cacheable and then at some point; flip the switch.

I think the BC roadmap should be something like this:
1. Explicitly specify max-age 0 on core forms
2.(a) In this issue, make it possible for forms to opt in to cacheability by setting non-zero explicit max-age.
(b) In this issue, deprecate NOT setting max-age on a form. i.e. throw a deprecation warning saying this is now required.
3. In D10, throw an error if a form does not specify max-age
4. In D11, remove the error and make forms cacheable by default.

prudloff’s picture

Here is a reroll of #99 for 9.3.6.

Quentin Massez’s picture

Here is a reroll of #102 for 9.3.12

Status: Needs review » Needs work

The last submitted patch, 103: drupal-2578855-103.patch, failed testing. View results

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

prudloff’s picture

It looks like #103 dropped the updated condition in handleInputElement().

GaëlG’s picture

GaëlG’s picture

Reroll after SA-CORE-2022-013. Applies to 9.4.3 and current 9.5.x

GaëlG’s picture

Status: Needs work » Needs review
longwave’s picture

I agree with #101. Let's not introduce new form keys when we don't have to.

And a further suggestion that I haven't fully thought through, but will throw out there: should forms start implementing CacheableDependencyInterface? FormBase could provide the defaults and max-age could be overridden there? This means we are more OO instead of tying ourselves to more array keys.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
FileSize
158 bytes

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

prudloff’s picture

Albert Volkman’s picture

Re-roll for 9.5.x

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Anybody’s picture

Very much like #110 by @longwave, I think that would be more clear and better DX.

BTW CAPTCHA has a long outstanding issue with caching (most of us know it, I guess): Pages with captcha can't be cached (#3311447: Find a way to allow page caching (including by CDNs) with captcha. AJAX, Lazy, ...?)
Perhaps CAPTCHA could also benefit from this a lot, and thereby ten thousand Drupal installations having CTA forms on many pages.
For that reason and importance, I'll reference the issue here as possible (very widely used) contrib follow-up.

prudloff’s picture

FileSize
35.24 KB

Here is a reroll for 10.1.

prudloff’s picture

#117 triggers a "Bubbling failed." assertion error.
This seems to fix it.

GaëlG’s picture

We use and re-roll this useful patch for ages, it may be the right time to stick my head in it and try to move it forward!

What the current patch contains:

  1. A removal of the bad-for-performance default cache max-age set to zero in FormBuilder.php. With only this, bugs happen because some forms do not declare well their cacheability metadata.
  2. A system based on the new opt-in form key #form_cacheable which:
    • - makes forms still uncacheable (to prevent bugs), but:
      - only if #form_cacheable is not explicitly set to TRUE
      - and by acting on subform $form['form_token'], not on "root" $form (because it makes it clearer that it's the token which prevents the form from being cacheable)
    • - changes some core forms so that they become cacheable, by adding the correct cacheability metadata - and tell they now are, with #form_cacheable:
      - CommentForm (with an update to the corresponding test: CommentDefaultFormatterCacheTagsTest.php)
      - MessageForm (+ContactSitewideTest.php)
      - NodeForm
    • - adapt a unit test for that: FormBuilderTest.php
  3. Correct cacheability metadata on some other form elements (+test changes) :
    - core/modules/editor/src/Element.php (+ EditorLoadingTest.php + EditorSecurityTest.php)
    - core/modules/filter/src/Element/TextFormat.php
    - NodeSearch.php (+SearchAdvancedSearchFormTest.php )
  4. Some code to ensure node forms remain uncacheable (NodePreviewController.php, NodeForm.php) (comment #17, but I guess it's now useless as form cacheability has been switched from default to optin during discussions)
  5. A bug fix (+test coverage in field_test_boolean_access_denied.module, BooleanFieldTest.php, FormTest.php ) for the fact that form widgets #access default value is sometimes a boolean whereas it should be an AccessResultInterface object. See comment #15.
  6. A work-around introduced in #15 to get rid of a problem at rendering (in Renderer.php, RendererTest.php )
  7. A bug fix and tests related to language (see comment #15) : language.module, LanguageConfiguration.php, ContentLanguageSettings.php, LanguageConfigurationElementTest.php, NodeTypeInitialLanguageTest.php, EntityTranslationFormTest.php
  8. Something to get rid of a bubbling error (#118)

What needs to be changed in the patch (probably rather by creating an issue branch from scratch), according to discussions above (#100, #101, #110, #116 mainly):

  1. Update the issue summary with part of this comment
  2. Do not keep the #form_cacheable system
  3. Do not keep the various cacheability metadata fixes, nor their unit tests (all of these will be handled in follow-ups, see #100)
  4. Initiate those follow-ups
  5. Do not keep the probably useless changes in NodePreviewController.php and NodeForm.php
  6. Move the #access bug fix to a new related issue
  7. Do not keep the Renderer.php/RendererTest.php workarounds (not sure about this one, but I guess it's needed only for some follow-up)
  8. Same for the language bug fix
  9. Same for the bubbling error #118
  10. Make forms implement CacheableDependencyInterface so that they provide cacheability metadata
  11. in FormBase, provide the default implementation of the interface, which set the max-age to zero as soon as there's a form token
  12. If there's still a problem with entity forms (see #74 and #101), "fix" it by explicitly marking them uncacheable + creating a @todo follow-up to make them cacheable correctly
  13. Deprecate this base implementation (so that developers are encouraged to implement it with correct metadata)
  14. Create issues for future major versions of core, according to #101 : 3. and 4.
GaëlG’s picture

Assigned: Unassigned » GaëlG
Issue tags: +DrupalCon Lille 2023
GaëlG’s picture

Issue summary: View changes
GaëlG’s picture

I guess we cannot just make FormInterface implement CacheableDependencyInterface because it would break any existing form directly implementing FormInterface without extending FormBase.
So... we could create something like CacheableFormInterface (it is the right name, as actually it could be used for an uncacheable form?) which would of course implement both CacheableDependencyInterface and FormInterface.
Then, where the buildForm() method is "consumed" (I got no time to figure out where it is for now), check if the form implements CacheableFormInterface and if so, add the #cache to the renderable array.
And progressively, make classes implementing FormInterface implement CacheableFormInterface.

GaëlG’s picture

Actually, I attended @xjm's DrupalCon Lille session about core issue reviews yesterday (https://t.co/Z5YLHtZwqr), and I'm now approaching this issue with a new look.
It started 8 years ago, has 62 followers and still, it seems it's not even close to be committed. I guess it's a good example of longstanding issue because of "scope creep", as already mentioned in #100. We need to break this issue into small pieces.

So, even if we all seem to like the CacheableDependencyInterface idea from #110 (me too), it may not be a good idea to do this within the scope of this issue. After all, we already have something to give cacheability metadata: the #cache which comes with any render array (and FormInterface::buildForm returns a render array). So, like for blocks, using CacheableDependencyInterface would allow developers to use a more OOP way to give cacheability metadata, but they could still use #cache directly. So let's stick to #cache for now, and the CacheableDependencyInterface can be a follow-up.

At least that's my view and the way I'll try to work on the issue (yes, I also saw the Sarah Furness keynote about not having fear of making mistakes 😀).

The very first thing we need, and probably the only one that should be done within this issue, is: allow forms to opt-in to be cacheable. The simplest way possible.

catch’s picture

CacheableFormInterface(is it the right name, as actually it could be used for an uncacheable form?)

All I can think of instead is CacheAwareFormInterface, not sure that is better.

jonathanshaw’s picture

I guess we cannot just make FormInterface implement CacheableDependencyInterface because it would break any existing form directly implementing FormInterface without extending FormBase.

I believe we have a 1:1 interface:base class BC policy rule that allows us to assume anything implementing the interface extends the base class.

jonathanshaw’s picture

A good solution to reducing the scope of this issue and making patches easier to review can be:
1. Create a patch here that exposes cache bugs in existing forms
2. Open other issues to fix each of those bugs
3. Come back to this issue when the bugs are fixed

I've worked this way before with @catch, I'm happy to rtbc the bug patches.

GaëlG’s picture

#126: Oh that's a good news! Anyway, as said in #123, I guess this CacheableDependencyInterface thing can be done in another issue: https://www.drupal.org/project/drupal/issues/3395157

#127: Thank you, this may indeed be a good way to do! For now I'm still trying to work the other way around, so that cacheability on some forms can be made available sooner, even if some other forms still have bugs.
So, I just set the opt-in system (https://git.drupalcode.org/project/drupal/-/merge_requests/5047/diffs?co...). Theoretically, it should change nothing as no form should already use an opt-in we just "invented". But in practice, tests fail because some forms still already opt in by accident because of the problem mentioned in #74. For them, I'm willing to apply the method you describe in #101:

The entity form display cache data is currently unused, all we need to do is add a temporary explicit max-age=0 to it with a @todo to remove that in a follow-up issue. This is less problematic than YetAnotherMysteriousRenderArrayKey.

GaëlG’s picture

Title: Form tokens are now rendered lazily, so forms now *can* be cacheable: stop setting max-age=0 on form token » Form tokens are now rendered lazily, allow forms to opt in to be cacheable

Just changed the issue title to better reflect what's in its scope.

GaëlG’s picture

Assigned: GaëlG » Unassigned
Status: Needs work » Needs review

It's ready to be reviewed :)

PS: Some related bugs and tasks have been discovered during the course of this issue, so some other d.o. issues still should be created for them. See #119.

GaëlG’s picture

Issue summary: View changes
GaëlG’s picture

jonathanshaw’s picture

Status: Needs review » Reviewed & tested by the community

The approach is simple, backwards compatible, and a step forward. I don't see a reason not to do it.

jonathanshaw’s picture

Status: Reviewed & tested by the community » Needs review

Actually, I think we need to consider the deprecation strategy a little more. The concern is that existing forms might have untested buggy cachability metadata.

The current solution in this issue, followed by the proposed solution in #3395157: Deprecate not giving cacheability metadata to forms will require every form class in every site to implement the CacheableDependencyInterface::getCacheMaxAge() method, and then allows them to remove them again in some future version.

I'm very unclear how this will impact on cacheability metadata added by form alter hooks, a very common use case. At the least, I don't think the CacheableDependencyInterface::getCacheMaxAge() approach is going to do anything to help alter hooks to make sure their cacheability metadata is correct.

I'm uncertain that we need to be concerned about existing cacheability metadata - this is code developers have explicitly added, and code that is rarely properly tested anyway even in well-tested projects.

If we do want to care about potentially buggy cacheability metadata in alter hooks, then what we could do in FormBuilder is:
- if the form has no cache max-age:
-- set it to zero
-- if the cacheability metadata other than max-age throw a deprecation warning that it needs to specify max age as well

This might provide a wider range of protection against buggy cacheability metadata (alter hooks, not just form classes), but actually require less work from developers (because only people specifying cache contexts and tags needs to make a change to their forms, most forms won't need to change).

We might even be able to provide a version of this that is more flexible, allowing a form class to specify that it knows xyz cacheability metadata that it provides is good, but that anything else added by an alter hook should trigger a deprecation warning unless is accompanied by an explciit max-age.

I wonder if we need a release manager opinion on this stuff. I'm leaving at RTBC for a commtter to decide on what the best approach to resolving BC concerns is.

jonathanshaw’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

- if the form has no cache max-age:
-- set it to zero
-- if the cacheability metadata other than max-age throw a deprecation warning that it needs to specify max age as well

Without re-reading the whole issue, why not always require max-age? We could deprecate not providing it for 12.x rather than 11.x so it's less disruptive.

GaëlG’s picture

Note that $render_array ['#cache']['max-age'] is sometimes set "by default" in a cache metadata handling method, and not explicitly by the developer (such as by addCacheableDependency() if I remember well). This explains why I had to force opt-out some forms in my merge request, in EntityFormDisplay, ContactController and NodeForm.
I'm not sure it invalidates your proposals, but I just wanted to make sure you took that into account.

catch’s picture

@GaëlG I would think that means that not all forms would trigger a deprecation, but that more forms than if we only check for other cacheability metadata would trigger a deprecation since we'd be doing it for every case it's not set?

catch’s picture

Just realised that #3395776: Make POST requests render cacheable would be another reason to always deprecate if no max age, since it adds a 'magic' cache tag to every form. Excluding that cache tag from the checks could be done, but starts getting complicated.

jonathanshaw’s picture

I think my idea had 2 motivations:

1. to minimise the number of places where an explicit max age has to be set, mimise the number of people getting this deprecation warning and having to (temporarily) add the explicit max age.

2. to make the locus of responsibility for cacheable metadata (the person getting the deprecation because their metadata might be buggy) be the developer adding cacheable metadata (possibly in an alter hook), not the developer providing the form class.

If we require max-age=0 on every form, then every form class will enforce it based on the needs of their form, but people altering those forms and adding buggy cacheability data in alter hooks will get no help.

I think that this may all be getting too complicated, I'm not recommending we do this.

I'm not even sure we should provide any deprecations at all, but that is a question we can handle in the follow-up provided we decide now that we don't want to do anything more fancy to help form-alters.

tldr; I think it's good a committer considers this, but my recommendation is: commit the MR as it is now.

catch’s picture

Ahh I see #3395157: Deprecate not giving cacheability metadata to forms is open, fine for me to just add the capability with no deprecation here. Left a couple of comments on the MR but leaving RTBC for now.

GaëlG’s picture

Yes, deprecation should rather be discussed in #3395157: Deprecate not giving cacheability metadata to forms. I answered there.

catch’s picture

Status: Reviewed & tested by the community » Needs review
smustgrave’s picture

Status: Needs review » Needs work

For the one thread from @catch.