Form cache is not cleared on submit when page cache is activated

Patch is trivial, so rtbc...

Committed to CVS HEAD. This might need to go into Drupal 6 so updating version for Gabor.

Huh, what? The presumption was that the form, as cached, is reusable for folks, why this patch was necessary?

Committed and then rolled back in Drupal 6 due to chx's comment.

I am not happy with Gabor's so quickly jumping the gun :( oh well.

When writing FAPI3 two years ago (wow so long) we put an extra effort in so that anonymous forms especially the login form is cacheable. The reason for this was, originally we wanted every form to be cached and in this case, forms would break if they would be cached by the page cache but not the form cache. We have decided at the end against this so the code might reflect the earlier decision. Rebuilding the login form when the page cache expires is a quick enough operation not to worry about.

However, the patch made us discover a bug , where #cache TRUE forms can be broken for anonymous users when page caching is on. See, the build id will be the same and then if anon user A rebuilds the form based on her submission then anon user B wil see the wrong form.

I am happy that I was quick in jumping the gun, after all you opposed committing the patch, and there should be nothing committed in a form of backport in D6, which is actively opposed in D7. I am also happy that this triggered finding an actual bug. Good.

Why not to leave - better to check cache than call 2 cache_clear_all()

-      if (variable_get('cache', CACHE_DISABLED) == CACHE_DISABLED && !empty($form_state['values']['form_build_id'])) {
+      if (variable_get('cache', CACHE_DISABLED) != CACHE_DISABLED && !empty($form_state['values']['form_build_id'])) {

First of all, this needs to be rolled back quickly.

Next (yeah, you know what comes), if there were tests, the original patch would not have passed.

@Damien Tournoud as performance I propose to include check for "is cache enabled"

I've rolled back this patch so we can discuss it more.

#7 no longer applies so back to cnw.

Re-rolling #7. Used a version of this patch in D6 and it works for solving problems with user registration, AHAH and page caching where the build ids were previously stepping on one another.

Let's revisit this. I'm not sure I understand chx's reasoning to additionally include the session ID in the cache ID, since a new form_build_id is generated for every build of a form, except when a form is retrieved from cache, which should only happen for POST requests.

@sun, If I have well understood chx in comment #7, if #cache = TRUE and page caching is ON, then anonymous users can get the same "form_build_id", even without performing any POST requests. The session_id stuff would allow to not get the cached form copy if it does not belong to the user that created the cached page the first time.

But I think that the condition to not clearing out the form cache if the cache of pages is ON, is still very valid, I confess that I have not tested the patch in #16, but I would expect to have problems with #cache = TRUE + a form in a cached page if we remove the "form" cached version and at the same time we keep running with the cached page version....

The right question to answer here is: does "form cache" + "page cache" + #cache = TRUE work (D6), and how does it work ?
The current answer (D6) is "yes, as long as the form cache is not removed before the cache page", and the current problem with the current behavior is "pollution of form_cache entries".

PS: I can confirm that I have experienced issues with AHAH forms inside cached pages if the form cache version is deleted without deleting the page's cache. Deleting the form cache always as the patch in #7 does, does not help IMHO.

PSS: The "global $user" right after the declaration of "fonction drupal_process_form" is useless IMHO too.

Hope this helps

Apparently, all patches here conflicted with #1694574: drupal_process_form() deletes cached form + form_state despite still needed for later POSTs with enabled page caching and would have made that bug even more apparent.

This one made us think for a bit, heres my recap.

Problem: cached forms may reach a state where the build id is no longer valid.
Cause: The form build id no longer exists in the caching table because it is older than 6 hours and we've performed garbage collection via cron, removing it from the form_cache table.
Solution (not really): set max page cache lifetime to 6 hours so that we regenerate form build ids around the same time our build ids become invalid and are set to be deleted by cron.

Timeline of a broken form:
0 hour: generate page and form id, cache both.
+6 hours: form_cache's build id is now expired, but still valid (Drupal just checks if it exists in the cache table)
Sometime after 6 hours: cron runs, deletes expired build ids and breaks cached pages.

Note: on form render, Drupal will create a new build id if the current id is expired.

Thanks @DaneMacaulay, that's a good summary. That helped me understand what we're dealing with here.

I should add that in almost all cases, the first build ID has absolutely no value, because it's just a blank form, possibly pre-populated with some default values. Let's say an anonymous user is leaving a comment and uses the "Preview" button. If the cache_form table (or key_value_expire table in D8) has an entry, then that version of the comment form is used. If not, the form is regenerated for anonymous (with a new build ID) so that it can be submitted. Regenerating the form to submit it is trivial, as we do this for all authenticated users on every page load with a form on it.

So in summary, keeping the form builds for first-load anonymous users has no value. Therefor, clearing the build on the first submit has no penalty. So the patch here makes an improvement as it prevent the cache_form table from being filled up with worthless caches (one per *every* form ever generated *or* submitted!) For consistency, in addition to this patch , the best thing to do would be to not save an initial build for anonymous users in the first place.

So in summary, keeping the form builds for first-load anonymous users has no value.

Scratch that. After reading through #1694574: drupal_process_form() deletes cached form + form_state despite still needed for later POSTs with enabled page caching, it's apparent that any use of #ajax is dependent up on this first-time build ID. If we removed it for anonymous users, things like AJAX file uploads would not work for anonymous users (very common in modules like Webform). So this exposes yet another bug: If your cache lifetime is longer than 6 hours, after that time period, AJAX uploads will fail as well for anonymous users.

This has been resolved by now, in issues like #512026: Move $form_state storage from cache to new state/key-value system. Marking outdated.