Form cache entries can expire before their host page's cache entry, resulting in all AJAX failing

subscribe

Hm. Let's start with this?

A 'solution' for this should be to set your page cache to three hours, as forms are by default cached for 6 hours. Not great but it should stop AJAX forms from failing.

The reason is that there's no way for it to know what the $form_id is securely: you can't trust $_POST['form_id']

Why is this less secure for Ajax forms than standard forms?

Views has a hack for this, it'd be nice to remove it: http://api.drupal.org/api/search/7/views_ui_ajax_get_form

I also ran into this in the Mollom module (which requires form caching to persist data relevant to a particular form submission).

The architectural flaw is that any module can go into the form at any time and just simply override #cache to FALSE.

(there are some funky contrib modules that do this for very odd reasons)

To prevent this from happening, in an ideal world, $form would have to be a class with a $form->enableCache() method, without a counterpart; i.e., without any chance for anyone else to revert that decision once enabled.

Of course, that doesn't help with the other AJAX/form-cache expiration issue mentioned in the OP, but at the same time, that doesn't look resolvable to me in any way in the first place.

Sure, a one-way boolean sounds fine to me.

Just wondering if there was any progress on this? #1694574: drupal_process_form() deletes cached form + form_state despite still needed for later POSTs with enabled page caching is about invalid caches on login which I think can be solved by one of a couple fairly easy options, but solving that just postpones the problem until we hit the 6 hours...

Would an acceptable solution be to make it so that anonymous AJAX form caches never expire? That way they will be valid for the duration of the page cache. This could obviously be tied into whether anonymous caching is enabled to make it only happen when relevant? #1286154: Allow custom default form cache expiration/lifetime may provide a solution to that by allowing forms to define their cache length?

Would an acceptable solution be to make it so that anonymous AJAX form caches never expire?

This would probably result in a whole lot of garbage in your form cache. I also think it'd be difficult to reliably determine which forms have AJAX-enabled fields and which forms do not.

#1286154: Allow custom default form cache expiration/lifetime may provide a solution to that by allowing forms to define their cache length?

Probably not a good idea; unless you're deeply in-tune with the underpinnings of FAPI (and I don't think anyone really is, including the maintainers), you're probably going to shoot yourself in the foot.

I think a reasonable solution is to make the default value somewhat dynamic: e.g. form expiration time = REQUEST_TIME + $EXPIRE where EXPIRE is MAX(6 hours, page cache minimum lifetime).

Also, adding a needs backport tag because this needs backport.

Something like this.

Note there seem to be a number of related but nevertheless distinct issues at play here. My summary below:

1. Bug: Form API, the AJAX part of it in particular, relies on form cache persistence and availability at an architectural level
2. Bug: Form cache entries can expire before their host page cache entries (because page cache min/max is configurable up to 24 hours while form cache is hard-coded at 6)
3. Feature request: Allow custom form cache expiration/lifetime

The patch above solves #2.

#1 is still a valid issue, but decoupling FAPI and form cache sounds unresolvable (as I believe @sun alluded to in #9) and at least this patch mitigates the possibility of issues cropping up.

#3 is also still a valid feature request, though its need is significantly reduced. Specifically, the only thing it would enable is a site admin to set form cache expiration greater than page cache expiration (which would only be handy in cases where people AJAX submit forms after having stayed on the page more than 6, 12, 24, etc hours).

#14: drupal-ajax_form_cache-774876-14.patch queued for re-testing.

Guessing there are some unit tests that this will break, but here's #14, now that everything's been moved to FormBuilder.

And, yup. Here are the PHPUnit test fixes. Not adding any tests, though.

And... Yup. install_begin_request

Just adding the argument to form_builder constructor there.

Let's ensure that we get unit test coverage, now that we can.

Adding tests for FormBuilder::setCache(). Trying to test all scenarios covered in code, currently: authenticated, unauthenticated, page cache max age > 6hrs, page cache max age < 6hrs.

To do the latter pair, I had to add a config factory setter on the FormBuilder test wrapper.

Whoops. The interdiff above (#24) is valid. Just forgot to add the changes in the interdiff to the actual patch itself. That's here:

Per discussion with @tim.plunkett on IRC, I'm breaking out all of those scenarios described in #24 into separate test methods. Much more readable and maintainable.

Also, moved the 6 hour minimum to a constant (FormBuilder::MIN_CACHE_TTL) on the FormBuilder class to ensure the test is properly decoupled.

Yes, that is much more readable.

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -133,6 +133,11 @@ class FormBuilder implements FormBuilderInterface {
   +   * A reasonable minimum cache TTL for form cache entries.
   ...
   +  const MIN_CACHE_TTL = 21600;
   
   @@ -402,9 +407,9 @@ public function getCache($form_build_id, &$form_state) {
   +    $expire = max(self::MIN_CACHE_TTL, $page_max_age);
   

   Isn't this the MAX_CACHE_TTL? Also, please use static:: not self::

2. +++ b/core/tests/Drupal/Tests/Core/Form/FormBuilderTest.php
   @@ -487,67 +487,130 @@ public function testGetCache() {
   +    $builder_class = get_class($this->formBuilder);
   ...
   +        'cache.page.max_age' => $builder_class::MIN_CACHE_TTL - 120,
   

   I think its safe to just use TestFormBuilder:: here, but it doesn't bother me much either way.

The min vs. max gets pretty semantically weedy. I think it could reasonably be considered max if keyValueStoreInterface::get() explicitly required that implementors not return items past expiration (which is not the case, I believe). Historically, Drupal's used "minimum cache lifetime" because we leave garbage collection to cron; min TTL just means: "I guarantee this will be in cache until a certain point, regardless of garbage collection. After that time, you may still get cache hits (because garbage collection hasn't run yet), but I cannot guarantee that."

Beyond that, though, I simply mean "minimum" because it could be more than that value in situations where page cache max age is greater. I'd also be fine making it ambiguous (e.g. FormBuilder::CACHE_TTL).

As for using TestFormBuilder instead of loading the class, I'll leave as is (just to keep the only hard-coded instance of that in the setUp method).

Duly noted on static::MIN_CACHE_TTL, though. Fix for that attached here.

That explanation makes sense, thanks.

One last nitpick!

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -135,6 +148,8 @@ class FormBuilder implements FormBuilderInterface {
+   * @param \Drupal\Core\Config\ConfigFactory $config

This should be $config_factory.

Also, can you rename the issue? The title doesn't seem to agree with what we're changing here.

Updating the title and attaching a patch to address #29.

I'll also be updating the issue summary shortly.

Updating issue to reflect latest solution, crosslink related issues, and use the issue summary template.

#30: drupal-ajax_form_cache-774876-30.patch queued for re-testing.

#30: drupal-ajax_form_cache-774876-30.patch queued for re-testing.

Rerolled.

Clarifying / fixing some grammar issues in the summary.

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -391,8 +407,10 @@ public function getCache($form_build_id, &$form_state) {
+    // Form cache expiration should be at least the max age of the page itself
+    // to ensure AJAX functionality.
+    $page_max_age = $this->configFactory->get('system.performance')->get('cache.page.max_age');
+    $expire = max(static::MIN_CACHE_TTL, $page_max_age);

hm, if you run a site with a high page cache ttl, but lots of forms for authenticated users - one will end up with lots of form cache entries :/

However, this is generally problematic and might cause problems also with block caching or panels caching. But the form, cannot know how it is cached.

However, we could allow setting a minimum cache expiration time per form, so if you have forms being cached longer you can increase it specifically for those forms?

Reroll attached.

However, we could allow setting a minimum cache expiration time per form, so if you have forms being cached longer you can increase it specifically for those forms?

This patch at least enables this type of functionality by making the cache TTL a constant on the FormBuilder class. If necessary, the class can be extended to provide custom functionality (like a per-form ttl; presumably, this could be provided by contrib). What's proposed here seems like a reasonable "80%" solution.

Is this still an issue in 8.1.x? If so, this is major according to the priority guidelines. It looks like the caching logic is now in FormCache.

@Reuben Unruh, which issue is this a duplicate of?

Sorry! I added the related issue but I see I'm supposed to add a link as a comment also to call out a duplicate.

#1286154: Allow custom form cache expiration/lifetime is about making the form cache expiry configurable. The most recent patch there doesn't seem to ensure that the expiry matches the page cache expiry but it is mentioned in the issue summary that this breaks ajax. Also, there doesn't seem to be any activity here.

I didn't update #1286154 at all.

So I don't think we should close this as a duplicate of a much more general feature request if the bug described in the summary still exists, because it is actually quite nasty.

However, we should confirm whether this is still a problem in 8.2.x, since there was a lot of change to this functionality since 2013.

Triaging this issue at DrupalCon Vienna with @Stela Oroz

@tim.plunkett @fago @iamEAP Can any of you elaborate on the steps to reproduce for this issue? We would like to see if it still happens.

More information about this issue was asked for in #50, 5 years. No additional information has been supplied, therefor closing.

If you are experiencing this problem on a supported version of Drupal reopen the issue, by setting the status to 'Active', and provide complete steps to reproduce the issue (starting from "Install Drupal core").

Thanks!