Global urlencode() for menu paths

The tabs don't really disappear for me, they just both become inactive. It's really a menu issue, I suppose.

Join me, for a fun String Handling Adventure!

This issue is not that complicated, but probably surprising to most people if you don't consider all the facts.

1. The menu system works with menu paths, which is essentially the value of $_GET['q']. A menu path is not the same as an URL, because (depending on your settings), the URL may require ?q= to be prefixed.
2. When putting dynamic data in an URL, it needs to be run through urlencode(). This ensures that special characters like ?&=/ are escaped (using %XX) so that they do not disturb normal URL semantics.
3. When PHP reads in $_GET[] values, it runs it through urldecode(), so that you get the value's actual meaning rather than the way it was transmitted.
4. arg(), and the menu system, split paths by slashes.
5. Apache does not allow %2F (= urlencode('/')) in the path section of clean URLs, for myserious 'security reasons'.

This has some important consequences:

1. When you point Drupal to ?q=foo/bar%2Fbaz/bingo, PHP will urldecode() it so that $_GET['q'] == 'foo/bar/baz/bingo'. So, if you were expecting to retrieve 'bar/baz' as arg(1), this will not work: you will only get 'bar', while arg(2) will be 'baz'. So, it is impossible to use slashes in dynamic arguments (unless you avoid arg() altogether or if there are no ambiguities).
2. (the bug this issue is about) When you request a page whose path has urlencoded data in it (e.g. search/node/%2A), it will arrive in the menu system as decoded data ($_GET['q'] == 'search/node/*'). This means that the menu system cannot find the right item anymore because the paths differ.

There are two ways to address this.

The first is to access the raw, non-urldecoded data instead of using $_GET['q']. This would mean ugly parsing code using the HTTP GET request / headers. And this is not really a good idea: like HTML entities, urlencoding can be applied more than is necessary and there are sometimes ambiguities (e.g. %20 and + both represent a space), so matching of paths would still not be exact. Furthermore, it still means a bunch of urlencode() calls in various places in Drupal.

The second solution is to say that the entire menu path should be urlencoded() when it is output. Here, we make the exception that slashes should not be escaped. We already don't do this for clean URLs because of the Apache bug, and as explained above (consequence 1), it actually has no effect anyway for non-clean URLs, so we might as well leave them unescaped for aesthetical reasons.

The attached patch implements the second solution. It centralizes a bunch of urlencode() calls and makes custom menu paths more robust, for example when using special URL characters or Unicode in custom paths (which also requires urlencoding).

Steven, it's not a big deal but your patch is listing replacements for the dates in the CVS id tag. I'm only mentioning it because it makes the patch longer and a bit harder to read.

It looks like the version checked out and the one use for the diff have different date formats. I know that TortoiseCVS has problems with this, I end up having to do an extra diff/merge before building the patch to overwrite the dates.

I haven't done a lot of testing but it seems to work for me.

Here's a re-roll.

Zounds like a peek-a-boo bug in your theme. What theme is it your using and what browser is it your working with? I guess it's IE, but not sure though..

Steef

this seems to break l() when you've got a full url, i.e. 'http://example.com'. it encodes the : character.

I guess that means the code needs more work?

Also, I might be mistaken, but it looks like drupal_goto() doesn't do any urlencoding?

-    drupal_goto('search/'. drupal_urlencode($type) .'/'. drupal_urlencode(is_null($keys) ? $_POST['edit']['keys'] : $keys));
+    drupal_goto('search/'. $type .'/'. (is_null($keys) ? $_POST['edit']['keys'] : $keys));

l() and drupal_goto() both take a menu path, not an URL. Passing URLs, relative or absolute, is not supported. It only happens to work before this patch when clean URLs are on. Otherwise, ?q= would get added.

Looks good. Feel free to commit.

Committed to HEAD.

Support for external URLs is implemented over here: http://drupal.org/node/32785