Skip to content

[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

Conversation

artursvonda
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

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.

@artursvonda
Copy link
Contributor Author

Bump/reminder

@fabpot
Copy link
Member

fabpot commented Aug 5, 2014

ping @Tobion

@artursvonda
Copy link
Contributor Author

ping @Tobion @fabpot
We're using expressions in access control to validate access to some resources that depend on route parameters. And there's just no easy way to get between route matching and security check to add required attributes manually. I'd really like to see this patch included in next release.

Thanks!

@Tobion
Copy link
Contributor

Tobion commented Aug 13, 2014

Seems sensible to me. But RedirectableUrlMatcher needs to be changed as well. Also please add a test for this.

@artursvonda
Copy link
Contributor Author

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.

@artursvonda
Copy link
Contributor Author

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;
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair enough

Copy link
Member

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

@fabpot
Copy link
Member

fabpot commented Sep 24, 2014

👍 to be merged in 2.3.

ping @symfony/deciders

@fabpot
Copy link
Member

fabpot commented Sep 25, 2014

@artursvonda Can you submit a new PR on 2.3? Thanks.

@Tobion
Copy link
Contributor

Tobion commented Sep 25, 2014

@fabpot I would rather consider this a new feature.

@fabpot
Copy link
Member

fabpot commented Sep 25, 2014

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]);
Copy link
Contributor

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

Copy link
Contributor

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));

@Tobion
Copy link
Contributor

Tobion commented Sep 25, 2014

@fabpot not yet, its buggy

@artursvonda artursvonda force-pushed the bugfix-add-matched-params-to-redirect branch from 3fae452 to cae9c1a Compare October 1, 2014 09:57
@artursvonda
Copy link
Contributor Author

ping @fabpot, @Tobion
Updated the code to fix the bug + rebased against master

@fabpot
Copy link
Member

fabpot commented Jan 25, 2016

@Tobion Does it look good now?

@artursvonda
Copy link
Contributor Author

I'll rebase, fix tests and push updated version

@artursvonda artursvonda force-pushed the bugfix-add-matched-params-to-redirect branch from cae9c1a to f20ad57 Compare January 26, 2016 11:51
@fabpot
Copy link
Member

fabpot commented Mar 4, 2016

@artursvonda Can you close this one and submit a new one on the 2.3 branch instead? Thanks.

@artursvonda
Copy link
Contributor Author

Sure! Will try to do it today.

@artursvonda artursvonda closed this Mar 4, 2016
@artursvonda artursvonda deleted the bugfix-add-matched-params-to-redirect branch March 4, 2016 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants