Make $query in url() only accept an array

Welp, I didn't know about http_build_query -- seems like it is worth investigating (in a separate issue). Good find.

subcribing.

+ * @param $parent
+ *   Internal use only.
+ *
+ * @return
+ *   An array containing query string parameters, which can used for url().
+ */
+function drupal_get_query_string($query = NULL, $exclude = NULL, $parent = '') {

Please add type-hinting here (NULL is still allowed, because it is specified as the default value).
The function name suggests that the function returns a string, but it returns an array. How about naming it drupal_get_query() ?
There is usually no blank line between @param and @return.

I suggest you add a comment explaining the rationale for using array_merge(array('q'), array_keys($_COOKIE)) as the default value. Also, this default is probably not good when called from url() for an external URL. I suggest you change url() to pass an empty array for $exclude when $options['external'] is TRUE.

+function drupal_query_string_encode(array $query, $exclude = array(), $parent = '') {

Use type hinting for $exclude also.

The Doxygen comment for url() should be updated to reflect that $query must now be an array.

+  if ($options['query']) {
+    $options['query'] = drupal_query_string_encode($options['query']);
+  }

Here the array is replaced by a string. Please use a new variable name for the encoded value. I suggest you don't put it in $options, but make a regular variable, e.g. $query_string, just like $script and $prefix.

I can reproduce the testbot problems if I turn off clean URLs. I think the problem is in the // Without Clean URLs. part of url().

pager_get_querystring() no longer returns a query string but an array. I think the function should be renamed, e.g. to pager_get_query(). Also, the Doxygen comment needs to reflect the change in return type. Likewise for tablesort_get_querystring(). If you these two functions are renamed, we also get rid of the two different ways of spelling query_string/querystring.

The Doxygen comment for drupal_get_destination() says "Prepare a destination query string ...", but the function no longer returns a string.

TableSort::getQueryString() no longer returns a query string, and should probably be renamed to getQuery(). Likewise, $ts['query_string'] should be renamed to $ts['query'].

It looks like the filtering logic (i.e. $exclude) is duplicated in drupal_get_query_string() and drupal_query_string_encode(). How changing the latter to just return http_build_query(drupal_get_query_string($query, $exclude), '', '&'); ?

+    $result_url = url($test_url, array('query' => array($query2 => NULL)));
     $this->assertEqual($test_url . '&' . $query2, $result_url, t("External URL with a query passed in the path paramater. url('http://drupal.org/?param1', array('query' => 'param2'));"));

The assertion message needs updating. I suggest you just remove the function call from the display string. Likewise for the following test.

     $query = $this->randomName(10);
-    $url = url('system-test/destination', array('absolute' => TRUE, 'query' => $query));
+    $url = url('system-test/destination', array('absolute' => TRUE, 'query' => array('destination' => $query)));

This changes the behaviour of the test. Before it requested system-test/destination?xyz1234xyz, now it requests system-test/destination?destination=xyz1234xyz. I don't think it matters, though. I think the test should be extended to two cases, with and without a ?destination=xyz1234xyz argument, but we can do that in another issue if you prefer.

Good job! Just a brief follow-up before bedtime:

+ * Compose an URL query parameter array to append to pager links.

Should be “a URL” instead of “an URL”.

- Renaming to drupal_get_query() (including pager and tablesort functions): Too ambiguous with database queries.

I don't understand what you mean by “ambitious”?

I think that "query string", when referring to URLs, is a pretty common term.

Agreed - when we are talking about the actual string representation, i.e. foo=1&bar=2. I think it is really confusing for developers that a function called get_foo_string() returns an array.

Leaving at needs review for more reviews, but user_login_destination(), a new function in user.module, will also need to be patched

Hmm why did you move the function? That's where I originally had it, but Dries suggested making it a generic function in user module for other authentication modules.

sub

Using http_build_query() makes drupal_query_string_encode() quite a bit faster (depending on the complexity of the array, from two to four times faster in my testing). The function I benchmarked is:

function drupal_query_string_encode($query, $exclude = array()) {
  return str_replace('+', '%20', http_build_query(drupal_get_query_string($query, $exclude), '', '&'));
} 

The only difference is that http_build_query() URL-encodes the square brackets as of PHP 5.1.3, after this bug report: http://bugs.php.net/bug.php?id=36656 I don't think URL-encoding square brackets would cause any issues (and apparently would make the URLs more RFC-compliant).

EDIT: One additional difference, http_build_query() removes any query parameter with a NULL value. Whereas, drupal_query_string_encode() treats NULL value as equivalent to empty string, i.e. does not remove it.

Another minor issue I see, in system_admin_compact_page(), drupal_goto($_GET['q'], drupal_get_destination()); can simply be drupal_goto(); Otherwise you end up on admin?destination=admin for no good reason.

The patch changes the current behavior, my suggestion preserves current behavior.

Can we defer that function renaming to a separate issue, please?

It's not that I am trying to pile up unrelated stuff in this issue, but I'd rather not that we add a completely new function, drupal_get_query_string(), and then immediately after file another issue about renaming it drupal_get_query(), knowing that the latter patch may be rejected due to API freeze.

Your patch still renames pager_get_querystring() to pager_get_query_string() and tablesort_get_querystring() to tablesort_get_query_string(), but there is a few variables left using the old spelling, $querystring.

-    // Without Clean URLs.
-    $variables = array();
+    $query = array();
     if (!empty($path)) {
-      $variables[] = 'q=' . $path;
+      $query[] = 'q=' . $path;
     }
-    if (!empty($options['query'])) {
-      $variables[] = $options['query'];
+    if ($options['query']) {
+      $query[] = drupal_query_string_encode($options['query']);
     }
-    if ($query = join('&', $variables)) {
+    if ($query = implode('&', $query)) {
       return $base . $script . '?' . $query . $options['fragment'];
     }
    else {
      return $base . $options['fragment'];
    }

I think this looks a bit confusing, with $query not referring to parameters as key-value-pairs but an array of 0, 1, or 2 URL-encoded strings. And later $query is reused to contain a string. Why not do $options['query']['q'] = $path; and then build the entire query string using drupal_query_string_encode() ?

-    $cell['data'] = l($cell['data'] . $image, $_GET['q'], array('attributes' => array('title' => $title), 'query' => 'sort=' . $ts['sort'] . '&order=' . urlencode($cell['data']) . $ts['query_string'], 'html' => TRUE));
+    $cell['data'] = l($cell['data'] . $image, $_GET['q'], array('attributes' => array('title' => $title), 'query' => array_merge(array('sort' => $ts['sort'], 'order' => $cell['data']), $ts['query']), 'html' => TRUE));

This change is perfectly fine, but I suggest that you change the order of the arguments to array_merge(). This would make it more obvious to readers of the code, that the 'sort' and 'order' arguments take precedence over any arguments in the $ts['query'] array. I know that the array does not contain these entries, but only after looking it up.

-    $destinations[] = urldecode(substr(drupal_get_destination(), 12));
+    $destination = drupal_get_destination();
+    $destinations[] = urldecode($destination['destination']);

Now that drupal_get_destination() no longer calls urlencode(), the urldecode() here should be removed.

Couple more issues:
* in field_ui.admin.inc, I don't think you want the urldecode() call in $destinations[] = urldecode($destination['destination']); because destination is no longer urlencoded by drupal_get_destination(). See above.
* in drupal_web_test_case.php, in two places you specify $this->drupalGet($url, array('absolute' => TRUE)) but drupalGet() always sets $options[absolute] = TRUE so this is unnecessary.

I reviewed this patch and it is a great clean-up.

I'm wondering about the performance impact of this: url() and l() are in the critical path ... some performance testing would be good.

Some additional review:

+++ includes/common.inc	19 Sep 2009 05:10:27 -0000
@@ -317,6 +317,51 @@ function drupal_get_feeds($delimiter = "
+ *   (optional) An list of $query array keys to remove. Use "parent[child]" to

"An list" should be "A list".

The explanation of $exclude is a bit wonky. Doesn't really explain why that parameter is useful, or to be used. Lacks some context.

It is a bit strange that $exclude is marked optional. Technically, it is optional, but in practice it is a parameter that needs to be non-empty for the function to do anything useful.

+++ includes/tablesort.inc	19 Sep 2009 05:10:27 -0000
@@ -174,11 +176,7 @@ function tablesort_header($cell, $header
-    $cell['data'] = l($cell['data'] . $image, $_GET['q'], array('attributes' => array('title' => $title), 'query' => 'sort=' . $ts['sort'] . '&order=' . urlencode($cell['data']) . $ts['query_string'], 'html' => TRUE));
+    $cell['data'] = l($cell['data'] . $image, $_GET['q'], array('attributes' => array('title' => $title), 'query' => array_merge(array('sort' => $ts['sort'], 'order' => $cell['data']), $ts['query']), 'html' => TRUE));

I think this change is harmless, but we have to be careful for XSS attacks. If I manually specify a custom query string attribute in the address bar, will it survive the array_merge()?

+++ modules/simpletest/simpletest.test	19 Sep 2009 05:10:27 -0000
@@ -270,6 +270,43 @@ class SimpleTestFunctionalTest extends D
+    $abs = url($url, array('absolute' => TRUE));

We don't usually abbreviate simple variables. We can use $absolute here.

See also the suggested implementation of drupal_parse_url() in #553262: Make getAbsoluteURL() standard compliant. PHP's parse_url() is not suitable for parsing relative URLs.

Reverted that change in url() for non-clean-URLs, because slashes in q= are escaped with that

This can be fixed by changing drupal_query_string_encode() to use drupal_encode_path() instead of rawurlencode() (if I understand you correctly).

@c960657: The patch in #553262: Make getAbsoluteURL() standard compliant seems to aim a completely different purpose.

Well, to some extent. But I'd rather not have two versions in core for parsing URLs. I think we should combine the two in one function (assuming that the patch for that bug is accepted). Perhaps we can take this discussion in #337261: parse_url in drupal_goto breaks destination url so that it does not block this issue - drupal_goto() seems to be the only function calling your suggested drupal_parse_url().

However, especially the suggestion to rename to drupal_get_query() does not work for me, because when reading that function name I'd rather expect a database query, not URL query parameters. drupal_url_get_query(), url_get_query(), or drupal_get_query_parameters() may all work better, but then again, renaming those functions will take 5 minutes whenever an agreement is there, and I don't want to see this patch hold off, just because of that.

I think query_parameters (or query_array) is better than query_string. Note that DatabaseStatementInterface::getQueryString() actually returns an SQL string with placeholders, so there is already total confusion :-) But okay, if there is agreement that we commit this in two stages, I'd be happy to roll the renaming patch when we get there.

drupal_encode_path() is a totally weird name. It's a wrapper around rawurlencode(). Therefore should be named drupal_rawurlencode().

drupal_encode_path() should only be used on paths, not on query string parameters. That is why its name was changed from drupal_urlencode() in #310139: drupal_query_string_encode() should not call drupal_urlencode(). AFAICT you are reintroducing the bug that was fixed in #310139.

Note that when #284899: Drupal url problem with clean urls is fixed, the function will indeed be a simple wrapper around rawurlencode().

- drupal_query_string_encode() is a custom implementation of http_build_query(). It returns a drupal_rawurlencode()'d query string. Therefore should be named drupal_http_build_query()

Good point.

- Renamed tablesort and pager "query_string" function suffix to "query_parameters".

w00t :-)

I'd love to see c960657 sign off on this patch, and mark it RTBC when he thinks it is.

-        $destination = 'destination=' . rawurlencode("node/$node->nid#comment-form");
+        $destination = array('destination', "node/$node->nid#comment-form");

Shouldn't this be array('destination' => "node/$node->nid#comment-form")?

+    $this->assertEqual($url . '&' . http_build_query($query), $result, t('External URL query string can be extended with a custom query string in $options.'));

You should specify all three parameters for http_build_query(), http_build_query($query, '', '&'). Otherwise its result depends on an ini setting, i.e. the test may fail if the setting is set to an unexpected value.

Apart from that it looks good.

This looks good now :-)

Great! Good job. Committed to CVS HEAD!

Small follow-up based on the example URL in #337261: parse_url in drupal_goto breaks destination url. Apparently parse_url('foo/bar:1') returns array ('host' => 'foo', 'port' => 1, 'path' => '/bar:1').

Shouldn't we double-encode the 'destination' query parameter instead then, because we know that we'll urldecode it?

I don't think the problem is specific to destination. drupal_parse_url('foo/bar:1') should work.

It's not a huge problem. The URL http://localhost/en/facetsearch/results/author:1 isn't generated by url() (because url() would have encoded the colon), so I guess the problem only occurs for "hand crafted" URLs. Anyway, the documentation for parse_url() explicitly says that it "doesn't work with relative URLs" (though elsewhere it states that "partial URLs are also accepted"), so I suggest we use the approach suggested in the patch anyway to guard against other (currently unknown) edge cases.'

I have extended the inline comment with an example.

Committed to CVS HEAD. Thanks!

Another small follow-up:
In testGetAbsoluteUrl(), we need to call variable_set() instead of accessing $conf['clean_url'] directly, because $conf is overwritten by dwtc::refreshVariables() that is called from dwtc::drupalGet(). Without this patch, the test fails when clean URLs are enabled.

Committed to CVS HEAD. Thanks.

http://api.drupal.org/api/function/drupal_query_string_encode/6 drupal_get_query_string() and friends no longer exist, couldn't find any documentation for that in the upgrade guide.

No longer a WTF.

@sun, I just realized that misunderstood your question about double-encoding in #69. Somehow I missed the fact the we call urldecode() in drupal_goto(). We shouldn't do that, so I just filed #796120: Do not urldecode() $_GET parameters in drupal_goto(). Your patch didn't do any double-encoding, so with the fix for #796120 I think we're good.

The renamed functions are now mentioned in the upgrade guide: http://drupal.org/update/modules/6/7#get_querystring.

The change to url() was already documented: http://drupal.org/update/modules/6/7#url_query_parameter.