Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Routing] Generate URLs in compliance with PHP_QUERY_RFC3986#19639
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
jameshalsall commentedAug 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 |
| if ($extra &&$query =http_build_query($extra,'','&')) { | ||
| // extract fragment | ||
| $fragment =isset($extra['_fragment']) ?$extra['_fragment'] :''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
variable$fragment is not used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
will remove, bad upstream merge
Tobion commentedAug 17, 2016
This is not a bug fix but a new feature to me as it changes the bahavior. |
jameshalsall commentedAug 17, 2016
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. |
jameshalsall commentedAug 17, 2016
Other PR for reference (ignore the number of commits, I switched target branch by mistake)#19637 |
fabpot commentedAug 18, 2016
Thank you@jameshalsall. |
| publicfunctiontestUrlEncoding() | ||
| { | ||
| if (defined('PHP_QUERY_RFC3986')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why was it merged with this that is not needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
it is needed, that was only added in PHP 5.4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
it was merged in master which requires 5.5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
it was merged to 2.7
jameshalsallAug 18, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
oh, well it was targeted at 2.7 - I can see Fabien merged to master instead - I will send another PR shortly to remove thedefined() checks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Because I made the change after the merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No you didn't do it for this file. Seehttps://github.com/symfony/symfony/blob/master/src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php#L328
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
oops, sorry about that. fixed now. Good catch as always :)
xabbuh commentedSep 6, 2016
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.txtCommits-------9e73887 [Routing] Mention minor BC break about UrlGenerator & query strings