Skip to content
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

[Routing] add query param if value is different from default #18280

Merged

Conversation

Projects
None yet
5 participants
@Tobion
Copy link
Member

commented Mar 23, 2016

Q A
Branch? 2.3
Bug fix? yes
New feature? yes
BC breaks? most likely not
Deprecations? no
Tests pass? no
Fixed tickets #10940, #18111, #18035
License MIT
Doc PR -
@Tobion

This comment has been minimized.

Copy link
Member Author

commented Mar 23, 2016

I hope this does not break the i18n routing bundle tests contrary to before (see #6814 and https://travis-ci.org/schmittjoh/JMSI18nRoutingBundle/jobs/3229075).
This time only query params are added that are not equal to the default value. I think this is consistent with the optional path placeholders which are also only added if different from default.

@fabpot

This comment has been minimized.

Copy link
Member

commented Apr 15, 2016

Thank you @Tobion.

@fabpot fabpot merged commit 1ef2edf into symfony:2.3 Apr 15, 2016

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Apr 15, 2016

bug #18280 [Routing] add query param if value is different from defau…
…lt (Tobion)

This PR was merged into the 2.3 branch.

Discussion
----------

[Routing] add query param if value is different from default

| Q             | A
| ------------- | ---
| Branch?       | 2.3
| Bug fix?      | yes
| New feature?  | yes
| BC breaks?    | most likely not
| Deprecations? | no
| Tests pass?   | no
| Fixed tickets | #10940, #18111, #18035
| License       | MIT
| Doc PR        | -

Commits
-------

1ef2edf [Routing] add query param if value is different from default

@Tobion Tobion deleted the Tobion:fix-10940-add-query-param-if-different-from-default branch Apr 16, 2016

@leofeyer

This comment has been minimized.

Copy link
Contributor

commented May 4, 2016

This is a BC break, I'm afraid. If you had not removed lines 296 and 297 of the UrlGeneratorTest class, you would have noticed. These are the former assertions:

public function testQueryParamSameAsDefault()
{
    $routes = $this->getRoutes('test', new Route('/test', array('default' => 'value')));

    $this->assertSame('/app.php/test', $this->getGenerator($routes)->generate('test', array('default' => 'foo')));
    $this->assertSame('/app.php/test', $this->getGenerator($routes)->generate('test', array('default' => 'value')));

We have relied on the first assertion being true in Contao, which is now no longer the case.

@Tobion

This comment has been minimized.

Copy link
Member Author

commented May 4, 2016

What is your use-case for this behavior?

@leofeyer

This comment has been minimized.

Copy link
Contributor

commented May 4, 2016

Our use case is pretty much what is in the first deleted assertion. We want the router to ignore the _locale request attribute if there is a default _locale route attribute. It worked like a charm up to Symfony 3.0.4 and it does no longer in Symfony 3.0.5, because now the ?_locale=xx query string is appended.

But actually the use-case should not matter. The changes affect the API in an incompatible way, which the deleted assertions would have shown if not removed. :)

@leofeyer

This comment has been minimized.

Copy link
Contributor

commented May 4, 2016

Also, I'm not implying that ignoring the _locale request attribute if there is a default _locale route attribute is the right way to solve the problem. It might be not and we are currently developing a replacement solution.

Still the router now behaves differently than in Symfony 3.0.4.

fabpot added a commit that referenced this pull request Jun 17, 2016

bug #19088 [Routing] treat fragment after resolving query string para…
…ms (xabbuh)

This PR was merged into the 3.2-dev branch.

Discussion
----------

[Routing] treat fragment after resolving query string params

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

The implementation from #12979 led to a conflict with #18280 which was merged in the meantime.

Commits
-------

7475aa8 treat fragment after resolving query string params
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.