Support Unicode characters in slugs #640

tech4him1 · 2017-09-30T00:23:35Z

- Summary

Right now, we only support Latin chars in slugs/filenames (we strip accents from chars if we can, and drop everything else). This isn't very user-friendly, and latest browsers (including IE11) support Unicode, so we might as well allow users to use it. ~~We still have to sanitize the slugs to make sure that the filenames are safe , but other than that, we don't have to do anything else.~~

This PR upgrades the slug formatter to use the new IRI standards (RFC 3987), which support other than latin-1 chars in slugs, instead of the URI one (RFC 3986).

One other thing I did here was to sanitize all types of slugs, not just the input titles. This should make it much easier to do slug customization later.

As a side effect, this closes #607.

- Test plan

Lots of testing needs done on this, and we should probably announce it as well (for users that were previously having accents stripped).

Tests were also added.

Example Test Cases:

Title	Old Slug	New Slug
ěščřžý	`escrzy`	`ěščřžý`
日本語のタイトル	(empty)	`日本語のタイトル`
duck\\goose.elephant	`duckgooseelephant`	`duck-goose-elephant`
Netlify CMS Planning: Working in Sprints	`netlify-cms-planning-working-in-sprints`	`netlify-cms-planning-working-in-sprints`
This, that, and the other!	`this-that-and-the-other`	`this-that-and-the-other`

- Description for the changelog

Support Unicode characters in slugs.

- A picture of a cute animal (not mandatory but encouraged)

tech4him1 · 2017-09-30T00:25:45Z

Should we still convert all characters to (localized) lower-case here? Done
Also, should we convert spaces to dashes? Done
What about symbols (commas, exclamation points)? Done

Or should we just do the bare minimum and let any SSG do the rest? But then why even have slug customization -- the CMS wouldn't even know what the slug was going to be, then.

tech4him1 · 2017-09-30T22:30:54Z

OK, everybody, this is ready for review. Please throw some test cases at it, and let me know if you find a unique one.

erquhart

Awesome work @tech4him1, thank you! Just a couple of small change requests, after which this should be good to go.

erquhart · 2017-10-02T22:14:52Z

src/backends/backend.js

@@ -42,7 +43,7 @@ const slugFormatter = (template = "{{slug}}", entryData) => {
    return identifier;
  };

-  return template.replace(/\{\{([^\}]+)\}\}/g, (_, field) => {
+  let slug = template.replace(/\{\{([^\}]+)\}\}/g, (_, field) => {


Although we allow some mutable ops for dense logic or areas where performance is a concern, we generally want to avoid them. I'd recommend something more like:

const slug = template.replace(...) .method() .method();

I'd also move sanitizeFilename and the trailing character replacement to a new sanitizeSlug export from urlHelper, so you can just return sanitizeSlug(slug).

Done, but I'm not sure if I have it the best way. Could you please review sanitizeSlug?

erquhart · 2017-10-02T22:15:35Z

src/lib/urlHelper.js

@@ -12,6 +12,22 @@ export function getNewEntryUrl(collectionName, direct) {
  return getUrl(`/collections/${ collectionName }/entries/new`, direct);
 }

+// Unreserved chars from RFC3987.


Can you be a little bit more verbose in this comment, maybe even add a reference link? You did a lot of research, just want to capture some of it here.

Sure, sounds good, because I sure don't want to have to do it over again :).

tech4him1 · 2017-10-03T15:06:03Z

@erquhart Can you please review the last commit on this and see if you have any improvements on the sanitizeSlug function?

erquhart · 2017-10-03T15:17:40Z

src/lib/urlHelper.js

+export function sanitizeSlug(str, { replacement = '-' }) {
+  if (!isString(str)) throw "`sanitizeSlug` only accepts strings as input.";
+  if (!isString(replacement)) throw "the `sanitizeSlug` replacement character must be a string.";
+  let slug = str;


I'd still like to avoid mutations - we can use a couple of Lodash functions (flow and partialRight) to go immutable without the cruft:

const sanitize = flow([ partialRight(sanitizeIRI, { replacement }), partialRight(sanitizeFilename, { replacement }), ]); const sanitizedSlug = sanitize(slug); // After declaring replacement expressions: const normalizedSlug = slug .replace(...) .replace(...);

Be sure to keep your source comments, those are helpful.

erquhart · 2017-10-03T15:19:17Z

src/backends/backend.js

    }
-  });
+  })


Can you bring back your source comment explaining this regex?

tech4him1 · 2017-10-03T16:08:31Z

@erquhart OK, done. Are the comments clear enough? Also, do you have any suggestions on making the sanitizeIRI immutable, or is that a case where we don't need to?

dopry · 2017-10-03T23:09:21Z

src/lib/urlHelper.js

+  return result;
+}
+
+export function sanitizeSlug(str, { replacement = '-' } = {}) {


Why use an options object here? Do we expect more complex arguments in the future?

@dopry No, I was just keeping the interface the same as sanitizeFilename and sanitizeIRI. Also, I think sanitizeSlug(slug, {replacement: '-'}) is clearer than sanitizeSlug(slug, '-').

Is there any particular reason that you don't think it should be there?

I use Intellisense so I get hinted variable names and types usually. If you were to write a jsdoc block for this you'd have to create a typedef for each options object. When there is only one option there isn't much function purpose to use it.

I personally think it's much easier to understand...

function sanitizeSlug(slug, replacement = '-') {} but this is a matter of preference and I'd approve with or without... just curious at this point

@dopry I'm talking about calling the function -- where sanitizeSlug(slug, {replacement: '-'}) is clearer than sanitizeSlug(slug, '-').

dopry · 2017-10-04T00:39:53Z

src/lib/urlHelper.js

+  const doubleReplacement = new RegExp('(?:' + escapeRegExp(replacement) + ')+', 'g');
+  const trailingReplacment = new RegExp(escapeRegExp(replacement) + '$');
+  const normalizedSlug = sanitizedSlug
+    .replace(doubleReplacement, '-')


use replacement instead of '-'

dopry · 2017-10-04T00:43:02Z

src/lib/urlHelper.js

+
+  return normalizedSlug;
+}
+
 export function urlize(string) {


urlize is no longer used, it should be removed so we're not dragging unused code around.

good point, but there are a couple reasons I don't want to remove it yet, plus it gets trimmed from the bundle anyway. Thanks for noting this, though, I'll look into it.

What reasons? It's in the git history, we can always dig it up if we need it again.

@biilmann added it. Also, we weren't using it before, either, so I think it should be a separate PR.

@dopry

Thanks @dopry!

tech4him1 · 2017-10-04T02:36:06Z

@erquhart @Benaiah Do you care at all whether the interface is sanitizeSlug(str, { replacement: '-' }) or sanitizeSlug(str, '-')?

originally removed in #640, accidently added as part of #638

tech4him1 changed the title ~~WIP: Support UTF-8 characters in slugs.~~ Support UTF-8 characters in slugs. Sep 30, 2017

tech4him1 requested review from erquhart, Benaiah and dopry September 30, 2017 00:28

tech4him1 mentioned this pull request Sep 30, 2017

WIP: Switch slugifier to urlize #620

Closed

tech4him1 changed the title ~~Support UTF-8 characters in slugs.~~ WIP: Support UTF-8 characters in slugs. Sep 30, 2017

tech4him1 changed the title ~~WIP: Support UTF-8 characters in slugs.~~ Support UTF-8 characters in slugs. Sep 30, 2017

tech4him1 changed the title ~~Support UTF-8 characters in slugs.~~ Support UTF-8 characters in slugs Sep 30, 2017

tech4him1 requested a review from biilmann September 30, 2017 22:25

tech4him1 force-pushed the utf8-slugs branch 2 times, most recently from 80690e4 to dcc7a73 Compare October 1, 2017 02:12

tech4him1 changed the title ~~Support UTF-8 characters in slugs~~ Support Unicode characters in slugs Oct 2, 2017

erquhart suggested changes Oct 2, 2017

View reviewed changes

erquhart removed the request for review from biilmann October 2, 2017 23:53

tech4him1 force-pushed the utf8-slugs branch from 13e3c9b to 8e78ec2 Compare October 3, 2017 15:01

tech4him1 force-pushed the utf8-slugs branch from 8e78ec2 to 30ba9f6 Compare October 3, 2017 15:09

erquhart suggested changes Oct 3, 2017

View reviewed changes

tech4him1 force-pushed the utf8-slugs branch from 30ba9f6 to 71fb90a Compare October 3, 2017 15:41

erquhart approved these changes Oct 3, 2017

View reviewed changes

tech4him1 added 7 commits October 3, 2017 13:03

Switch from slug to sanitize-filename.

51b89cc

Remove periods from filenames/slugs.

d21de7e

Replace spaces in slugs/filenames with dashes.

d895112

Sanitize entire slug instead of just parts.

8a2b4fc

Re-implement standard slugification with IRIs instead of URIs.

9bc65cd

Make ucschars regex smaller.

4e5a004

Move slug sanitizer to a seperate function.

7249274

tech4him1 added 3 commits October 3, 2017 16:15

Add tests for sanitizeIRI.

476ff2e

Make sure sanitizeIRI replacement character is safe.

716f55c

Allow empty options object for sanitizeSlug.

2b64fbf

tech4him1 force-pushed the utf8-slugs branch from 2181649 to 2b64fbf Compare October 3, 2017 22:16

dopry reviewed Oct 3, 2017

View reviewed changes

add sanitizeSlug tests.

ddcf009

tech4him1 force-pushed the utf8-slugs branch from 4212013 to fe68cbe Compare October 4, 2017 00:10

Update slug sanitization errors.

b8006bb

tech4him1 force-pushed the utf8-slugs branch from fe68cbe to b8006bb Compare October 4, 2017 00:11

dopry suggested changes Oct 4, 2017

View reviewed changes

tech4him1 added 4 commits October 3, 2017 18:46

Fix alternate replacement not always working in sanitizeSlug.

451c69c

Remove recursion for sanitizeIRI replacement checking.

fdc4054

Remove mutation from sanitizeIRI.

793afc8

Update tests.

5720237

Thanks @dopry!

tech4him1 force-pushed the utf8-slugs branch from 7c6db11 to 5720237 Compare October 4, 2017 02:27

tech4him1 requested a review from erquhart October 4, 2017 02:32

tech4him1 requested review from dopry and Benaiah October 4, 2017 13:37

erquhart approved these changes Oct 4, 2017

View reviewed changes

erquhart merged commit 876cb2c into master Oct 4, 2017

erquhart deleted the utf8-slugs branch October 4, 2017 14:34

tech4him1 removed request for dopry and Benaiah October 4, 2017 14:35

tech4him1 mentioned this pull request Jan 17, 2018

slug fix #1012

Closed

tech4him1 added a commit that referenced this pull request Jan 17, 2018

remove unneeded package

e878c28

originally removed in #640, accidently added as part of #638

tech4him1 mentioned this pull request Jan 17, 2018

remove unneeded package #1020

Merged

tech4him1 added a commit that referenced this pull request Jan 17, 2018

remove unneeded package

12ff47d

originally removed in #640, accidently added as part of #638

This was referenced Apr 12, 2018

Filename sanitation should only apply to variable parts #1254

Closed

Pre-sanitize slug portions individually #1255

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Unicode characters in slugs #640

Support Unicode characters in slugs #640

tech4him1 commented Sep 30, 2017 •

edited

Loading

tech4him1 commented Sep 30, 2017 •

edited

Loading

tech4him1 commented Sep 30, 2017 •

edited

Loading

erquhart left a comment

erquhart Oct 2, 2017

tech4him1 Oct 3, 2017

erquhart Oct 2, 2017

tech4him1 Oct 2, 2017 •

edited

Loading

tech4him1 commented Oct 3, 2017

erquhart Oct 3, 2017

erquhart Oct 3, 2017

tech4him1 Oct 3, 2017

tech4him1 commented Oct 3, 2017 •

edited

Loading

dopry Oct 3, 2017

tech4him1 Oct 3, 2017

dopry Oct 3, 2017

tech4him1 Oct 4, 2017

dopry Oct 4, 2017

tech4him1 Oct 4, 2017

dopry Oct 4, 2017

tech4him1 Oct 4, 2017

dopry Oct 4, 2017

tech4him1 Oct 4, 2017

erquhart Oct 4, 2017

tech4him1 commented Oct 4, 2017

Support Unicode characters in slugs #640

Support Unicode characters in slugs #640

Conversation

tech4him1 commented Sep 30, 2017 • edited Loading

tech4him1 commented Sep 30, 2017 • edited Loading

tech4him1 commented Sep 30, 2017 • edited Loading

erquhart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tech4him1 Oct 2, 2017 • edited Loading

Choose a reason for hiding this comment

tech4him1 commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tech4him1 commented Oct 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tech4him1 commented Oct 4, 2017

tech4him1 commented Sep 30, 2017 •

edited

Loading

tech4him1 commented Sep 30, 2017 •

edited

Loading

tech4him1 commented Sep 30, 2017 •

edited

Loading

tech4him1 Oct 2, 2017 •

edited

Loading

tech4him1 commented Oct 3, 2017 •

edited

Loading