FAPI sets cache_form entry expiration incorrectly

It 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.

Help for cache_set():

 * @param $expire
 *   One of the following values:
 *   - CACHE_PERMANENT: Indicates that the item should never be removed unless
 *     explicitly told to using cache_clear_all() with a cache ID.
 *   - CACHE_TEMPORARY: Indicates that the item should be removed at the next
 *     general cache wipe.
 *   - A Unix timestamp: Indicates that the item should be kept at least until
 *     the given time, after which it behaves like CACHE_TEMPORARY.

But you are right - form api uses it in different way! Oops:

// includes\form.inc
function form_set_cache($form_build_id, $form, $form_state) {
  $expire = max(ini_get('session.cookie_lifetime'), 86400); // here shouls be time() + 86400 or something

  cache_set('form_'. $form_build_id, $form, 'cache_form', $expire);
  if (!empty($form_state['storage'])) {
    cache_set('storage_'. $form_build_id, $form_state['storage'], 'cache_form', $expire);
  }
}

But the other code modules use it like this:

// modules\filter\filter.module 
cache_set($cache_id, $text, 'cache_filter', time() + (60 * 60 * 24));

This is absolutely a FAPI bug. It should read:

cache_set('storage_'. $form_build_id, $form_state['storage'], 'cache_form', time() + $expire);

Any interested parties can roll a patch on that, or I can dive in when I have a chance late tonight.

Changing this to a FAPI bug in 6, as well. We can't wait a year to fix this one.

Indeed, this patch shoudl be applied urgently.

Just another thumbs up on this one. Definitely a critical fix.

Even 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.

my 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.

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.

There may actually be a way of doing this by manipulating $form_state already. I'll double-check.

As 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.

JakeG, 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.

Maybe better to make this parameter configurable in admin/settings/performance with default value 2 hours?

Maybe better to make this parameter configurable in admin/settings/performance with default value 2 hours?

I'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.

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

Meh, 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.

Meh, 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.

Is 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!

I 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.

I 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?

+

hello,

i have the same problem. The cache_form table is never emptied.
I use Drupal Core 6.20....

Any fix available?

@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.

I'm using Drupal 6.22 and the cache table called cache_form grew from 100 to 1.1GB.