[Security] use current request attributes to generate redirect url? #7325
Conversation
|
Can you explain why? |
|
@Seldaek let say I prefixed all my URLs with a |
|
Fair enough. The main issue I see is that you end up with "garbage" query params in the URL. Any params that was needed by the previous page and not needed by the new one ends up as ?foo=bar in the URL. It's usually not harmful, but not very clean either. I'm not sure what it would take to grab all the params that a route can use, and only copy those over. |
|
@Seldaek indeed, I didn't think about those query parameters... I'll try to fix this in a simple way this afternoon. |
|
@Seldaek tell me if what you think of this, it may look like a hack (which wont be acceptable). |
|
Eh I see. I can't say it's the less hacky thing I ever saw, but it might be alright. I don't think I'm the best person to take this call though.. Let's see what @fabpot thinks. |
|
|
||
| // unnecessary query string parameters must be removed from url | ||
| // (ie. query parameters that are presents in $attributes) | ||
| return $this->cleanQueryString($url, array_keys($attributes)); |
There was a problem hiding this comment.
You can just remove the query string altogether as it will always only contain attributes.
There was a problem hiding this comment.
Just for info, I have seen people generate routes with patterns like:
/foo?bar={bar} just because of some legacy reasons. So removing the
query string in all cases might create issues.
There was a problem hiding this comment.
@Seldaek: Using such a path pattern, the ? gets encoded anyway. So it wouldn't generate a query string.
There was a problem hiding this comment.
Well it was working in 2.0, but maybe they had to fix this since then.
| // unnecessary query string parameters must be removed from url | ||
| // (ie. query parameters that are presents in $attributes) | ||
| // fortunately, they all are, so we have to remove entire query string | ||
| return $this->removeQueryString($url); |
There was a problem hiding this comment.
Can you inline the method instead?
This PR was squashed before being merged into the 2.1 branch (closes #7325). Commits ------- 6575df6 [Security] use current request attributes to generate redirect url? Discussion ---------- [Security] use current request attributes to generate redirect url? Maybe we should consider to use current request attributes to generate the login/logout redirections URL? | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #5080 --------------------------------------------------------------------------- by Seldaek at 2013-03-11T08:33:37Z Can you explain why? --------------------------------------------------------------------------- by jfsimon at 2013-03-11T09:30:07Z @Seldaek let say I prefixed all my URLs with a `{domain}` var (`_locale` for instance), I'd like it to be passed to my redirected request. I guess it could lead to side effects, that's why I tagged this PR `RFC`. --------------------------------------------------------------------------- by Seldaek at 2013-03-11T09:46:33Z Fair enough. The main issue I see is that you end up with "garbage" query params in the URL. Any params that was needed by the previous page and not needed by the new one ends up as ?foo=bar in the URL. It's usually not harmful, but not very clean either. I'm not sure what it would take to grab all the params that a route can use, and only copy those over. --------------------------------------------------------------------------- by jfsimon at 2013-03-11T10:12:49Z @Seldaek indeed, I didn't think about those query parameters... I'll try to fix this in a simple way this afternoon. --------------------------------------------------------------------------- by jfsimon at 2013-03-11T14:54:31Z @Seldaek tell me if what you think of this, it may look like a hack (which wont be acceptable). --------------------------------------------------------------------------- by Seldaek at 2013-03-11T14:59:39Z Eh I see. I can't say it's the less hacky thing I ever saw, but it might be alright. I don't think I'm the best person to take this call though.. Let's see what @fabpot thinks.
| $url = $this->urlGenerator->generate($route, $attributes, $absolute); | ||
|
|
||
| // unnecessary query string parameters must be removed from url | ||
| // (ie. query parameters that are presents in $attributes) |
This PR was merged into the 2.1 branch. Commits ------- bd38483 [Security] fixed HttpUtils class tests Discussion ---------- [Security] fixed HttpUtils class tests This fixes tests broken in #7325. | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #7325
Maybe we should consider to use current request attributes to generate the login/logout redirections URL?