I noticed there is a function called drupal_get_query_array() in common.inc, this function seems to do exactly what the core PHP function parse_str() does, except parse_str does it in c code, and can handle arrays in the query string.

It seems the function was introduced in #282191: TF #1: Allow different interface language for the same path with commit 1b9cde9d85b2c04df35b04cfbd12f68243a118bc.

I had a look through that issue, and it seems that parse_str() may not have been known to people involved in that issue.

I'm wondering first, if there is any reason this function cannot be replaced with parse_str(), and if not, then the patches attached in the next comment should work for D7 and D8.

Files: 
CommentFileSizeAuthor
#1 drupal-get-query-array-to-parse-str-D8-1597784-1.patch2.17 KBcdale
PASSED: [[SimpleTest]]: [MySQL] 36,647 pass(es).
[ View ]
#1 drupal-get-query-array-to-parse-str-D7-1597784-1.patch1.78 KBcdale
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-get-query-array-to-parse-str-D7-1597784-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

cdale’s picture

StatusFileSize
new1.78 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-get-query-array-to-parse-str-D7-1597784-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new2.17 KB
PASSED: [[SimpleTest]]: [MySQL] 36,647 pass(es).
[ View ]

D8 patch removes the function, D7 patch leaves the API in tact, but changes the inner workings.

Status:Needs review» Needs work

The last submitted patch, drupal-get-query-array-to-parse-str-D7-1597784-1.patch, failed testing.

cdale’s picture

Status:Needs work» Needs review

This is needs review.

*Aside, is there some naming convention in the patches to stop them testing against the wrong bot? Or is it best not to post patches for two versions at the same time?

droplet’s picture

read the drupal_get_query_array function. It's a difference. Im not sure it's a bug or not.

<?php
$str
= "arr[]=foo&arr[]=bar";
parse_str($str, $output);
print_r($output);

/**

Array
(
    [arr] => Array
        (
            [0] => foo
            [1] => bar
        )

)
**/
print_r(drupal_get_query_array($str));

/**
Array
(
    [arr[]] => bar
)
**/
?>

EDIT:
use xxxxxxxxxxxxxxxxxx-do-not-test.patch for D7 patches. testbot doesn't support multiple version at once.

droplet’s picture

Category:feature» bug
cdale’s picture

It does seem like a bug to me as well, but perhaps drupal_get_query_array() was never designed to handle array's in query parameters? Either way, parse_str() seems like a much better option.

cesarmiquel’s picture

I agree that parse_str() is a much more robust implementation. drupal_get_query_array() fails at parsing anything non trivial. We came across a bug in the redirect module where it would fail to save correctly destination URLs because it was relying on this function.
Here's an example from a real life web where some URLs needed to be redirected to an Ad server with veeery weird URLs. As you can see drupal_get_query_array() does it all wrong. +1 to use parse_str() and even better to remove from D8 that evil function ;-) (no offense meant to whoever wrote it).

Code which show how drupal_get_query_array() fails:

<?php
 

echo "Result of parse_url():\n";
$p = parse_url("http://adserver.adtechus.com/?adlink|3.0|5412|4925982|1|16|AdId=3687610;BnId=17;link=http://www.blah.com/ownership/locate_a_dealer/?region=1117");

print_r($p);

echo
"\n\nResult of drupal_get_query_array(\$p['query']):\n";
print_r( drupal_get_query_array($p['query']) );

echo
"\n\nResult of parse_str(\$p['query']):\n";
parse_str($p['query'], $result);
print_r($result);
?>

Results of executing said code:

Result of parse_url():
Array
(
    [scheme] => http
    [host] => adserver.adtechus.com
    [path] => /
    [query] => adlink|3.0|5412|4925982|1|16|AdId=3687610;BnId=17;link=http://www.blah.com/ownership/locate_a_dealer/?region=1117
)

Result of drupal_get_query_array($p['query']):
Array
(
    [adlink|3.0|5412|4925982|1|16|AdId] => 3687610;BnId
)

Result of parse_str($p['query']):
Array
(
    [adlink|3_0|5412|4925982|1|16|AdId] => 3687610;BnId=17;link=http://www.blah.com/ownership/locate_a_dealer/?region=1117
)

junedkazi’s picture

parse_str() is subject to php max_input_vars limitation and there are sites that use parse_str() to parse things that aren't directly coming from users query args.

junedkazi’s picture

I have also created a new ticket to support same key params in the query. #2023815: Update drupal_get_query_array function to handle same name array in query parameters

Tiaan’s picture

Unless you also intend to compensate for the "magic quotes" settings of a website, as determined by get_magic_quotes_gpc() and similar to what is done with fix_gpc_magic(), one cannot simply replace drupal_get_query_array() with parse_str(), since that will not work reliably on all websites when values contain quotes. For example, see #2123447: ServicesParserURLEncoded->parse() mangles data when magic quotes enabled. However, we do not need to worry about backwards compatibility for magic quotes if this change is only for Drupal 8 and if it targets PHP 5.4.0+ only (although I don't think that is the case).

dewalt’s picture

Issue summary:View changes

I'm argry, that using parse_str function instead of drupal_get_query_array() is more correct - it process array values as arrays and decodes both param key and value.

But it also decodes "+" character to " " - plus characters from user input should be pre-encoded in menu_edit_item_validate() function. See more in #2212951: Special symbols encoding is duplicated in query keys of menu item path each time, menu item is saved..

jhedstrom’s picture

Status:Needs review» Needs work
Issue tags:+Needs reroll
jyotisankar’s picture

Status:Needs work» Closed (fixed)

Hi
drupal_get_query_array() is nolonger in use. It is already replaced by native parse_str() function. I think we should close this issue now, changing the status to Closed (fixed).