Skip to content

[Routing] remove route query param when it is the same as default #6905

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

gimler
Copy link
Contributor

@gimler gimler commented Jan 29, 2013

Q A
Bug fix? yes
New feature? no
BC breaks? maybe not shure i think this is a bug
Deprecations? no
Tests pass? yes
License MIT

i think the original testcase for query params is broken see:
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php#L294

the generate method from the UrlGenerator removes parameter that has the same value as the default see:
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php#L344

but this is not the case for query parameters

@Tobion
Copy link
Contributor

Tobion commented Jan 30, 2013

It's a bc break. And I think fixing this inconsistency would be better the other way round: see #5410

@lsmith77
Copy link
Contributor

lsmith77 commented Feb 7, 2013

i agree with @Tobion as the approach in #5410 would prevent breaking bookmarks if the defaults change. at the same time this way urls might be needlessly long, which can cause confusions when users copy urls to each other. so ideally it would be up to the developer the decide what behavior they want.

@Tobion
Copy link
Contributor

Tobion commented Feb 7, 2013

Exactly the developer should have to possibility to decide. Which is what #5410 makes possible. Currently the developer is forced in one way (which is even inconsistent between placeholders and query params).

@fabpot
Copy link
Member

fabpot commented Apr 9, 2013

Closing in favor of the discussion in #5410

@fabpot fabpot closed this Apr 9, 2013
@gimler gimler deleted the fix_route_query_requirements branch January 20, 2021 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants