Patch #579366 by sun, litwol | chx, Dries: simplified form API redirection handling. I can actually understand it now. ;-).

14 files changed, 103 insertions, 94 deletions

diff --git a/includes/ajax.inc b/includes/ajax.inc
index 478adf572..6bfd86d77 100644
--- a/includes/ajax.inc
+++ b/includes/ajax.inc
@@ -184,7 +184,7 @@ function ajax_get_form() {
   }
 
   // Since some of the submit handlers are run, redirects need to be disabled.
-  $form['#redirect'] = FALSE;
+  $form_state['no_redirect'] = TRUE;
 
   // The form needs to be processed; prepare for that by setting a few internal
   // variables.
diff --git a/includes/batch.inc b/includes/batch.inc
index b136ac999..5fe876f9d 100644
--- a/includes/batch.inc
+++ b/includes/batch.inc
@@ -426,29 +426,24 @@ function _batch_finished() {
   if ($_batch['progressive']) {
     // Revert the 'destination' that was saved in batch_process().
     if (isset($_batch['destination'])) {
-      $_REQUEST['destination'] = $_batch['destination'];
+      $_GET['destination'] = $_batch['destination'];
     }
 
     // Determine the target path to redirect to.
-    if (isset($_batch['form_state']['redirect'])) {
-      $redirect = $_batch['form_state']['redirect'];
-    }
-    elseif (isset($_batch['redirect'])) {
-      $redirect = $_batch['redirect'];
-    }
-    else {
-      $redirect = $_batch['source_page'];
+    if (!isset($_batch['form_state']['redirect'])) {
+      if (isset($_batch['redirect'])) {
+        $_batch['form_state']['redirect'] = $_batch['redirect'];
+      }
+      else {
+        $_batch['form_state']['redirect'] = $_batch['source_page'];
+      }
     }
 
     // Use drupal_redirect_form() to handle the redirection logic.
-    $form = isset($batch['form']) ? $batch['form'] : array();
-    if (empty($_batch['form_state']['rebuild']) && empty($_batch['form_state']['storage'])) {
-      drupal_redirect_form($form, $redirect);
-    }
+    drupal_redirect_form($_batch['form_state']);
 
-    // We get here if $form['#redirect'] was FALSE, or if the form is a
-    // multi-step form. We save the final $form_state value to be retrieved
-    // by drupal_get_form(), and redirect to the originating page.
+    // If no redirection happened, save the final $form_state value to be
+    // retrieved by drupal_get_form() and redirect to the originating page.
     $_SESSION['batch_form_state'] = $_batch['form_state'];
     drupal_goto($_batch['source_page']);
   }
diff --git a/includes/common.inc b/includes/common.inc
index 2fee010a4..479d0952e 100644
--- a/includes/common.inc
+++ b/includes/common.inc
@@ -364,8 +364,8 @@ function drupal_query_string_encode($query, $exclude = array(), $parent = '') {
  * @see drupal_goto()
  */
 function drupal_get_destination() {
-  if (isset($_REQUEST['destination'])) {
-    return 'destination=' . urlencode($_REQUEST['destination']);
+  if (isset($_GET['destination'])) {
+    return 'destination=' . urlencode($_GET['destination']);
   }
   else {
     // Use $_GET here to retrieve the original path in source form.
@@ -418,9 +418,8 @@ function drupal_get_destination() {
  * @see drupal_get_destination()
  */
 function drupal_goto($path = '', $query = NULL, $fragment = NULL, $http_response_code = 302) {
-
-  if (isset($_REQUEST['destination'])) {
-    extract(parse_url(urldecode($_REQUEST['destination'])));
+  if (isset($_GET['destination'])) {
+    extract(parse_url(urldecode($_GET['destination'])));
   }
 
   $args = array(
@@ -471,8 +470,8 @@ function drupal_not_found() {
   watchdog('page not found', check_plain($_GET['q']), NULL, WATCHDOG_WARNING);
 
   // Keep old path for reference, and to allow forms to redirect to it.
-  if (!isset($_REQUEST['destination'])) {
-    $_REQUEST['destination'] = $_GET['q'];
+  if (!isset($_GET['destination'])) {
+    $_GET['destination'] = $_GET['q'];
   }
 
   $path = drupal_get_normal_path(variable_get('site_404', ''));
@@ -502,8 +501,8 @@ function drupal_access_denied() {
   watchdog('access denied', check_plain($_GET['q']), NULL, WATCHDOG_WARNING);
 
   // Keep old path for reference, and to allow forms to redirect to it.
-  if (!isset($_REQUEST['destination'])) {
-    $_REQUEST['destination'] = $_GET['q'];
+  if (!isset($_GET['destination'])) {
+    $_GET['destination'] = $_GET['q'];
   }
 
   $path = drupal_get_normal_path(variable_get('site_403', ''));
diff --git a/includes/form.inc b/includes/form.inc
index 30df84c54..d92e2a7ee 100644
--- a/includes/form.inc
+++ b/includes/form.inc
@@ -122,8 +122,13 @@ function drupal_get_form($form_id) {
  *     structure, which is passed on to the actual form builder function.
  *     Such forms cannot use drupal_get_form() and need to prepare $form_state
  *     on their own.
+ *   Further $form_state properties controlling the redirection behavior after
+ *   form submission may be found in drupal_redirect_form().
+ *
  * @return
  *   The rendered form or NULL, depending upon the $form_state flags that were set.
+ *
+ * @see drupal_redirect_form()
  */
 function drupal_build_form($form_id, &$form_state) {
   // Ensure some defaults; if already set they will not be overridden.
@@ -503,6 +508,8 @@ function drupal_retrieve_form($form_id, &$form_state) {
 }
 
 /**
+ * Processes a form submission.
+ *
  * This function is the heart of form API. The form gets built, validated and in
  * appropriate cases, submitted.
  *
@@ -579,20 +586,8 @@ function drupal_process_form($form_id, &$form, &$form_state) {
       // Set a flag to indicate the the form has been processed and executed.
       $form_state['executed'] = TRUE;
 
-      // The form is executed. By default, we're finished now and redirect to a
-      // new destination page. The path of the destination page can be set in
-      // $form['#redirect'] or $form_state['redirect']. If neither of the two is
-      // set, the user is redirect to the current page, which means a fresh,
-      // unpopulated copy of the form.
-      // Redirection is skipped, though, if
-      // - the form was called by drupal_form_submit(),
-      // - the form has to be rebuilt because either $form_state['rebuild'] was
-      //   set to TRUE or $form_state['storage'] was populated by a submit handler.
-      // - $form_state['no_redirect'] is set to TRUE,
-      // - $form_state['redirect'] or $form['#redirect'] is set to FALSE.
-      if (!$form_state['programmed'] && empty($form_state['rebuild']) && empty($form_state['storage']) && empty($form_state['no_redirect'])) {
-        drupal_redirect_form($form, $form_state['redirect']);
-      }
+      // Redirect the form based on values in $form_state.
+      drupal_redirect_form($form_state);
     }
   }
 }
@@ -732,33 +727,67 @@ function drupal_validate_form($form_id, $form, &$form_state) {
 }
 
 /**
- * Redirect the user to a URL after a form has been processed.
+ * Redirects the user to a URL after a form has been processed.
+ *
+ * After a form was executed, the data in $form_state controls whether the form
+ * is redirected. By default, we redirect to a new destination page. The path of
+ * the destination page can be set in $form_state['redirect']. If that is not
+ * set, the user is redirected to the current page to display a fresh,
+ * unpopulated copy of the form.
+ *
+ * There are several triggers that may prevent a redirection though:
+ * - If $form_state['redirect'] is FALSE, a form builder function or form
+ *   validation/submit handler does not want a user to be redirected, which
+ *   means that drupal_goto() is not invoked. For most forms, the redirection
+ *   logic will be the same regardless of whether $form_state['redirect'] is
+ *   undefined or FALSE. However, in case it was not defined and the current
+ *   request contains a 'destination' query string, drupal_goto() will redirect
+ *   to that given destination instead. Only setting $form_state['redirect'] to
+ *   FALSE will prevent any redirection.
+ * - If $form_state['no_redirect'] is TRUE, then the callback that originally
+ *   built the form explicitly disallows any redirection, regardless of the
+ *   redirection value in $form_state['redirect']. For example, ajax_get_form()
+ *   defines $form_state['no_redirect'] when building a form in an AJAX
+ *   callback to prevent any redirection. $form_state['no_redirect'] should NOT
+ *   be altered by form builder functions or form validation/submit handlers.
+ * - If $form_state['programmed'] is TRUE, the form submission was usually
+ *   invoked via drupal_form_submit(), so any redirection would break the script
+ *   that invoked drupal_form_submit().
+ * - If $form_state['rebuild'] is TRUE or $form_state['storage'] is populated,
+ *   the form is most probably a multi-step form and needs to be rebuilt without
+ *   redirection.
  *
- * @param $form
- *   An associative array containing the structure of the form.
- * @param $redirect
- *   An optional value containing the destination path to redirect
- *   to if none is specified by the form.
+ * @param $form_state
+ *   A keyed array containing the current state of the form.
+ *
+ * @see drupal_process_form()
+ * @see drupal_build_form()
  */
-function drupal_redirect_form($form, $redirect = NULL) {
-  $goto = NULL;
-  if (isset($redirect)) {
-    $goto = $redirect;
+function drupal_redirect_form($form_state) {
+  // Skip redirection for form submissions invoked via drupal_form_submit().
+  if (!empty($form_state['programmed'])) {
+    return;
   }
-  if ($goto !== FALSE && isset($form['#redirect'])) {
-    $goto = $form['#redirect'];
+  // Skip redirection for multi-step forms.
+  if (!empty($form_state['rebuild']) || !empty($form_state['storage'])) {
+    return;
+  }
+  // Skip redirection if it was explicitly disallowed.
+  if (!empty($form_state['no_redirect'])) {
+    return;
   }
-  if (!isset($goto) || ($goto !== FALSE)) {
-    if (isset($goto)) {
-      if (is_array($goto)) {
-        call_user_func_array('drupal_goto', $goto);
+  // Only invoke drupal_goto() if redirect value was not set to FALSE.
+  if (!isset($form_state['redirect']) || $form_state['redirect'] !== FALSE) {
+    if (isset($form_state['redirect'])) {
+      if (is_array($form_state['redirect'])) {
+        call_user_func_array('drupal_goto', $form_state['redirect']);
       }
       else {
         // This function can be called from the installer, which guarantees
         // that $redirect will always be a string, so catch that case here
         // and use the appropriate redirect function.
         $function = drupal_installation_attempted() ? 'install_goto' : 'drupal_goto';
-        $function($goto);
+        $function($form_state['redirect']);
       }
     }
     drupal_goto($_GET['q']);
@@ -2922,12 +2951,11 @@ function batch_process($redirect = NULL, $url = NULL) {
     $batch += $process_info;
 
     if ($batch['progressive']) {
-      // Clear the way for the drupal_goto redirection to the batch processing
-      // page, by saving and unsetting the 'destination' if any, on both places
-      // drupal_goto looks for it.
-      if (isset($_REQUEST['destination'])) {
-        $batch['destination'] = $_REQUEST['destination'];
-        unset($_REQUEST['destination']);
+      // Clear the way for the drupal_goto() redirection to the batch processing
+      // page, by saving and unsetting the 'destination', if there is any.
+      if (isset($_GET['destination'])) {
+        $batch['destination'] = $_GET['destination'];
+        unset($_GET['destination']);
       }
 
       // Initiate db storage in order to get a batch id. We have to provide
diff --git a/modules/field_ui/field_ui.admin.inc b/modules/field_ui/field_ui.admin.inc
index 6cdf699d4..9335ecc8a 100644
--- a/modules/field_ui/field_ui.admin.inc
+++ b/modules/field_ui/field_ui.admin.inc
@@ -540,7 +540,7 @@ function field_ui_field_overview_form_submit($form, &$form_state) {
 
   if ($destinations) {
     $destinations[] = urldecode(substr(drupal_get_destination(), 12));
-    unset($_REQUEST['destination']);
+    unset($_GET['destination']);
     $form_state['redirect'] = field_ui_get_destinations($destinations);
   }
 }
diff --git a/modules/node/node.pages.inc b/modules/node/node.pages.inc
index a0bbf92b0..e794fd0b2 100644
--- a/modules/node/node.pages.inc
+++ b/modules/node/node.pages.inc
@@ -301,9 +301,9 @@ function node_form($form, &$form_state, $node) {
  */
 function node_form_delete_submit($form, &$form_state) {
   $destination = '';
-  if (isset($_REQUEST['destination'])) {
+  if (isset($_GET['destination'])) {
     $destination = drupal_get_destination();
-    unset($_REQUEST['destination']);
+    unset($_GET['destination']);
   }
   $node = $form['#node'];
   $form_state['redirect'] = array('node/' . $node->nid . '/delete', $destination);
diff --git a/modules/openid/openid.module b/modules/openid/openid.module
index 0facb8ebd..61edf053a 100644
--- a/modules/openid/openid.module
+++ b/modules/openid/openid.module
@@ -445,7 +445,7 @@ function openid_authentication($response) {
       $_SESSION['openid']['values'] = $form_state['values'];
       // We'll want to redirect back to the same place.
       $destination = drupal_get_destination();
-      unset($_REQUEST['destination']);
+      unset($_GET['destination']);
       drupal_goto('user/register', $destination);
     }
     else {
@@ -463,7 +463,7 @@ function openid_authentication($response) {
       // Let other modules act on OpenID login
       module_invoke_all('openid_response', $response, $account);
     }
-    drupal_redirect_form($form, $form_state['redirect']);
+    drupal_redirect_form($form_state);
   }
   else {
     drupal_set_message(t('Only site administrators can create new user accounts.'), 'error');
diff --git a/modules/openid/openid.pages.inc b/modules/openid/openid.pages.inc
index b900c413c..0091e33fa 100644
--- a/modules/openid/openid.pages.inc
+++ b/modules/openid/openid.pages.inc
@@ -111,5 +111,5 @@ function openid_user_delete_form_submit($form, &$form_state) {
   if ($query) {
     drupal_set_message(t('OpenID deleted.'));
   }
-  $form_state['#redirect'] = 'user/' . $form_state['args'][0]->uid . '/openid';
+  $form_state['redirect'] = 'user/' . $form_state['args'][0]->uid . '/openid';
 }
diff --git a/modules/search/search.module b/modules/search/search.module
index 35b817d3f..9d15edb5e 100644
--- a/modules/search/search.module
+++ b/modules/search/search.module
@@ -917,8 +917,8 @@ function search_box_form_submit($form, &$form_state) {
   // functionality, so we override any static destination set in the request,
   // for example by drupal_access_denied() or drupal_not_found()
   // (see http://drupal.org/node/292565).
-  if (isset($_REQUEST['destination'])) {
-    unset($_REQUEST['destination']);
+  if (isset($_GET['destination'])) {
+    unset($_GET['destination']);
   }
 
   $form_id = $form['form_id']['#value'];
diff --git a/modules/system/system.admin.inc b/modules/system/system.admin.inc
index 48806cdb4..2d8c81713 100644
--- a/modules/system/system.admin.inc
+++ b/modules/system/system.admin.inc
@@ -1822,7 +1822,7 @@ function system_site_maintenance_mode() {
  * @ingroup forms
  * @see system_settings_form()
  */
-function system_clean_url_settings($form) {
+function system_clean_url_settings($form, &$form_state) {
   global $base_url;
 
   // When accessing this form using a non-clean URL, allow a re-check to make
@@ -1852,7 +1852,7 @@ function system_clean_url_settings($form) {
   else {
     drupal_add_js(drupal_get_path('module', 'system') . '/system.js');
 
-    $form['#redirect'] = $base_url . '/admin/config/search/clean-urls';
+    $form_state['redirect'] = $base_url . '/admin/config/search/clean-urls';
     $form['clean_url_description'] = array(
       '#type' => 'markup',
       '#markup' => '<p>' . t('Use URLs like <code>example.com/user</code> instead of <code>example.com/?q=user</code>.') . ' ' . t('If you are directed to a <em>Page not found (404)</em> error after testing for clean URLs, see the <a href="@handbook">online handbook</a>.', array('@handbook' => 'http://drupal.org/node/15365')) . '</p>',
diff --git a/modules/system/system.test b/modules/system/system.test
index d6fd1a98e..6d1e4d142 100644
--- a/modules/system/system.test
+++ b/modules/system/system.test
@@ -512,20 +512,14 @@ class AdminMetaTagTestCase extends DrupalWebTestCase {
 class AccessDeniedTestCase extends DrupalWebTestCase {
   protected $admin_user;
 
-  /**
-   * Implement getInfo().
-   */
   public static function getInfo() {
     return array(
       'name' => '403 functionality',
-      'description' => "Tests page access denied functionality, including custom 403 pages.",
+      'description' => 'Tests page access denied functionality, including custom 403 pages.',
       'group' => 'System'
     );
   }
 
-  /**
-   * Implement setUp().
-   */
   function setUp() {
     parent::setUp();
 
diff --git a/modules/taxonomy/taxonomy.admin.inc b/modules/taxonomy/taxonomy.admin.inc
index ee32be4b0..b7cd4aba6 100644
--- a/modules/taxonomy/taxonomy.admin.inc
+++ b/modules/taxonomy/taxonomy.admin.inc
@@ -469,10 +469,7 @@ function taxonomy_overview_terms($form, &$form_state, $vocabulary) {
       '#type' => 'submit',
       '#value' => t('Reset to alphabetical')
     );
-    $form['destination'] = array(
-      '#type' => 'hidden',
-      '#value' => $_GET['q'] . (isset($_GET['page']) ? '?page=' . $_GET['page'] : '')
-    );
+    $form_state['redirect'] = array($_GET['q'], (isset($_GET['page']) ? array('page' => $_GET['page']) : ''));
   }
 
   return $form;
@@ -780,7 +777,7 @@ function taxonomy_form_term($form, &$form_state, $vocabulary, $edit = array()) {
       '#value' => $edit['tid']);
   }
   else {
-    $form['destination'] = array('#type' => 'hidden', '#value' => $_GET['q']);
+    $form_state['redirect'] = $_GET['q'];
   }
 
   return $form;
diff --git a/modules/user/user.module b/modules/user/user.module
index 9cb594c73..cf41f9d7e 100644
--- a/modules/user/user.module
+++ b/modules/user/user.module
@@ -3078,7 +3078,7 @@ function user_register($form, &$form_state) {
     );
     // Redirect back to page which initiated the create request;
     // usually admin/people/create.
-    $form['destination'] = array('#type' => 'hidden', '#value' => $_GET['q']);
+    $form_state['redirect'] = $_GET['q'];
   }
 
   // Create a dummy variable for pass-by-reference parameters.
diff --git a/modules/user/user.pages.inc b/modules/user/user.pages.inc
index ac1115a80..80e00ae49 100644
--- a/modules/user/user.pages.inc
+++ b/modules/user/user.pages.inc
@@ -292,9 +292,9 @@ function user_profile_form_submit($form, &$form_state) {
  */
 function user_edit_cancel_submit($form, &$form_state) {
   $destination = '';
-  if (isset($_REQUEST['destination'])) {
+  if (isset($_GET['destination'])) {
     $destination = drupal_get_destination();
-    unset($_REQUEST['destination']);
+    unset($_GET['destination']);
   }
   // Note: We redirect from user/uid/edit to user/uid/cancel to make the tabs disappear.
   $form_state['redirect'] = array("user/" . $form_state['values']['_account']->uid . "/cancel", $destination);
@@ -391,9 +391,7 @@ function user_cancel_confirm_form_submit($form, &$form_state) {
   if (user_access('administer users') && empty($form_state['values']['user_cancel_confirm']) && $account->uid != $user->uid) {
     user_cancel($form_state['values'], $account->uid, $form_state['values']['user_cancel_method']);
 
-    if (!isset($_REQUEST['destination'])) {
-      $form_state['redirect'] = 'admin/people';
-    }
+    $form_state['redirect'] = 'admin/people';
   }
   else {
     // Store cancelling method and whether to notify the user in $account for
@@ -407,9 +405,7 @@ function user_cancel_confirm_form_submit($form, &$form_state) {
     drupal_set_message(t('A confirmation request to cancel your account has been sent to your e-mail address.'));
     watchdog('user', 'Sent account cancellation request to %name %email.', array('%name' => $account->name, '%email' => '<' . $account->mail . '>'), WATCHDOG_NOTICE);
 
-    if (!isset($_REQUEST['destination'])) {
-      $form_state['redirect'] = "user/$account->uid";
-    }
+    $form_state['redirect'] = "user/$account->uid";
   }
 }