- Patch #622922 by chx, fago, sun, effulgentsia: form API clean-up, bugfixes and extra tests.

includes/form.inc

@@ -284,10 +284,7 @@ function form_state_defaults() {
 }
 
 /**
- * Retrieves a form, caches it and processes it with an empty $_POST.
- *
- * This function clears $_POST and passes the empty $_POST to the form_builder.
- * To preserve some parts from $_POST, pass them in $form_state.
+ * Retrieves a form, caches it and processes it again.
  *
  * If your AHAH callback simulates the pressing of a button, then your AHAH
  * callback will need to do the same as what drupal_get_form would do when the
@@ -315,6 +312,13 @@ function form_state_defaults() {
  *   The newly built form.
  */
 function drupal_rebuild_form($form_id, &$form_state, $form_build_id = NULL) {
+  // AJAX and other contexts may call drupal_rebuild_form() even when
+  // $form_state['rebuild'] isn't set, but _form_builder_handle_input_element()
+  // needs to distinguish a rebuild from an initial build in order to process
+  // user input correctly. Form constructors and form processing functions may
+  // also need to handle a rebuild differently than an initial build.
+  $form_state['rebuild'] = TRUE;
+
   $form = drupal_retrieve_form($form_id, $form_state);
 
   if (!isset($form_build_id)) {
@@ -331,13 +335,8 @@ function drupal_rebuild_form($form_id, &$form_state, $form_build_id = NULL) {
     form_set_cache($form_build_id, $form, $form_state);
   }
 
-  // Clear out all post data, as we don't want the previous step's
-  // data to pollute this one and trigger validate/submit handling,
-  // then process the form for rendering.
-  $form_state['input'] = array();
-
-  // Also clear out all group associations as these might be different
-  // when rerendering the form.
+  // Clear out all group associations as these might be different when
+  // re-rendering the form.
   $form_state['groups'] = array();
 
   // Do not call drupal_process_form(), since it would prevent the rebuilt form
@@ -409,10 +408,13 @@ function form_set_cache($form_build_id, $form, $form_state) {
  *   different $form_id values to the proper form constructor function. Examples
  *   may be found in node_forms(), search_forms(), and user_forms().
  * @param $form_state
- *   A keyed array containing the current state of the form. Most
- *   important is the $form_state['values'] collection, a tree of data
- *   used to simulate the incoming $_POST information from a user's
- *   form submission.
+ *   A keyed array containing the current state of the form. Most important is
+ *   the $form_state['values'] collection, a tree of data used to simulate the
+ *   incoming $_POST information from a user's form submission. If a key is not
+ *   filled in $form_state['values'], then the default value of the respective
+ *   element is used. To submit an unchecked checkbox or other control that
+ *   browsers submit by not having a $_POST entry, include the key, but set the
+ *   value to NULL.
  * @param ...
  *   Any additional arguments are passed on to the functions called by
  *   drupal_form_submit(), including the unique form constructor function.
@@ -935,9 +937,11 @@ function _form_validate(&$elements, &$form_state, $form_id = NULL) {
  */
 function form_execute_handlers($type, &$form, &$form_state) {
   $return = FALSE;
+  // If there was a button pressed, use its handlers.
   if (isset($form_state[$type . '_handlers'])) {
     $handlers = $form_state[$type . '_handlers'];
   }
+  // Otherwise, check for a form-level handler.
   elseif (isset($form['#' . $type])) {
     $handlers = $form['#' . $type];
   }
@@ -1205,13 +1209,40 @@ function _form_builder_handle_input_element($form_id, &$element, &$form_state) {
     $value_callback = !empty($element['#value_callback']) ? $element['#value_callback'] : 'form_type_' . $element['#type'] . '_value';
 
     if ($form_state['programmed'] || ($form_state['process_input'] && (!isset($element['#access']) || $element['#access']))) {
+      // Get the input for the current element. NULL values in the input need to
+      // be explicitly distinguished from missing input. (see below)
       $input = $form_state['input'];
+      $input_exists = TRUE;
       foreach ($element['#parents'] as $parent) {
-        $input = isset($input[$parent]) ? $input[$parent] : NULL;
+        if (is_array($input) && array_key_exists($parent, $input)) {
+          $input = $input[$parent];
+        }
+        else {
+          $input = NULL;
+          $input_exists = FALSE;
+          break;
+        }
+      }
+      // For browser-submitted forms, the submitted values do not contain values
+      // for certain elements (empty multiple select, unchecked checkbox).
+      // During initial form processing, we add explicit NULL values for such
+      // elements in $form_state['input']. When rebuilding the form, we can
+      // distinguish elements having NULL input from elements that were not part
+      // of the initially submitted form and can therefore use default values
+      // for the latter, if required. Programmatically submitted forms can
+      // submit explicit NULL values when calling drupal_form_submit(), so we do
+      // not modify $form_state['input'] for them.
+      if (!$input_exists && !$form_state['rebuild'] && !$form_state['programmed']) {
+        // We leverage the internal logic of form_set_value() to change the
+        // input values by passing $form_state['input'] instead of the usual
+        // $form_state['values']. In effect, this adds the necessary parent keys
+        // to $form_state['input'] and sets the element's input value to NULL.
+        _form_set_value($form_state['input'], $element, $element['#parents'], NULL);
+        $input_exists = TRUE;
       }
-      // If we have input for the current element, assign it to the #value property.
-      if (!$form_state['programmed'] || isset($input)) {
-        // Call #type_value to set the form value;
+      // If we have input for the current element, assign it to the #value
+      // property, optionally filtered through $value_callback.
+      if ($input_exists) {
         if (function_exists($value_callback)) {
           $element['#value'] = $value_callback($element, $input, $form_state);
         }
@@ -1595,11 +1626,10 @@ function form_set_value($element, $value, &$form_state) {
 }
 
 /**
- * Helper function for form_set_value().
+ * Helper function for form_set_value() and _form_builder_handle_input_element().
  *
- * We iterate over $parents and create nested arrays for them
- * in $form_state['values'] if needed. Then we insert the value into
- * the right array.
+ * We iterate over $parents and create nested arrays for them in $form_values if
+ * needed. Then we insert the value into the last parent key.
  */
 function _form_set_value(&$form_values, $element, $parents, $value) {
   $parent = array_shift($parents);

modules/field/field.form.inc

@@ -131,6 +131,11 @@ function field_default_form($obj_type, $object, $field, $instance, $langcode, $i
  * - drag-n-drop value reordering
  */
 function field_multiple_value_form($field, $instance, $langcode, $items, &$form, &$form_state) {
+  // This form has its own multistep persistance.
+  if ($form_state['rebuild']) {
+    $form_state['input'] = array();
+  }
+
   $field_name = $field['field_name'];
 
   // Determine the number of widgets to display.

modules/field/tests/field.test

@@ -1071,7 +1071,8 @@ class FieldAttachOtherTestCase extends FieldAttachTestCase {
     $entity_type = 'test_entity';
     $entity = field_test_create_stub_entity(0, 0, $this->instance['bundle']);
 
-    $form = $form_state = array();
+    $form = array();
+    $form_state = form_state_defaults();
     field_attach_form($entity_type, $entity, $form, $form_state);
 
     $langcode = FIELD_LANGUAGE_NONE;
@@ -1090,7 +1091,8 @@ class FieldAttachOtherTestCase extends FieldAttachTestCase {
     $entity = field_test_create_stub_entity(0, 0, $this->instance['bundle']);
 
     // Build the form.
-    $form = $form_state = array();
+    $form = array();
+    $form_state = form_state_defaults();
     field_attach_form($entity_type, $entity, $form, $form_state);
 
     // Simulate incoming values.

modules/node/node.pages.inc

@@ -109,6 +109,10 @@ function node_object_prepare($node) {
  */
 function node_form($form, &$form_state, $node) {
   global $user;
+  // This form has its own multistep persistance.
+  if ($form_state['rebuild']) {
+    $form_state['input'] = array();
+  }
 
   if (isset($form_state['node'])) {
     $node = (object)($form_state['node'] + (array)$node);

modules/simpletest/tests/ajax.test

@@ -141,3 +141,48 @@ class AJAXCommandsTestCase extends AJAXTestCase {
     $this->assertTrue($command['command'] == 'restripe' && $command['selector'] == '#restripe_table', "'restripe' AJAX command issued with correct selector");
   }
 }
+
+/**
+ * Test that $form_state['values'] is properly delivered to $ajax['callback'].
+ */
+class AJAXFormValuesTestCase extends AJAXTestCase {
+  public static function getInfo() {
+    return array(
+      'name' => 'AJAX command form values',
+      'description' => 'Tests that form values are properly delivered to AJAX callbacks.',
+      'group' => 'AJAX',
+    );
+  }
+
+  function setUp() {
+    parent::setUp();
+
+    $this->web_user = $this->drupalCreateUser(array('access content'));
+    $this->drupalLogin($this->web_user);
+  }
+
+  /**
+   * Create a simple form, then POST to system/ajax to change to it.
+   */
+  function testSimpleAJAXFormValue() {
+    // Verify form values of a select element.
+    foreach(array('red', 'green', 'blue') as $item) {
+      $edit = array(
+        'select' => $item,
+      );
+      $commands = $this->drupalPostAJAX('ajax_forms_test_get_form', $edit, 'select');
+      $data_command = $commands[2];
+      $this->assertEqual($data_command['value'], $item);
+    }
+
+    // Verify form values of a checkbox element.
+    foreach(array(FALSE, TRUE) as $item) {
+      $edit = array(
+        'checkbox' => $item,
+      );
+      $commands = $this->drupalPostAJAX('ajax_forms_test_get_form', $edit, 'checkbox');
+      $data_command = $commands[2];
+      $this->assertEqual((int) $data_command['value'], (int) $item);
+    }
+  }
+}

modules/simpletest/tests/form.test

@@ -70,7 +70,8 @@ class FormsTestCase extends DrupalWebTestCase {
       foreach ($data['empty_values'] as $key => $empty) {
         foreach (array(TRUE, FALSE) as $required) {
           $form_id = $this->randomName();
-          $form = $form_state = array();
+          $form = array();
+          $form_state = form_state_defaults();
           form_clear_error();
           $form['op'] = array('#type' => 'submit', '#value' => t('Submit'));
           $element = $data['element']['#title'];
@@ -131,9 +132,7 @@ class FormsTestCase extends DrupalWebTestCase {
     $this->assertRaw(t('!name field is required.', array('!name' => 'required_checkbox')), t('A required checkbox is actually mandatory'));
 
     // Now try to submit the form correctly.
-    $this->drupalPost(NULL, array('required_checkbox' => 1), t('Submit'));
-
-    $values = json_decode($this->drupalGetContent(), TRUE);
+    $values = drupal_json_decode($this->drupalPost(NULL, array('required_checkbox' => 1), t('Submit')));
     $expected_values = array(
       'disabled_checkbox_on' => 'disabled_checkbox_on',
       'disabled_checkbox_off' => '',
@@ -468,16 +467,15 @@ class FormsFormStorageTestCase extends DrupalWebTestCase {
 
   function setUp() {
     parent::setUp('form_test');
+
+    $this->web_user = $this->drupalCreateUser(array('access content'));
+    $this->drupalLogin($this->web_user);
   }
 
   /**
    * 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.'));
 
@@ -490,9 +488,6 @@ class FormsFormStorageTestCase extends DrupalWebTestCase {
    * Tests using the form with an activated $form_state['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' => array('cache' => 1)));
     $this->assertText('Form constructions: 1', t('The form has been constructed one time till now.'));
 
@@ -505,12 +500,47 @@ class FormsFormStorageTestCase extends DrupalWebTestCase {
    * 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."));
   }
+
+  /**
+   * Tests updating cached form storage during form validation.
+   *
+   * If form caching is enabled and a form stores data in the form storage, then
+   * the form storage also has to be updated in case of a validation error in
+   * the form. This test re-uses the existing form for multi-step tests, but
+   * triggers a special #element_validate handler to update the form storage
+   * during form validation, while another, required element in the form
+   * triggers a form validation error.
+   */
+  function testCachedFormStorageValidation() {
+    // Request the form with 'cache' query parameter to enable form caching.
+    $this->drupalGet('form_test/form-storage', array('query' => array('cache' => 1)));
+
+    // Skip step 1 of the multi-step form, since the first step copies over
+    // 'title' into form storage, but we want to verify that changes in the form
+    // storage are updated in the cache during form validation.
+    $edit = array('title' => 'foo');
+    $this->drupalPost(NULL, $edit, 'Continue');
+
+    // In step 2, trigger a validation error for the required 'title' field, and
+    // post the special 'change_title' value for the 'value' field, which
+    // conditionally invokes the #element_validate handler to update the form
+    // storage.
+    $edit = array('title' => '', 'value' => 'change_title');
+    $this->drupalPost(NULL, $edit, 'Save');
+
+    // At this point, the form storage should contain updated values, but we do
+    // not see them, because the form has not been rebuilt yet due to the
+    // validation error. Post again with an arbitrary 'title' (which is only
+    // updated in form storage in step 1) and verify that the rebuilt form
+    // contains the values of the updated form storage.
+    $edit = array('title' => 'foo', 'value' => '');
+    $this->drupalPost(NULL, $edit, 'Save');
+    $this->assertFieldByName('title', 'title_changed', t('The altered form storage value was updated in cache and taken over.'));
+    $this->assertText('Title: title_changed', t('The form storage has stored the values.'));
+  }
 }
 
 /**
@@ -559,8 +589,7 @@ class FormStateValuesCleanTestCase extends DrupalWebTestCase {
    * Tests form_state_values_clean().
    */
   function testFormStateValuesClean() {
-    $this->drupalPost('form_test/form-state-values-clean', array(), t('Submit'));
-    $values = json_decode($this->content, TRUE);
+    $values = drupal_json_decode($this->drupalPost('form_test/form-state-values-clean', array(), t('Submit')));
 
     // Setup the expected result.
     $result = array(
@@ -588,3 +617,46 @@ class FormStateValuesCleanTestCase extends DrupalWebTestCase {
   }
 }
 
+/**
+ * Tests form rebuilding.
+ *
+ * @todo Add tests for other aspects of form rebuilding.
+ */
+class FormsRebuildTestCase extends DrupalWebTestCase {
+  public static function getInfo() {
+    return array(
+      'name' => 'Form rebuilding',
+      'description' => 'Tests functionality of drupal_rebuild_form().',
+      'group' => 'Form API',
+    );
+  }
+
+  function setUp() {
+    parent::setUp('form_test');
+
+    $this->web_user = $this->drupalCreateUser(array('access content'));
+    $this->drupalLogin($this->web_user);
+  }
+
+  /**
+   * Tests preservation of values.
+   */
+  function testRebuildPreservesValues() {
+    $edit = array(
+      'checkbox_1_default_off' => TRUE,
+      'checkbox_1_default_on' => FALSE,
+      'text_1' => 'foo',
+    );
+    $this->drupalPost('form-test/form-rebuild-preserve-values', $edit, 'Add more');
+
+    // Verify that initial elements retained their submitted values.
+    $this->assertFieldChecked('edit-checkbox-1-default-off', t('A submitted checked checkbox retained its checked state during a rebuild.'));
+    $this->assertNoFieldChecked('edit-checkbox-1-default-on', t('A submitted unchecked checkbox retained its unchecked state during a rebuild.'));
+    $this->assertFieldById('edit-text-1', 'foo', t('A textfield retained its submitted value during a rebuild.'));
+
+    // Verify that newly added elements were initialized with their default values.
+    $this->assertFieldChecked('edit-checkbox-2-default-on', t('A newly added checkbox was initialized with a default checked state.'));
+    $this->assertNoFieldChecked('edit-checkbox-2-default-off', t('A newly added checkbox was initialized with a default unchecked state.'));
+    $this->assertFieldById('edit-text-2', 'DEFAULT 2', t('A newly added textfield was initialized with its default value.'));
+  }
+}

modules/simpletest/tests/form_test.module

@@ -57,7 +57,7 @@ function form_test_menu() {
   $items['form_test/form-storage'] = array(
     'title' => 'Form storage test',
     'page callback' => 'drupal_get_form',
-    'page arguments' => array('form_storage_test_form'),
+    'page arguments' => array('form_test_storage_form'),
     'access arguments' => array('access content'),
     'type' => MENU_CALLBACK,
   );
@@ -86,6 +86,14 @@ function form_test_menu() {
     'type' => MENU_CALLBACK,
   );
 
+  $items['form-test/form-rebuild-preserve-values'] = array(
+    'title' => 'Form values preservation during rebuild test',
+    'page callback' => 'drupal_get_form',
+    'page arguments' => array('form_test_form_rebuild_preserve_values_form'),
+    'access arguments' => array('access content'),
+    'type' => MENU_CALLBACK,
+  );
+
   return $items;
 }
 
@@ -370,9 +378,12 @@ function form_test_mock_form_submit($form, &$form_state) {
  * request parameter "cache" the form can be tested with caching enabled, as
  * it would be the case, if the form would contain some #ajax callbacks.
  *
- * @see form_storage_test_form_submit().
+ * @see form_test_storage_form_submit().
  */
-function form_storage_test_form($form, &$form_state) {
+function form_test_storage_form($form, &$form_state) {
+  if ($form_state['rebuild']) {
+    $form_state['input'] = array();
+  }
   // Initialize
   if (empty($form_state['storage'])) {
     if (empty($form_state['input'])) {
@@ -391,25 +402,29 @@ function form_storage_test_form($form, &$form_state) {
   // Count how often the form is constructed
   $_SESSION['constructions']++;
 
+  $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'],
+    '#element_validate' => array('form_test_storage_element_validate_value_cached'),
+  );
   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['body'] = array('#value' => 'This is the second step.');
+    $form['body'] = array(
+      '#type' => 'item',
+      '#value' => 'This is the second step.',
+    );
     $form['submit'] = array(
       '#type' => 'submit',
       '#value' => 'Save',
@@ -426,9 +441,27 @@ function form_storage_test_form($form, &$form_state) {
 }
 
 /**
- * Multistep form submit callback.
+ * Form element validation handler for 'value' element in form_test_storage_form().
+ *
+ * Tests updating of cached form storage during validation.
  */
-function form_storage_test_form_submit($form, &$form_state) {
+function form_test_storage_element_validate_value_cached($element, &$form_state) {
+  // If caching is enabled and we receive a certain value, change the value of
+  // 'title'. This presumes that another submitted form value triggers a
+  // validation error elsewhere in the form. Form API should still update the
+  // cached form storage though.
+  if (isset($_REQUEST['cache']) && $form_state['values']['value'] == 'change_title') {
+    $form_state['storage']['thing']['title'] = 'title_changed';
+    // @todo Fix FAPI to make it unnecessary to explicitly set the cache flag in
+    //   this situation. @see http://drupal.org/node/641356.
+    $form_state['cache'] = TRUE;
+  }
+}
+
+/**
+ * Form submit handler for form_test_storage_form().
+ */
+function form_test_storage_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'];
@@ -567,3 +600,81 @@ function _form_test_checkbox_submit($form, &$form_state) {
   drupal_json_output($form_state['values']);
   exit();
 }
+
+/**
+ * Form builder for testing preservation of values during a rebuild.
+ */
+function form_test_form_rebuild_preserve_values_form($form, &$form_state) {
+  // Start the form with two checkboxes, to test different defaults, and a
+  // textfield, to test more than one element type.
+  $form = array(
+    'checkbox_1_default_off' => array(
+      '#type' => 'checkbox',
+      '#title' => t('This checkbox defaults to unchecked.'),
+      '#default_value' => FALSE,
+    ),
+    'checkbox_1_default_on' => array(
+      '#type' => 'checkbox',
+      '#title' => t('This checkbox defaults to checked.'),
+      '#default_value' => TRUE,
+    ),
+    'text_1' => array(
+      '#type' => 'textfield',
+      '#title' => t('This textfield has a non-empty default value.'),
+      '#default_value' => 'DEFAULT 1',
+    ),
+  );
+  // Provide an 'add more' button that rebuilds the form with an additional two
+  // checkboxes and a textfield. The test is to make sure that the rebuild
+  // triggered by this button preserves the user input values for the initial
+  // elements and initializes the new elements with the correct default values.
+  if (empty($form_state['storage']['add_more'])) {
+    $form['add_more'] = array(
+      '#type' => 'submit',
+      '#value' => 'Add more',
+      '#submit' => array('form_test_form_rebuild_preserve_values_form_add_more'),
+    );
+  }
+  else {
+    $form += array(
+      'checkbox_2_default_off' => array(
+        '#type' => 'checkbox',
+        '#title' => t('This checkbox defaults to unchecked.'),
+        '#default_value' => FALSE,
+      ),
+      'checkbox_2_default_on' => array(
+        '#type' => 'checkbox',
+        '#title' => t('This checkbox defaults to checked.'),
+        '#default_value' => TRUE,
+      ),
+      'text_2' => array(
+        '#type' => 'textfield',
+        '#title' => t('This textfield has a non-empty default value.'),
+        '#default_value' => 'DEFAULT 2',
+      ),
+    );
+  }
+  // A submit button that finishes the form workflow (does not rebuild).
+  $form['submit'] = array(
+    '#type' => 'submit',
+    '#value' => 'Submit',
+  );
+  return $form;
+}
+
+/**
+ * Button submit handler for form_test_form_rebuild_preserve_values_form().
+ */
+function form_test_form_rebuild_preserve_values_form_add_more($form, &$form_state) {
+  // Rebuild, to test preservation of input values.
+  $form_state['storage']['add_more'] = TRUE;
+  $form_state['rebuild'] = TRUE;
+}
+
+/**
+ * Form submit handler for form_test_form_rebuild_preserve_values_form().
+ */
+function form_test_form_rebuild_preserve_values_form_submit($form, &$form_state) {
+  // Finish the workflow. Do not rebuild.
+  drupal_set_message(t('Form values: %values', array('%values' => var_export($form_state['values'], TRUE))));
+}