Problem/Motivation
- #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
. - #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.
- #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 themax-age=0
that point 1 introduced, because removing that merits further discussion. - 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.
Comment | File | Size | Author |
---|---|---|---|
#118 | interdiff-2578855-117-118.txt | 609 bytes | prudloff |
#118 | drupal-2578855-118.patch | 35.63 KB | prudloff |
#114 | drupal-2578855-114-9.5.x.patch | 35.53 KB | Albert Volkman |
#112 | 2578855-nr-bot.txt | 158 bytes | needs-review-queue-bot |
#108 | drupal-2578855-108-9.4.3.patch | 35.5 KB | GaëlG |
Issue fork drupal-2578855
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:
Comments
Comment #2
Wim Leers#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 thefull
node display uncacheable!Comment #3
Wim LeersSo this is basically what we want to do. I know some unit tests break. Let's see what else breaks.
Comment #4
Wim LeersThe "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.Comment #9
Wim LeersHere we go, unit tests updated. The unit test became approximately two orders of magnitude easier to understand.
Comment #12
Wim LeersA bunch of fixes, still working on the rest.
Comment #15
Wim LeersBy 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 theContentLanguageSettings
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 correspondingcontentLanguageSettings
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.
Comment #17
Wim LeersThis 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.Comment #18
moshe weitzman CreditAttribution: moshe weitzman at Acquia commentedDoes the mere viewing of the form interact with tempstore? Or does that happen once you submit the form?
Comment #19
Wim LeersYes, 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
.Comment #20
moshe weitzman CreditAttribution: moshe weitzman at Acquia commentedThanks 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.
Comment #22
Wim LeersThis 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.Comment #27
Wim LeersI 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.Comment #28
moshe weitzman CreditAttribution: moshe weitzman at Acquia commentedYeah, 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).
Comment #29
BerdirIt 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.
Comment #30
catchThat 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.
Comment #31
moshe weitzman CreditAttribution: moshe weitzman at Acquia commentedI'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.
Comment #32
xjmLinking the discussion of the rc-target-ness of this in the summary.
Comment #33
dawehnerI'm confused, didn't we threw an exception when #access was not an object?
Don't we want to use
\Drupal::entityManager()->getDefinition('editor')->getListCacheTags()
Do we actually need both lines, it feels a bit redundant. At least it could be merged into one line, if needed
I guess its not worth to specify the exact gazillion possible query parameters here ...
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?
Comment #34
catch@Moshe is #30 not a tentative green light?
Comment #35
Wim LeersFirst, a straight reroll of #22.
(Exact same changes, the context has just become slightly smaller, hence slightly different patch size.)
Comment #36
Wim Leers#22 said
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.)
Comment #38
Wim LeersSo, comparing #36 to #22 (sadly #35 didn't run tests because I stupidly forgot to mark it NR):
So, Comment's cacheability metadata was improved in the last weeks. And Search will still need some fixing.
Comment #39
Wim Leers#29:
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:
AccessResultInterface
objects. Requiring them to always beAccessResultInterface
objects would have caught this problem months ago… :(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 fromform.inc
, >2 years ago (revision82eb2924cfcc40dcedd981e4462176a6d9256f0f
):And in Drupal 7's
drupal_prepare_form()
:Comment #40
Wim LeersThis fixes the last fail in Comment.
Comment #43
Wim LeersThis makes cacheable forms opt-in for the first time!
It opts in the following two forms:
CommentForm
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.
Comment #44
Wim LeersUgh, #40's interdiff was correct, but the patch didn't contain it, it was identical to #39… Sorry about that.
Comment #45
Wim LeersUgh, 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).
Comment #48
xjmComment #51
nielsaers CreditAttribution: nielsaers commentedDoes anyone have a status update on this issue?
Comment #54
Mile23I'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. :-)
Comment #55
Wim LeersForms aren't cacheable yet, so this is still an issue.
Comment #58
DuaelFrThis would be a huge game changer!
@Wim Leers, do you still plan to work on this? <3
Comment #61
AndyF CreditAttribution: AndyF at Fabb commentedHere's a reroll of #45, thanks!
Comment #63
AndyF CreditAttribution: AndyF at Fabb commentedSo we're seeing one or two extra failures (: New patch removes use of
\Drupal::entityManager()
.Comment #65
AndyF CreditAttribution: AndyF at Fabb commentedassertCacheContext()
without the corresponding test trait.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!
Comment #67
johnwebdev CreditAttribution: johnwebdev commentedThis should fix some more failing tests.
Comment #68
johnwebdev CreditAttribution: johnwebdev commentedI'm not entirely sure this was the right fix. I might have misinterpreted #15.
Comment #69
jonathanshawComment #71
johnwebdev CreditAttribution: johnwebdev commentedSo the reasons why we get:
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.
Comment #72
johnwebdev CreditAttribution: johnwebdev commentedUnassigning Wim. Adding needs issue summary update.
Comment #74
johnwebdev CreditAttribution: johnwebdev commentedTurns 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.
Comment #76
johnwebdev CreditAttribution: johnwebdev commentedShould be green now!
Comment #77
johnwebdev CreditAttribution: johnwebdev commentedComment #78
Wim LeersInteresting. 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.Why this cache tag?
Wouldn't "thanks to the comment form being cacheable" be more accurate than "despite the lazy-built form"?
Why assert this?
I think this should then also assert that this particular response is not cached by Dynamic Page Cache.
Comment #79
johnwebdev CreditAttribution: johnwebdev commentedSo, 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.
Comment #80
AndyF CreditAttribution: AndyF at Fabb commentedAnd it looks like at least some contrib is already adding cacheability metadata to
$form
.Comment #81
johnwebdev CreditAttribution: johnwebdev commentedAddresses #78
1. Removed.
2. Fixed.
3. Removed.
4. Fixed.
Comment #83
DuaelFrComment #85
AndyF CreditAttribution: AndyF at Fabb for FRUITION commentedI 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.
Comment #87
GaëlGThis one is for 8.9.6.
Comment #88
prudloff CreditAttribution: prudloff at Insite commentedHere 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
.)Comment #89
GaëlGAnd this one is for 9.1.5.
Comment #90
moshe weitzman CreditAttribution: moshe weitzman commentedI'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.
Comment #91
BerdirLogin 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.
Comment #92
Quentin Massez CreditAttribution: Quentin Massez at Insite commentedHere is a reroll for 9.1.7
Comment #93
renatogPlease 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)
Comment #95
nikitagupta CreditAttribution: nikitagupta at Srijan | A Material+ Company for Drupal India Association commentedComment #96
prudloff CreditAttribution: prudloff at Insite commentedHere is a reroll for Drupal 9.2.0.
Comment #97
longwaveThe first instance here looks wrong, #form_cacheable should just be a boolean flag and not an array?
Comment #99
prudloff CreditAttribution: prudloff at Insite commentedA reroll for Drupal 9.3.0.
Comment #100
jonathanshawThis 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.
Comment #101
jonathanshawI'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:
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.
@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?
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.
Comment #102
prudloff CreditAttribution: prudloff at Insite commentedHere is a reroll of #99 for 9.3.6.
Comment #103
Quentin Massez CreditAttribution: Quentin Massez at Insite commentedHere is a reroll of #102 for 9.3.12
Comment #106
prudloff CreditAttribution: prudloff at Insite commentedIt looks like #103 dropped the updated condition in
handleInputElement()
.Comment #107
GaëlGHere's the fix for #106.
Comment #108
GaëlGReroll after SA-CORE-2022-013. Applies to 9.4.3 and current 9.5.x
Comment #109
GaëlGComment #110
longwaveI 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.
Comment #112
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe 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.
Comment #113
prudloff CreditAttribution: prudloff at Insite for Muséum national d'Histoire naturelle commentedComment #114
Albert Volkman CreditAttribution: Albert Volkman commentedRe-roll for 9.5.x
Comment #116
AnybodyVery 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.
Comment #117
prudloff CreditAttribution: prudloff at Insite for Muséum national d'Histoire naturelle commentedHere is a reroll for 10.1.
Comment #118
prudloff CreditAttribution: prudloff at Insite for Muséum national d'Histoire naturelle commented#117 triggers a "Bubbling failed." assertion error.
This seems to fix it.
Comment #119
GaëlGWe 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:
- 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)
- CommentForm (with an update to the corresponding test: CommentDefaultFormatterCacheTagsTest.php)
- MessageForm (+ContactSitewideTest.php)
- NodeForm
- core/modules/editor/src/Element.php (+ EditorLoadingTest.php + EditorSecurityTest.php)
- core/modules/filter/src/Element/TextFormat.php
- NodeSearch.php (+SearchAdvancedSearchFormTest.php )
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):
Update the issue summary with part of this commentComment #120
GaëlGComment #121
GaëlGComment #122
GaëlGI 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.
Comment #123
GaëlGActually, 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.
Comment #124
catchAll I can think of instead is
CacheAwareFormInterface
, not sure that is better.Comment #126
jonathanshawI 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.
Comment #127
jonathanshawA 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.
Comment #128
GaëlG#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:
Comment #129
GaëlGJust changed the issue title to better reflect what's in its scope.
Comment #130
GaëlGIt'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.
Comment #131
GaëlGComment #132
GaëlGComment #133
jonathanshawThe approach is simple, backwards compatible, and a step forward. I don't see a reason not to do it.
Comment #134
jonathanshawActually, 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.
Comment #135
jonathanshawComment #136
catchWithout 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.
Comment #137
GaëlGNote 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.
Comment #138
catch@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?
Comment #139
catchJust 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.
Comment #140
jonathanshawI 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.
Comment #141
catchAhh 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.
Comment #142
GaëlGYes, deprecation should rather be discussed in #3395157: Deprecate not giving cacheability metadata to forms. I answered there.
Comment #143
catchOpened #3397987: Node form reliance on temp store for preview makes it uncacheable and added a suggestion to the MR.
Comment #144
smustgrave CreditAttribution: smustgrave at Mobomo commentedFor the one thread from @catch.