This is a follow up to #2091511: Make cache_form expiration configurable, to mitigate runaway cache_form tables, and should go in after it lands.

Yes, @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

$expire = variable_get('form_cache_expiry_' . $form_id, 21600);
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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Cameron Tod created an issue. See original summary.

Cameron Tod’s picture

David_Rothstein’s picture

I think option #3 is better than option #2, personally. This issue is already a pretty big edge case, so might as well go with the solution that gives people maximum flexibility to override it based on whatever conditions they want, rather than trying to predict exactly what they will need.

The argument against it was:

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.

But I think that's assuming option #3 on its own, not in combination with option #1. In reality, option #1 is what's being done in #2091511: Make cache_form expiration configurable, to mitigate runaway cache_form tables so anything we do here would be in combination with that, and I think option #1 would be sufficient for the emergency scenario where something needs to be quickly configured in settings.php.

David_Rothstein’s picture

poker10’s picture

Title: Allow custom cache expiry per-form » Allow custom cache expiry per-form
Category: Task » Feature request
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

Updating the issue status and category - I think this should be a Feature request, because it is not something that *needs to be done*. The option #1 was already commited by #2091511: Make cache_form expiration configurable, to mitigate runaway cache_form tables and it is working as a workaround for the cache_form problems. Therefore setting this back to Needs work (patch needs to reflect this).

Ideally the issue summary should be updated too, because we need to decide between options #2 and #3 (option #1 is no longer relevant).