Skip to content

[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

Merged
merged 1 commit into from
Feb 24, 2018

Conversation

nicolas-grekas
Copy link
Member

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

$pathinfo = '/' !== $pathinfo[-1] ? $pathinfo.'/' : substr($pathinfo, 0, -1);
$ret = $this->doMatch($pathinfo);

return $this->redirect($pathinfo, $ret['_route'] ?? null) + $ret;
Copy link
Contributor

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 ??

Copy link
Member Author

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,
Copy link
Contributor

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@Tobion
Copy link
Contributor

Tobion commented Feb 23, 2018

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.
For frontend apps it's usually the best to enable that behavior for good user experience. But APIs might want to disable that because often clients cannot deal with redirections anyway.
The other advantage is that this allows to compare the Symfony router with other routers that don't support redirections. Otherwise it skews the benchmark result for non-matching routes.

@nicolas-grekas nicolas-grekas force-pushed the route-simple-slash branch 2 times, most recently from 5665fff to 5e42162 Compare February 23, 2018 11:51
@nicolas-grekas
Copy link
Member Author

add a config for enabling/disabling redirection behavior

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

@nicolas-grekas nicolas-grekas force-pushed the route-simple-slash branch 2 times, most recently from 94ca7d9 to 1cbd15d Compare February 23, 2018 12:21
@sebastianblum
Copy link

I would also suggest to enable / disable redirection behaviour by route.
Direct from xxx to xxx/ should be enabled by default (otherwise it is a bc break) but for an api I would have the possibility to disable the redirects.
On the other hand, if I use urls without a trailing slash, I would prefer the possibility to enable the redirect from xxx/ to xxx.

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

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.

Copy link
Member Author

@nicolas-grekas nicolas-grekas Feb 23, 2018

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine for me.

@nicolas-grekas
Copy link
Member Author

possibility to enable/disable the redirect from xxx/ to xxx

@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.

@sebastianblum
Copy link

@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.

@Tobion
Copy link
Contributor

Tobion commented Feb 24, 2018

Thank you Nicolas.

@Tobion Tobion merged commit 69a4e94 into symfony:master Feb 24, 2018
Tobion added a commit that referenced this pull request Feb 24, 2018
…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
@nicolas-grekas nicolas-grekas deleted the route-simple-slash branch February 26, 2018 18:23
@fabpot fabpot mentioned this pull request May 7, 2018
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.

4 participants