The query string should be encoded using rawurlencode() and not drupal_urlencode(). The latter causes double-encoding of the query string contents, which is not needed: the double-encoding is only needed for clean URLs due to issues with apache's mod_rewrite, and these issues don't affect the query string part of the page request.

Related issues:
http://drupal.org/node/178615 - includes the double-encoding problem, proposes a fix by making drupal_urlencode() more complex.
http://drupal.org/node/74070 - an old "fix" that I think was wrong, or is now wrong, and needs undoing.
http://drupal.org/node/301192 - workaround for the same bug, as found in the Printer module.

Note: the only place that drupal_urlencode() is called (correctly) from core is within the url() function. It might make sense to remove the drupal_urlencode() function and move its contents into url(). This would remove the temptation to call it from inappropriate places. Contributed modules should use url() to construct encoded URLs.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fonant’s picture

FileSize
882 bytes

Here's the very simple patch needed to fix the double-encoding problem.

fonant’s picture

FileSize
1.29 KB

Here's an improved patch, including a comment within the drupal_urlencode() description to warn that the function shouldn't be used for query strings.

(For some reason diff thinks the change is in drupal_json(), but it isn't).

Damien Tournoud’s picture

Version: 6.4 » 7.x-dev
Status: Active » Needs work

Ok, please remove drupal_urlencode().

Also this will need to go to the current development branch (Drupal 7.x), and we will need to write a complete test case for this.

fonant’s picture

FileSize
1.28 KB

Here's a patch for HEAD (1.797), not much different apart from the additional space in string concatenation on the second changed line. Hopefully I'm doing this correctly - I'm new to getting involved with Drupal core (I've been coding sites with Drupal 5 and 6 for a year or so now).

This just corrects query_string_encode(), leaving drupal_urlencode() as a callable function. I suspect there's a whole separate discussion needed as to whether drupal_urlencode() is redundant (and dangerous?) - unless by "please remove" you mean get rid of the function too?

Now I just need to investigate writing test cases! I found the problem with data in a query string and the standard Drupal sortable tables (which use drupal_query_string_encode() to generate the sort links in the headers).

fonant’s picture

Here's another patch, with fixes for some other places where drupal_urlencode() is called on query strings.

I've also improved the function documentation for url() to make it clear when the query optional argument should be urlencoded or not. [I've a feeling the argument should never be encoded, but that would seem to be a fairly big task to change everywhere that calls url() with a pre-encoded and non-array query option.]

Not sure about the two remaining places where this sort of thing happens:

1) misc/drupal.js has a copy of drupal_urlencode(). We might also need to investigate usage of encodeURIComponent() in javascript code, to check it isn't being used on query strings.

2) modules/search/search.test uses drupal_urlencode(), but I don't know if drupalGet() also does encoding...

c960657’s picture

Now that Drupal 7 requires PHP5, I think that drupal_query_string_encode() can be simplified a lot by using http_build_query().

c960657’s picture

Status: Needs work » Needs review
FileSize
9.69 KB

The patch looks good.

You are right about both points in #5.
1) The problem with drupal.js can be seen on the page admin/settings/date-time: Select “Custom format” and add "//" to the text field. Note how the example date below the field looks wrong.
2) It's true that drupalGet() encodes the path (via url()), so the drupal_encode() call can simply be removed. Your can verify the problem by modifying drupal_web_test_case::drupalCreateNode() to include some special characters in the node title.

I fixed these two things.

Status: Needs review » Needs work

The last submitted patch failed testing.

c960657’s picture

Status: Needs work » Needs review
FileSize
10.09 KB

Reroll.

Status: Needs review » Needs work

The last submitted patch failed testing.

c960657’s picture

Status: Needs work » Needs review
FileSize
9.68 KB

Reroll.

Gábor Hojtsy’s picture

Just marked the issue submitted at #298498: Drupal assumes mod_rewrite bugs which are not there a duplicate of this one. That involves the JS Drupal.encodeURIComponent() usage of which breaks URLs in l10n_client. Unfortunately when implementing the encoding code for l10n_client, I've followed the usage of Drupal.encodeURIComponent() from system.js, which turns out is bugos. This bugfix should also be backported to Drupal 6!

I'd also ask for a little doc update to Drupal.encodeURIComponent() to mention that it should not be used on URL components other then the path.

Pedro Lozano’s picture

Subscribing, I found this today in a site, especial characters in query values encoded twice.

c960657’s picture

FileSize
11.92 KB

I renamed Drupal.encodeURIComponent() to Drupal.urlencode() to emphasize that it is closer related to drupal_urlencode() than to JavaScript's built-in encodeURIComponent(), and added a comment about its usage.

See also #284899: Drupal url problem with clean urls that suggests another workaround for the mod_rewrite limitation that eliminates the need to use different functions for encoding the path and the query string.

Status: Needs review » Needs work

The last submitted patch failed testing.

c960657’s picture

Status: Needs work » Needs review
FileSize
11.91 KB

Reroll.

fago’s picture

>The latter causes double-encoding of the query string content.

Have a look at the related issue #385918: destination parameter gets two times urlencoded.
As I've noted there I think it gets double-encoded because it calls drupal_url_encode/url_encode twice on it.

c960657’s picture

FileSize
11.92 KB
cburschka’s picture

Status: Needs review » Needs work
Issue tags: +test case

The patch looks great. Now we just need a test case! :)

c960657’s picture

Status: Needs work » Needs review
FileSize
14.45 KB

I added a few tests. I added them to the class that already contains the tests for l(), as the functions seem related. Future tests for url() may be added to this class too.

Status: Needs review » Needs work

The last submitted patch failed testing.

c960657’s picture

Status: Needs work » Needs review
FileSize
14.39 KB

Reroll.

Status: Needs review » Needs work

The last submitted patch failed testing.

c960657’s picture

Status: Needs work » Needs review
FileSize
14.4 KB

Reroll.

Damien Tournoud’s picture

Status: Needs review » Needs work

Let's rename drupal_urlencode() to drupal_encode_path(), and stop the confusion. The patch itself looks good.

c960657’s picture

FileSize
15.27 KB

Renamed drupal_urlencode() to drupal_encode_path(), and the JS function Drupal.urlencode() to Drupal.encodePath().

This should be added to the upgrade docs before this bug is marked as fixed.

c960657’s picture

Status: Needs work » Needs review
Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

Nice! Let's get this in.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

lilou’s picture

Status: Needs work » Reviewed & tested by the community
pwolanin’s picture

I'm ready to roll a D6 backport as soon as this goes in.

Status: Reviewed & tested by the community » Needs work

The last submitted patch failed testing.

pwolanin’s picture

testbot is right - patch does not apply. Re-rolling

pwolanin’s picture

Status: Needs work » Needs review
FileSize
10.28 KB

re-rolled - must have just been whitespace changes.

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

Makes complete sense, the test bot it happy, let's get this in.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

This makes sense indeed. Committed.

pwolanin’s picture

Version: 7.x-dev » 6.x-dev
Status: Fixed » Patch (to be ported)

getting ready to backport minus the function name changes.

pwolanin’s picture

Status: Patch (to be ported) » Needs review
FileSize
5.5 KB
c960657’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Committed to Drupal 6, thanks!

pwolanin’s picture

Version: 6.x-dev » 5.x-dev
Status: Fixed » Patch (to be ported)
Island Usurper’s picture

Version: 5.x-dev » 7.x-dev
Status: Patch (to be ported) » Needs work
Issue tags: -test case +Needs documentation

This should have been documented before it was backported. http://drupal.org/node/224333 in particular needs to be updated because I had to track this issue down from the commit logs.

Island Usurper’s picture

Version: 7.x-dev » 5.x-dev
Status: Needs work » Patch (to be ported)
Issue tags: -Needs documentation +test case
pwolanin’s picture

Status: Patch (to be ported) » Needs review
FileSize
3.21 KB

possible Drupal 5 patch.

c960657’s picture

Status: Needs review » Reviewed & tested by the community

Looks good.

pwolanin’s picture

@c960657 - did you test it?

c960657’s picture

Yes, I did some rudimentary testing.

marcingy’s picture

Status: Reviewed & tested by the community » Closed (won't fix)

Marking as won't fix as d5 is end of life.