[Routing] Add matched and default parameters to redirect responses #11380
Conversation
|
Bump/reminder |
|
ping @Tobion |
|
ping @Tobion @fabpot Thanks! |
|
Seems sensible to me. But |
|
I added a test and fixed up matchCollection method in UrlMatcher to add matched parameters to route. I'm not sure that was the correct way to do it but it was the easiest place to add it. Let me know if something's still missing. I'm not that familiar with the internals. |
|
Ping @Tobion |
| @@ -29,7 +29,8 @@ public function match($pathinfo) | |||
|
|
|||
| // foo | |||
| if (0 === strpos($pathinfo, '/foo') && preg_match('#^/foo/(?P<bar>baz|symfony)$#s', $pathinfo, $matches)) { | |||
| return $this->mergeDefaults(array_replace($matches, array('_route' => 'foo')), array ( 'def' => 'test',)); | |||
| $ret = $this->mergeDefaults(array_replace($matches, array('_route' => 'foo')), array ( 'def' => 'test',)); | |||
| return $ret; | |||
There was a problem hiding this comment.
Is it possible to tweak the code to avoid creating a temp var when not needed?
There was a problem hiding this comment.
Possible but would complicate the Dumper. I opted for this approach because it's better to have readable Dumper than dumped file. Performance-wise I don't think this affects anything in a meaningful way.
There was a problem hiding this comment.
I'm pretty sure OPCache will be able to drop the assignment anyway in this case
|
👍 to be merged in 2.3. ping @symfony/deciders |
|
@artursvonda Can you submit a new PR on 2.3? Thanks. |
|
@fabpot I would rather consider this a new feature. |
|
ok, fair enough, I'm going to merge it into master then. |
| @@ -157,7 +157,9 @@ protected function matchCollection($pathinfo, RouteCollection $routes) | |||
| $status = $this->handleRouteRequirements($pathinfo, $name, $route); | |||
|
|
|||
| if (self::ROUTE_MATCH === $status[0]) { | |||
| return $status[1]; | |||
| $attributes = array_replace($matches, $hostMatches, $status[1]); | |||
There was a problem hiding this comment.
$status[1] can be null which will create an error http://3v4l.org/bLISs
There was a problem hiding this comment.
I really wonder about the current implementation (without your changes).
When ROUTE_MATCH then it only returned the handleRouteRequirements params without the defaults and without the route name (_route)?!
And when its neither ROUTE_MATCH nor REQUIREMENT_MISMATCH only then it did return $this->getAttributes($route, $name, array_replace($matches, $hostMatches));
|
@fabpot not yet, its buggy |
artursvonda
force-pushed
the
bugfix-add-matched-params-to-redirect
branch
from
3fae452 to
cae9c1a
Compare
|
@Tobion Does it look good now? |
|
I'll rebase, fix tests and push updated version |
artursvonda
force-pushed
the
bugfix-add-matched-params-to-redirect
branch
from
cae9c1a to
f20ad57
Compare
|
@artursvonda Can you close this one and submit a new one on the 2.3 branch instead? Thanks. |
|
Sure! Will try to do it today. |
I ran into an issue recently. We're using security expressions and we're matching against route params. Since redirect didn't add matched route params to redirect response, firewall wasn't matching rule and was throwing AccessDenied exception. This patch adds matched and defaults to redirect params.
Code could be improved to return straight away if there won't be redirect.