'destination' URL param breaks user registration form

D6 needs $_REQUEST['destination'] unset

These patches can now be tested with Bakery-Chef (https://github.com/bjeavons/Bakery-Chef) BDD tests. I've confirmed they work but will leave open for some further review.

+++ b/bakery.moduleundefined
@@ -382,6 +382,38 @@ function _bakery_reset_submit($form, &$form_state) {
 }
 
+

Extra space?

+++ b/bakery.moduleundefined
@@ -382,6 +382,38 @@ function _bakery_reset_submit($form, &$form_state) {
+
+  unset($_GET['destination']);

Unrelated to saving in the data arguement? Shouldn't this be up to the drupal_goto() call to handle?

+++ b/bakery.moduleundefined
@@ -382,6 +382,38 @@ function _bakery_reset_submit($form, &$form_state) {
+
+}
+
+
+

More extra space

+++ b/bakery.moduleundefined
@@ -427,15 +460,8 @@ function _bakery_login_submit($form, &$form_state) {
+  _bakery_save_destination_param($form , $data);
+
   // Create cookie and redirect to master.
   bakery_bake_oatmeal_cookie($data['name'], $data);
   unset($_REQUEST['destination']);

We added the unset but then aren't removing the previous unset. The D7 patch does remove it so maybe this was an oversight.

+++ b/bakery.moduleundefined
@@ -666,11 +698,20 @@ function bakery_register_return() {
+    // Destination.
+    if (empty($cookie['data']['destination'])) {
+      $destination = '<front>';
+    }
+    else {
+      $destination = $cookie['data']['destination'];
+    }
+
+

We can see its doing something to the destination but what does that mean? The documentation is too vague. Also, extra space after the code block.

+++ b/bakery.moduleundefined
@@ -705,7 +746,8 @@ function bakery_register_return() {
-      drupal_goto('user/register');
+      // Redirect to destination.

This is a change. Its going to the frontpage now if destination isn't set for some reason.

So a little more general then just looking at the code, I'd suggest we create a bakery_goto() that wrapper that wraps the loading of the redirect from the saved value. Untested but maybe something like:

function bakery_goto($data, $path = '', $query = NULL, $fragment = NULL, $http_response_code = 302) {
  if (!empty($data['destination'])) {
    $_REQUEST['destination'] = $data['destination'];
    $path = '';
  }
  drupal_goto($path, $query, $fragment, $http_response_code);
}

then we can call it like bakery_goto($cookie['data'], '<front>') and have all the logic in one place.

Thanks for the review @neclimdul! You're right in catching that the redirection back to /user/register was lost.

Here's an update with more comments and those cleanups. I think bakery_goto() is an unnecessary abstraction so didn't go with that approach.

A alpha4 release of Bakery is pending so I may commit this without waiting long for more review.

I haven't looked at this code in a while but this looks like it addressed all my concerns and it was all trivial stuff. +1

http://drupalcode.org/project/bakery.git/commit/c01b6aa and http://drupalcode.org/project/bakery.git/commit/7db0092