8000 [Routing] Current context overridden with defaults in Routing · Issue #5437 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content
8000

[Routing] Current context overridden with defaults in Routing #5437

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
mstaessen opened this issue Sep 5, 2012 · 1 comment · Fixed by #5445
Closed

[Routing] Current context overridden with defaults in Routing #5437

mstaessen opened this issue Sep 5, 2012 · 1 comment · Fixed by #5445

Comments

@mstaessen
Copy link

I noticed that the current Route context is overridden when generating new routes This behaviour is the result of line 165 in https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Routing/Generator/UrlGenerator.php#L136.

$mergedParams = array_replace($this->context->getParameters(), $defaults, $parameters);

I think this is a problem because when you work with locales, you need to explicitly add the locale param to each route. A more sensible idea would be to override the defaults with the current context, and override that result with additional params, like so:

$mergedParams = array_replace($defaults, $this->context->getParameters(), $parameters);

In this case, the locale is kept for each generated route, unless you explicitly specify it.
@Tobion Tobion mentioned this issue Sep 7, 2012
Merged
@Tobion
Copy link
Contributor
Tobion commented Sep 7, 2012

Sorry about the error. There was no test case that this was the intended behavior. I fixed it in the referenced PR.

@fabpot fabpot closed this as completed in e22823b Oct 3, 2012
fabpot added a commit that referenced this issue Oct 3, 2012
@fabpot
merged branch Tobion/strictrequirements2 (PR #5445)
7276b2d 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Routing] UrlGenerator: fixed missing query param and some ignored requirements Tobion/symfony
3 participants
@fabpot @mstaessen @Tobion
0