drupal_goto() does not follow same parameters as url()

Here is a patch that takes a stab at this. However, I'm not quite clear whether you expect the same $options array to be interchangeable between url(), l(), and drupal_goto(). drupal_goto takes a distinct set of parameters from that of url() and l(). Regardless, I think we should follow the standard of using associative $options arrays rather than long parameter headers.

Patch no longer applied.

Tempted to reroll this, but have the same question as jgoldberg. Certainly for consistencies sake, having drupal_goto take its argument(s) as an associative array makes sense, but is the intent broader than this? Is the expectation that drupal_goto will somehow take an associative array with similar elements, or just that it will take an associative array?

@drawk: the only irrelevant parameter is $absolute. All other parameters for url() are the same, we just need to add 'http_response_code'.

Here's a reroll that applies cleanly. Still needs work, though. I found a problem in the original patch - specifically the following bit in form.inc

       if (is_array($goto)) {
         call_user_func_array('drupal_goto', array('query' => $goto));
       }

The first argument is still expected to be a string, so this always redirects you to www.example.com/Array when submitting forms and such ('Array' because of the array to string conversion). In the attached patch, I changed this to

       if (is_array($goto)) {
         call_user_func_array('drupal_goto', array($goto[0]));
       }

Which at least gets you to the desired destination page, and without errors, but obviously you lose any additional parameters.

Tagging.

subscribing

Tagging absolutely critical clean-ups for D7. Do not touch this tag. Please either help with this or one of the other patches having this tag.

Much more recent reroll plus a variety of improvements attached.

The main thing I haven't decided on yet is whether to do:

  $args = array('path' => &$path, 'options' => &$options);
  drupal_alter('drupal_goto', $args);

which seems cleaner and more sensible but means altering like $args['options']['http_response_code'] = 500; or something like:

  $options['path'] = $path;
  drupal_alter('drupal_goto', $options);
  $path = $options['path'];
  unset($options['path']);

which is backward compatible with all the $args['http_response_code'] = 500; etc. code already in place and is flat.

The tests won't pass until that's resolved.

+++ includes/common.inc	13 Oct 2009 00:02:37 -0000
@@ -620,41 +620,57 @@
+ * @param $options
+ *   An associative array of additional options, with the following keys:
+ *     'query'
+ *       A query string to append to the link, or an array of query key/value
+ *       properties.
+ *     'fragment'
+ *       A fragment identifier (named anchor) to append to the link.
+ *       Do not include the '#' character.
+ *     'http_response_code' (default 302)
+ *       Valid values for an actual "goto" as per RFC 2616 section 10.3 are:
+ *       - 301 Moved Permanently (the recommended value for most redirects)
+ *       - 302 Found (default in Drupal and PHP, sometimes used for spamming
+ *             search engines)
+ *       - 303 See Other
+ *       - 304 Not Modified
+ *       - 305 Use Proxy
+ *       - 307 Temporary Redirect (alternative to "503 Site Down for
+ *             Maintenance")
+ *       Note: Other values are defined by RFC 2616, but are rarely used and
+ *       poorly supported.

Please read http://drupal.org/node/1354 on proper formatting of Doxygen lists.

+++ includes/common.inc	13 Oct 2009 00:02:37 -0000
@@ -620,41 +620,57 @@
-function drupal_goto($path = '', array $query = array(), $fragment = NULL, $http_response_code = 302) {
+function drupal_goto($path = '', $options = array()) {

$http_response_code needs to stay a separate argument.

+++ includes/common.inc	13 Oct 2009 00:02:37 -0000
@@ -620,41 +620,57 @@
+  $options += array(
+    'query' => array(),
+    'fragment' => NULL,
+    'http_response_code' => 302,
+    'absolute' => TRUE,
+  );

'absolute' must be forced to TRUE, so it must be assigned after the destination has been parsed.

+++ includes/common.inc	13 Oct 2009 00:02:37 -0000
@@ -620,41 +620,57 @@
+    $options['query'] = $destination['query'];
+    $options['fragment'] = $destination['fragment'];

You can simply array_merge($options, $destination) here, because 'path' doesn't matter for url().

+++ includes/common.inc	13 Oct 2009 00:02:37 -0000
@@ -620,41 +620,57 @@
-  $args = array(
-    'path' => &$path,
-    'query' => &$query,
-    'fragment' => &$fragment,
-    'http_response_code' => &$http_response_code,
-  );
+  // The $args array is only used so that $path and $options can be directly
+  // altered by reference without a PHP error.
+  $args = array('path' => &$path, 'options' => &$options);

Unless you want to alter some further places throughout core, I wouldn't necessarily change this alter hook.

But if you do, then I really wonder why we pass that stuff so strangely.

drupal_alter('drupal_goto', $path, $options, $http_response_code);

will do. The previous munging into an array was only done, because we had to pass multiple arguments....

ugh. Now I realize that you can't do what I propose until #593522: Upgrade drupal_alter() lands. :-/

This review is powered by Dreditor.

* "$http_response_code needs to stay a separate argument."

I'm not sure why this is true. Is this a precedent set somewhere, etc.?

* "'absolute' must be forced to TRUE, so it must be assigned after the destination has been parsed."

My thought was that anything passed in should be possible to alter. If that shouldn't be the case, I'll set it after the alter comes back so it doesn't imply that it can be changed.

* "You can simply array_merge($options, $destination) here, because 'path' doesn't matter for url()."

I was doing that at first, and then stopped. There are exactly 3 parts to the array $destination, and path has to be treated special regardless. Since there are only two left, I opted to handle them individually rather than risk any (somewhat unlikely) side effects.

* drupal_alter

For now I'm just going to shoot for backward compatibility, and if the drupal_alter issue gets committed I'll set it to use drupal_alter('drupal_goto', $path, $options, $http_response_code); or something equivalent.

#593522: Upgrade drupal_alter() is in now, so you can use the much more readable and usable way of multiple arguments instead of a totally strange $args argument. :)

I really hope we get this done ASAP. Please also do not forget that we may need changes in drupal_redirect_form() according to what we do here.

1) The response code does not belong to url() arguments, and the goal here is to make the primary arguments of drupal_goto() consistent with url(). The response code is an additional, special argument for drupal_goto().

2) 'absolute' must always be TRUE, because the generated URL is used as Location HTTP header. Partially also required for SimpleTest.

3) Actually, it doesn't matter, I just would find it cleaner.

4) Clarified in #12 - it's much cleaner to pass meaningful variable names (which, again, are consistent with drupal_goto()) instead of a clumsy $args array, which you need to read docs for first to understand what's in.

Some code was moved around because of the drupal_exit commit earlier today, and the drupal_alter patch makes the function much cleaner.

The form.inc lines in #5 didn't work because 'redirect' needed to use the new drupal_goto format where it was used with multiple parameters, now resolved without changing form.inc (4 places).

All *_drupal_goto_alter functions changed to use the new args (2 places, both were tests).

Anywhere that called drupal_goto directly AND previously used more than just $path changed (openid only).

Documentation cleaned up some ($path and $http_response_code unchanged, $options pulled from url() minus absolute, other minor cleanup).

System.api.php sample hook updated.

This should get us somewhere past the installer.

I see that you added some type hinting. Any other changes?

yes, the changes to includes/batch.inc are the trick to make this pass the installer ;) Also, I've corrected some more instances of $form_state['redirect'].

So, actually, this should be ready to fly.

RTBC seconded, looks good.

Committed to HEAD!

Needs documenting in the upgrade guide.

Just closed #609922: missing documentation for changes to $form_state['redirect'] structure as a duplicate of this issue...

This change does still need documentation in the upgrade guide, and that other issue gives some guidance as to part of what needs to be documented...

Added docs to http://drupal.org/node/727352

Adding docs to a "More..." page does not fix the issue. It needs to be added to the upgrade page. http://drupal.org/update/modules/6/7

updating tagging scheme

Actually, it looks like that aux page has been incorporated into the main page.