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
#57 cache_form_expiry_to_variable-2091511-57.patch858 bytescam8001
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 bytescam8001
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
#36 2091511-cache_form_expiry_to_variable-35.patch10.82 KBcam8001
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 KBcam8001
#32 2091511-cache_form_expiry_to_variable-31.patch10.18 KBcam8001
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
#27 2091511-cache_form_expiry_to_variable-27.patch2.9 KBcam8001
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,715 pass(es).
[ View ]
#23 interdiff.txt1.97 KBcam8001
#23 2091511-cache_form_expiry_to_variable-23.patch2.83 KBcam8001
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 KBcam8001
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 KBcam8001
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 bytescam8001
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 bytescam8001
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 bytescam8001
PASSED: [[SimpleTest]]: [MySQL] 40,379 pass(es).
[ View ]

Comments

meba’s picture

Status:Active» Needs review

This sounds pretty good

typhonius’s picture

Assigned:cam8001» 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

StatusFileSize
new741 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.

cam8001’s picture

StatusFileSize
new747 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.

cam8001’s picture

Status:Needs work» Needs review
Issue tags:+Needs tests
StatusFileSize
new901 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.

cam8001’s picture

Status:Needs work» Needs review
StatusFileSize
new1.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.

cam8001’s picture

Issue tags:+LSDHACK
cam8001’s picture

Status:Needs work» Needs review
StatusFileSize
new2.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.

cam8001’s picture

Status:Needs work» Needs review
StatusFileSize
new2.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 ]
new1.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.

cam8001’s picture

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

cam8001’s picture

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

Added in a stub config value for unit tests.

cam8001’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?

cam8001’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.

cam8001’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.

cam8001’s picture

Related issues:+#512026: Move $form_state storage from cache to new state/key-value system
StatusFileSize
new10.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]
new8.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.

cam8001’s picture

cam8001’s picture

Status:Needs work» Needs review
StatusFileSize
new10.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 ]
new658 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

cam8001’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

lokapujya’s picture

Assigned:Unassigned» lokapujya

Will try to reroll.

lokapujya’s picture

Assigned:lokapujya» Unassigned
Status:Needs work» Needs review
StatusFileSize
new5.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.

cam8001’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
StatusFileSize
new747 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.

cam8001’s picture

Status:Needs work» Needs review
StatusFileSize
new858 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.

cam8001’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.

cam8001’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 ++