[Routing] Generate URLs in compliance with PHP_QUERY_RFC3986 #19639
Conversation
jameshalsall
commented
Aug 16, 2016
| Q | A |
|---|---|
| Branch? | 2.7 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no? |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #19625 |
| License | MIT |
| Doc PR |
| @@ -268,7 +268,17 @@ protected function doGenerate($variables, $defaults, $requirements, $tokens, $pa | |||
| return $a == $b ? 0 : 1; | |||
| }); | |||
|
|
|||
| if ($extra && $query = http_build_query($extra, '', '&')) { | |||
| // extract fragment | |||
| $fragment = isset($extra['_fragment']) ? $extra['_fragment'] : ''; | |||
There was a problem hiding this comment.
will remove, bad upstream merge
|
This is not a bug fix but a new feature to me as it changes the bahavior. |
|
Yeah I opened this against master originally as I wasn't 100% sure whether it would break BC, technically it shouldn't because both URL variations are valid, but it there may be people running assertions on generated URL formats which it could break. |
|
Other PR for reference (ignore the number of commits, I switched target branch by mistake) #19637 |
|
Thank you @jameshalsall. |
| @@ -325,18 +325,24 @@ public function testGenerateWithSpecialRouteName() | |||
|
|
|||
| public function testUrlEncoding() | |||
| { | |||
| if (defined('PHP_QUERY_RFC3986')) { | |||
There was a problem hiding this comment.
Why was it merged with this that is not needed?
There was a problem hiding this comment.
it is needed, that was only added in PHP 5.4
There was a problem hiding this comment.
it was merged in master which requires 5.5
There was a problem hiding this comment.
it was merged to 2.7
There was a problem hiding this comment.
oh, well it was targeted at 2.7 - I can see Fabien merged to master instead - I will send another PR shortly to remove the defined() checks
There was a problem hiding this comment.
Because I made the change after the merge.
There was a problem hiding this comment.
No you didn't do it for this file. See https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php#L328
There was a problem hiding this comment.
oops, sorry about that. fixed now. Good catch as always :)
|
What about documenting this in the upgrade file? It may break some users' functional tests. |
…ery strings (ogizanagi) This PR was merged into the 3.2 branch. Discussion ---------- [Routing] Mention minor BC break about UrlGenerator & query strings | Q | A | ------------- | --- | Branch? | 3.2 | Bug fix? | no | New feature? | no | BC breaks? | no, mention one | Deprecations? | no | Tests pass? | yes | Fixed tickets | N/A | License | MIT | Doc PR | N/A --- Refs: - https://github.com/javiereguiluz/EasyAdminBundle/pull/1421#issuecomment-266201441 - #19639 - http://php.net/manual/en/function.http-build-query.php - https://www.ietf.org/rfc/rfc3986.txt Commits ------- 9e73887 [Routing] Mention minor BC break about UrlGenerator & query strings