Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:masterfromlmasforne:fix_ticket_36383_slug_only_in_english
Apr 24, 2020
Merged

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

fabpot merged 1 commit intosymfony:masterfromlmasforne:fix_ticket_36383_slug_only_in_english
Apr 24, 2020

Conversation

lmasforne
Copy link
Contributor

QA
Branch?master
Bug fix?no
New feature?yes
Deprecations?no
TicketsFix#36383
LicenseMIT

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-grekasnicolas-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.

wouterj and lmasforne reacted with thumbs up emoji
@nicolas-grekasnicolas-grekas added this to thenext milestoneApr 15, 2020
@lmasforne
Copy link
ContributorAuthor

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

Copy link
Member

@NyholmNyholm 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 isfr 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?

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment
edited
Loading

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 theAsciiSlugger already does these simple mappings, I think it makes sense to make the map locale-aware (as the class already is).

@nicolas-grekasnicolas-grekas changed the titleFix ticket 36383 slug only in english[String] Add locale-sensitive map for slugging symbolsApr 16, 2020
@nicolas-grekas
Copy link
Member

nicolas-grekas commentedApr 16, 2020
edited
Loading

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
ContributorAuthor

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

Copy link
Member

@nicolas-grekasnicolas-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

@NyholmNyholm 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

Thank you@lmasforne.

@fabpotfabpot merged commit260dea0 intosymfony:masterApr 24, 2020
@nicolas-grekasnicolas-grekas modified the milestones:next,5.1May 4, 2020
@fabpotfabpot mentioned this pull requestMay 5, 2020
ThomasLandauer added a commit to ThomasLandauer/symfony that referenced this pull requestNov 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 insymfony#36456
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@NyholmNyholmNyholm approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.

[String] Slugger should only replace special characters for the English locale
5 participants
@lmasforne@nicolas-grekas@fabpot@Nyholm@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp