url() still accepts a query string both as string AND as an array.

Ugly code everywhere. if ... then ... else ... then ... massacre.

What breaks?

CommentFileSizeAuthor
#74 578520-follow-up-3.patch1.16 KBc960657
Passed: 13662 passes, 0 fails, 0 exceptions View
#70 578520-follow-up-2.patch2.52 KBc960657
Failed: Failed to apply patch. View
#68 578520-follow-up-1.patch2.46 KBc960657
Failed: Failed to apply patch. View
#65 drupal.url-query-array.65.patch65.44 KBsun
Failed: Failed to apply patch. View
#64 drupal.url-query-array.64.patch65.39 KBsun
Failed: Failed to apply patch. View
#60 drupal.url-query-array.60.patch65.24 KBsun
Failed: Failed to apply patch. View
#58 drupal.url-query-array.58.patch65.18 KBsun
Failed: Failed to apply patch. View
#57 drupal.url-query-array.57.patch63.83 KBsun
Failed: Failed to apply patch. View
#55 drupal.url-query-array.55.patch64.39 KBsun
Failed: Failed to apply patch. View
#53 drupal.url-query-array.53.patch56.37 KBsun
Failed: Failed to apply patch. View
#50 drupal.url-query-array.49.patch56.47 KBsun
Failed: Failed to apply patch. View
#47 drupal.url-query-array.47.patch56.55 KBsun
Failed: 13186 passes, 121 fails, 460 exceptions View
#46 drupal.url-query-array.46.patch54.52 KBsun
Failed: Failed to apply patch. View
#42 drupal.url-query-array.42.patch54.57 KBsun
Failed: Failed to apply patch. View
#35 drupal.url-query-array.35.patch54.61 KBsun
Failed: Failed to apply patch. View
#33 drupal.url-query-array.33.patch55.41 KBsun
Failed: Failed to apply patch. View
#31 drupal.url-query-array.31.patch53.6 KBsun
Failed: Failed to apply patch. View
#29 url-test-grokme.png38.03 KBsun
#29 url-test-readme.png41.09 KBsun
#29 drupal.url-query-array.28.patch48.61 KBsun
Failed: 12970 passes, 10 fails, 3 exceptions View
#27 drupal.url-query-array.27.patch49.68 KBsun
Failed: 12968 passes, 10 fails, 3 exceptions View
#24 drupal.url-query-array.24.patch49.51 KBsun
Failed: 12891 passes, 111 fails, 460 exceptions View
#20 drupal.url-query-array.20.patch36.09 KBsun
Failed: 12987 passes, 10 fails, 3 exceptions View
#14 drupal.url-query-array.14.patch36.09 KBsun
Failed: 12978 passes, 10 fails, 3 exceptions View
#12 drupal.url-query-array.12.patch36.02 KBsun
Failed: 12992 passes, 10 fails, 3 exceptions View
#8 drupal.url-query-array.8.patch32.44 KBsun
Failed: 12895 passes, 56 fails, 15 exceptions View
#5 drupal.url-query-array.5.patch23.62 KBsun
Failed: 12945 passes, 30 fails, 31 exceptions View
#4 drupal.url-query-array.4.patch23.11 KBsun
Failed: 12770 passes, 227 fails, 1250 exceptions View
#2 drupal.url-query-array.2.patch22.39 KBsun
Failed: Failed to install HEAD. View
drupal.url-query-array.patch1.58 KBsun
Failed: Failed to install HEAD. View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
22.39 KB
Failed: Failed to install HEAD. View

More?

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
23.11 KB
Failed: 12770 passes, 227 fails, 1250 exceptions View

Lovely Batch API. :)

sun’s picture

FileSize
23.62 KB
Failed: 12945 passes, 30 fails, 31 exceptions View

Added type-hinting to ensure it's an array.

sun’s picture

+++ includes/tablesort.inc	16 Sep 2009 00:15:06 -0000
@@ -205,11 +205,8 @@ function tablesort_header($cell, $header
-    if (!empty($ts['query_string'])) {
-      $ts['query_string'] = '&' . $ts['query_string'];
-    }
-    $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));
+    // @todo $ts['query_string']...
+    $cell['data'] = l($cell['data'] . $image, $_GET['q'], array('attributes' => array('title' => $title), 'query' => array('sort' => $ts['sort'], 'order' => $cell['data'], $ts['query_string']), 'html' => TRUE));
+++ modules/simpletest/drupal_web_test_case.php	16 Sep 2009 00:15:06 -0000
@@ -1804,7 +1804,7 @@ class DrupalWebTestCase extends DrupalTe
       if (isset($parts['query'])) {
-        $options['query'] = $parts['query'];
+        parse_str($parts['query'], $options['query']);
       }
       $path = url($path, $options);

Remaining todos.

Especially not sure about the second change - I think that's in drupalGet(). According to parse_str(), that should retrieve key/value pairs for a passed in query string-alike string.

+++ modules/openid/tests/openid_test.module	16 Sep 2009 00:15:06 -0000
@@ -229,5 +229,5 @@ function _openid_test_endpoint_authentic
-  header('Location: ' . url($_REQUEST['openid_return_to'], array('query' => http_build_query($response, '', '&'), 'external' => TRUE)));
+  header('Location: ' . url($_REQUEST['openid_return_to'], array('query' => $response, 'external' => TRUE)));

Not sure whether this is the same, but I guess so.

Also opens up the question why we don't use http://de2.php.net/manual/en/function.http-build-query.php in general? Anyway, separate issue...

I'm on crack. Are you, too?

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
32.44 KB
Failed: 12895 passes, 56 fails, 15 exceptions View

This is kind of patch that really really really really really really highlights how important it is to break backwards compatibility when doing API changes.

Debugging the test failures and finding the real cause is a monster pain, I can tell you.

Dries’s picture

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

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

subcribing.

sun’s picture

Assigned: Unassigned » sun
Status: Needs work » Needs review
FileSize
36.02 KB
Failed: 12992 passes, 10 fails, 3 exceptions View

Lovely $destination munging in a couple of form button submit handlers....

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
36.09 KB
Failed: 12978 passes, 10 fails, 3 exceptions View

This one should finally pass.

sun’s picture

Hmmm.... those tests pass for me locally :(

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
sun’s picture

Title: Remove BC-layer from url() allowing to pass query string as string » Make $query in url() only accept an array
Issue tags: +DrupalWTF

pwolanin asked for a better title.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
36.09 KB
Failed: 12987 passes, 10 fails, 3 exceptions View

All tests keep passing on my local Windows machine. WTF?

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review

Sorry guys, I really tried everything and still can't replicate those failing tests. Tested with new install with patch applied, ran all tests, all pass.

Maybe a PHPWTF on Windows/Unix?

c960657’s picture

Status: Needs review » Needs work
+ * @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.

sun’s picture

Status: Needs work » Needs review
FileSize
49.51 KB
Failed: 12891 passes, 111 fails, 460 exceptions View

Thanks for that awesome review!

And also thanks for catching clean URLs as cause - I didn't thought of that. (And it makes me a bit worry that the existing test that should have caught this should have failed on my box, too.)

I incorporated almost all of your suggestions except:

- Renaming to drupal_get_query() (including pager and tablesort functions): Too ambiguous with database queries. Thought of alternatives, but wasn't able to come up with better names. I think that "query string", when referring to URLs, is a pretty common term. So the only thing we could do would be to squeeze a 'url_' somewhere into those function names, but I'd like to defer that to a separate issue.

- Blank line between @param and @return: Actually, we do, and most developers prefer it. It's just not defined in the coding standards yet. There is an issue about this floating around somewhere, but I'm unable to find it currently.

- Generation of external URLs, which optionally allows to append a custom query string, is already covered in tests. NIH. :)

- http_build_query() cannot be used, because it doesn't use rawurlencode() like drupal_query_string_encode() does.

Additionally, I merged a stale test case for url(), which only tested external URLs, into the existing URL generation unit test, where l(), url(), aso. are already tested.

So, plenty of additional clean-ups in this patch - hence the almost doubled size...

Status: Needs review » Needs work

The last submitted patch failed testing.

c960657’s picture

Status: Needs work » Needs review

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.

sun’s picture

FileSize
49.68 KB
Failed: 12968 passes, 10 fails, 3 exceptions View

This one should pass.

Can we defer that function renaming to a separate issue, please? Those function names are a tiny WTF compared to the major WTF of $query being allowed to be two different data types.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
48.61 KB
Failed: 12970 passes, 10 fails, 3 exceptions View
41.09 KB
38.03 KB

Additionally.... boy, that url() test in common.test is a *unit* test. Replaced those novels with developer friendly output.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
53.6 KB
Failed: Failed to apply patch. View

The test failures are caused by SimpleTest's own internal URL handling. Specifically, DrupalWebTestCase->getAbsoluteUrl().

I've fixed the bug after hours of debugging, but it's certainly not fixing the cause: That entire DrupalWebTestCase->getAbsoluteUrl() looks totally wrong to me. I've left a @todo plus lengthy inline comment in there explaining the parts I understand.

Also added tests to SimpleTest to ensure proper URL handling inside of DrupalWebTestCase.

In case this one doesn't pass, then this is beyond my horizon, and aliens will eat me tonight.

mfb’s picture

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

sun’s picture

FileSize
55.41 KB
Failed: Failed to apply patch. View

/me stops counting other issues to clean up in this patch.

mfb’s picture

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.

sun’s picture

FileSize
54.61 KB
Failed: Failed to apply patch. View

Because you missed the accompanying PHPDoc that explains why this function lives there?

mattyoung’s picture

sub

mfb’s picture

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.

sun’s picture

Can we please move that discussion about whether http_build_query() may be used or not or whether it may be faster into a separate issue? :)

It was horrible pain to get this patch done and I don't see why any http_build_query()-related talk can't be moved elsewhere. The much more important part of this patch is the API clean-up to make $query in url() always an array.

Thus, I'd love to see a RTBC here.

mfb’s picture

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.

sun’s picture

Sorry, that would potentially change the current behavior, so I'm not going to incorporate that change in this patch. :)

mfb’s picture

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

sun’s picture

FileSize
54.57 KB
Failed: Failed to apply patch. View

See, that's what happens if you delve into the "defender" role! ;) Thanks, man! :)

c960657’s picture

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.

mfb’s picture

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.

Dries’s picture

Status: Needs review » Needs work

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.

sun’s picture

Status: Needs work » Needs review
FileSize
54.52 KB
Failed: Failed to apply patch. View

One of my other patches broke this one. :)

Straight re-roll for now.

I'll have another look at the follow-ups later on. 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.

sun’s picture

FileSize
56.55 KB
Failed: 13186 passes, 121 fails, 460 exceptions View

- Added drupal_parse_url() to properly parse a URL into an associative array suitable for url(). Basically an improved fork of my patch in #582456: Make confirm_form() respect query string destination.

- Changed query parameter handling for non-clean-URLs as suggested by c960657 in #43. Was that way before already, but changed in order to find the cause for the failing tests (which has been found elsewhere now, as mentioned before).

- Removed urldecode() in field_ui.admin.inc.

- Removed 'absolute' => TRUE changes from drupal_web_test_case.php.

@Dries:

- Regarding performance: Since most URLs already use the array-syntax for query parameters, the only difference in performance is the few times in core that url() was invoked with a string for $options['query']. There are no further changes regarding the amount of times drupal_query_string_encode()/drupal_get_query_string() or drupal_get_destination() is called.

- Optional arguments to drupal_get_query_string(): Actually, invoking drupal_get_query_string() without any arguments is a valid use-case. It defaults to $query = $_GET and $exclude = array('q'). Hence, you get $_GET with 'q' filtered out - the most common use-case.

- Security in tablesort: The last patch equaled the logic in the current code we have. Anyway, due to c960657's aforementioned suggestion, the 'sort' and 'order' parameters now come last, so they always have precedence.

...

Which leaves us with the function naming issue. Please all speak up now.

Status: Needs review » Needs work

The last submitted patch failed testing.

c960657’s picture

Status: Needs work » Needs review

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.

sun’s picture

FileSize
56.47 KB
Failed: Failed to apply patch. View

Reverted that change in url() for non-clean-URLs, because slashes in q= are escaped with that and we want those to be readable (according to comments in existing code). There are even tests that verify that.

sun’s picture

@c960657: The patch in #553262: Make getAbsoluteURL() standard compliant seems to aim a completely different purpose. For regular URLs, we don't need any extra parsing. We could just use parse_url(), if url() would still support $options['query'] to be a string. However, this patch here removes that ambiguous support for string or array, so we need a simple wrapper around parse_url() to get a result that is suitable for url().

The second and more hidden issue that drupal_parse_url() fixes is that there are several places where code tries to directly pass $_GET['destination'] to url(). However, url() does not support a relative path containing a query string or fragment. Query parameters or a fragment always have to be passed via $options for internal/relative paths.

Status: Needs review » Needs work

The last submitted patch failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
56.37 KB
Failed: Failed to apply patch. View
c960657’s picture

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.

sun’s picture

FileSize
64.39 KB
Failed: Failed to apply patch. View

#582456: Make confirm_form() respect query string destination will use the introduced drupal_parse_url() as well. Actually, that issue is currently blocked on this one. :(

So we need drupal_parse_url() for drupal_goto() in this patch and for confirm_form() in aforementioned issue. Yes, we can discuss further enhancements to that function in #337261: parse_url in drupal_goto breaks destination url.

Now I finally had some clues:

- drupal_encode_path() is a totally weird name. It's a wrapper around rawurlencode(). Therefore should be named drupal_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().

- And we now have drupal_get_query_parameters(). A bit long, but yeah. Basically, this function is a custom implementation of array_diff_key(). It just accepts different data to filter out of the first argument array. Actually, it works with any array, not necessarily query parameters. However, for now, let's go with that. Optionally re-think the function name in a separate issue.

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

- Renamed TableSort->getQueryString() to ->getQueryParameters().

- Added tests for drupal_get_query_parameters(). Let me note that the existing $exclude argument handling of the current/old drupal_encode_query_string() contains a bug. ;)

Benchmarks... it looks like my system is totally unsuitable for doing benchmarks. Anyway:

HEAD:

# ab -c 1 -n 50 http://drupal.test/

Document Path:          /
Document Length:        7651 bytes

Concurrency Level:      1
Time taken for tests:   16.312 seconds
Complete requests:      50
Failed requests:        0
Write errors:           0
Total transferred:      404800 bytes
HTML transferred:       382550 bytes
Requests per second:    3.07 [#/sec] (mean)
Time per request:       326.231 [ms] (mean)
Time per request:       326.231 [ms] (mean, across all concurrent requests)
Transfer rate:          24.24 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0   11   8.5     16      31
Processing:   266  314  54.8    281     500
Waiting:      266  314  55.1    281     500
Total:        281  325  55.7    297     516

Patch:

# ab -c 1 -n 50 http://drupal.test/

Document Path:          /
Document Length:        7651 bytes

Concurrency Level:      1
Time taken for tests:   15.093 seconds
Complete requests:      50
Failed requests:        0
Write errors:           0
Total transferred:      404800 bytes
HTML transferred:       382550 bytes
Requests per second:    3.31 [#/sec] (mean)
Time per request:       301.858 [ms] (mean)
Time per request:       301.858 [ms] (mean, across all concurrent requests)
Transfer rate:          26.19 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0   10   7.6     16      16
Processing:   266  290  36.3    281     516
Waiting:      266  290  36.3    281     516
Total:        281  300  37.0    297     531

Lastly, we want to put all PHP function wrappers and custom implementations of PHP functions into a new doxygen group: #584966: Add doxygen group for PHP function wrappers in Drupal

c960657’s picture

Status: Needs review » Needs work

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 :-)

sun’s picture

Status: Needs work » Needs review
FileSize
63.83 KB
Failed: Failed to apply patch. View

Further discussion on IRC made me partially revert a couple of the last changes.

However, drupal_encode_path() is still moved to the URL/HTTP request handling function group and I rewrote its description to prevent this in the future.

sun’s picture

FileSize
65.18 KB
Failed: Failed to apply patch. View

Note to self: Always. Add. Tests.

This one should be ready to go. Added a unit test for drupal_parse_url(), and cleaned up PHPDoc some further.

sun’s picture

RTBC, anyone?

As mentioned before, at least one issue is blocked by this one.

sun’s picture

FileSize
65.24 KB
Failed: Failed to apply patch. View

Fixed a few nitpicks by chx.

Dries’s picture

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

sun’s picture

c960657 somehow vanished... the latest patch is based on a IRC discussion with c960657, and based on that discussion, I think he was fine with the implementation.

c960657’s picture

-        $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.

sun’s picture

FileSize
65.39 KB
Failed: Failed to apply patch. View

Thanks! I tried hard to catch you on IRC. :)

Attached patch fixes those two issues.

sun’s picture

FileSize
65.44 KB
Failed: Failed to apply patch. View
c960657’s picture

Status: Needs review » Reviewed & tested by the community

This looks good now :-)

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Great! Good job. Committed to CVS HEAD!

c960657’s picture

Status: Fixed » Needs review
FileSize
2.46 KB
Failed: Failed to apply patch. View

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').

sun’s picture

Actually, it does:

# php -r "var_dump(parse_url('foo/bar?foo=bar&bar=baz:1#foo'));"
array(3) {
  ["path"]=>
  string(7) "foo/bar"
  ["query"]=>
  string(17) "foo=bar&bar=baz:1"
  ["fragment"]=>
  string(3) "foo"
}

But your specific example of a path containing a non-urlencoded colon is interpreted differently:

# php -r "var_dump(parse_url('foo/bar:1'));"
array(3) {
  ["host"]=>
  string(3) "foo"
  ["port"]=>
  int(1)
  ["path"]=>
  string(6) "/bar:1"
}

We rawurlencode() all query parameters, and we urldecode() $_GET['destination'] in drupal_goto().

# php -r "var_dump(rawurlencode('foo/bar:1'));"
string(13) "foo%2Fbar%3A1"

# php -r "var_dump(urldecode('foo/bar%3A1'));"
string(9) "foo/bar:1"

# php -r "var_dump(rawurlencode('foo/bar%3A1'));"
string(15) "foo%2Fbar%253A1"

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

If not, then we at least want to make the inline comments more specific, i.e. mention the exact problem of parse_url().

c960657’s picture

FileSize
2.52 KB
Failed: Failed to apply patch. View

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.

sun’s picture

+++ includes/common.inc	30 Sep 2009 16:18:20 -0000
@@ -514,20 +514,23 @@ function drupal_parse_url($url) {
+    $parts = parse_url('http://example.com/' . $url);
+    // Strip the leading slash that was just added.
+    $options['path'] = substr($parts['path'], 1);

This code also runs for absolute (but not external) URLs -- so we probably want to use

ltrim($parts['path'], '/')

here instead (ltrim(), not trim(), so a trailing slash is kept) + also add a test for that.

I'm on crack. Are you, too?

sun’s picture

Status: Needs review » Reviewed & tested by the community

uhm, scratch that. That would make absolute paths relative. ;)

So, looks much better now, and I agree that we should do this to be on the safe side.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

c960657’s picture

Status: Fixed » Needs review
FileSize
1.16 KB
Passed: 13662 passes, 0 fails, 0 exceptions View

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.

Dries’s picture

Status: Needs review » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

catch’s picture

Priority: Critical » Normal
Status: Closed (fixed) » Needs work
Issue tags: +Needs Documentation

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.

jpmckinney’s picture

Issue tags: -DrupalWTF

No longer a WTF.

c960657’s picture

@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.

c960657’s picture

Status: Needs work » Fixed

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.

Status: Fixed » Closed (fixed)
Issue tags: -Needs Documentation, -API clean-up

Automatically closed -- issue fixed for 2 weeks with no activity.