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
#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