10000 Undocumented BC break in Routing · Issue #6814 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Undocumented BC break in Routing #6814

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.

Sign up for GitHub

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

Jump to bottom
Closed
schmittjoh opened this issue Jan 20, 2013 · 12 comments
Closed

Undocumented BC break in Routing #6814

schmittjoh opened this issue Jan 20, 2013 · 12 comments
Labels
BC Break Routing

Comments

@schmittjoh
Copy link
Contributor

Someone submitted a patch for JMSI18nRoutingBundle to make the bundle compatible with Symfony 2.2. After reading through the upgrade file, I couldn't find any mention of this change though.

So, the question is whether this was actually an intended BC break which is just not documented, or an unintentional BC break?

see schmittjoh/JMSI18nRoutingBundle#80
@Tobion
Copy link
Contributor
Tobion commented Jan 20, 2013

I don't see what could have changed the behavior for this PR in 2.2. And I'm not really sure what this PR solves.

@stof
Copy link
Member
stof commented Jan 20, 2013

yeah, a failing testcase would probably help to understand the issue, but the PR does not seem to add one

@kachkaev
Copy link

That PR fixes 4 test cases instead of breaking them, see bundle build results before it: https://travis-ci.org/schmittjoh/JMSI18nRoutingBundle/jobs/3229075

The tests started failing immediately after switching to symfony 2.2: https://travis-ci.org/schmittjoh/JMSI18nRoutingBundle/builds

@stof
Copy link
Member
stof commented Jan 24, 2013

@kachkaev I'm talking about failing testcases for the routing component showing the BC breaks

@schmittjoh
Copy link
Contributor Author

I think it would make more sense if @Tobion, or someone familar with the changes that were made to the routing component takes a look.

The bundle has not changed, and you can run its unit tests or even copy them to the core component if you like to see the regression.

@schmittjoh schmittjoh mentioned this issue Mar 8, 2013
Closed
@tiagojsag
Copy link
Contributor

I've been investigating the above mentioned JMSI18nRoutingBundle issue, and I believe I found the possible source of the problem:

symfony/routing@c66d1f9#diff-0

It think the problem is on the UrlGenerator class:

196   -        $extra = array_diff_key($originParameters, $variables, $defaults);
190  +        $extra = array_diff_key($parameters, $variables);

Reverting the change to this line and recreating the $originParameters like it was before this commit seem to solve the issue with JMSI18nRoutingBundle. Of course that this doesn't mean that @Tobion commit is to blame, but it was all I could come up with. Hope it helps

@bendavies
Copy link
Contributor

@Tobion any thoughts?

@Tobion
Copy link
Contributor
Tobion commented Mar 18, 2013

@tiagojsag is right. This commit changed the behavior. But IMO it's a bug fix as the added test there shows:

public function testQueryParamSameAsDefault()
{
     $routes = $this->getRoutes('test', new Route('/test', array('default' => 'value')));
     $this->assertSame('/app.php/test?default=foo', $this->getGenerator($routes)->generate('test', array('default' => 'foo')));
     $this->assertSame('/app.php/test?default=value', $this->getGenerator($routes)->generate('test', array('default' => 'value')));
     $this->assertSame('/app.php/test', $this->getGenerator($routes)->generate('test'));
}

Previously the default parameter was not given in the query params as soon as a default with the same name existed. But that doesn't make sense to me because the query params and the defaults for placeholders are not related. Just image this example a page default. I would not be able to add a query for a different page just because I defined a default for it.

@acasademont
Copy link
Contributor

Athough I agree that test proves the bug...some of us used it as a feature, it was useful to tag routes. For example I have some controllers that display product lists and all those methods had a defaults: {list: true} to flag them and then do some common things on a RequestListener. Maybe not the better option, but it worked :p

@Tobion I also think that this should be made explicit in the changelog, the distinction between bug/feature is not very clear on this one.

8000
@schmittjoh
Copy link
Contributor Author

@Tobion, was there a practical use-case that you fixed here? The example with "page" is pretty common, but never caused any problems for me with the old system. Probably, because "page" is a query parameter anyway and you do not rely on the routing system there. In general, having a route placeholder and a query parameter with the same name seems like an edge case to me.

If this is more a theoretical fix, then I think we should roll back for now until we have found a good upgrade path because there does not seem to be one yet. It seems more that the defaults array is used for used for multiple purposes, and we might need to split it into different arrays with more specific meanings.

@schmittjoh
Copy link
Contributor Author

@fabpot, could you share your view/make a decision on this one? Basically, we have a behavior change on a method tagged with @api for a non-security issue. According to our own rules, this should not happen and since this was not documented my guess is that it was not intentional.

If this is not reverted, then I'd love to see some documented upgrade path which at the moment is not available unfortunately.

@schmittjoh schmittjoh mentioned this issue Mar 23, 2013
Merged
@schmittjoh
Copy link
Contributor Author

I've created a pull request which makes viewing the change in question a bit easier.

fabpot added a commit that referenced this issue Mar 23, 2013
@fabpot
merged branch schmittjoh/routingFix (PR #7458)
9885798 
This PR was merged into the 2.2 branch.

Discussion
----------

Reverts behavior change to UrlGenerator

I do not want to talk much about the behavior change and whether it makes sense or not because I think it does not matter in this situation anyway.

The ``generate`` method is tagged with ``@api``, there is no security issue that was fixed. According to the rules set forth at http://symfony.com/doc/current/book/stable_api.html, the semantics of such a method must not be changed.

There is some more discussion in #6814 and the commit changing the behavior is this one: symfony/routing@c66d1f9#diff-0

Commits
-------

a765375 reverts some behavior changes made in c66d1f9de30fd1b6a86cca10dd79d12c9ba9ff25
@Tobion Tobion mentioned this issue Mar 23, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
BC Break Routing
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@schmittjoh @stof @kachkaev @Tobion @bendavies @acasademont @tiagojsag
0