decodes some special chars in a URL query #12223
Conversation
dawehner
commented
Oct 13, 2014
| Q | A |
|---|---|
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | yes |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #12200 |
| License | MIT |
| Doc PR |
| @@ -331,7 +331,7 @@ public function testUrlEncoding() | |||
| $routes = $this->getRoutes('test', new Route("/$chars/{varpath}", array(), array('varpath' => '.+'))); | |||
| $this->assertSame('/app.php/@:%5B%5D/%28%29*%27%22%20+,;-._~%26%24%3C%3E|%7B%7D%25%5C%5E%60!%3Ffoo=bar%23id' | |||
| .'/@:%5B%5D/%28%29*%27%22%20+,;-._~%26%24%3C%3E|%7B%7D%25%5C%5E%60!%3Ffoo=bar%23id' | |||
| .'?query=%40%3A%5B%5D%2F%28%29%2A%27%22+%2B%2C%3B-._%7E%26%24%3C%3E%7C%7B%7D%25%5C%5E%60%21%3Ffoo%3Dbar%23id', | |||
| .'?query=@:%5B%5D/%28%29*%27%22++,;-._%7E%26%24%3C%3E|%7B%7D%25%5C%5E%60!%3Ffoo=bar%23id', | |||
There was a problem hiding this comment.
Unencoding the = is not safe. There is a difference between ?foo%3Dbar=baz and ?foo=bar=baz!
There was a problem hiding this comment.
One means array('foo=bar' => 'baz') and the other array('foo' => 'bar=baz')
c6c528c to
c0e6436
Compare
|
You are absolute right, let's imit it to the two special cases described in the RFC |
|
@Tobion Is it good now? |
| $url .= '?'.$query; | ||
| // "/" and "?" can be left decoded for better user experience, see | ||
| // http://tools.ietf.org/html/rfc3986#section-3.4 | ||
| $url .= '?'.strtr($query, array('%2F' => '/', '%3F' => '?')); |
There was a problem hiding this comment.
I don't agree that decoding ? makes things better. For example: /path/sub?foo?bar it gets very hard to see where the query part starts. I think other chars are more worth to keep decoded like @, e.g. when you pass an email like ?to=me@example.org. So I think its ok to use $this->decodedChars BUT without = and & (in case somebody added it in a subclass). Just unset them in a local variable.
There was a problem hiding this comment.
Also the documentation of $this->decodedChars should be updated then to mention its also used for query decoding.
There was a problem hiding this comment.
Damn + is probably risky as well. Since it is the encoded form of space when using forms.
So ?foo=bar+baz will result in _GET["foo"] = 'bar baz' which is bad.
So I guess we should just decode the / for now.
|
Okay, let's just do the "/" for now. |
c0e6436 to
cd2c2e3
Compare
|
Thank you @dawehner. |
This PR was merged into the 2.6-dev branch. Discussion ---------- decodes some special chars in a URL query | Q | A | ------------- | --- | Bug fix? | yes|no (not sure :) ) | New feature? | no | BC breaks? | yes|no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #12200 | License | MIT | Doc PR | Commits ------- cd2c2e3 decodes some special chars in a URL query