-
Notifications
You must be signed in to change notification settings - Fork 615
PRO-8154: Strip Latin accents #5137
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
base: main
Are you sure you want to change the base?
Conversation
modules/@apostrophecms/i18n/index.js
Outdated
| await self.apos.migration.eachDoc({}, 5, async doc => { | ||
| const slug = doc.slug; | ||
| const req = self.apos.task.getAdminReq({ | ||
| locale: doc.aposLocale?.split(':')[0] || 'en' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forcing the locale to en seems wrong. We have i18n defaultLocale for that.
If you're using doc.aposLocale you can also pass it as query: { aposLocale: doc.aposLocale } and let the i18n middleware do its magic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aposLocale can be undefined. I'll switch to default locale though.
| // If the page does not yet have a slug, add one based on the | ||
| // title; throw an error if there is no title | ||
| ensureSlug(page) { | ||
| ensureSlug(page, { stripAccents } = {}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this scales well. We should work towards ensureSlug and slugify keeping the same signature somehow and fetching the stripAccents from the i18n locales option in my opinion. If we don't do that, it means that we have to change multiple external/pro modules to support the new feature, and apostrophe users will have to do the same. There will always be a piece of code somewhere where stripAccents is not passed and it will lead to inconsistent behavior that will be hard to track.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ensureSlug is internal method. It's called internally via event.
Utils should not fetch/know what i18n options are, so slugify is what it is - an util with an option.
Having an option for ensureSlug is a clear contract. This is not a public API so no BC is required.
I reverted option per locale because of many and bad showstoppers. I explained above that the there was an agreement to go with the simple "rule them all" option.
| slugify(s, options) { | ||
| return require('sluggo')(s, options); | ||
| const { stripAccents, ...opts } = options || {}; | ||
| const slug = require('sluggo')(s, opts); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We should put that
requirestatement at the top. - Is the order of operations important here? slugify then strip accents vs strip accents then slugify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving the require is out of context for this PR. (Many other utils does it).
The current order of operation makes sense. You don't want to change the behavior of sluggo with pre-filtering the input, but to filter the output of the sluggo.
| // set options.stripAccents to true. | ||
| slugify(s, options) { | ||
| return require('sluggo')(s, options); | ||
| const { stripAccents, ...opts } = options || {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new option is a challenge because we need to update all the usages of the slugify method, in our modules but our clients needs to do the same. There is a default value, but this is some kind of a breaking change, mainly because it requires changes to the code for the behavior to remain consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The accent cleaning behavior can't break BC because it's a new feature. This code is fully BC.
| return this.field.page && apos.i18n.locales[apos.i18n.locale].prefix; | ||
| }, | ||
| stripAccents() { | ||
| return apos.i18n.stripUrlAccents === true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having this here will make it difficult to have this feature enabled/disabled per locales.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one of the main showstoppers for per locale feature, which we don't implement with the current PR.
|
@haroun Added a patch to address the problems
|
Summary
Summary
Introduce a configurable option
stripUrlAccentsin the@apostrophecms/i18nmodule to control whether Latin accents are stripped from URLs (slugs) and attachment names.@apostrophecms/i18n.options.stripUrlAccentsistrue:namevalues are normalized without accents on upload.@apostrophecms/i18n:strip-slug-accentsis provided to update existing document slugs and attachment names when adopting this option.What are the specific steps to test this change?
stripUrlAccentsis absent or set tofalsein@apostrophecms/i18noptions.C'est déjà l'été.c-est-déjà-l-été/c-est-déjà-l-étéété-image.pngété-image@apostrophecms/i18n.options.stripUrlAccents = true.C'est déjà l'étéc-est-deja-l-ete/c-est-deja-l-eteété-image.pngete-imagestripUrlAccents: true, run the task:node app @apostrophecms/i18n:strip-slug-accents/c-est-déjà-l-étébecome/c-est-deja-l-ete.été-imagebecomeete-image.Notes
sluggo; the deburring only affects Latin accent marks.What kind of change does this PR introduce?
(Check at least one)
Make sure the PR fulfills these requirements:
If adding a new feature without an already open issue, it's best to open a feature request issue first and wait for approval before working on it.
Other information: