As noted in http://drupal.org/node/644150#comment-2322222 currently the cache is written two times when form caching is activated and the form is rebuilt. I've re-rolled the patch from the linked issue making sure it only caches if we are not rebuilding.

If we rebuild, we cache nevertheless inside the rebuild function. But with a new form-build-id.

Also the code contained a needless !empty(), which is cleaned up as we have default values for that now.

CommentFileSizeAuthor
#4 form-state-cache.patch2.16 KBfago
form-state-cache.patch2.13 KBfago
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

+++ includes/form.inc
@@ -252,6 +237,20 @@ function drupal_build_form($form_id, &$form_state) {
+  elseif ($form_state['cache'] && empty($form_state['no_cache'])) {
...
+      form_set_cache($form_build_id, $original_form, $form_state);
...
+      form_set_cache($form_build_id, NULL, $form_state);

Now we need to put back in the additional isset($form_build_id) condition, because we don't have one in the batch scenario.

This review is powered by Dreditor.

fago’s picture

Yep, I've seen that and noted before:

I improved your patch to fix that by moving the caching block below the rebuild, which also has the side-affect of making caching possible when $_SESSION['batch_form_state'] is set. However the case when running from $_SESSION['batch_form_state'] seems broken to me anyway, as $form never gets set somewhere? But that's another issue then.

If we'd like to fix caching for that case, we should use $form['#build_id'] - but there is no $form...

sun’s picture

Right, so let's just prepend isset($form_build_id) to the elseif condition and we're done, I think. Curious to see whether the new multi-step form storage tests will still pass with this patch then.

fago’s picture

FileSize
2.16 KB

ok done so. Why shouldn't they pass?

Dries’s picture

Status: Needs review » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)
Issue tags: -D7 Form API challenge

Automatically closed -- issue fixed for 2 weeks with no activity.