drupal_urlencode() does two things:

1. Changes %2F to slashes, because there is actually no reason to encode slashes.
2. Works around a mod_rewrite bug where the path, the q argument, is un-encoded before it is passed to PHP.

#2 is not relevant for the query string and should not happen there.

- drupal_url_encode() gets another argument to determine the query string or not context.
- drupal_query_string_encode() uses the new argument.
- url() gets better documentation.
- code in core is corrected to properly use these API functions.

CommentFileSizeAuthor
#6 url_encoding.patch6.8 KBdrumm
#5 url_encoding.patch7.47 KBdrumm
#4 url_encoding.patch0 bytesdrumm
drupal_urlencode_0.patch7.06 KBdrumm

Comments

hass’s picture

hass commented

Is this a bugfix for the problem i reported in http://drupal.org/node/178432?

moshe weitzman’s picture

moshe weitzman commented

Steven would be a great reviewer on this if available.

moshe weitzman’s picture

moshe weitzman commented

Is there any example of a bug caused by this in core or contrib?

drumm’s picture

drumm
he/him
Location NY, US
commented
StatusFileSize
new url_encoding.patch0 bytes

Updated to HEAD.

What raised this issue for me was doing an autocomplete URL like http://example.com/autocomplete/string, where string was a URL with a double slash. The second slash got lost due to apache.

drumm’s picture

drumm
he/him
Location NY, US
commented
StatusFileSize
new url_encoding.patch7.47 KB
drumm’s picture

drumm
he/him
Location NY, US
commented
StatusFileSize
new url_encoding.patch6.8 KB
moshe weitzman’s picture

moshe weitzman commented
Priority: Critical » Normal

seems like a regular old bug to me. downgrading.

hass’s picture

hass commented

Shouldn't we get this in?

Pawel Gawlowski’s picture

Pawel Gawlowski commented

Could you apply this also to 5.x?
It works great and solves my issue http://drupal.org/node/221915

jcnventura’s picture

jcnventura
Primary language English
commented

Just FYI, the print module has had to create a work around around a double URL encoding that happens when using drupal_query_string_encode() #285496: Problem with drupal_query_string_encode().

João

fonant’s picture

fonant commented

My vote would be for a simpler fix, leaving drupal_urlencode() as it is, but only calling it when it's actually needed. The documentation for drupal_urlencode() should make it clear that it should not be used for query string encoding.

However drupal_query_string_encode() is currently broken: it should not call drupal_urlencode() but should call rawurlencode() or urlencode() instead. This bug seems to have been introduced here: http://drupal.org/node/74070.

comment.module and update.fetch.inc also need correcting to use drupal_query_string_encode() instead of drupal_urlencode() for their query strings.

Apart from these three places, drupal_urlencode() isn't called from anywhere else in Drupal core.

fonant’s picture

fonant commented

Since, in fact, the only place that Drupal core calls drupal_urlencode() correctly is within url(), perhaps the function should be removed and its code merged into the url() function?

fonant’s picture

fonant commented

See a separate issue, and patch, for the double-encoding problem for query strings:
http://drupal.org/node/310139

jcnventura’s picture

jcnventura
Primary language English
commented

Shouldn't this be marked as a duplicate then?

I prefer the patch in #310139: drupal_query_string_encode() should not call drupal_urlencode()...

Joao

pwolanin’s picture

pwolanin commented
Status: Needs review » Closed (duplicate)