- Patch #756762 by effulgentsia, fago, sun: AJAX should follow same rules for whether to call drupal_rebuild_form() as non-AJAX submissions.

7 files changed, 66 insertions, 51 deletions

diff --git a/includes/ajax.inc b/includes/ajax.inc
index e1ea518d7..2c987f035 100644
--- a/includes/ajax.inc
+++ b/includes/ajax.inc
@@ -26,14 +26,34 @@
  * also return a richer set of @link ajax_commands AJAX framework commands @endlink.
  *
  * Standard form handling is as follows:
- *   - A form element has a #ajax member.
+ *   - A form element has a #ajax property that includes #ajax['callback'] and
+ *     omits #ajax['path']. See below about using #ajax['path'] to implement
+ *     advanced use-cases that require something other than standard form
+ *     handling.
  *   - On the specified element, AJAX processing is triggered by a change to
  *     that element.
- *   - The form is submitted and rebuilt.
- *   - The function named by #ajax['callback'] is called, which returns content
- *     or an array of AJAX framework commands.
- *   - The content returned by the callback replaces the div on the page
- *     referenced by #ajax['wrapper'].
+ *   - The browser submits an HTTP POST request to the 'system/ajax' Drupal
+ *     path.
+ *   - The menu page callback for 'system/ajax', ajax_form_callback(), calls
+ *     drupal_process_form() to process the form submission and rebuild the
+ *     form if necessary. The form is processed in much the same way as if it
+ *     were submitted without AJAX, with the same #process functions and
+ *     validation and submission handlers called in either case, making it easy
+ *     to create AJAX-enabled forms that degrade gracefully when JavaScript is
+ *     disabled.
+ *   - After form processing is complete, ajax_form_callback() calls the
+ *     function named by #ajax['callback'], which returns the form element that
+ *     has been updated and needs to be returned to the browser, or
+ *     alternatively, an array of custom AJAX commands.
+ *   - The page delivery callback for 'system/ajax', ajax_deliver(), renders the
+ *     element returned by #ajax['callback'], and returns the JSON string
+ *     created by ajax_render() to the browser.
+ *   - The browser unserializes the returned JSON string into an array of
+ *     command objects and executes each command, resulting in the old page
+ *     content within and including the HTML element specified by
+ *     #ajax['wrapper'] being replaced by the new content returned by
+ *     #ajax['callback'], using a JavaScript animation effect specified by
+ *     #ajax['effect'].
  *
  * A simple example of basic AJAX use from the
  * @link http://drupal.org/project/examples Examples module @endlink follows:
diff --git a/includes/form.inc b/includes/form.inc
index 4aa536ff5..40fff1f25 100644
--- a/includes/form.inc
+++ b/includes/form.inc
@@ -219,11 +219,13 @@ function drupal_get_form($form_id) {
  *     request (so a browser refresh does not re-submit the form). However, if
  *     'rebuild' has been set to TRUE, then a new copy of the form is
  *     immediately built and sent to the browser; instead of a redirect. This is
- *     used for multi-step forms, such as wizards and confirmation forms. Also,
- *     if a form validation handler has set 'rebuild' to TRUE and a validation
- *     error occurred, then the form is rebuilt prior to being returned,
- *     enabling form elements to be altered, as appropriate to the particular
- *     validation error.
+ *     used for multi-step forms, such as wizards and confirmation forms.
+ *     Normally, $form_state['rebuild'] is set by a submit handler, since it is
+ *     usually logic within a submit handler that determines whether a form is
+ *     done or requires another step. However, a validation handler may already
+ *     set $form_state['rebuild'] to cause the form processing to bypass submit
+ *     handlers and rebuild the form instead, even if there are no validation
+ *     errors.
  *   - input: An array of input that corresponds to $_POST or $_GET, depending
  *     on the 'method' chosen (see below).
  *   - method: The HTTP form method to use for finding the input for this form.
@@ -362,6 +364,7 @@ function form_state_defaults() {
     'build_info' => array('args' => array()),
     'temporary' => array(),
     'submitted' => FALSE,
+    'executed' => FALSE,
     'programmed' => FALSE,
     'cache'=> FALSE,
     'method' => 'post',
@@ -526,6 +529,7 @@ function form_state_keys_no_cache() {
     'method',
     'submit_handlers',
     'submitted',
+    'executed',
     'validate_handlers',
     'values',
   );
@@ -804,23 +808,39 @@ function drupal_process_form($form_id, &$form, &$form_state) {
     if (!empty($form_state['programmed'])) {
       return;
     }
-  }
 
-  // If $form_state['rebuild'] has been set and input has been processed without
-  // validation errors, we're in a multi-step workflow that is not yet complete.
-  // We need to construct a new $form based on the changes made to $form_state
-  // during this request.
-  if ($form_state['rebuild'] && $form_state['process_input'] && !form_get_errors()) {
-    $form = drupal_rebuild_form($form_id, $form_state, $form);
+    // If $form_state['rebuild'] has been set and input has been processed
+    // without validation errors, we are in a multi-step workflow that is not
+    // yet complete. A new $form needs to be constructed based on the changes
+    // made to $form_state during this request. Normally, a submit handler sets
+    // $form_state['rebuild'] if a fully executed form requires another step.
+    // However, for forms that have not been fully executed (e.g., AJAX
+    // submissions triggered by non-buttons), there is no submit handler to set
+    // $form_state['rebuild']. It would not make sense to redisplay the
+    // identical form without an error for the user to correct, so we also
+    // rebuild error-free non-executed forms, regardless of
+    // $form_state['rebuild'].
+    // @todo D8: Simplify this logic; considering AJAX and non-HTML front-ends,
+    //   along with element-level #submit properties, it makes no sense to have
+    //   divergent form execution based on whether the triggering element has
+    //   #executes_submit_callback set to TRUE.
+    if (($form_state['rebuild'] || !$form_state['executed']) && !form_get_errors()) {
+      // Form building functions (e.g., _form_builder_handle_input_element())
+      // may use $form_state['rebuild'] to determine if they are running in the
+      // context of a rebuild, so ensure it is set.
+      $form_state['rebuild'] = TRUE;
+      $form = drupal_rebuild_form($form_id, $form_state, $form);
+    }
   }
+
   // After processing the form, the form builder or a #process callback may
   // have set $form_state['cache'] to indicate that the form and form state
   // shall be cached. But the form may only be cached if the 'no_cache' property
   // is not set to TRUE. Only cache $form as it was prior to form_builder(),
   // because form_builder() must run for each request to accomodate new user
-  // input. We do not cache here for forms that have been rebuilt, because
-  // drupal_rebuild_form() takes care of that.
-  elseif ($form_state['cache'] && empty($form_state['no_cache'])) {
+  // input. Rebuilt forms are not cached here, because drupal_rebuild_form()
+  // already takes care of that.
+  if (!$form_state['rebuild'] && $form_state['cache'] && empty($form_state['no_cache'])) {
     form_set_cache($form['#build_id'], $unprocessed_form, $form_state);
   }
 }
diff --git a/modules/book/book.admin.inc b/modules/book/book.admin.inc
index 6e5bfd20d..b3c3eaff7 100644
--- a/modules/book/book.admin.inc
+++ b/modules/book/book.admin.inc
@@ -91,7 +91,6 @@ function book_admin_edit($form, $form_state, $node) {
 function book_admin_edit_validate($form, &$form_state) {
   if ($form_state['values']['tree_hash'] != $form_state['values']['tree_current_hash']) {
     form_set_error('', t('This book has been modified by another user, the changes could not be saved.'));
-    $form_state['rebuild'] = TRUE;
   }
 }
 
diff --git a/modules/image/image.admin.inc b/modules/image/image.admin.inc
index 9665a8523..43cb479d1 100644
--- a/modules/image/image.admin.inc
+++ b/modules/image/image.admin.inc
@@ -166,7 +166,6 @@ function image_style_form($form, &$form_state, $style) {
 function image_style_form_add_validate($form, &$form_state) {
   if (!$form_state['values']['new']) {
     form_error($form['effects']['new']['new'], t('Select an effect to add.'));
-    $form_state['rebuild'] = TRUE;
   }
 }
 
diff --git a/modules/simpletest/tests/form.test b/modules/simpletest/tests/form.test
index 793d0c0bf..d8e0b21f1 100644
--- a/modules/simpletest/tests/form.test
+++ b/modules/simpletest/tests/form.test
@@ -788,24 +788,21 @@ class FormsFormStorageTestCase extends DrupalWebTestCase {
     $this->assertText('Form constructions: 1');
 
     $edit = array('title' => 'new', 'value' => 'value_is_set');
-    // Reload the form, but don't rebuild.
-    $this->drupalPost(NULL, $edit, 'Reload');
-    $this->assertText('Form constructions: 2');
 
-    // Now use form rebuilding triggered by a submit button.
+    // Use form rebuilding triggered by a submit button.
     $this->drupalPost(NULL, $edit, 'Continue submit');
+    $this->assertText('Form constructions: 2');
     $this->assertText('Form constructions: 3');
-    $this->assertText('Form constructions: 4');
 
     // Reset the form to the values of the storage, using a form rebuild
     // triggered by button of type button.
     $this->drupalPost(NULL, array('title' => 'changed'), 'Reset');
     $this->assertFieldByName('title', 'new', 'Values have been resetted.');
     // After rebuilding, the form has been cached.
-    $this->assertText('Form constructions: 5');
+    $this->assertText('Form constructions: 4');
 
     $this->drupalPost(NULL, $edit, 'Save');
-    $this->assertText('Form constructions: 5');
+    $this->assertText('Form constructions: 4');
     $this->assertText('Title: new', t('The form storage has stored the values.'));
   }
 
@@ -817,11 +814,8 @@ class FormsFormStorageTestCase extends DrupalWebTestCase {
     $this->assertText('Form constructions: 1');
 
     $edit = array('title' => 'new', 'value' => 'value_is_set');
-    // Reload the form, but don't rebuild.
-    $this->drupalPost(NULL, $edit, 'Reload');
-    $this->assertNoText('Form constructions');
 
-    // Now use form rebuilding triggered by a submit button.
+    // Use form rebuilding triggered by a submit button.
     $this->drupalPost(NULL, $edit, 'Continue submit');
     $this->assertText('Form constructions: 2');
 
diff --git a/modules/simpletest/tests/form_test.module b/modules/simpletest/tests/form_test.module
index ca3538c75..57a96863c 100644
--- a/modules/simpletest/tests/form_test.module
+++ b/modules/simpletest/tests/form_test.module
@@ -522,16 +522,10 @@ function form_test_storage_form($form, &$form_state) {
     '#default_value' => $form_state['storage']['thing']['value'],
     '#element_validate' => array('form_test_storage_element_validate_value_cached'),
   );
-  $form['button'] = array(
-    '#type' => 'button',
-    '#value' => 'Reload',
-    // Reload the form (don't rebuild), thus we start at the initial step again.
-  );
   $form['continue_button'] = array(
     '#type' => 'button',
     '#value' => 'Reset',
     // Rebuilds the form without keeping the values.
-    '#validate' => array('form_storage_test_form_continue_validate'),
   );
   $form['continue_submit'] = array(
     '#type' => 'submit',
@@ -577,15 +571,6 @@ function form_storage_test_form_continue_submit($form, &$form_state) {
 }
 
 /**
- * Form validation handler, which doesn't preserve the values but rebuilds the
- * form. We cannot use a submit handler here, as buttons of type button don't
- * submit the form.
- */
-function form_storage_test_form_continue_validate($form, &$form_state) {
-  $form_state['rebuild'] = TRUE;
-}
-
-/**
  * Form submit handler to finish multi-step form.
  */
 function form_test_storage_form_submit($form, &$form_state) {
diff --git a/modules/taxonomy/taxonomy.admin.inc b/modules/taxonomy/taxonomy.admin.inc
index e8225132e..e89923ba9 100644
--- a/modules/taxonomy/taxonomy.admin.inc
+++ b/modules/taxonomy/taxonomy.admin.inc
@@ -868,8 +868,6 @@ function taxonomy_form_term_submit($form, &$form_state) {
 
   $form_state['values']['tid'] = $term->tid;
   $form_state['tid'] = $term->tid;
-  // Do not rebuild here. The term is saved by now and the form should clear.
-  $form_state['rebuild'] = FALSE;
 }
 
 /**