Form tokens are now rendered lazily, allow forms to opt in to be cacheable

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.

Re-roll for 9.5.x

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 ( ✨ Find a way to allow page caching (including by CDNs) with captcha. AJAX, Lazy, ...? Active )
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.

Here is a reroll for 10.1.

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

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.

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.

Actually, I attended to @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.

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.

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.

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.

#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 📌 Deprecate not giving cacheability metadata to forms Needs work

#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.

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

I still have to fill the descriptions of the 3 new issues I reference in code changes, but otherwise 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.

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

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 📌 Deprecate not giving cacheability metadata to forms Needs work 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.

- 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.

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.

@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?

Just realised that 📌 Make POST requests render cacheable Needs work 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.

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.

Ahh I see 📌 Deprecate not giving cacheability metadata to forms Needs work 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.

Yes, deprecation should rather be discussed in 📌 Deprecate not giving cacheability metadata to forms Needs work . I answered there.

Opened 🐛 Node form reliance on temp store for preview makes it uncacheable Postponed and added a suggestion to the MR.

For the one thread from @catch.

There was one failed test that's a known intermittent, see 🐛 [random test failure] Random test fail in EntityReferenceWidgetTest Active . I've just started a rerun.

smustgrave → changed the visibility of the branch 2578855-cacheabledependencyinterface to hidden.

smustgrave → changed the visibility of the branch 2578855-all-in-one-scope-creep to hidden.

smustgrave → changed the visibility of the branch 11.x to hidden.

Hiding patches and unused MRs for clarity

Re-ran failing test and confirmed it was random.

Feedback appears to be addressed but MR was moved to draft so will leave in review.

The MR was automatically put into draft by a fixup commit (I guess if I'd rebased with autosquash it wouldn't have), and I don't seem to have permission to remove the draft status; I'm not sure it makes a meaningful difference to the state of review of the code contained within however.

I think this is ready again.

Created a CR

I've updated the comments based on the most recent feedback.