'drupal_goto' / 'drupal_get_destination' broken with query params [#49375]

The drupal_get_destination / drupal_goto mechanism doesn't work when the destination contains query parameters.

Example : on content admin page ('admin/node'), go to page 2 (if you have enough nodes...),
and go 'edit' a node. When you submit the form, you are not redirected where you should be...

The 'destination' is handled in drupal_goto by (approx) the following code :

extract(parse_url($_REQUEST['destination']));
...
$url = url($path, $query, $fragment, TRUE);

drupal_get_destination, builds a value for the 'destination' param like :
the/path/goes/here&param1=value1&param2=value2

When ran through parse_url, this value gives

Array
(
    [path] => the/path/goes/here&param1=value1&param2=value2
)

The query part is left in $path, and then the 'url' function messes with it...

In order for the query part to be recognized, drupal_get_destination should build a value like :
the/path/goes/here?param1=value1&param2=value2
(note the ? instead of & after 'the/path/goes/here').

So that parse_url correctly parses :

Array
(
    [path] => the/path/goes/here
    [query] => param1=value1&param2=value2
)

Comment	File	Size	Author
#18	common.inc_22.patch	1.03 KB	ebruts
#18
#17	common.inc_21.patch	1.02 KB	ebruts
#17
#9	common.inc.2006-02-21.patch	660 bytes	rmiotke
#9
#8	drupal_get_destination_1.patch	1.01 KB	yched
#8
#5	drupal_get_destination_0.patch	935 bytes	Bèr Kessels
#5
	drupal_get_destination.patch	908 bytes	yched

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

yched CreditAttribution: yched commented 14 February 2006 at 17:59

Probably not critical, though... Sorry about that.

Comment #2

yched CreditAttribution: yched commented 14 February 2006 at 18:00

Priority:

Critical

» Normal

Comment #3

Bèr Kessels CreditAttribution: Bèr Kessels commented 16 February 2006 at 09:22

Priority:

Normal

» Critical

It is critical, because it causes a lot of links to no longer work.

Another example is the admin/path/delete/54?destination=admin%2Fpath deletion. It is no longer possible to delete paths, without hacking the url. Instead you get a blank page.

Comment #4

Bèr Kessels CreditAttribution: Bèr Kessels commented 16 February 2006 at 09:25

Status:

Active

» Needs work

Looking at the patch. It works. And it looks good.
however: + $querystring = (!empty($query)) ? ('?'.implode('&', $query)) : ''; is undrupalish.
We try to avoid one-line conditions at all costs (well... allmost).

Comment #5

Bèr Kessels CreditAttribution: Bèr Kessels commented 16 February 2006 at 09:54

Status:

Needs work

» Needs review

File	Size
drupal_get_destination_0.patch	935 bytes

Comment #6

yched CreditAttribution: yched commented 16 February 2006 at 12:18

Right, I wasn't aware of the 'one-line condition' reluctance :-)

BTW, does anyone know a good reason why 'drupal_get_destination' only keeps the 'pager query params' ?

$params = array('page', 'sort', 'order');
foreach ($params as $param) {
    if (isset($_GET[$param])) {
        ...
    }
}

Why not take all the parameters of the current url ? I needed that feature and I hade to code my own 'my_drupal_get_destination' in a module somewhere - which makes me... not so happy...

I could provide a patch for that too, but there has to be a reason in the first place ?

Comment #7

Dries CreditAttribution: Dries commented 21 February 2006 at 09:07

Status:

Needs review

» Needs work

It would be nice if drupal_get_destination were more generic. I guess that might solve the problem more elegantly?

The coding style of Ber's patch needs work: missing spaces.

Comment #8

yched CreditAttribution: yched commented 21 February 2006 at 14:56

File	Size
drupal_get_destination_1.patch	1.01 KB

Dries : It would be nice if drupal_get_destination were more generic. I guess that might solve the problem more elegantly?
I'm not sure what you mean by that.
If it's : "let's include all query params in the destination (and not only the pager ones)", well, the included patch does that.
(But then again, maybe i'm just happily interpreting your post according to my own wishes :-)

This patch also corrects missing spaces (well I think it does ?).

BTW, I hadn't noticed Ber made a mistake translating my one-liner conditional into a proper if statement :
if(empty($query)) {...
should be :
if(!empty($query)) {...

Comment #9

rmiotke CreditAttribution: rmiotke commented 22 February 2006 at 05:37

File	Size
common.inc.2006-02-21.patch	660 bytes

i had to patch (or at least i think i had to) drupal_goto to check for destination ($_REQUEST['destination'] and $_REQUEST['edit']['destination']) via isset, rather than with shorthand true/false.

the problem--at least with logintoboggan access denied login pages enabled, and with anonymous access totally disallowed, and destination slated for drupal root--is that drupal root (ie, null) would get interpreted as false and hence cause redirect to user page upon successful login. (why, anyway, by the way, is a user profile page considered a better failover redirect than home? i'm assuming there's a good reason.)

i'm attaching my little patch here, but maybe just be included in this other stuff? unless of course i'm totally wrong.

Comment #10

Bèr Kessels CreditAttribution: Bèr Kessels commented 22 February 2006 at 10:43

rmiotke, is your patch replacnig the one by yched? I fail to see how your patch fixes this issue.

yched, your patch seems to work, I tested it on a clean site.
But, I am wondering if we need not add a line to legacy.module?

Comment #11

yched CreditAttribution: yched commented 22 February 2006 at 10:58

rmiotke's patch is a separate point. I have no opinion about it being an issue or not, but it should IMO have its own thread.

As for legacy.module : well, I've only been hanging around drupal for a few months, so I guess I'm too much of a drupal noob to be aware of legacy issues.

Comment #12

yched CreditAttribution: yched commented 22 February 2006 at 18:16

Status:

Needs work

» Needs review

forgot to update status...

Comment #13

rmiotke CreditAttribution: rmiotke commented 22 February 2006 at 18:35

posted my issue under separate cover. sorry for muddying the waters ....

Comment #14

Dries CreditAttribution: Dries commented 23 February 2006 at 10:35

Code looks good. Needs some testing though.

Comment #15

yched CreditAttribution: yched commented 23 February 2006 at 21:25

I feel I should stress the fact the my patch actually does two different things :

correct a bug that was stopping the redirection mechanism from working in some (non rare) cases.
I think it does that allright, more reviews are welcome.
On Ber's request (or at least the way I understood his comment), it also modifies the extent to which query params in the curent url are mirrored in the destination url.
Previous behaviour : only params generated by a pager (namely 'page', 'sort' and 'order') are mirrored
Patch behaviour : all the params (except 'q'...) are mirrored.
I like (and I need..) this new behaviour, but I can't help but thinking that maybe there was a good reason for this "pager-limited" destination thing, even if I can't think of any. Like, "too easy"...

So : reviews AND historical point of views are welcome

Comment #16

yched CreditAttribution: yched commented 23 February 2006 at 21:39

Mistake. I meant "On Dries's request...". Sorry.

Comment #17

ebruts CreditAttribution: ebruts commented 23 February 2006 at 23:33

Assigned:

Unassigned

» ebruts

File	Size
common.inc_21.patch	1.02 KB

I was about to address the very same issue and just found this.

The supplied patch almost do the job but keep in mind that a param can already be encoded. Like

http://drupal.org/node?key1=val%26ue2&key2=value2 (%26 is &)

Inside the $_GET array it will appear as decoded and so $_GET['key1'] is 'val&ue2'.

This will encode into
"destination=node%3Fkey1%3Dval%26ue2%26key2%3Dvalue2"
but it should really be
"destination=node%3Fkey1%3Dval%2526ue2%26key2%3Dvalue2"

Comment #18

ebruts CreditAttribution: ebruts commented 23 February 2006 at 23:41

File	Size
common.inc_22.patch	1.03 KB

Just noticed that I forgot the urlencode() around the key.

Comment #19

moshe weitzman CreditAttribution: moshe weitzman commented 24 February 2006 at 07:45

i can't seem to reproduce this. i clicked on 'edit' from the 2nd page of admin/node and admin/comment and arrived back at my destination correctly. how are you guys seeing this?

i can't recall why i limited the destination handling to just tablesort and pager variables. i might have been trying to simplify some urlencoding issue? seems correct to just pass along all querystring params. note the array2uri function proposed at http://drupal.org/node/5371

Comment #20

moshe weitzman CreditAttribution: moshe weitzman commented 24 February 2006 at 14:09

with clean urls disabled, i can now reproduce this. i will review the patch this weekend.

Comment #21

ebruts CreditAttribution: ebruts commented 24 February 2006 at 16:01

Actually this applies to both, clean and non-clean urls. It just "works" with clean urls because it is rewritten by mod_rewrite (more on this later).

You can reproduce this with a fresh installation, at last 1 node and clean urls enabled.

1. Get and install the cvs.
2. Add a story with non-sense content.
3. Open http://example.com/admin/node?page=2 (there is no second page but it does not matter for the test).
4. Click "edit" your node you just created. This will open the page
   http://example.com/node/1/edit?destination=admin%2Fnode%26page%3D2
5. Save the node. This will redirect you to
   http://example.com/admin/node%26page%3D2
      but it should redirect to
   http://example.com/admin/node?page=2

It just works with http://example.com/admin/node%26page%3D2 because mod_rewrite calls the index.php with ?q=admin/node&page=2. Whereas the uri in the browser is still wrong!

For non-clean urls the current method does not work at all because after you save the node it will redirect you to

?q=admin/node%26page%3D2

which does not contain any other arguments except 'q' and therefore e.g. the pager will not be able to access $_GET['pager'].

The submitted patch fixes both, clean and non-clean urls. (Where clean urls work without the patch, too, but look worse in the url textfield of your browser).