-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[Routing] Add matched and default parameters to redirect responses #11380
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.
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
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to tweak the code to avoid creating a temp var when not needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$status[1] can be null which will create an error http://3v4l.org/bLISs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
3fae452
to
cae9c1a
Compare
@Tobion Does it look good now? |
I'll rebase, fix tests and push updated version |
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.