-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Routing] Redirect from trailing slash to no-slash when possible #26283
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
$pathinfo = '/' !== $pathinfo[-1] ? $pathinfo.'/' : substr($pathinfo, 0, -1); | ||
$ret = $this->doMatch($pathinfo); | ||
|
||
return $this->redirect($pathinfo, $ret['_route'] ?? null) + $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.
$ret['_route']
is always set isn't it? So there would be no need for ??
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.
correct, fixed
@@ -294,7 +312,6 @@ private function compileDynamicRoutes(RouteCollection $collection, bool $support | |||
'mark' => 0, | |||
'markTail' => 0, | |||
'supportsRedirections' => $supportsRedirections, |
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 suggest to set supportsRedirections
as a private property on the PhpMatcherDumper. Then we don't need to forward the variable to all the methods as an argument.
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.
done
I think it would make sense to add a config for enabling/disabling redirection behavior on the framework.router config. This config would get passed to the PhpMatcherDumper. |
5665fff
to
5e42162
Compare
I'm not sure it makes sense to configure this globally, that's my main objection. Then about adding a per-route option, it doesn't make sense either IMHO: API clients should just hit the correct URL. Trailing slash is focused at browsers and users typing in the address bar. For benchs, I wouldn't care much :) |
94ca7d9
to
1cbd15d
Compare
1cbd15d
to
69a4e94
Compare
I would also suggest to enable / disable redirection behaviour by route. routes.yaml controllers:
resource: '../src/Controller/'
type: annotation
prefix: /{_locale}
add_trailing_slash: true|false // true by default
remove_trailing_slash: true|false // false by default it would be also great if this behaviour is available in the @route Annotation /**
* @Route("/xxx", name="my_route", remove_trailing_slash=true)
*/
public function xxxAction() |
return $ret; | ||
} | ||
if ('/' !== $pathinfo && in_array($this->context->getMethod(), array('HEAD', 'GET'), true)) { | ||
$pathinfo = '/' !== $pathinfo[-1] ? $pathinfo.'/' : substr($pathinfo, 0, -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.
The routing component does not dintinguish between /
and encoded %2F
. So I think the redirection should handle both the same as well. That means we would need to rawurldecode in match and not only in doMatch.
E.g. if you try to match http://symfony.com/blog/category/documentation%2F
-> fail because wrong traling slash.
It should then try to match http://symfony.com/blog/category/documentation
and not http://symfony.com/blog/category/documentation%2F/
as now.
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.
That's not how RedirecteableUrlMatcher
behaves. Tests also expect the original encoding be preserved. In the end, I think we should not try harder handling these edge cases. The goal of trailing slash redirections is helping navigating via the browser bar. And webservers & browsers already normalize this for us most of the time. Let's keep things simple.
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.
Fine for me.
@sebastianblum I would MUCH prefer NOT adding any new option to teach to anyone. If you write an API, just hit the correct path, as previously said. |
@nicolas-grekas I totally agree and I prefer the urls without trailing slash and I don't need the redirect from xxx/ to xxx. If symfony implements a configuration option, I would disable it. In my opinion, we can ignore the feature of this pr and if users need the redirect from xxx/ to xxx, they can configure it using the redirect controller. Maybe we should deprecate the auto redirect from xxx to xxx/ and remove this functionality in symfony 5. Like the other case, these redirects are only a few redirects in routes.yml. |
Thank you Nicolas. |
…n possible (nicolas-grekas) This PR was merged into the 4.1-dev branch. Discussion ---------- [Routing] Redirect from trailing slash to no-slash when possible | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #26207 | License | MIT | Doc PR | - Implemented as suggest by @Tobion in #26059 (comment) When a route for `/foo` exists but the request is for `/foo/`, we now redirect. (this complements the flipped side redirection, which already exists.) Commits ------- 69a4e94 [Routing] Redirect from trailing slash to no-slash when possible
Implemented as suggest by @Tobion in #26059 (comment)
When a route for
/foo
exists but the request is for/foo/
, we now redirect.(this complements the flipped side redirection, which already exists.)