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
#116 interdiff-2091511-113-116.txt1.21 KBmcdruid
#116 cache_form_expiry_to_variable-2091511-116.patch4.82 KBmcdruid
#113 interdiff-2091511-92-113.txt2.78 KBmcdruid
#113 cache_form_expiry_to_variable-2091511-113.patch4.83 KBmcdruid
#111 cache_form_expiry_to_variable-2091511-111-test-only.patch2.98 KBmcdruid
#92 interdiff.txt2.12 KBDavid_Rothstein
#92 cache_form_expiry_to_variable-2091511-92.patch1.85 KBDavid_Rothstein
#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 default 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 default 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 default 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

mcdruid’s picture

Giving this a gentle bump...

As outlined in #85, the patch from #79 should not actually change anything on existing sites and is therefore low risk.

It does, however, provide the ability to tweak the overall cache_form lifetime setting on sites where that's beneficial, and includes some documentation around this. Lots of big sites have had to do this already as a core hack.

There's a follow up issue for a more tune-able approach e.g. per-form cache lifetimes (linked to in #84). I also agree with @quicksketch that it would be good to address the cause of most cache_form problems, namely ajax forms (#87).

In the meantime though, it would be great to get this simple patch into the next D7 release if at all possible.

mcdruid’s picture

So this missed 7.54 at the start of Feb 2017.

I've checked that the patch in #79 still applies.

Is there anything we can do to get it into the next D7 release?

David_Rothstein’s picture

Sorry for the delay, although I think this issue is still waiting for a change record to be written (that's what other people said above), and for the Drupal 8 issue (#1286154: Allow custom default form cache expiration/lifetime)?

This patch looks fine to me, since it is nice and simple and won't change anything for existing sites. For a site that does opt-in, I also think the risk (of conflict with contrib modules) is low, but maybe I'm missing something. (Wouldn't it only be an issue for a site that tried to increase the expiration time, and even then the worst that happens is that the form stops working correctly after 6 hours which is exactly what it already does now?) A change record is still a good idea though, and if a Drupal 7.60 does wind up coming out soon it would be a good release for this to be in (like stefan said in #68).

Small nitpicks: It should be "expiration" rather than "expiry", since Drupal uses American English (https://www.drupal.org/drupalorg/style-guide/content#english) and "expiry" is, in practice, British. Also, "Ajax" rather than "AJAX". I have fixed those in the attached. I am leaving this at RTBC since those are trivial changes.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work

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

David_Rothstein’s picture

Status: Needs work » Reviewed & tested by the community
mpdonadio’s picture

Issue tags: -Needs change record

Added CR; fine with simple approach and second to RTBC.

stefan.r’s picture

Issue tags: +Drupal 7.60 target
David_Rothstein’s picture

I made some minor edits, but the change record looks good. Hopefully the Drupal 8 patch will be RTBC and ready to commit soon, in time to get this one into Drupal 7.60 in a month or so.

webservant316’s picture

Why not make this setting available at /admin/config/development/performance? More cache control and monitoring is needed from the UI.

The problem is so prevalent yet undetected that I and many were or still are unaware that their 'cache_form' has grown out of control. With a little more effort the patch could make the setting available from the UI and hey why not even add a report of the size of each cache table and even a link to a web page explaining what cache tables sizes are reasonable.

While we are at it we could even add a cache truncate option to clear certain or even all cache tables on demand or on cycle with cron at particular times during the day. There are a number of articles explaining that if 'cache_form' gets too big then there can be problems with Drupal even clearing the table and then a manual table truncate must be performed. I don't understand why, but I have witnessed this myself. This issue goes into further detail, https://www.drupal.org/node/1506196. This module was also created to try to fix the problem, https://www.drupal.org/project/safe_cache_form_clear.

In my case I installed patch #92 and then assigned $conf['form_cache_expiration'] = 1800; in settings.php. I ran clear all cache and cron numerous times with no impact on the size of 'cache_form'. I assume it is because my 'cache_table' is too big. So I manually truncated the table and will now watch to see if the new 1800 expiration will keep 'cache_form' to a reasonable size.

webservant316’s picture

My experiment in #99 seems to be keeping my 'cache_form' table to about 1000 rows and 50mb.

However, that still seems way to high to me. What is in that 'cache_table' blob anyway? Is that the cached form + the CSRF id per user session? If that is the case I might suggest that the right solution also needs to consider better normalization of the data. The form itself should only be cached once for the entire website and the CSRF id once per user or once per user per form. Then the CSRF id can be removed when the session is over. If all a user's form usage can share the same CSRF id then you could even store the CSRF id in the user sessions table or create a new table for that alone. This solution kinda gets at that point, https://www.drupal.org/project/session_cache_form.

Consider a website with a huge form on the front page to collect insurance applications or whatever now hit by millions of people across the globe. Would 'cache_form' create a row storing the cached form and a unique CSRF id for every user? If that is the case then messing with the expiration date is not enough of a solution.

webservant316’s picture

Here is a script I wrote to monitor this if it helps anyone...

echo -e "MYSQL INFO\n" > ./RUN_AMYSQL_INFO.out
mysql 2>&1 >> ./RUN_AMYSQL_INFO.out <<EOF
SELECT sum(data_length + index_length)/1024/1024 "TOTAL MEGABYTES" FROM information_schema.TABLES;
SELECT '';
SELECT table_schema "DATABASE", sum(data_length + index_length)/1024/1024 "MEGABYTES" FROM information_schema.TABLES GROUP BY table_schema ORDER BY sum(data_length + index_length)/1024/1024 DESC;
SELECT '';
SELECT table_schema "DATABASE", table_name "TABLE", sum(data_length + index_length)/1024/1024 "MEGABYTES" FROM information_schema.TABLES GROUP BY table_schema, table_name ORDER BY sum(data_length + index_length)/1024/1024 DESC LIMIT 20;
EOF
echo -e "\nDONE" >> ./RUN_AMYSQL_INFO.out
cat ./RUN_AMYSQL_INFO.out | mail -s "MYSQL DATABASE MONITOR" youremail@yourdomain.com
David_Rothstein’s picture

@webservant316 This patch affects the expiration time of future cache_form entries, not existing ones. So applying this patch and changing the variable won't affect anything with existing cache entries. That said, they certainly should clear out soon anyway, so if they're not you're probably right that there is another issue such as #1506196: cache_form is never cleared on some sites responsible.

I don't think there should be an admin UI for this in core - it is an edge case and it's easy to mess things up if someone changes this and doesn't know what they're doing. However, there could certainly be a contrib module written to provide a UI. (Looks like there are already some contrib modules that try to deal with problems related to this, including the ones you mentioned and also https://www.drupal.org/project/optimizedb. But they may just be working around an actual bug in core.)

I've never fully understood all the details of the form cache, but I'm pretty sure the entire form does need to be cached so that there's something representing the actual state of the form for that particular user. (I can't really think of a good way to figure out what parts of the form can be shared with other users and which can't.) That said, Ajax requests are a big culprit here, and there's an issue at #2819375: Do not make entries in "cache_form" when viewing forms that use #ajax['callback'] (Drupal 7 port) that is trying to make it so they don't have to use cache_form at all.

webservant316’s picture

Okay thanks.

The size of the 'cache_form' table can be disturbing. It was fully 70% of my entire database. If there is someone who understands the data store in that table perhaps something can be done to economize through normalizing the data. Now it may be too difficult because the processed form is simply cached as a blob. One idea is that the user specific elements of the blob could be represented with a substitution string and the user specific elements store elsewhere in the database. Then when the blob for a form could be shared by all users and the user data substituted back in when needed.

mpdonadio’s picture

#103, the main problem is that the flexibility of the form system makes it pretty much impossible to say what can and can't be shared between different displays of the "same" form. Consider node forms. Depending on how you have things configured, you can show/hide fields or make then read-only based on just about any criteria (user roles, permissions, other field values, other node values, other database values). This means you can't predict what can be shared in a generic way.

webservant316’s picture

Hmm, yes I see. User perms could allow completely different views of the form. I assumed it was the unique CSRF id that resulted in the need for each user to have their own cache of the form, but there are many other factors also. Is this a case where we want to allow the option for the form to not be cached at all? But then again wouldn't all anonymous users always have the same view of the form, yet I think they each have their own copy of the form in cache_form. Why is that? Anonymous users ought to be able to share the same cached form.

webservant316’s picture

Could an attacker exploit this weakness? Suppose an attacker hit a form repeatedly could they create rows in this table at will fast enough to endanger the system because of bloated table size either harming MySQL or abusing disk space resources? I read one post where a Drupal admin discovered their 'cache_form' table had grown beyond 1Gb with typical use. So why not 300Gb from an attacker?

mcdruid’s picture

Title: Make cache_form $expiry configurable, to mitigate runaway cache_form table or key:value store growth » Make cache_form $expiry configurable, to mitigate runaway cache_form table

Title tweaked to reflect this being the D7 issue (D8 being dealt with in #1286154: Allow custom default form cache expiration/lifetime)

@webservant316 - I'm sure many of your points are valid (and some have been discussed before in this issue and elsewhere). However, we're hopefully getting close to getting a simple change into D7 here to mitigate the runaway cache_form problem. I'd argue any wider discussion should happen in a separate issue in order to avoid derailing this RTBC'd one.

I'd also point out that cache_form is quite different in D8 (see the other issue mentioned above) and that D7's at a phase in its development life-cycle when radical changes to the architecture are perhaps quite unlikely. That said, #2819375: Do not make entries in "cache_form" when viewing forms that use #ajax['callback'] (Drupal 7 port) which was mentioned by @quicksketch in #87 may be of some interest to you.

webservant316’s picture

okay thanks.

anavarre’s picture

#1286154: Allow custom default form cache expiration/lifetime is now fixed, which means it unblocks this one.

anavarre’s picture

Issue tags: +Needs tests
mcdruid’s picture

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

Tests similar to those added for D8.

These test simple storage and retrieval of a form to / from cache_form (didn't find existing tests specifically for that), plus verify that setting a negative value for form_cache_expiration means that a newly created entry in cache_form is already expired. Unlike in D8 we have to actually clear expired entries from cache_form in order to prevent them being returned.

Test only patch initially; the pair of "expired form" tests should fail.

Status: Needs review » Needs work

The last submitted patch, 111: cache_form_expiry_to_variable-2091511-111-test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

mcdruid’s picture

This time with the actual changes too.

mcdruid’s picture

Would be great to get this back to RTBC if possible; the only thing we've added is similar tests to those we added in #1286154: Allow custom default form cache expiration/lifetime (although the implementation's a little different in D7).

Anyone able to do a quick review?

mpdonadio’s picture

  1. +++ b/modules/simpletest/tests/form.test
    @@ -1421,6 +1421,59 @@ class FormsFormStoragePageCacheTestCase extends DrupalWebTestCase {
    +    return array(
    +        'name' => 'Form caching',
    +        'description' => 'Tests storage and retrieval of forms from cache.',
    +        'group' => 'Form API',
    +    );
    

    Nit, two space indent.

  2. +++ b/modules/simpletest/tests/form.test
    @@ -1421,6 +1421,59 @@ class FormsFormStoragePageCacheTestCase extends DrupalWebTestCase {
    +    variable_set('form_cache_expiration', -1 * (24 * 60 * 60));
    

    Nifty trick for testing this w/o a sleep.

  3. +++ b/modules/simpletest/tests/form_test.module
    @@ -919,6 +919,24 @@ function form_test_storage_page_cache_rebuild($form, &$form_state) {
    +  $form['title'] = array(
    +      '#type' => 'textfield',
    +      '#title' => 'Title',
    +      '#required' => TRUE,
    +  );
    +
    +  $form['submit'] = array(
    +      '#type' => 'submit',
    +      '#value' => 'Save',
    +  );
    

    Nit, two space indent.

Otherwise, looks good to me.

mcdruid’s picture