Ensure forms are marked max-age=0, but have a way to opt-in to being cacheable

Note that the patch in #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!) currently sets max-age = 0 on all forms.

Sounds like a good plan!

We need to ensure that login form, exposed forms and the search block are marked as cacheable.

We need to ensure that login form, exposed forms and the search block are marked as cacheable.

Indeed!

The login form is not render cached at the moment. That is one of the most expensive things on non-page cached anonymous requests and was partly why I opened #2503755: Switch from user login block to login menu link and search block in standard profile.

user/register is less important but could be added to the list.

Once #2503429: [PP-*] Allow both AJAX and non-AJAX forms to POST to dedicated URLs is done we've got the chance to mark more things cacheable, but I think that's probably the full list until then.

What about this?

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -615,11 +615,23 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
   +    // If the form method is specified in the form, pass it on to FormState.
   +    if (isset($form['#method'])) {
   +      $form_state->setMethod($form['#method']);
   +    }
   

   Comment here seems unnecessary.

2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -615,11 +615,23 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
   +    // If the form does not specify cacheability metadata (#cache is not set),
   +    // mark the form as uncacheable (max-age=0). Exempt GET forms, which Drupal
   +    // assumes to be cacheable.
   +    if (!isset($form['#cache']) && !$form_state->isMethodType('get')) {
   +      $form['#cache']['max-age'] = 0;
   +    }
   

   Nit: Since forms are exclusively GET or POST, shouldn't we instead flip it and say "if it's a POST and no cache is set, then set it to no-cache?" (Both code and comment.)

Looks like a reasonable and direct approach, but needs a basic test or two.

2. I didn't know no other HTTP method could ever work for forms. That's why I did it this way: we only allow GET forms to be cacheable, not anything else. Whitelisting instead of blacklisting. If others agree, I'm happy to change it though.

Oh, and absolutely, this needs tests!

Let's first hear what others think about this overall approach.

The comment form on node detail pages is a common killer - #2552873: node/1 flamegraphs

Not sure if that will help.

The comment form is personalized to the user. Not exactly sure how the changed form caching now affects this as we rebuild the form now anyway on submission.

Alternative solutions to the problem outlined in #10: move the comment form to a separate page (which is just a setting) and/or load it via JS.

The comment form is personalized to the user.

1. I see text at the top of the form with the logged in user's name. If we get rid of that, is the form now unpersonalized and cacheable?
2. If thats not acceptable, could we placeholder the name and do standard render cache of the rest of the form?

If none of these are fruitful, I'm fine with moving the form or loading via JS.

, I'm fine with moving the form

Seriously, but this would be just a horrible movement. Just think any website, like the one I'm currently posting one, the comment form on a new page simply does not make sense for most sites.

#13:

1. Yes, at least once #2463567: Push CSRF tokens for forms to placeholders + #lazy_builder lands. Help welcome there. It's blocked on @effulgentsia pretty much, or anybody who knows Form API extremely well.
2. Maybe. The problem is that the current authenticated user ends up in FormState, at least in HEAD. If the form logic is changed so that that is no longer the case, then that'd be possible.

#14:

Seriously, but this would be just a horrible movement. Just think any website, like the one I'm currently posting one, the comment form on a new page simply does not make sense for most sites.

It wouldn't make sense for sites/use cases where the majority of users are commenting. So, d.o and forums would be a bad match for that.

But… on most news sites, commerce sites, etc. only a tiny minority of users are actually commenting. Heck, on many news sites, you even have to load the already written comments manually: Ars Technica, The Verge, et cetera. Any site using Disqus only loads the comments as soon as you scroll far enough down.

So, I don't think it'd be that horrible.

But, #10–#15 are not what this issue is about. This issue is about forms in general, not the tricky aspects of the comment form in specific.

Comment form is a separate issue, also there's AJAX Comments

Comments #10 through #17 are indeed a distraction. They're not what this issue is about.

The last relevant comments were #8 (a review by Crell) and #9 (my answer to that review). Addressed all his feedback and wrote tests.

Seems legit.

Forms are an important part of Drupal, but currently they are uncacheable (CSRF tokens, parameters can come from anywhere), but not marked as such.

If it's only CSRF tokens, then why not put the max-age=0 on the token element instead of the entire form? If it's because parameters can come from anywhere, then why does that concern not apply to GET forms?

The general answer

You're right that this will need to be refined further.

This issue is only about setting sensible defaults.

The sensible defaults being introduced here is:

1. mark all POST forms as uncacheable (because they typically have lots of dynamic bits, and they're the 99% case))
2. don't do anything for GET forms (because they typically are very simple, and are the 1% case)

In other words, this ensures that the 99% case will not be cached incorrectly.

The specific answer

If it's because parameters can come from anywhere, then why does that concern not apply to GET forms?

You're technically right here. See the above for the reasoning in this patch.

If you feel strongly that we should mark all forms as uncacheable (max-age = 0) unless they opt out by specifying their own cacheability, I'm fine with that.

mark all POST forms as uncacheable (because they typically have lots of dynamic bits, and they're the 99% case))

I'm skeptical about this. In D7, by default, all forms that are accessible to anonymous users can be page cached. #18 changes that, because it doesn't distinguish anonymous from authenticated. So I don't think we want to say "by default, forms should have max-age=0". Instead, maybe what we want is for forms by default to not get cached for authenticated users, because we don't trust module developers who write forms to assign cache contexts everywhere that they do something dynamic. This to me seems very similar to the same concern we have in #2556889: [policy, no patch] Decide if SmartCache is still in scope for 8.0 and whether remaining risks require additional mitigation for non-form controllers. In that issue, we're discussing the possibility to make SmartCache require explicit per-route or per-controller opt-in. If we end up doing that, I think then we can apply the same opted out by default rule to form routes and with the same API to opt-in. So I suggest postponing form-wide cache changes on that discussion.

But, I think it makes sense for this issue (or a child issue) to set max-age = 0 on the 'form_token' element. I think that might be enough to unblock this part of the SmartCache patch, since that would bubble up to the 99% of forms that we're concerned about. #2463567: Push CSRF tokens for forms to placeholders + #lazy_builder might end up fixing the problem of form tokens breaking the cacheability of forms, but I think it's still worthwhile to first commit a patch that sets max-age = 0 on 'form_token' to reflect its current reality, and then work on that issue to improve that situation.

In D7, by default, all forms that are accessible to anonymous users can be page cached. #18 changes that, because it doesn't distinguish anonymous from authenticated.

This is not true. The page cache happily caches pages with max-age = 0 for anonymous users.

If we end up doing that, I think then we can apply the same opted out by default rule to form routes and with the same API to opt-in.

But forms don't always have routes. Forms first and foremost have IDs, and they can be (and are) embedded anywhere in the page.

I think it's still worthwhile to first commit a patch that sets max-age = 0 on 'form_token'

This is totally true. Opened #2559011: Ensure form tokens are marked max-age=0.

So, #2559011: Ensure form tokens are marked max-age=0 has now landed, but turns out it's not enough. Quoting the comment I just posted on that issue:

Hurray!

This was the last blocker for #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!). It was split off from #2526472: Ensure forms are marked max-age=0, but have a way to opt-in to being cacheable, and uses a more granular, more precise approach to ensure SmartCache doesn't cache forms that should not be cached.

So, I tested #2429617-339: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!) + #13 (the patch here that just got committed), to see if SmartCache would still be green. But, unfortunately, the answer is no: #2560959-4: Testing issue for #2429617 is red.

Because of the more granular (and better!) approach taken by this issue compared to #2526472, we need one more step: #2561775: Forms without $form['#action'] set get their action automatically generated based on current path + query args: cacheability metadata is missing. Then forms will have sufficient cacheability metadata. Hopefully see you there!

Given that #2559011 + #2561775 together already solve the important part of this issue, demoting this to normal.

So, we did #2559011: Ensure form tokens are marked max-age=0 and now we also did #2504139: Blocks containing a form include the form action in the cache, so they always submit to the first URL the form was viewed at (the more efficient alternative solution of #2561775: Forms without $form['#action'] set get their action automatically generated based on current path + query args: cacheability metadata is missing). Together, they solve this problem. Closing this as a duplicate.

To clarify: you can make a form opt out from being marked as uncacheable by specifying $form['#token'] = FALSE;. If your form truly is cacheable, then it also shouldn't need the token. But be aware that this has security implications!