- Patch #302240 by fago: fixed various problems with form storage and added tests. Yay.

includes/form.inc

@@ -188,9 +188,8 @@ function drupal_build_form($form_id, &$form_state) {
 
     if ($cacheable && !empty($form['#cache']) && empty($form['#no_cache'])) {
       // Caching is done past drupal_process_form so #process callbacks can
-      // set #cache. By not sending the form state, we avoid storing
-      // $form_state['storage'].
-      form_set_cache($form_build_id, $original_form, NULL);
+      // set #cache.
+      form_set_cache($form_build_id, $original_form, $form_state);
     }
   }
 
@@ -201,14 +200,14 @@ function drupal_build_form($form_id, &$form_state) {
   // the form will simply be re-rendered with the values still in its
   // fields.
   //
-  // If $form_state['storage'] or $form_state['rebuild'] have been
-  // set by any submit or validate handlers, however, we know that
-  // we're in a complex multi-part process of some sort and the form's
-  // workflow is NOT complete. We need to construct a fresh copy of
-  // the form, passing in the latest $form_state in addition to any
-  // other variables passed into drupal_get_form().
-
-  if (!empty($form_state['rebuild']) || !empty($form_state['storage'])) {
+  // If $form_state['storage'] or $form_state['rebuild'] has been set
+  // and the form has been submitted, we know that we're in a complex
+  // multi-part process of some sort and the form's workflow is NOT
+  // complete. We need to construct a fresh copy of the form, passing
+  // in the latest $form_state in addition to any other variables passed
+  // into drupal_get_form().
+
+  if ((!empty($form_state['storage']) || !empty($form_state['rebuild'])) && !empty($form_state['submitted']) && !form_get_errors()) {
     $form = drupal_rebuild_form($form_id, $form_state);
   }
 

modules/simpletest/tests/form.test

@@ -383,3 +383,69 @@ class FormAPITestCase extends DrupalWebTestCase {
   }
 
 }
+
+/**
+ * Test the form storage on a multistep form.
+ *
+ * The tested form puts data into the storage during the initial form
+ * construction. These tests verify that there are no duplicate form
+ * constructions, with and without manual form caching activiated. Furthermore
+ * when a validation error occurs, it makes sure that changed form element
+ * values aren't lost due to a wrong form rebuild.
+ */
+class FormsFormStorageTestCase extends DrupalWebTestCase {
+
+  function getInfo() {
+    return array(
+      'name'  => t('Multistep form using form storage'),
+      'description'  => t('Tests a multistep form using form storage and makes sure validation and caching works right.'),
+      'group' => t('Form API'),
+    );
+  }
+
+  function setUp() {
+    parent::setUp('form_test');
+  }
+
+  /**
+   * Tests using the form in a usual way.
+   */
+  function testForm() {
+    
+    $user = $this->drupalCreateUser(array('access content'));
+    $this->drupalLogin($user);
+
+    $this->drupalPost('form_test/form-storage', array('title' => 'new', 'value' => 'value_is_set'), 'Continue');
+    $this->assertText('Form constructions: 2', t('The form has been constructed two times till now.'));
+
+    $this->drupalPost(NULL, array(), 'Save');
+    $this->assertText('Form constructions: 3', t('The form has been constructed three times till now.'));
+    $this->assertText('Title: new', t('The form storage has stored the values.'));
+  }
+
+  /**
+   * Tests using the form with an activated #cache property.
+   */
+  function testFormCached() {
+    $user = $this->drupalCreateUser(array('access content'));
+    $this->drupalLogin($user);
+
+    $this->drupalPost('form_test/form-storage', array('title' => 'new', 'value' => 'value_is_set'), 'Continue', array('query' => 'cache=1'));
+    $this->assertText('Form constructions: 1', t('The form has been constructed one time till now.'));
+    
+    $this->drupalPost(NULL, array(), 'Save', array('query' => 'cache=1'));
+    $this->assertText('Form constructions: 2', t('The form has been constructed two times till now.'));
+    $this->assertText('Title: new', t('The form storage has stored the values.'));
+  }
+
+  /**
+   * Tests validation when form storage is used.
+   */
+  function testValidation() {
+    $user = $this->drupalCreateUser(array('access content'));
+    $this->drupalLogin($user);
+
+    $this->drupalPost('form_test/form-storage', array('title' => '', 'value' => 'value_is_set'), 'Continue');
+    $this->assertPattern('/value_is_set/', t("The input values have been kept."));
+  }
+}

modules/simpletest/tests/form_test.module

@@ -58,6 +58,14 @@ function form_test_menu() {
     'type' => MENU_CALLBACK,
   );
 
+  $items['form_test/form-storage'] = array(
+    'title' => 'Form storage test',
+    'page callback' => 'drupal_get_form',
+    'page arguments' => array('form_storage_test_form'),
+    'access arguments' => array('access content'),
+    'type' => MENU_CALLBACK,
+  );
+
   return $items;
 }
 
@@ -279,3 +287,81 @@ function form_test_mock_form($form_state) {
 function form_test_mock_form_submit($form, &$form_state) {
   variable_set('form_test_mock_submit', $form_state['values']['test_value']);
 }
+
+/**
+ * A multistep form for testing the form storage.
+ *
+ * It uses two steps for editing a virtual "thing". Any changes to it are saved
+ * in the form storage and have to be present during any step. By setting the
+ * request parameter "cache" the form can be tested with caching enabled, as
+ * it would be the case, if the form would contain some #ahah callbacks.
+ *
+ * @see form_storage_test_form_submit().
+ */
+function form_storage_test_form(&$form_state) {
+  // Initialize
+  if (!isset($form_state['storage'])) {
+    if (empty($form_state['input'])) {
+      $_SESSION['constructions'] = 0;
+    }
+    // Put the initial thing into the storage
+    $form_state['storage'] = array(
+      'thing' => array(
+        'title' => 'none',
+        'value' => '',
+      ),
+    );
+    $form_state['storage'] += array('step' => 1);
+  }
+
+  // Count how often the form is constructed
+  $_SESSION['constructions']++;
+
+  if ($form_state['storage']['step'] == 1) {
+    $form['title'] = array(
+      '#type' => 'textfield',
+      '#title' => 'title',
+      '#default_value' => $form_state['storage']['thing']['title'],
+      '#required' => TRUE,
+    );
+    $form['value'] = array(
+      '#type' => 'textfield',
+      '#title' => 'value',
+      '#default_value' => $form_state['storage']['thing']['value'],
+    );
+    $form['submit'] = array(
+      '#type' => 'submit',
+      '#value' => 'Continue',
+    );
+  }
+  else {
+    $form['content'] = array('#value' => 'This is the second step.');
+    $form['submit'] = array(
+      '#type' => 'submit',
+      '#value' => 'Save',
+    );
+  }
+
+  if (isset($_REQUEST['cache'])) {
+    // Manually activate caching, so we can test that the storage keeps working
+    // when it's enabled.
+    $form['#cache'] = TRUE;
+  }
+
+  return $form;
+}
+
+/**
+ * Multistep form submit callback.
+ */
+function form_storage_test_form_submit($form, &$form_state) {
+  if ($form_state['storage']['step'] == 1) {
+    $form_state['storage']['thing']['title'] = $form_state['values']['title'];
+    $form_state['storage']['thing']['value'] = $form_state['values']['value'];
+  }
+  else {
+    drupal_set_message("Title: ". check_plain($form_state['storage']['thing']['title']));
+  }
+  $form_state['storage']['step']++;
+  drupal_set_message("Form constructions: ". $_SESSION['constructions']);
+}