Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
As per http://drupal.org/node/226728#comment-757511, should delete db entries for cache tables where expire + created < time() not just expire < time().
This patched against HEAD
Comment | File | Size | Author |
---|---|---|---|
#17 | fapi_cache.patch | 1.03 KB | eaton |
#12 | form_cache.patch | 1 KB | killes@www.drop.org |
#7 | fapi_cache_expires_6.patch | 887 bytes | jakeg |
#7 | fapi_cache_expires_head.patch | 875 bytes | jakeg |
#1 | cache_clear_all_6.patch | 1.17 KB | jakeg |
Comments
Comment #1
jakeg CreditAttribution: jakeg commentedNot sure if this is how it should be done, but the same patch against DRUPAL-6. Let me know if I'm doing this wrong.
Comment #2
vladimir.dolgopolov CreditAttribution: vladimir.dolgopolov commentedIt seems to me "expire + created < time()" is wrong solution (if the issue exists anyway).
I have written a test for it and got the sample situation in 'cache' table:
cache.created = 1204799443
cache.expire = 1204800443 (+1000 sec)
If we apply your suggestion then we will get: 1204799443 + 1204800443 < time() ?
A kind of strange inequality, IMHO.
Comment #3
jakeg CreditAttribution: jakeg commentedhmm... in which case the 'cache' and 'cache_form' tables use the 'expire' column in different ways. Cache uses it to refer to the timestamp to expire, cache_form uses it to refer to the number of seconds since its creation date. We need to get these to do the same thing instead.
Comment #4
vladimir.dolgopolov CreditAttribution: vladimir.dolgopolov commentedHelp for cache_set():
But you are right - form api uses it in different way! Oops:
But the other code modules use it like this:
Comment #5
eaton CreditAttribution: eaton commentedThis is absolutely a FAPI bug. It should read:
Any interested parties can roll a patch on that, or I can dive in when I have a chance late tonight.
Comment #6
eaton CreditAttribution: eaton commentedChanging this to a FAPI bug in 6, as well. We can't wait a year to fix this one.
Comment #7
jakeg CreditAttribution: jakeg commentedEaton: I take it then that correct practice is to set the 'version' for an issue to the earliest version that it affects?
Attached are patches against HEAD and DRUPAL-6.
Comment #8
jakeg CreditAttribution: jakeg commentedComment #9
Gerhard Killesreiter CreditAttribution: Gerhard Killesreiter commentedIndeed, this patch shoudl be applied urgently.
Comment #10
jakeg CreditAttribution: jakeg commentedComment #11
eaton CreditAttribution: eaton commentedJust another thumbs up on this one. Definitely a critical fix.
Comment #12
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedEven with this fix, the time for expiry is far too long. If you use Drupal's standard settings, this will be three weeks + one day! Enough to really fill your disks if you have some ajaxy forms on every page.
The attached patch includes the above one and changes the time for expiry to two hours.
Comment #13
jakeg CreditAttribution: jakeg commentedOur cache_form is at 1.6GB and 109,000 rows with my earlier patch applied and hence expiry set to 86400 (one day, rather than 3 and a half weeks as my session.cookie_lifetime is set to 0). Less than before my patch, but not good.
Killes' updated patch makes a lot of sense. One or two hours is easily enough for the forms to be in cache. I'd go for one hour, but two should be okay.
Comment #14
eaton CreditAttribution: eaton commentedmy biggest concern is the impact on people who pop open a blog page and compose a longish post, leave, and return only to find that they can't post anymore. Perhaps there's some way we can balance this? I agree, though, that based on jake's data the day-long timeout is insufficient.
Comment #15
jakeg CreditAttribution: jakeg commentedEaton: this only applies to forms with #cache FAPI element, no? A form with #ahah has #cache in by default, but I don't think most other forms do.
And if I ever write something on a form and leave it there for 2 hours, I wouldn't expect to be able to submit it after all that time without at least the chance of a problem.
But perhaps we at least need a way of overriding 2 hours, rather than having this hard-coded. Perhaps you may have a site where you need it to be longer or shorter, and without a way of overriding this it would need a core hack, which is never a good idea.
Comment #16
eaton CreditAttribution: eaton commentedThere may actually be a way of doing this by manipulating $form_state already. I'll double-check.
Comment #17
eaton CreditAttribution: eaton commentedAs an illustration, several days ago I had to fill out an EXTENSIVE registration form for a service. It included no less than seventeen textareas, to be filled out with longish answers to important questions. By the time I finished it, and hit submit, I'd been logged out for inactivity and all the data I'd typed in was lost. Two hours of typing, down the drain. I'd like to prevent that kind of situation, or at least make it possible for those kinds of forms to be 'special cased' if we drop the expiration time down that far.
The attached patch expires every two hours, as suggested by Killes, but allows the form structure itself to override the cache lifetime if it's problematic. This would allow modules to automatically extend the 'lifetime' of a node form while dropping the lifetime of, say, the admin/build/blocks form to 30 minutes.
I think this is the safest solution and allows 'problematic' forms to be tweaked on a one-off basis. It also allows forms that are being altered into 'high traffic forms' to have their cache lifetime shortened during the form_alter() process. I believe this is still a very reasonable solution that falls within the realm of 'safe bug fix' rather than 'API change', and merits inclusion in D6. It's definitely safer, I think, than making a blanket number for every possible form.
Comment #18
jakeg CreditAttribution: jakeg commentedEaton: great idea and use-case. I would probably add that I'd go for just one hour by default rather than two. As per my previous comment, I don't think it gets cached anyway unless you have #cache e.g. with #ahah, no?
Comment #19
jakeg CreditAttribution: jakeg commentedA second case of a critical issue for Drupal 6 which hasn't been applied with the latest 6.2 release. On upgrading to 6.2, I will hence have to manually apply this patch, and so will other large and busy Drupal 6 sites. Very frustrating and encourages users like me to think twice next time before upgrading to a new Drupal release within days of its launch (even though we're now weeks past its launch).
Comment #20
eaton CreditAttribution: eaton commentedJakeG, if you've tested and applied the patch, please mark it 'reviewed and tested by the community.' It can't be committed without that. Period. End of story. And since I wrote the last version, I can't mark it.
Comment #21
andypostMaybe better to make this parameter configurable in admin/settings/performance with default value 2 hours?
Comment #22
eaton CreditAttribution: eaton commentedI'm definitely opposed to adding yet another arcane setting to our already packed administration forms for something that would only need to be changed under extreme circumstances. Support for the $form['#cache_lifetime'] flag means that forms can override the cache setting if need be, and global changes could be made using a sweeping hook_form_alter implementation.
Comment #23
andypostpatch #17 tested for 3 days on live site on stage of content filling.
2 hours enough for most editors to enter an article.
table cache_form not growing - now testing with memcached....
Comment #24
Dries CreditAttribution: Dries commentedMeh, I don't like the special hack to overwrite the lifetime. I increased the lifetime from 2 hours to 6 hours and committed the patch to DRUPAL-6 and CVS HEAD. Hopefully that is a fair trade-off. We can refine in incremental steps. Thanks folks.
Comment #25
eaton CreditAttribution: eaton commentedIs there any willingness to consider the property for inclusion in D7? I don't think it's any more of a hack than other special-use properties -- setting props on the form structure is really the only mechanism we have to set metadata. Perhaps the key should be moved to the form state? In any case, thanks for committing the fix! Three cheers for squashed bugs!
Comment #26
bdragon CreditAttribution: bdragon commentedI would like to note that I got bit by this something fierce today.
We just deployed yesterday on 6.2 and immediately started getting weird form bugs.
Turns out it was this issue. We run cron every minute (for reasons I will not go into at the moment) and forms were being killed faster than people could submit them.
Comment #27
Anonymous (not verified) CreditAttribution: Anonymous commentedAutomatically closed -- issue fixed for two weeks with no activity.
Comment #28
loze CreditAttribution: loze commentedI am still getting this in 6.15
cache_form table is never emptied. (regardless of minimum cache lifetime setting)
the table fills up very quickly.
any suggestions?
Comment #29
AgaPe CreditAttribution: AgaPe commented+
Comment #30
clude CreditAttribution: clude commentedhello,
i have the same problem. The cache_form table is never emptied.
I use Drupal Core 6.20....
Any fix available?
Comment #31
dalin@loze @AgaPe @clude
New issues for new bugs please. The patch applied in #24 fixed "FAPI sets cache_form entry expiration incorrectly". It sounds like you guys are having different issues. If not then you need to show that {cache_form}.expire is being set incorrectly.
Comment #32
Vako CreditAttribution: Vako commentedI'm using Drupal 6.22 and the cache table called cache_form grew from 100 to 1.1GB.