If there were a priority between normal and critical I'd have probably put this there. It's important, but not system-breaking.

drupal_get_destination() parses $_GET and re-creates the URL query strings. The problem is that it doesn't recurse into arrays, which are valid PHP query strings used in many applications. (In this case, G2)

If you notice, the URL below includes an array. g2_form[useDefaultSettings]=1

When drupal_get_destination() gets to this value and passes it through urlencode(), a PHP watchdog error gets thrown.

URL:

/gallery?g2_view=search.SearchScan&g2_form%5BuseDefaultSettings%5D=1&g2_return=http%3A%2F%2Fwww.chimericvisions.com%2Fv%2F2005%2

Callstack:

urlencode() expects parameter 1 to be string, array given
in call_user_func (error_handler) at line 0
in includes/common.inc (urlencode) at line 179
in modules/user.module (drupal_get_destination) at line 525
in call_user_func (user_block) at line 0
in includes/module.inc (call_user_func_array) at line 185
in modules/block.module (module_invoke) at line 615
in call_user_func (block_list) at line 0
in includes/module.inc (call_user_func_array) at line 185
in includes/theme.inc (module_invoke) at line 924
in call_user_func (theme_blocks) at line 0
in includes/theme.inc (call_user_func_array) at line 166
in themes/engines/phptemplate/phptemplate.engine (theme) at line 163
in call_user_func (phptemplate_page) at line 0
in includes/theme.inc (call_user_func_array) at line 166
in modules/gallery.module (theme) at line 306
in call_user_func (gallery_page) at line 0
in includes/menu.inc (call_user_func_array) at line 414
in index.php (menu_execute_active_handler) at line 15 in includes/common.inc on line 179.

CommentFileSizeAuthor
#8 drupal_get_destination_4.patch1.42 KBSigne
#5 drupal_get_destination_3.patch1.41 KBSigne
#1 drupal_get_destination_2.patch1.48 KBSigne

Comments

Signe’s picture

Signe commented
StatusFileSize
new drupal_get_destination_2.patch1.48 KB

Attached a patch which implements a new function "drupal_recursive_urlencode()" which is in turn used by "drupal_get_destination()"

Test case:

require_once('includes/common.inc');

$_GET = array(
        'q' => 'gallery',
        'g2_path' => '/my/private/album',
        'g2_form' => array(
            'useDefaultValues' => '1',
            'subFormArray' => array(
                'level3' => 'value3'
                ),
            )
        );

$ret = drupal_get_destination();

echo $ret . "\n";
echo urldecode($ret) . "\n";
Signe’s picture

Signe commented
Status: Active » Needs review
ebruts’s picture

ebruts commented
Version: 4.7.0-beta6 » x.y.z
Status: Needs review » Needs work

You've by accident removed the major part of drupal_get_destination().

-    return 'destination='. urlencode($path . implode('&', $params));
+    return 'destination='. $path . implode('&', $params);

The code needs to be Drupal-ished, though. There is no $lowerUppercase and $parent . '[' . $key should get $parent .'['. $key

Another thing is that drupal_recursive_urlencode() is very confusing as it has a totally different aim then urlencode(). It should be renamed.

Other then this, code looks reasonable and will work after the mentioned changes.

Signe’s picture

Signe commented

Style changes can be fixed, however I'm completely confused about why you would think that the function name doesn't apply.

recursive urlencode is its only purpose. Maybe more specific? recursive_query_string_encode? I could see that.

Signe’s picture

Signe commented
StatusFileSize
new drupal_get_destination_3.patch1.41 KB

Removing that line wasn't so much an accident as getting confused about what I was trying to achieve. I wasn't thinking that the entire string needed to be a further part of another GET variable - just that it was an encoded query string.

Attached new version.

Signe’s picture

Signe commented
Status: Needs work » Needs review
chx’s picture

chx commented
Status: Needs review » Needs work

sorry for being nitpicky but a) i'd like to see this in b) you won't get it in until it conforms to the code standards.
Instead of

} else {

we use 

}
else {
Signe’s picture

Signe commented
StatusFileSize
new drupal_get_destination_4.patch1.42 KB

Good thing you pointed that 'else' out. I'd missed renaming the recursive function call.

Signe’s picture

Signe commented
Status: Needs work » Needs review
Signe’s picture

Signe commented

ping - still needs review

ebruts’s picture

ebruts commented
Status: Needs review » Needs work

I really like the consistent code standard in Drupal.

http://drupal.org/node/318

What still does not conform
1. http://drupal.org/node/1354
2. http://drupal.org/node/30785
3. $urlparams should be renamed into $url_params or better $params

moshe weitzman’s picture

moshe weitzman commented
Status: Needs work » Closed (duplicate)

please consider the patch at http://drupal.org/node/5371 and submit a better one there if you wish.