Problem/Motivation

Busy sites that use a lot of forms can make a lot of entries in the cache_form table. This is particuarly true of sites using Ubercart, Commerce, or other modules like Fivestar, Ideal Comments, or Hierarchical Select.

This is a very common problem which I have seen cause problems for my clients, especially when database replication is involved. A search shows how widespread the issue is: https://www.google.com/search?q=cache_form%20big

The cache_form table is truncated via cron, in system_cron. The default expiry for cache_form entries is 6 hours, hardcoded in form_set_cache.

Proposed resolution

By changing $expiry = 21600; from a hardcoded variable to variable_get('cache_form_expiry', 21600);, users can choose how often entries are pruned from cache. With shorter lifetimes, the form_cache table will be truncated more often, and will not grow as large.

I would suggest making the default expiry one hour, rather than 6, also.

Remaining tasks

None, subject to community review (especially of the comment).

User interface changes

None.

API changes

None.

#226728: Temporary cache table entries are not flushed
#1694574: drupal_process_form() deletes cached form + form_state despite still needed for later POSTs with enabled page caching

Safe cache_form clear

Files: 
CommentFileSizeAuthor
#79 cache_form_expiry_to_variable-2091511-79.patch1.82 KBmcdruid
#77 cache_form_expiry_to_variable-2091511-77.patch1.82 KBmcdruid
#75 interdiff-57-75.txt762 bytesmpdonadio
#75 2091511-75.patch959 bytesmpdonadio
#57 cache_form_expiry_to_variable-2091511-57.patch858 bytesCameron Tod
PASSED: [[SimpleTest]]: [MySQL] 41,755 pass(es). View
#54 cache_form_expiry_to_variable-2091511-54.patch747 bytesmcdruid
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch cache_form_expiry_to_variable-2091511-54.patch. Unable to apply patch. See the log in the details link for more information. View
#44 2091511.43.patch5.42 KBlokapujya
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2091511.43.patch. Unable to apply patch. See the log in the details link for more information. View
#36 interdiff_32-35.patch658 bytesCameron Tod
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#36 2091511-cache_form_expiry_to_variable-35.patch10.82 KBCameron Tod
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#32 interdiff.txt8.3 KBCameron Tod
#32 2091511-cache_form_expiry_to_variable-31.patch10.18 KBCameron Tod
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#27 2091511-cache_form_expiry_to_variable-27.patch2.9 KBCameron Tod
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,715 pass(es). View
#23 interdiff.txt1.97 KBCameron Tod
#23 2091511-cache_form_expiry_to_variable-23.patch2.83 KBCameron Tod
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#19 2091511-cache_form_expiry_to_variable-19.patch2.3 KBCameron Tod
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,719 pass(es), 2 fail(s), and 0 exception(s). View
#15 2091511-cache_form_expiry_to_variable-D8-15.patch1.59 KBCameron Tod
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#8 2091511-cache_form_expiry_to_variable-D8.patch901 bytesCameron Tod
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
#4 cache_form_expiry_to_variable-2091511-4.patch747 bytesCameron Tod
PASSED: [[SimpleTest]]: [MySQL] 40,382 pass(es). View
#3 cache_form_expiry_to_variable-2091511-3.patch741 bytesmcdruid
PASSED: [[SimpleTest]]: [MySQL] 40,382 pass(es). View
cache_form_expiry_to_variable.patch728 bytesCameron Tod
PASSED: [[SimpleTest]]: [MySQL] 40,379 pass(es). View

Comments

meba’s picture

Status: Active » Needs review

This sounds pretty good

typhonius’s picture

Assigned: Cameron Tod » Unassigned
Status: Needs review » Reviewed & tested by the community

This patch makes a lot of sense and is a minor non-destructive change that won't affect sites which do not need the alteration since the default value remains at 6 hours.

mcdruid’s picture

FileSize
741 bytes
PASSED: [[SimpleTest]]: [MySQL] 40,382 pass(es). View

New patch which only tweaks the comments, but this looks great; a simple solution to what can be a very serious problem.

Cameron Tod’s picture

FileSize
747 bytes
PASSED: [[SimpleTest]]: [MySQL] 40,382 pass(es). View

Rerolled patch to fit comments within 80 characters.

David_Rothstein’s picture

Version: 7.x-dev » 8.x-dev
Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
Issue tags: +needs backport to D7

This would need to go into Drupal 8 first.

swentel’s picture

Note that we don't have cache_form anymore in D8

Berdir’s picture

It does have a expiration in FormBuilder::setCache() that has a hardcoded expiration.

Note that the "expiration" of temporary files does have the same hardcoded value, which is also being made configurabe #1399846: Make unused file 'cleanup' configurable.

I think those values are the same for a reason, otherwise it could happen that you submit a form after uploading a file and the file has been removed in the meantime. Not sure what to do about it, though.

+1 on making this configurable though.

Cameron Tod’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
901 bytes
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Here is my first pass. FormBuilder doesn't seem to like loading my variable from config in testing, but it seems to work in a normal Drupal bootstrap.

Status: Needs review » Needs work

The last submitted patch, 8: 2091511-cache_form_expiry_to_variable-D8.patch, failed testing.

beejeebus’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 8: 2091511-cache_form_expiry_to_variable-D8.patch, failed testing.

The last submitted patch, 8: 2091511-cache_form_expiry_to_variable-D8.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 8: 2091511-cache_form_expiry_to_variable-D8.patch, failed testing.

Cameron Tod’s picture

Status: Needs work » Needs review
FileSize
1.59 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

Seems like DrupalUnitTestBase only loads a very limited set of config. Setting it explicitly in FormCacheTest makes things go green.

Status: Needs review » Needs work

The last submitted patch, 15: 2091511-cache_form_expiry_to_variable-D8-15.patch, failed testing.

Berdir’s picture

You need to inject the configuration object for that and update the PhpUnit tests.

Cameron Tod’s picture

Issue tags: +LSDHACK
Cameron Tod’s picture

Status: Needs work » Needs review
FileSize
2.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,719 pass(es), 2 fail(s), and 0 exception(s). View

OK, moved the config key into system.performance.yml, injected the config in FormCacheTest, and added the config to the configuration schema.

If this is all good, I guess the next step is to write some tests against this specific functionality.

Status: Needs review » Needs work

The last submitted patch, 19: 2091511-cache_form_expiry_to_variable-19.patch, failed testing.

alexpott’s picture

+++ b/core/modules/system/config/system.performance.yml
@@ -2,6 +2,8 @@ cache:
+  expire:
+    form: 21600

I would swap this around so it is cache.form.expire

Berdir’s picture

Also not sure about naming it cache at all in 8.x as it is currently using key_value_expirable and not the cache API. That said, there's an issue which aims to change that again.

Cameron Tod’s picture

Status: Needs work » Needs review
FileSize
2.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
1.97 KB

Changed cache setting key, added a stub for config.factory to the failing test class.

Status: Needs review » Needs work

The last submitted patch, 23: 2091511-cache_form_expiry_to_variable-23.patch, failed testing.

Cameron Tod’s picture

The last submitted patch, 23: 2091511-cache_form_expiry_to_variable-23.patch, failed testing.

Cameron Tod’s picture

Status: Needs work » Needs review
FileSize
2.9 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,715 pass(es). View

Added in a stub config value for unit tests.

Cameron Tod’s picture

I'm not sure how to test this - as it's stored via a KeyValueStore, isn't caching and expiration covered by the tests in the tests for the classes implementing KeyValueStoreExpirableInterface? And the specific form validation stuff is already covered in FormCacheTest?

Cameron Tod’s picture

Title: Make cache_form $expiry a variable, to mitigate runaway cache_form table growth » Make cache_form $expiry a variable, to mitigate runaway cache_form table or key:value store growth
Issue tags: -Needs tests +DrupalCampLDN

Spoken to alexpott on IRC - expiry and form validation is covered in other tests, namely the tests for KeyValueStore and FormCacheTest. Removing tag.

Cameron Tod’s picture

Title: Make cache_form $expiry a variable, to mitigate runaway cache_form table or key:value store growth » Make cache_form $expiry configurable, to mitigate runaway cache_form table or key:value store growth
damiankloip’s picture

  1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -390,9 +390,7 @@ public function getCache($form_build_id, &$form_state) {
    +    $expire = \Drupal::config('system.performance')->get('cache.form.expire');
    

    Dare I say it, this should really be injected instead. This form builder constructor is getting kind of crazy.

  2. +++ b/core/modules/system/config/system.performance.yml
    +++ b/core/modules/system/config/system.performance.yml
    @@ -2,6 +2,8 @@ cache:
    
    @@ -2,6 +2,8 @@ cache:
    +  form:
    +    expire: 21600
    

    mirroring what berdir mentioned, this is not really coming from cache. Where is the issue to change this? I thought when this was initially converted we had good reasons for this to live in kve instead.

Cameron Tod’s picture

Related issues: +#512026: Move $form_state storage from cache to new state/key-value system
FileSize
10.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
8.3 KB

Ok, injected the config now.

I have also made $expiry an argument, as I feel like being able to set expiries shouldn't be a global setting.

I have integrated the changes into FormTestBase but it fails locally with a fatal. I'm not sure why, but am uploading as is to keep the momentum. Hopefully someone will be able to spot the error I'm making.

I'm not really sure how best to rename the config key. Should we change the setCache/getCache method names to indicate that this isn't real cache data?

Adding related-ish issue: #512026: Move $form_state storage from cache to new state/key-value system

Status: Needs review » Needs work

The last submitted patch, 32: 2091511-cache_form_expiry_to_variable-31.patch, failed testing.

The last submitted patch, 32: 2091511-cache_form_expiry_to_variable-31.patch, failed testing.

Cameron Tod’s picture

Cameron Tod’s picture

Status: Needs work » Needs review
FileSize
10.82 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View
658 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

Wow. Much reading. Such code.

I think this should fix the install failures.

Status: Needs review » Needs work

The last submitted patch, 36: interdiff_32-35.patch, failed testing.

The last submitted patch, 36: 2091511-cache_form_expiry_to_variable-35.patch, failed testing.

tim.plunkett’s picture

Cameron Tod’s picture

Issue summary: View changes
lokapujya’s picture

I don't think we need the configFactory part anymore? Can't we do something like : \Drupal::config('system.performance')->get(cache:form:expire)

lokapujya’s picture

Issue tags: -DrupalCampLDN +Performance
lokapujya’s picture

Assigned: Unassigned » lokapujya

Will try to reroll.

lokapujya’s picture

Assigned: lokapujya » Unassigned
Status: Needs work » Needs review
FileSize
5.42 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2091511.43.patch. Unable to apply patch. See the log in the details link for more information. View

Status: Needs review » Needs work

The last submitted patch, 44: 2091511.43.patch, failed testing.

Marko B’s picture

I have a question. Why don't we have flexibility of setting this per context. I used form my projects on drupal 7

drupal_alter('form_set_cache', $expire, $form, $form_state);

and then altered time needed for cleaning the forms per different page.

Berdir’s picture

This should be largely irrelevant in D8 now. D8 no longer puts forms into the form cache on the first request, only when users actually start to do partial form submissions like uploading images.

mcdruid’s picture

This should be largely irrelevant in D8 now.

That being the case, shall we move this back to D7 then and reconsider the patch from #4 (and / or possibly the approach suggested in #46 whereby drupal_alter is used to allow every entry to be tweaked before it goes into cache_form)?

Just adding the variable into form_set_cache would be a substantial improvement for D7.

lokapujya’s picture

That's great news that it's fixed in a better way in 8. It could still go in 8, if not just for removing a hardcoded number.

Cameron Tod’s picture

Version: 8.0.x-dev » 7.x-dev
Issue tags: -needs backport to D7

Setting back to D7. I would suggest that we use the patch from #4, then create a follow up issue to implement the approach in #46.

mcdruid’s picture

Status: Needs work » Reviewed & tested by the community

+1 for #4

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 44: 2091511.43.patch, failed testing.

The last submitted patch, 44: 2091511.43.patch, failed testing.

mcdruid’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
747 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch cache_form_expiry_to_variable-2091511-54.patch. Unable to apply patch. See the log in the details link for more information. View

Re-uploading the exact same patch from #4 to try to get that tested against D7 (instead of the last D8 patch which failed).

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 54: cache_form_expiry_to_variable-2091511-54.patch, failed testing.

The last submitted patch, 54: cache_form_expiry_to_variable-2091511-54.patch, failed testing.

Cameron Tod’s picture

Status: Needs work » Needs review
FileSize
858 bytes
PASSED: [[SimpleTest]]: [MySQL] 41,755 pass(es). View

Rerolled against 7.x head.

Status: Needs review » Needs work

The last submitted patch, 57: cache_form_expiry_to_variable-2091511-57.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 57: cache_form_expiry_to_variable-2091511-57.patch, failed testing.

Cameron Tod’s picture

Status: Needs work » Needs review

Unless I'm missing something, the patch is green but the testbot marked it as failed in this thread?

https://qa.drupal.org/pifr/test/1186423
https://dispatcher.drupalci.org/job/default/42601/console

I guess I need to log an issue with the testbot project. In the meantime, moving back to NR.

Cameron Tod’s picture

mcdruid’s picture

Status: Needs review » Reviewed & tested by the community

trying RTBC again...

David_Rothstein’s picture

Status: Reviewed & tested by the community » Closed (duplicate)

This looks like a duplicate of #1286154: Allow custom form cache expiration/lifetime (which is already marked for Drupal 7 backport). I don't think there's any reason we'd want to make this configurable in Drupal 7 while leaving it unconfigurable in Drupal 8...

dakku’s picture

subscribe ++

mcdruid’s picture

Status: Closed (duplicate) » Reviewed & tested by the community

Discussed this briefly with Fabianx at DrupalCon Dublin...

Changing this back to RTBC in the hope that we can get the very simple patch in #57 (which I've checked still applies to 7.x HEAD) committed to D7.

Whilst #1286154: Allow custom form cache expiration/lifetime shares the same objective as this issue, I don't see how the approach being followed there for D8 applies in D7, unless I'm missing something.

So in the interests of pragmatism and getting this done, I'd suggest that we get this simple change into D7 and keep working on the D8 implementation in the other issue.

David_Rothstein’s picture

I am not sure why the Drupal 8 patch in that issue got so complicated either. Ideally it would just check a configuration setting like this one does...

We can leave this open given that we now do Drupal 7 backports in separate issues anyway, but I think it should be postponed on the other issue. We don't commit fixes to Drupal 7 that aren't in Drupal 8 yet unless there's a really good reason, and I don't see what the reason would be in this case.

stefan.r’s picture

+1 to postponing this on the other issue -- I also wonder if making this configurable may have any unintended consecuences in places that still assume the cache lifetime is hardcoded at 6 hours, so perhaps this should go into a 7.60?

mpdonadio’s picture

#67, a good reason to commit this now is that you can DOS or hard crash a server w/o it, #2341447: Possible Denial of Service Attack with {cache_form}.

catch’s picture

This is much less of an issue in 8.x after #2263569: Bypass form caching by default for forms using #ajax. landed - i.e. simply viewing a form never creates a {cache_form} entry in 8.x, only submits, so the number of cache form entries is much more restricted.

I left a comment on the 8.x issue, but since the bug is much more severe in 7.x, I don't see a reason to postpone on the 8.x fix (also I think the 8.x fix should just use $settings, which is equivalent to the variable_get() call here).

stefan.r’s picture

Thanks @catch

For D7, can we add this in $settings a $conf variable in default.settings.php with a long comment explaining all the implications of setting this to too high of a value, as well as too low of a value (AJAX requests failing).

I also wonder if we should set this on a per form basis, rather than for all forms on the site at the same time, as from what I understand it's usually only one form that's problematic.

stefan.r’s picture

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

If I understood catch's suggestion correctly, it was that the D8 implementation of this could be similar to the simple variable_get here in D7 if we use Settings::get (which can include a default value in the call). I've put a new patch based on this in #1286154-81: Allow custom form cache expiration/lifetime.

So I'm not sure if the suggestion is that the D7 implementation here needs to change? stefan.r you're saying there could be an example with comments in settings.php? I suppose so, but not sure how necessary that is.

stefan.r’s picture

@mcdruid that's right, I meant a commented out $conf variable (see default.settings.php) -- which shouldn't change the implementation.

The only thing that could slightly change it is if we allowed for the 6 hours to be overridden on a per-form basis (with a variable based on form ID) instead of globally.

mpdonadio’s picture

#71, rather than doing this to settings, I think adding a new special element would be better. Then people can form_alter, or just define it in their forms. This would help in situations where form IDs are more dynamic (like with webforms? don't remember right now). Something like the attached.

That said, I am not sure what the real benefit would be. I have had the approach in #57 live on client sites for about two years now, one of which had a lot of large, public forms and was getting hammered by bots (see the issue I linked above). Even dropping the TTL to about an hour had a huge impact.

ndobromirov’s picture

I like the approach in #75.
We still need to document the new variable in settings.default.php.

mcdruid’s picture

I can see why some sites might want a different expiry for different forms... That does introduce more complexity though.

On the one hand, using a pattern for a variable names like form_cache_expiry_$form_id (presumably along with a default variable too) would make it easy to tweak for certain forms without writing code.

However, might there be use cases this wouldn't cater for? How about if you want to set a custom expiry for a set of forms based on a pattern e.g. perhaps those with form_id's matching webform_*.

The approach of adding a custom form element might be a better match for examples like that. Or alternatively, a drupal_alter might allow for ultimate flexibility.

Once you're into the realm of having to write code to customise the expiry for given forms though, I'm not sure what the advantage of drupal_alter over a custom form element would be. Not adding new elements to the Form API would perhaps be one.

All that said, here's a patch with just the simple single variable_get (from #57) along with a first pass at a comment block for default.settings.php

I've also tweaked the comment in form.inc slightly.

Cameron Tod’s picture

Status: Needs review » Reviewed & tested by the community

I think the idea of per-form cache settings is a good one, but I think it should be addressed in a follow up issue. We should keep this issue's scope narrow to try and get it in - it's been kicking around for a while already.

mcdruid’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
1.82 KB

Ooops - referenced the wrong function in the comments:

  * example to prolong cache_form entries for AJAX forms in cached HTML.
  *
- * @see form_get_cache()
+ * @see form_set_cache()
  * @see system_cron()

Will set back to RTBC assuming tests pass.

mcdruid’s picture

Status: Needs review » Reviewed & tested by the community
stefan.r’s picture

Assigned: Unassigned » mpdonadio

The idea behind a per form setting was to narrow the impact -- we'd get rid of the global setting entirely and stick to the 6 hours for non-problematic forms, as other custom/contrib code might assume it's still set to 6 hours. But not 100% sure about this, the current patch may be fine, as long as we clearly spell out that changing the global variable may have unintended effects (ideally listing those as well).

The settings.php comment could also be a bit clearer on the impact of setting this to too low of a value (AJAX would be affected, multi-step forms as well).

Assigning to @mpdonadio to further think about how to proceed here, as we had talked about this on IRC.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Status: Reviewed & tested by the community » Needs work
Issue tags: +Security improvements, +Needs change record
Related issues: +#2341447: Possible Denial of Service Attack with {cache_form}

Yes, @stefan_r talked about this in IRC. Don't think I have the logs, but I recall what we talked about.

First, a little background. I independently arrived at the same solution we have in #79 during the course of working on a site that led to #2341447: Possible Denial of Service Attack with {cache_form}. So, this isn't necessarily just about a performance improvement or keeping a database small; a runaway {cache_form} can crash a system. In my case, it was caused by a bad bot and led to a severe crash (total database corruption, had to reinstall mysql and restore backups). That truly sucked.

We have three options on the table here, and they aren't necessarily mutually exclusive:

1. Change the TTL to a global setting. This is what #79 does.

2. Make the TTL configurable per form in $settings.php. This would essentially be

$expire = variable_get('form_cache_expiry_' . $form_id, 21600);

3. Add a special form element #cache_expiry, and people could form_alter to their individual needs. #75 shows this.

It is totally possible to combine the above and come up with a flexible solution.

As we decide on a solution, we need to keep a few things in mind.

- This is likely only a problem with a few forms on a few sites. My case was a public node_form where nodes had lots of fields.
- We can't ignore site owners and site builders who may not be or have developers available at the necessary time. I think this rules out #3 above.
- We need to implement something with minimal disruption, even if the disruption is to a small subset of users. Remember the itok patch? Remember the followups because it caused problems for some site owners? We need to avoid that.

I think the last point above should be at the forefront of our minds given the state and maturity of Drupal 7. I think we are reasonable sure that #1 above wouldn't break anything in core alone (though DRUPAL_MAXIMUM_TEMP_FILE_AGE being the same value concerns me a bit). However, our test coverage of Drupal 7 isn't as good as Drupal 8, and we can't be sure if contrib or custom modules have made any assumptions about the expiration being 6 hours. Do we know of any? I don't think so. Is is possible? Definitely. Would the problems that this would cause be easy to debug? I don't think so.

Problems created by this would potentially cause the loss of user-input (not true data loss from the database, but still vanished into the ether), I think we need to extra precautious about side effects.

I think the TTL being 6 hours is way too high, but it has been that way for a long, long time. Given the audience needing this including site owners and working towards the goal of minimal disruption, I think we need to go with option #2 here. So, I am setting this back to Needs Work so we can

- update the hunk in form.inc/form_set_cache to take the $form_id into account
- update and polish the docs in default.settings.php
- i think this warrants a change record, which can also be used in the docs for further reference

Not sure if I will get to a new version of the patch tonight.

Cameron Tod’s picture

I think that is a great suggestion. Could we defer it to a follow up issue? I can write up a candidate patch and change record so we are ready to go as soon as the simpler patch lands.

Cameron Tod’s picture

Status: Needs work » Reviewed & tested by the community

I've created a follow up issue here: #2813973: Allow custom cache expiry per-form .

It would be great to see the simple patch in #79 make it in!

mcdruid’s picture

The idea behind a per form setting was to narrow the impact -- we'd get rid of the global setting entirely and stick to the 6 hours for non-problematic forms, as other custom/contrib code might assume it's still set to 6 hours. But not 100% sure about this, the current patch may be fine, as long as we clearly spell out that changing the global variable may have unintended effects (ideally listing those as well).

As we're not actually changing the default value from 6 hours, there's not any immediate impact to existing sites. FWIW I think that the comments in settings.php are probably sufficient - AJAX forms are mentioned, for example. I'm not sure it's practical to enumerate every possible consequence of changing the default value.

As mpdonadio mentioned, I suspect that a lot of sites where this has been a problem already have a hack (along the lines of this patch) in place. Getting the simple patch in so that those hacks can be reverted would be great.

Work could then continue on perfecting the implementation of a per-form setting in the follow up issue.

stefan.r’s picture

Assigned: Unassigned » David_Rothstein
quicksketch’s picture

Everyone interested in this issue: if you could also take a look at #2819375: Do not make entries in "cache_form" when viewing forms that use #ajax['callback'] (Drupal 7 port) that would be fantastic. The issue here is still great to have as an option (and is ready to go), but if we can fix this at its root cause, then we could avoid generating the cache entries in the first place in ~9 out of 10 situations.

Fabianx’s picture

I put my RTBC + 1, but given the far reaching consequences of the patch, this indeed is an issue that needs David's sign-off.

alexmoreno’s picture

just my two cents, we've been using this patch in 2 high high traffic sites. RTBC from my point of view