- Patch #671184 by Scott Reynolds, sun, effulgentsia, yched, rfay, Pasqualle: AJAX form can submit inappropriately to system/ajax after failed validation.

4 files changed, 81 insertions, 12 deletions

diff --git a/includes/ajax.inc b/includes/ajax.inc
index fbf1da89e..8a55adb58 100644
--- a/includes/ajax.inc
+++ b/includes/ajax.inc
@@ -270,7 +270,7 @@ function ajax_form_callback() {
 
   // This call recreates the form relying solely on the $form_state that
   // drupal_process_form() set up.
-  $form = drupal_rebuild_form($form_id, $form_state, $form_build_id);
+  $form = drupal_rebuild_form($form_id, $form_state, $form);
 
   // As part of drupal_process_form(), the element that triggered the form
   // submission is determined, and in the case of AJAX, it might not be a
diff --git a/includes/form.inc b/includes/form.inc
index 42424cc56..c05fd6fd1 100644
--- a/includes/form.inc
+++ b/includes/form.inc
@@ -306,14 +306,16 @@ function form_state_defaults() {
  *   may be found in node_forms(), search_forms(), and user_forms().
  * @param $form_state
  *   A keyed array containing the current state of the form.
- * @param $form_build_id
- *   If the AHAH callback calling this function only alters part of the form,
- *   then pass in the existing form_build_id so we can re-cache with the same
- *   csid.
+ * @param $old_form
+ *   (optional) A previously built $form. Used to retain the #build_id and
+ *   #action properties in AJAX callbacks and similar partial form rebuilds.
+ *   Should not be passed for regular rebuilds, for which the entire $form
+ *   should be rebuilt freshly.
+ *
  * @return
  *   The newly built form.
  */
-function drupal_rebuild_form($form_id, &$form_state, $form_build_id = NULL) {
+function drupal_rebuild_form($form_id, &$form_state, $old_form = 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
@@ -323,17 +325,27 @@ function drupal_rebuild_form($form_id, &$form_state, $form_build_id = NULL) {
 
   $form = drupal_retrieve_form($form_id, $form_state);
 
-  if (!isset($form_build_id)) {
-    // We need a new build_id for the new version of the form.
-    $form_build_id = 'form-' . md5(mt_rand());
+  // If only parts of the form will be returned to the browser (e.g. AJAX or
+  // RIA clients), re-use the old #build_id to not require client-side code to
+  // manually update the hidden 'build_id' input element.
+  // Otherwise, a new #build_id is generated, to not clobber the previous
+  // build's data in the form cache; also allowing the user to go back to an
+  // earlier build, make changes, and re-submit.
+  $form['#build_id'] = isset($old_form['#build_id']) ? $old_form['#build_id'] : 'form-' . md5(mt_rand());
+
+  // #action defaults to request_uri(), but in case of AJAX and other partial
+  // rebuilds, the form is submitted to an alternate URL, and the original
+  // #action needs to be retained.
+  if (isset($old_form['#action'])) {
+    $form['#action'] = $old_form['#action'];
   }
-  $form['#build_id'] = $form_build_id;
+
   drupal_prepare_form($form_id, $form, $form_state);
 
   if (empty($form_state['no_cache'])) {
     // We cache the form structure and the form state so it can be retrieved
     // later for validation.
-    form_set_cache($form_build_id, $form, $form_state);
+    form_set_cache($form['#build_id'], $form, $form_state);
   }
 
   // Clear out all group associations as these might be different when
diff --git a/modules/file/file.module b/modules/file/file.module
index 84a006dd6..b3153e647 100644
--- a/modules/file/file.module
+++ b/modules/file/file.module
@@ -239,7 +239,7 @@ function file_ajax_upload() {
 
   // This call recreates the form relying solely on the form_state that the
   // drupal_process_form() set up.
-  $form = drupal_rebuild_form($form_id, $form_state, $form_build_id);
+  $form = drupal_rebuild_form($form_id, $form_state, $form);
 
   // Retrieve the element to be rendered.
   foreach ($form_parents as $parent) {
diff --git a/modules/simpletest/tests/form.test b/modules/simpletest/tests/form.test
index 76a9589bf..60523f0a0 100644
--- a/modules/simpletest/tests/form.test
+++ b/modules/simpletest/tests/form.test
@@ -783,6 +783,63 @@ class FormsRebuildTestCase extends DrupalWebTestCase {
     $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.'));
   }
+
+  /**
+   * Tests that a form's action is retained after an AJAX submission.
+   *
+   * The 'action' attribute of a form should not change after an AJAX submission
+   * followed by a non-AJAX submission, which triggers a validation error.
+   */
+  function testPreserveFormActionAfterAJAX() {
+    // Create a multi-valued field for 'page' nodes to use for AJAX testing.
+    $field_name = 'field_ajax_test';
+    $field = array(
+      'field_name' => $field_name,
+      'type' => 'text',
+      'cardinality' => FIELD_CARDINALITY_UNLIMITED,
+    );
+    field_create_field($field);
+    $instance = array(
+      'field_name' => $field_name,
+      'entity_type' => 'node',
+      'bundle' => 'page',
+    );
+    field_create_instance($instance);
+
+    // Log in a user who can create 'page' nodes.
+    $this->web_user = $this->drupalCreateUser(array('create page content'));
+    $this->drupalLogin($this->web_user);
+
+    // Get the form for adding a 'page' node. Save the content in a local
+    // variable, because drupalPostAJAX() will replace $this->content.
+    $this->drupalGet('node/add/page');
+    $content = $this->content;
+
+    // Submit an "add another item" AJAX submission and verify it worked by
+    // ensuring it returned two text fields.
+    $commands = $this->drupalPostAJAX(NULL, array(), array('field_ajax_test_add_more' => t('Add another item')));
+    $fragment = simplexml_load_string('<div>' . $commands[1]['data'] . '</div>');
+    $this->assert(count($fragment->xpath('//input[@type="text"]')) == 2, t('AJAX submission succeeded.'));
+
+    // Submit the form with the non-AJAX "Save" button. Leave the title field
+    // blank to trigger a validation error. Restore $this->content first, as
+    // drupalPost() needs that to contain the form, not the JSON string left by
+    // drupalPostAJAX().
+    // @todo While not necessary for this test, we would be emulating the
+    //   browser better by calling drupalPost() with the AJAX-modified content
+    //   rather than with the original content from the drupalGet(), but that's
+    //   not possible with the current implementation of drupalPostAJAX(). See
+    //   http://drupal.org/node/384992
+    $this->drupalSetContent($content);
+    $this->drupalPost(NULL, array(), t('Save'));
+
+    // Ensure that a validation error occurred, since this test is for testing
+    // what happens to the form action after a validation error.
+    $this->assertText('Title field is required.', t('Non-AJAX submission correctly triggered a validation error.'));
+
+    // Ensure that the form's action is correct.
+    $this->assertFieldByXPath('//form[@id="page-node-form" and @action="' . url('node/add/page') . '"]', NULL, t('Re-rendered form contains the correct action value.'));
+  }
 }
 
 /**