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.
Related Issues
#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
Related Modules
Comment | File | Size | Author |
---|---|---|---|
#117 | 2091511-d7-test-only.patch | 2.96 KB | mpdonadio |
#116 | interdiff-2091511-113-116.txt | 1.21 KB | mcdruid |
#116 | cache_form_expiry_to_variable-2091511-116.patch | 4.82 KB | mcdruid |
#113 | interdiff-2091511-92-113.txt | 2.78 KB | mcdruid |
#113 | cache_form_expiry_to_variable-2091511-113.patch | 4.83 KB | mcdruid |
Comments
Comment #1
meba CreditAttribution: meba commentedThis sounds pretty good
Comment #2
adammaloneThis 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.
Comment #3
mcdruidNew patch which only tweaks the comments, but this looks great; a simple solution to what can be a very serious problem.
Comment #4
Cameron Tod CreditAttribution: Cameron Tod commentedRerolled patch to fit comments within 80 characters.
Comment #5
David_Rothstein CreditAttribution: David_Rothstein commentedThis would need to go into Drupal 8 first.
Comment #6
swentel CreditAttribution: swentel commentedNote that we don't have cache_form anymore in D8
Comment #7
BerdirIt 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.
Comment #8
Cameron Tod CreditAttribution: Cameron Tod commentedHere 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.
Comment #10
Anonymous (not verified) CreditAttribution: Anonymous commented8: 2091511-cache_form_expiry_to_variable-D8.patch queued for re-testing.
Comment #13
swentel CreditAttribution: swentel commented8: 2091511-cache_form_expiry_to_variable-D8.patch queued for re-testing.
Comment #15
Cameron Tod CreditAttribution: Cameron Tod commentedSeems like DrupalUnitTestBase only loads a very limited set of config. Setting it explicitly in FormCacheTest makes things go green.
Comment #17
BerdirYou need to inject the configuration object for that and update the PhpUnit tests.
Comment #18
Cameron Tod CreditAttribution: Cameron Tod commentedComment #19
Cameron Tod CreditAttribution: Cameron Tod commentedOK, 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.
Comment #21
alexpottI would swap this around so it is cache.form.expire
Comment #22
BerdirAlso 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.
Comment #23
Cameron Tod CreditAttribution: Cameron Tod commentedChanged cache setting key, added a stub for config.factory to the failing test class.
Comment #25
Cameron Tod CreditAttribution: Cameron Tod commented23: 2091511-cache_form_expiry_to_variable-23.patch queued for re-testing.
Comment #27
Cameron Tod CreditAttribution: Cameron Tod commentedAdded in a stub config value for unit tests.
Comment #28
Cameron Tod CreditAttribution: Cameron Tod commentedI'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?
Comment #29
Cameron Tod CreditAttribution: Cameron Tod commentedSpoken to alexpott on IRC - expiry and form validation is covered in other tests, namely the tests for KeyValueStore and FormCacheTest. Removing tag.
Comment #30
Cameron Tod CreditAttribution: Cameron Tod commentedComment #31
damiankloip CreditAttribution: damiankloip commentedDare I say it, this should really be injected instead. This form builder constructor is getting kind of crazy.
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.
Comment #32
Cameron Tod CreditAttribution: Cameron Tod commentedOk, 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
Comment #35
Cameron Tod CreditAttribution: Cameron Tod commentedComment #36
Cameron Tod CreditAttribution: Cameron Tod commentedWow. Much reading. Such code.
I think this should fix the install failures.
Comment #39
tim.plunkettCross posting #2252763: Views exposed filter form causes enormous form state cache entries
Comment #40
Cameron Tod CreditAttribution: Cameron Tod commentedComment #41
lokapujyaI don't think we need the configFactory part anymore? Can't we do something like : \Drupal::config('system.performance')->get(cache:form:expire)
Comment #42
lokapujyaComment #43
lokapujyaWill try to reroll.
Comment #44
lokapujyaComment #46
Marko B CreditAttribution: Marko B commentedI 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.
Comment #47
BerdirThis 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.
Comment #48
mcdruidThat 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.
Comment #49
lokapujyaThat'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.
Comment #50
Cameron Tod CreditAttribution: Cameron Tod commentedSetting 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.
Comment #51
mcdruid+1 for #4
Comment #54
mcdruidRe-uploading the exact same patch from #4 to try to get that tested against D7 (instead of the last D8 patch which failed).
Comment #57
Cameron Tod CreditAttribution: Cameron Tod as a volunteer and at Acquia commentedRerolled against 7.x head.
Comment #61
Cameron Tod CreditAttribution: Cameron Tod as a volunteer and at Acquia commentedUnless 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.
Comment #62
Cameron Tod CreditAttribution: Cameron Tod as a volunteer and at Acquia commentedComment #63
mcdruidtrying RTBC again...
Comment #64
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThis 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...
Comment #65
dakku CreditAttribution: dakku as a volunteer commentedsubscribe ++
Comment #66
mcdruidDiscussed 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.
Comment #67
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI 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.
Comment #68
stefan.r CreditAttribution: stefan.r commented+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?
Comment #69
mpdonadio#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}.
Comment #70
catchThis 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).
Comment #71
stefan.r CreditAttribution: stefan.r commentedThanks @catch
For D7, can we add this in
$settingsa $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.
Comment #72
stefan.r CreditAttribution: stefan.r commentedComment #73
mcdruidIf 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 useSettings::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.
Comment #74
stefan.r CreditAttribution: stefan.r commented@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.
Comment #75
mpdonadio#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.
Comment #76
ndobromirov CreditAttribution: ndobromirov commentedI like the approach in #75.
We still need to document the new variable in settings.default.php.
Comment #77
mcdruidI 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 fordefault.settings.php
I've also tweaked the comment in form.inc slightly.
Comment #78
Cameron Tod CreditAttribution: Cameron Tod as a volunteer and at Acquia commentedI 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.
Comment #79
mcdruidOoops - referenced the wrong function in the comments:
Will set back to RTBC assuming tests pass.
Comment #80
mcdruidComment #81
stefan.r CreditAttribution: stefan.r commentedThe 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.
Comment #82
mpdonadioYes, @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
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.
Comment #83
Cameron Tod CreditAttribution: Cameron Tod as a volunteer and at Acquia commentedI 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.
Comment #84
Cameron Tod CreditAttribution: Cameron Tod as a volunteer and at Acquia commentedI'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!
Comment #85
mcdruidAs 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.
Comment #86
stefan.r CreditAttribution: stefan.r commentedComment #87
quicksketchEveryone 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.
Comment #88
Fabianx CreditAttribution: Fabianx as a volunteer commentedI put my RTBC + 1, but given the far reaching consequences of the patch, this indeed is an issue that needs David's sign-off.
Comment #89
alexmoreno CreditAttribution: alexmoreno at BBC Worldwide commentedjust my two cents, we've been using this patch in 2 high high traffic sites. RTBC from my point of view
Comment #90
mcdruidGiving 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.
Comment #91
mcdruidSo 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?
Comment #92
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedSorry 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.
Comment #93
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedComment #95
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedComment #96
mpdonadioAdded CR; fine with simple approach and second to RTBC.
Comment #97
stefan.r CreditAttribution: stefan.r commentedComment #98
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedI 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.
Comment #99
webservant316 CreditAttribution: webservant316 commentedWhy 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.
Comment #100
webservant316 CreditAttribution: webservant316 commentedMy 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.
Comment #101
webservant316 CreditAttribution: webservant316 commentedHere is a script I wrote to monitor this if it helps anyone...
Comment #102
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented@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.
Comment #103
webservant316 CreditAttribution: webservant316 commentedOkay 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.
Comment #104
mpdonadio#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.
Comment #105
webservant316 CreditAttribution: webservant316 commentedHmm, 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.
Comment #106
webservant316 CreditAttribution: webservant316 commentedCould 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?
Comment #107
mcdruidTitle 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.
Comment #108
webservant316 CreditAttribution: webservant316 commentedokay thanks.
Comment #109
anavarre#1286154: Allow custom default form cache expiration/lifetime is now fixed, which means it unblocks this one.
Comment #110
anavarreComment #111
mcdruidTests 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 forform_cache_expiration
means that a newly created entry incache_form
is already expired. Unlike in D8 we have to actually clear expired entries fromcache_form
in order to prevent them being returned.Test only patch initially; the pair of "expired form" tests should fail.
Comment #113
mcdruidThis time with the actual changes too.
Comment #114
mcdruidWould 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?
Comment #115
mpdonadioNit, two space indent.
Nifty trick for testing this w/o a sleep.
Nit, two space indent.
Otherwise, looks good to me.
Comment #116
mcdruidThanks - fixed the indentation.
Comment #117
mpdonadioJust want to see this test fail w/o the fix before I RTBC it.
Comment #119
mpdonadioOK, I think my hands are clean enough at this point on this to RTBC; if anyone else things otherwise, bump back to Needs Review. This looks good to me, esp the test.
Comment #120
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedCommitted to 7.x - thanks! The tests look great.
I went ahead and published the change notice also.
Comment #123
webservant316 CreditAttribution: webservant316 commentedSeems like this patch is not included in 7.60. Why is that?
Comment #124
mcdruidThe last several D7 releases have been Security fixes only. There are a several commits which have gone into 7.x-dev but that have yet to make it into an actual release. This is one of them.
7.61 perhaps? (tagging accordingly)