$url = "http://my.site.com/some_path?foo%26bar=42&foo=20&bar=22";
$parsed = parse_url($url);
$query = $parsed['query'];
$array = drupal_get_query_array($query);
$url_back = drupal_http_build_query($array);

var_dump($array);
var_dump($url_back);
// OUTPUT
// array(3) {
//   ["foo%26bar"] => string(2) "42"
//   ["foo"] => string(2) "20"
//   ["bar"] => string(2) "22"
// }
// string(28) "foo%2526bar=42&foo=20&bar=22"

Since drupal_http_build_query is rawurlencoding both the keys and values given to it in the $query array,

// line 490 in includes/common.inc
$key = ($parent ? $parent . '[' . rawurlencode($key) . ']' : rawurlencode($key));

it would seem logical that drupal_get_query_array, which seems to be to opposite function of drupal_http_build_query, is rawurldecoding the keys as well.
However, because it does not rawurldecode the keys, the example above does not result back in the original url, but has a double encoded "%".

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Sneakyvv’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
1.49 KB

Simply added rawurlencode around the key.

Obviously, this change might have implications for existing code, but I do think this is the correct way the function should work.

I also added an automated test (as it was suggested in #2143461: drupal_get_query_array encodes not enough if query parameter contains a equal sign (=), my previous fix for drupal_get_query_array).

Sneakyvv’s picture

I also found another problem with drupal_get_query_array (#2446671: drupal_get_query_array should make a distinction between empty and non-existing parameters). Since this issue's patch is changing the same line, the patches conflict. Therefore I attached a combined patch in that issue, but I'll upload it here as well.

Sneakyvv’s picture

markus_petrux’s picture

Status: Needs review » Closed (duplicate)

Since both issues provide the same patch... to make it asier to track... may we consider this one a dup of the other?

David_Rothstein’s picture

Status: Closed (duplicate) » Needs review

Let's keep these issues separate (as I commented on the other issue).

I think this makes sense since it's inconsistent as is, and also the behavior suggested here seems to be what http://php.net/manual/en/function.parse-str.php does.

There's a chance this could break someone's code, though (if they wrote their code expecting the current behavior). So I'm not sure what to do about that. Maybe a release notes mention would be enough to deal with that...?

Sneakyvv’s picture

I hear what you're saying that this could break someone's code. However, like you said, it isinconsistent. So I agree that perhaps a release note should suffice.