[Routing] Add matched and default parameters to redirect responses #19037
Conversation
b6acff1 to
76a03c4
Compare
|
Tests do not pass. |
|
|
||
| return $this->redirect($pathinfo.'/', null); | ||
| return array_replace($parameters, $this->redirect($pathinfo.'/', isset($parameters['_route']) ? $parameters['_route'] : null)); |
There was a problem hiding this comment.
Passing the _route to the redirect method makes the tests fail in the FrameworkBundle: https://github.com/symfony/symfony/pull/19037/files#diff-7b7153654c7ee1d563e8421f3f9be20e
I could extract this fix as a bugfix as the current behavior is not consistent with the dumped matcher, see https://github.com/symfony/symfony/pull/19037/files#diff-3b72491a9ba1cff58442b845ae837eb3L292 which passed the route name as second argument to redirect.
But I don't know how to fix the frameworkbundle test which will also be run against the lowest allowed routing version, right? So I need to increase the routing dependency in https://github.com/symfony/symfony/blob/2.7/src/Symfony/Bundle/FrameworkBundle/composer.json#L28 but to a version that is not released yet?
There was a problem hiding this comment.
right, @nicolas-grekas can you help @Tobion with the best composer changes to do here?
There was a problem hiding this comment.
@nicolas-grekas I could still use your help here
There was a problem hiding this comment.
to a version that is not released yet
yes, that works, wouldn't be the first time
There was a problem hiding this comment.
So "symfony/routing": "~2.7.21|~2.8.14|^3.1.7"? I don't see how composer will know to install the latest 2.7 branch then as 2.7.21 is not released yet. And even if it does, when the PR is not merged yet, the fix won't be installed. Thus the tests cannot pass before they are merged?
There was a problem hiding this comment.
Sorry, late answer. Yes, it will work, composer will know thanks to the branch-alias in composer.json
There was a problem hiding this comment.
The PR doesn't need to be merged: a local composer repository is made on travis to simulate the merge before it really happens. See .github/build-packages.php
|
@Tobion Can you finish this one? |
…ct responses (artursvonda, Tobion) This PR was merged into the 3.4 branch. Discussion ---------- [Routing] Add matched and default parameters to redirect responses | Q | A | | --- | --- | | Branch? | master | | Bug fix? | no | | New feature? | yes | | BC breaks? | no | | Deprecations? | no | | Tests pass? | yes | | Fixed tickets | #18012, #19037 | | License | MIT | | Doc PR | | Finished #18012 and #19037 Commits ------- 4c1fdd4 [Routing] also add matched params for redirect due to trailing slash dc3f7a9 [Routing] Add matched and default parameters to redirect responses
Finished #18012