[Routing] UrlGenerator: fixed missing query param and some ignored requirements #5181
Conversation
|
ping @fabpot |
|
@fabpot I think it's important to merge this before 2.1 final. |
| $this->assertEquals('/app.php/testing//bar', $url); | ||
| // This must raise an exception because the default requirement for "foo" is "[^/]+" which is not met with these params. | ||
| // Generating path "/testing//bar" would be wrong as matching this route would fail. | ||
| $this-& 8000 gt;getGenerator($routes)->generate('test', array(), false); |
There was a problem hiding this comment.
Yes, but the test is wrong and and even if it breaks bc, it was not really useful before because it generated a URL that doesnt get matched by the route. And thats not what should happen which is why we have the requirements check on url generation in the first place.
There was a problem hiding this comment.
Btw, with #5187 it would be possible to generate the URL as before (no bc break) although it doesn't really make sense in this case.
There was a problem hiding this comment.
Yep, I just wanted to draw your attention to #5187, so it gets merged too ;)
There was a problem hiding this comment.
Haha, the first developer issue that is revealed by this change: see #5396
Commits ------- 0706d18 [Routing] fixed 4 bugs in the UrlGenerator Discussion ---------- [Routing] UrlGenerator: fixed missing query param and some ignored requirements This was pretty hard to figure out. I could fix 4 bugs and refactor the code to safe 2 variables and several assignments. Sorry for doing this in one commit, but they were highly interdependent. See the added tests for what was fixed. The most obvious bug was that a query param was ignored if it had by accident the same name as a default param (but wasn't used in the path). In 3 cases it generated the wrong URL that wouldn't match this route. The generator wrongly ignored either the requirements or the passed parameter. I had to adjust one test that was asserting something wrong (see comments). --------------------------------------------------------------------------- by Tobion at 2012-08-13T14:22:35Z ping @fabpot --------------------------------------------------------------------------- by Tobion at 2012-08-29T17:53:07Z @fabpot I think it's important to merge this before 2.1 final.
|
I've just reverted this pull request as it introduces some regressions? It was a very bad idea to merge this so close to the release of 2.1 and I'm the one to blame. Sorry about that. @Tobion As apparently it is not possible to reopen a pull request, can you re-submit it again, so that it is not lost? Thanks and sorry again for the trouble. |
|
What regressions does it introduce? I cannot reopen as i already deleted the branch locally and remote. |
|
@fabpot What branch should i reopen the PR against? master or 2.1? Same questions applies to the other reverted PRs. |
|
On the master branch please. Thanks. |
This PR was merged into the master branch. Commits ------- e22823b [Routing] context params should have higher priority than defaults 16c1b01 [Routing] fixed 4 bugs in the UrlGenerator Discussion ---------- [Routing] UrlGenerator: fixed missing query param and some ignored requirements reopened version of #5181 (cherry-picked) On top of that I fixed #5437 in my code and added test case. --------------------------------------------------------------------------- by Tobion at 2012-10-03T18:41:45Z @fabpot ping --------------------------------------------------------------------------- by fabpot at 2012-10-03T18:43:43Z IIUC, #5437 is a regression in 2.1 and should be done in 2.1, no? --------------------------------------------------------------------------- by Tobion at 2012-10-03T18:46:42Z It's not in 2.1 anymore as you reverted the PR. #5437 is not valid currently and can be closed. So this can either be merged in master or 2.1 whatever you prefer.
This was pretty hard to figure out. I could fix 4 bugs and refactor the code to safe 2 variables and several assignments. Sorry for doing this in one commit, but they were highly interdependent.
See the added tests for what was fixed. The most obvious bug was that a query param was ignored if it had by accident the same name as a default param (but wasn't used in the path).
In 3 cases it generated the wrong URL that wouldn't match this route. The generator wrongly ignored either the requirements or the passed parameter. I had to adjust one test that was asserting something wrong (see comments).