8000 [Proposal] Remove the trailing slash in URL · Issue #58 · laravel/framework · GitHub
[go: up one dir, main page]

Skip to content

[Proposal] Remove the trailing slash in URL #58

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

Closed
bpierre opened this issue Jan 15, 2013 · 21 comments
Closed

[Proposal] Remove the trailing slash in URL #58

bpierre opened this issue Jan 15, 2013 · 21 comments

Comments

@bpierre
Copy link
bpierre commented Jan 15, 2013

With the following URLs:

http://myapp.dev/admin
http://myapp.dev/admin/users

I can’t “hack” the URL by removing the users part of the second one, a 404 will be triggered because of the trailing slash:

http://myapp.dev/admin/

I think that “hackable URLs” are a nice way to interact with an application.

Proposal

The Router class could handle this: if a route is not matched, but the path ends with a slash, then try again to match the route after removing the trailing slash.

If the route exists, then redirect to the correct route, without trailing slash. Or maybe don’t even redirect, like GitHub does (it could be a setting):

https://github.com/laravel/framework/issues
https://github.com/laravel/framework/issues/

Router->dispatch() could handle this, or maybe Router->handleRoutingException().

@billmn
Copy link
Contributor
billmn commented Jan 15, 2013

👍

@ipalaus
Copy link
Contributor
ipalaus commented Jan 15, 2013

I'm sure github does that before hitting the application.

@zimt28
Copy link
Contributor
zimt28 commented Jan 15, 2013

We've already had this discussion, it's wanted behavior since there are some advantages.
See http://googlewebmastercentral.blogspot.de/2010/04/to-slash-or-not-to-slash.html

@bpierre
Copy link
Author
bpierre commented Jan 15, 2013

@zimt28 I don’t see the point, could you add a link to the discussion? The article you are mentioning is about returning 200 or 301/302 redirects to handle trailing slashes, not 404.

More precisely, it seems Google is ok with 200 on both, even without rel="canonical":

If both slash and non-trailing-slash versions contain the same content and each returns 200, you can:

  • […]
  • Leave it as-is. Many sites have duplicate content. Our indexing process often handles this case for webmasters and users. While it’s not totally optimal behavior, it’s perfectly legitimate and a-okay. :)

And recommends to pick one and redirect the other one:

If only one version can be returned (i.e., the other redirects to it), that’s great! This behavior is beneficial because it reduces duplicate content.

@zimt28
Copy link
Contributor
zimt28 commented Jan 15, 2013

I actually wanted to do that but the discussion took place on the old illuminate/router or something which has been deleted in between.

@franzliedke
Copy link
Contributor

@bpierre The problem is that Laravel 4 currently only works with the variant that you specify in your routes. If you don't use a slash there, the variant with the slash will return a 404 error message.

@robclancy
Copy link
Contributor

I was actually surprised when I noticed laravel didn't do this already, it just seems logical to me that it should match fine with or without the trailing slash.

@bpierre
Copy link
Author
bpierre commented Jan 16, 2013

@franzliedke Yes, and with a slash, that’s the variant without slash which returns a 404. I want both :-)

I tried to add /? to the route, but the route syntax is not a regex.
Then I tried hacking the route params, which supports regexes. It works, but it’s really ugly!

Route::get('admin{slash?}', function($slash)
{
    // (╯°⁔°)╯ ︵ ┻━┻
})
->where('slash', '\/');

Plus, this still requires to handle the redirect.

@frankmayer
Copy link

+1 on having some way to do this.

@BenJenkinson
Copy link

I agree entirely, hackable URLs are brilliant. I actually raised this issue earlier, but @taylorotwell closed it saying "Just don't use trailing slashes."

@bpierre
Copy link
Author
bpierre commented Jan 17, 2013

@BenJenkinson Thanks for the link. As you said, it’s not our fault, it’s our users, and we have to guide them to the no-trailing-slash enlightenment :-)

@franzliedke
Copy link
Contributor

That's super annoying during development, though. I certainly don't remember to delete the slash when I manually go up in the hierarchy by deleting the last URL segment.

@BenJenkinson
Copy link

It is both annoying and non-obvious, as demonstrated by the identical issues raised by myself earlier, @bpierre here and @DPr00f over on laravel/laravel Issue #1596.

I also found this earlier laravel/laravel Issue #1368 where they point out that redirecting will sometimes cause problems with form submission; if the URL used for the form action attribute is not the same (with or without slash) as the non-redirected URL.

@wishfoundry
Copy link

@bpierre , @frankmayer, @zimt28 , @billmn

Using regex in L4 makes things so much more powerful. Let's not fix what isn't broken.
Trailing slashes can be easily supported by the user adding his own function in routes, no need to modify core

Route::get('{any}', function($url){
    return Redirect::to(mb_substr($url, 0, -1), 301);
})->where('any', '(.*)\/$');

@frankmayer
Copy link

@wishfoundry, yes that would be easy, but for people just starting off with L4 I see a lot of feature and help requests. Maybe it would be good to have this in the docs?

@raftalks
Copy link
Contributor

@wishfoundry Your method looks like it can do the desired need to handle the kind of url issue we have here, but in my opinion it is little cumbersome to do the redirection on each route.

@tlgreg
Copy link
tlgreg commented Jan 21, 2013

Although the greatest flexibility would be if Laravel would use the routes to decide what to redirect, but personally I'm ok with choosing one and redirecting in the htaccess.
My rewrites right now:

<IfModule mod_rewrite.c>
    Options -MultiViews
    RewriteEngine On

    # redirect everything to url without trailing slash

    RewriteCond %{HTTPS} =on
    RewriteRule ^(.+)$ - [env=ps:https]
    RewriteCond %{HTTPS} !=on
    RewriteRule ^(.+)$ - [env=ps:http]

    RewriteCond %{REQUEST_FILENAME} !-f
    RewriteCond %{REQUEST_FILENAME} !-d

    RewriteCond %{REQUEST_METHOD} ^GET

    RewriteRule ^(.+)/$ %{ENV:ps}://%{SERVER_NAME}/$1 [R=301,L]

    # pretty urls

    RewriteCond %{REQUEST_FILENAME} !-f
    RewriteCond %{REQUEST_FILENAME} !-d

    RewriteRule ^ index.php [L]
</IfModule>

@billmn
Copy link
Contributor
billmn commented Jan 21, 2013

@tlgreg With your htaccess I'm unable to load assets from /public/packages/vendor/...

@bpierre
Copy link
Author
bpierre commented Jan 21, 2013

@tlgreg I may be wrong, I am far from being an Apache guru:

  • It seems that your mod_rewrite conf is redirecting every request (POST, PUT, etc.): not a big deal, but only GET requests should be redirected.
  • Every (real) 404 with a / will be redirected to another 404 without /.

@tlgreg
Copy link
tlgreg commented Jan 21, 2013

@billmn Sorry, missed the non-existing file condition for the index. Actually the file condition is not that important for the redirects as they shouldn't end with a slash anyway. I updated the post above.

@bpierre Yes, this is a bit quick and dirty in a way that it redirects everything.
You can't really handle "real 404" in apache alone, that case really has to be done in Laravel.
As for the methods, you're most likely right it's better to limit to GET for the same url case. You can do it putting the following condition in:

    RewriteCond %{REQUEST_METHOD} ^GET

This way for trailing slashes with POST/PUT/DELETE, etc a 404 error will be returned by Laravel.

I'm not an Apache guru either, this was just my quick take on it and needs testing. :)

@taylorotwell
Copy link
Member

One trailing slash is now allowed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

0