symfony/symfony — pull request #3780 by vicb (2012-04-04 13:38:58+00:00)
Title: [RFC] Fix issues with url decoding

Related: #2324, #2963, #2962, #2579

### This PR fixes two issues:

* `+` in paths were turned to " " by `urldecode()`
* `urldecode()` was called a few times (and a different number of times according to which part of the path was handled, see #2962 for details).

### BC Breaks:

* `Request::getPathInfo()`, `Request::getBaseUrl()` and `Request::getBasePath` now return the raw (encoded) path (vs a decoded path before this PR). You should check any calls to these methods in your code and wrap them in `rawurldecode()` if needed.
* The `UrlMatcher` now decodes the URL only once (i.e. variable are no more decoded twice) and use `rawurldecode()` to support `+`.

### Notes:

* @arnaud-lb, the first commit is based on your #2963 so I put your name for the commit,

### Comment history from the original PR:

@vicb

**The state before this PR:**

* getRequestUri() returns an **encoded** value
* getPathInfo() returns a **decoded** value
* getBaseUrl() returns a **decoded** value
* getBasePath() returns a **decoded** value

The decoded value is wrong as `urldecode` is used in place of `rawurldecode` turning `+` into a space character (#2324).

The matcher starts by urldecoing the path (it is already decoded as explained right before) and then urldecodes each variable one more time.

We end up with a path being decoded twice and variables being decoded three times.

`Request::getUri()` calls both `getBaseUrl()` and `getPathInfo()` so that the return URI is **decoded**.

**The state after the PR:**

* getRequestUri() returns an **encoded** value
* getPathInfo() returns an **encoded** value
* getBaseUrl() returns an **encoded** value
* getBasePath() returns an **encoded** value

We are consistent and we have the raw values everywhere - there is no (easy) way to get the encoded value back once it has been decoded as it is done in the current code.

The matcher relies on an encoded value and decode the value only once (using `rawurldecode` to support `+`s).

So basically this PR:

* fix a bug - URL with `+` are now supported,
* makes paths consistent - encoded values everywhere, including `getUri()`
* makes variables consistent: they are decoded only once - the same as query string parameters.

There are some BC breaks:

* getPathInfo() returns an encoded value vs a decoded one before,
* getBaseUrl() returns an encoded value vs a decoded one before.
* getBasePath() returns an encoded value vs a decoded one before.

Any code relying on the output of one of the 2 previous methods should be checked and upgraded if needed. I am interested in the use cases where your code need to be updated.

@Seldaek

I checked a few projects and this is what I found (usage of getPathInfo & getBaseUrl):

- One use case of getPathInfo to check if the url started with `/something/` which is a prefix used for all "overlay" content which had to be treated differently somewhere => most likely unaffected
- One use case for checking path prefixes by regex in our [CorsBundle](https://github.com/nelmio/NelmioCorsBundle/blob/master/EventListener/CorsListener.php#L52-56) => potentially affected depending on the complexity of regexes I'd say

@vicb

Thanks @Seldaek for reporting the use cases. You second case would be solved by `rawurldecode`ing the path info which is a minimal change.

And in general I think we have to expand to doc to specify the url format that should be used.