Skip to content
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

Fix r2-4195: Handle double slash in splash pattern #1100

Merged
merged 3 commits into from
Dec 12, 2016

Conversation

mewtaylor
Copy link
Contributor

This fixes https://github.com/LLK/scratchr2/issues/4195 by updating the regex to catch two slashes. I decided to intentionally not catch more than 2 – it’ll 404 on 3 or more, which seems appropriate.

We could also redirect on 2 slashes to the homepage, but this seemed just as well.

This fixes https://github.com/LLK/scratchr2/issues/4195 by updating the regex to catch two slashes. I decided to intentionally not catch more than 2 – it’ll 404 on 3 or more, which seems appropriate.
@rschamp
Copy link
Contributor

rschamp commented Dec 8, 2016

What do you think about leaving this simple and just removing the endpoint so it 404s instead?

@mewtaylor
Copy link
Contributor Author

I personally like the idea of handling just the double slash – it's a mistake that happens, and that other sites handle for you that i've seen.

@rschamp
Copy link
Contributor

rschamp commented Dec 8, 2016

If we're going to support it, I think I would rather have ^//+ redirect to / than resolving to the homepage, since ideally you only have one URL per page. (At one point Google would dock you for multiple URLs for the same content, but I'm not sure if that is still true.) Eventually maybe we should add a couple of generic redirect rules

  • ^//+(.+)$ to /\1
  • ^(/.+)//+$ to \1/

@mewtaylor
Copy link
Contributor Author

I had it as a redirect before, but was trying to keep the rules simplified – I have no strong preference for keeping it as is if you'd rather see it as its own redirect rule.

@rschamp
Copy link
Contributor

rschamp commented Dec 8, 2016

In addition to the functional difference, I think I would actually prefer the redirect for simplicity's sake, since then the route pattern is easier to read.

@mewtaylor
Copy link
Contributor Author

Same as in the PR details – I kind of felt that anything more than two slashes should 404.

@@ -7,6 +7,12 @@
"title": "Imagine, Program, Share"
},
{
"name": "splash-redirect",
"pattern": "^//?$",

This comment was marked as abuse.

@rschamp rschamp assigned mewtaylor and unassigned rschamp Dec 8, 2016
@mewtaylor mewtaylor removed their assignment Dec 12, 2016
@mewtaylor mewtaylor merged commit 117dbcd into scratchfoundation:develop Dec 12, 2016
@mewtaylor mewtaylor deleted the issue/r2-4195 branch December 12, 2016 20:04
@chrisgarrity
Copy link
Contributor

Testing on El Capitan, Chrome 54, Safari 10.0.1, Firefox 45.5:

mewtaylor added a commit that referenced this pull request Dec 16, 2016
Remove `routeAlias` from splash redirect
@jwzimmer-zz
Copy link
Contributor

@mewtaylor has fixed this, I just checked it & discussed it with him, 3 slashes no longer 404s but anything more than 3 will.

Chrome 55, Mac OS El Capitan, Staging & IE 11 on Win 7 VM, Staging

  • 3 slashes redirects to homepage
  • 2 slashes redirects to homepage
  • 1 slashes redirects to homepage
  • 0 slashes redirects to homepage
  • 4 slashes 404s
  • Random numbers higher than 4, 404

@jwzimmer-zz jwzimmer-zz self-assigned this Dec 19, 2016
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.

None yet

4 participants