Unused form redirect

Great find @pjcdawkins. I added a test for this and updated the code comments in the attached patch. Care to review?

I'm curious, in your custom form alter in what position do you set the form submit handler (ie. beginning or end of the array)? Presently tfa_login_form_redirect() needs to be last in the array but I don't have a foolproof way of enforcing that.

Just a heads for the time being as I havent had the time yet to check if D7 is similar in this regard but for the D6 version I had to modify the above patch a tad bit:

diff --git a/docroot/sites/all/modules/contrib/tfa/tfa.module b/docroot/sites/all/modules/contrib/tfa/tfa.module
index dd75fb2..1b26dbf 100644
--- a/docroot/sites/all/modules/contrib/tfa/tfa.module
+++ b/docroot/sites/all/modules/contrib/tfa/tfa.module
@@ -323,7 +323,7 @@ function tfa_user_login_form_validate_authenticate($form, &$form_state) {
 /**
  * Login submit handler to determine if TFA process is applicable.
  */
-function tfa_user_login_form_submit($form, &$form_state) {
+function tfa_user_login_form_submit(&$form, &$form_state) {
   $account = (isset($form_state['uid']) ?
     user_load($form_state['uid']) :
     user_load(array('name' => $form_state['values']['name']))
@@ -348,20 +348,36 @@ function tfa_user_login_form_submit($form, &$form_state) {
     $identifier = variable_get('user_failed_login_identifier_uid_only', FALSE) ? $account->uid : $account->uid . '-' . ip_address();
     tfa_flood_clear_event('tfa_user', $identifier);
     tfa_flood_register_event('tfa_begin');
-    $context = tfa_start_context($account);
+    tfa_start_context($account);
     $tfa = tfa_get_process($account);
 
     $query = tfa_drupal_get_query_parameters();
+
+    // Begin TFA and set process context.
+    $tfa->begin();
+    $context = $tfa->getContext();
     if (!empty($form_state['redirect'])) {
       // If there's an existing redirect set it in TFA context and
       // tfa_form_submit() will extract and set once process is complete.
       $context['redirect'] = $form_state['redirect'];
     }
-    unset($_GET['destination']);
-
-    // Begin TFA and set process context.
-    $tfa->begin();
-    $context = $tfa->getContext();
+    // The $form['#redirect'] gets priority in drupal_redirect_form() so this
+    // has to be stored and unset to allow tfa to send the user to the next
+    // page.
+    if (!empty($form['#redirect'])) {
+      $context['redirect'] = $form['#redirect'];
+      unset($form['#redirect']);
+    }
+    // The drupal_redirect_form() uses drupal_goto() to send the user to the
+    // next page after form submit. On the other hand, drupal_goto() considers
+    // the contents of $_REQUEST['destination'] more important than what has
+    // been passed to it in its arguments, so to be able to get to the next TFA
+    // page, the contents of $_REQUEST['destination'] needs to be stored and
+    // unset.
+    if (!empty($_REQUEST['destination'])) {
+      $context['redirect'] = $_REQUEST['destination'];
+    }
+    unset($_REQUEST['destination']);
     tfa_set_context($account, $context);
 
     $login_hash = tfa_login_hash($account);

once i get a bit of breathing room i'll try to follow up on this.

@nagba I think that in D6 checking REQUEST['destination'] is necessary but in D7 its not. Would love your testing and feedback but I think patch in #2 is looking ready to go.

Committed.

Re: #6, the testRedirection method sets filter/tips on the URL.