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

Case insensitive redirects #9755

Merged
merged 6 commits into from Sep 24, 2018

Conversation

Projects
None yet
3 participants
@jessehouwing
Contributor

jessehouwing commented Jul 26, 2018

While migrating from Blogger to Ghost I ran into the issue that Blogger is case-insensitive in a couple of places:

  • Tag urls
  • Post urls

I've integrated Google Search Console with my current blog and I'm seeing all kinds of people hitting 404's due to them using urls in all-lower-case.

I've updated my Blogger2Ghost.NET application to generate redirects like these

  {
    "from": "^\\/search\\/label\\/[Hh][Dd][Mm][Ii]$",
    "to": "/tag/windows-devices",
    "permanent": false
  }

But that's not really nice. This Pull request adds the ability to add case-insensitivity to the redirects.json format:

    { 
        "from": "^\\/case-insensitive",
        "caseInsensitive": true,
        "to": "/redirected-insensitive"
    },
    {
        "from": "^\\/Case-Sensitive",
        "caseInsensitive": false,
        "to": "/redirected-sensitive"
    },
    {
        "from": "^\\/Default-Sensitive",
        "to": "/redirected-default"
    }

The caseInsensitive is a boolean option, it's optional and the default is to keep the current behavior. By adding caseInsensitive=true the redirect will be initialized with the "i" RegExp option.

I've considered checking the start of the regex for (?i:) as well and settign the flag, which is the way for most regex flavors to set case insensitivity inline. But since that's not part of the JavaScript regexp standard, that may be confusing.


This change is Reviewable

@jessehouwing

This comment has been minimized.

Contributor

jessehouwing commented Jul 26, 2018

Currently the redirect API doesn't return a 302, which is what I expect, but it does return 301 to the passed in URL (preserving case). I'm not sure why, this seems the standard behavior.

@kirrg001 kirrg001 self-assigned this Jul 26, 2018

@kirrg001

This comment has been minimized.

Contributor

kirrg001 commented Jul 26, 2018

Thanks will review asap 👍

@jessehouwing

This comment has been minimized.

Contributor

jessehouwing commented Jul 26, 2018

On windows there are some issues running tests and the eslint configuration.

  1. There are more _spec files than the windows commandline will grok, causing a "command to long" error half-way running the tests. No clue how to easily fix this.
  2. The repo doesn't enforce checkout as-is commit-as-is. So on Windows it defaults to windows line ends on a default git install. This causes so many lint errors it becomes impossible to use. Setting the repository default will help people on Windows. See: https://help.github.com/articles/dealing-with-line-endings/
@kirrg001

This comment has been minimized.

Contributor

kirrg001 commented Jul 30, 2018

Hey!

I am not sure about adding a new option 🤔

If we would allow passing the options in the from field, we could parse it out and forward the regex options to RegExp?

e.g. "from": "^\/EXAMPLE/i",

@jessehouwing

This comment has been minimized.

Contributor

jessehouwing commented Jul 30, 2018

@jessehouwing

This comment has been minimized.

Contributor

jessehouwing commented Jul 30, 2018

The latest version of my Blogger Importer now generates case insensitive links and can also generate lower-case links if needed.
jessehouwing/Blogger2Ghost@26fa340

@kirrg001

This comment has been minimized.

Contributor

kirrg001 commented Jul 30, 2018

Or the JS synrax: from: "/^\\/ACTION\\/$/i"

👍

@jessehouwing

This comment has been minimized.

Contributor

jessehouwing commented Jul 30, 2018

@kirrg001 kirrg001 changed the title from Case insensitive redirects to [WIP] Case insensitive redirects Aug 3, 2018

@jessehouwing

This comment has been minimized.

Contributor

jessehouwing commented Aug 6, 2018

Pushed the requested changes. It now uses the requested format. Dropped the parameter and updated the tests to reflect.

@kirrg001

This comment has been minimized.

Contributor

kirrg001 commented Aug 6, 2018

Thanks @jessehouwing - will review asap.

@@ -0,0 +1,2 @@
# enforce unix style line endings

This comment has been minimized.

@kirrg001

kirrg001 Sep 10, 2018

Contributor

Could you pls revert this change? It does not belong to this PR.

Feel free to open a new issue and explain why Windows development requires such a change. Then we can discuss & decide :)

@@ -27,6 +27,13 @@ _private.registerRoutes = function registerRoutes() {
* - you define /my-blog-post-1/ as from property
* - /my-blog-post-1 or /my-blog-post-1/ should work
*/
var options = '';

This comment has been minimized.

@kirrg001

kirrg001 Sep 10, 2018

Contributor

Could you pls use let or const? (ES6) Thanks!

@jessehouwing jessehouwing force-pushed the jessehouwing:master branch 2 times, most recently from 3337953 to 00c2146 Sep 13, 2018

@jessehouwing

This comment has been minimized.

Contributor

jessehouwing commented Sep 13, 2018

Rebased against master and incorporated comments.

@kirrg001 kirrg001 changed the title from [WIP] Case insensitive redirects to Case insensitive redirects Sep 17, 2018

@ErisDS

ErisDS approved these changes Sep 17, 2018

LGTM 👍

@@ -27,6 +27,13 @@ _private.registerRoutes = function registerRoutes() {
* - you define /my-blog-post-1/ as from property
* - /my-blog-post-1 or /my-blog-post-1/ should work
*/
let options = '';
if (redirect.from.match(/\/.*\/i/)) {

This comment has been minimized.

@kirrg001

kirrg001 Sep 17, 2018

Contributor

The comment above belongs to the condition below. Can you pls reorder? :)

A possible regex improvement could be: instead of /\/.*\/i/, use /\/i$/

@jessehouwing

This comment has been minimized.

Contributor

jessehouwing commented Sep 18, 2018

Not sure what you mean with the last comment... Are you suggesting to drop the first forward slash?

I didn't do that cause one may have a pattern that ends with /i and there would be no way to distinguish. That's why I did only the variant where both leading and trailing slash are present eg: /..../i

@kirrg001

This comment has been minimized.

Contributor

kirrg001 commented Sep 24, 2018

Not sure what you mean with the last comment... Are you suggesting to drop the first forward slash?

No. I only was suggesting to match the url with "ends with" /i.

This PR needs a rebase again 😕

@jessehouwing jessehouwing force-pushed the jessehouwing:master branch from 1956143 to 8e1db43 Sep 24, 2018

@jessehouwing

This comment has been minimized.

Contributor

jessehouwing commented Sep 24, 2018

Right, just scanning for /i won't be enough, if we're following the /... /i format in javascript. The leading slash is part of the expression syntax. I could split up the check in a Startswith('/') and EndsWith('/i'), but regex seems the to be tool of trade in this class :D.

I did find a bug in the regex I used (it didn't use ^...$ to restrict this regex to the whole input.

I can rewrite it as: from.match(/^\//) && from.match(/\/i$/) if that's preferred.

@jessehouwing

This comment has been minimized.

Contributor

jessehouwing commented Sep 24, 2018

Rebased.

@kirrg001

LGTM - thanks so much :)

@kirrg001 kirrg001 merged commit e3234bc into TryGhost:master Sep 24, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

allouis added a commit that referenced this pull request Sep 25, 2018

Merge branch 'master' @ 2.1.4 into stable
* master: (25 commits)
  Version bump to 2.1.4
  Updated Ghost-Admin to 2.1.4
  Version bump to 2.1.4-beta.1
  Updated Ghost-Admin to 2.1.4-beta.1
  Updated private-sites to not redirect to full urls
  Bumped ghost-ignition to version 2.9.6
  Upgrading Casper to 2.6.3
  Refactored url utility to generate multiple API version URLs (#9897)
  Added new endpoint to upload square profile images with dimension validation (#9862)
  Removed change frequency and priority fields from sitemap generator (#9771)
  🎨Added case insensitive support for redirects (#9755)
  Refactored direct usages of api controllers
  Normalised how we require models
  Refactored how we require shared middlewares from web/ (#9893)
  Removed duplicate 'id' for User permittedOptions
  Updated permittedOptions to correctly call super
  Updated base model to remove extraAllowedProperties
  Extended uncapitalise unit tests (#9891)
  Removed `res.isAdmin` from admin express app (#9889)
  Refactored `web/middleware` and `web/utils` to `web/shared` (#9892)
  ...

@woolfyx woolfyx referenced this pull request Oct 3, 2018

Merged

Upgrade to v2.2.0 #12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment