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

[String] Add locale-sensitive map for slugging symbols #36456

Merged
merged 1 commit into from Apr 24, 2020
Merged

[String] Add locale-sensitive map for slugging symbols #36456

merged 1 commit into from Apr 24, 2020

Conversation

lmasforne
Copy link
Contributor

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #36383
License MIT

By default chars '@' and '&' are respectively replaced by 'at' and 'and' (so limited by enlgish language).
I had an $options arguments to 'slug' method to replace chars with your own logic.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we need an options array for this, let's pass a map directly.
The map should be locale-sensitive and the current map should be used only with an English locale IMHO.

CHANGELOG-5.0.md Outdated Show resolved Hide resolved
@nicolas-grekas nicolas-grekas added this to the next milestone Apr 15, 2020
@lmasforne
Copy link
Contributor Author

I modify the PR as i understand of your recommandations @nicolas-grekas ;) thank you for your feedbacks

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, I like this feature.

But what if the $locale is fr and I use the default $charsReplacement = ['en' => ['@' => 'at', '&' => 'and']]. Then nothing will be slugged, right?

Does it make sense to be more complex here? Maybe dedicate this replacement thing to a separate class?

CHANGELOG-5.0.md Outdated Show resolved Hide resolved
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a constructor argument, not a method argument. The argument should default to null and the default value be set on the property I think.

But what if the $locale is fr and I use the default $charsReplacement = ['en' => ['@' => 'at', '&' => 'and']]. Then nothing will be slugged, right?

Yes, that's the expected behavior to me.

Does it make sense to be more complex here? Maybe dedicate this replacement thing to a separate class?

Getting more complex is always possible by implementing SluggerInterface. Since the AsciiSlugger already does these simple mappings, I think it makes sense to make the map locale-aware (as the class already is).

src/Symfony/Component/String/Slugger/AsciiSlugger.php Outdated Show resolved Hide resolved
src/Symfony/Component/String/Slugger/AsciiSlugger.php Outdated Show resolved Hide resolved
src/Symfony/Component/String/Slugger/AsciiSlugger.php Outdated Show resolved Hide resolved
src/Symfony/Component/String/Slugger/AsciiSlugger.php Outdated Show resolved Hide resolved
src/Symfony/Component/String/Slugger/AsciiSlugger.php Outdated Show resolved Hide resolved
@nicolas-grekas nicolas-grekas changed the title Fix ticket 36383 slug only in english [String] Add locale-sensitive map for slugging symbols Apr 16, 2020
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Apr 16, 2020

In the linked issue, I mentioned using the translator instead of a simple map. (bad idea: what would be the string to translate?)

There is still one question remaining here: how will people take advantage of the new argument in a Symfony app? How will they configure the map? Do we need tighter integration?

@lmasforne
Copy link
Contributor Author

@nicolas-grekas i have fixed your feedbacks, thank you for your reviews

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed my last comments. About the idea to use a translator, or how this could be leveraged from framework-bundle, we don't have to decide now. Let's wait for someone interested in resolving these challenges. Good as is on my side.

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

@fabpot
Copy link
Member

fabpot commented Apr 24, 2020

Thank you @lmasforne.

@fabpot fabpot merged commit 260dea0 into symfony:master Apr 24, 2020
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
@fabpot fabpot mentioned this pull request May 5, 2020
ThomasLandauer added a commit to ThomasLandauer/symfony that referenced this pull request Nov 22, 2021
Am I right that there is no way to configure the `$ymbolsMap` when using the Symfony framework integration (i.e. dependency injection)?
What about adding a public method for this?
The feature to set it in the constructor was added in symfony#36456
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.

[String] Slugger should only replace special characters for the English locale
5 participants