Skip to content

[String] Slugger with Emoji throws IntlException for not supported locale #48364

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.

Sign up for GitHub

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

Jump to bottom
Closed
alexander-schranz opened this issue Nov 28, 2022 · 5 comments
Closed

[String] Slugger with Emoji throws IntlException for not supported locale #48364

alexander-schranz opened this issue Nov 28, 2022 · 5 comments
Labels
Bug Status: Needs Review String

Comments

@alexander-schranz
Copy link
Contributor

alexander-schranz commented Nov 28, 2022 •
edited
Loading

Symfony version(s) affected

6.2.0

Description

In @sulu we try to add the Slugger with Emoji support. Currently the Slugger will hardly fail when the locale is not found with IntlException: https://github.com/sulu/sulu/actions/runs/3554459366/jobs/5970575754#step:10:578

How to reproduce

$slugger = new \Symfony\Component\String\Slugger\AsciiSlugger();
$slugger = $slugger->withEmoji();
$slugger->slug('Menus with 🍕 or 🍝', '-', 'de-at');

Possible Solution

Catch exception in fallback to normal slug for not supported locale.

Additional Context

We currently don't know which locales are supported by withEmoji and which ones not. Which ends up in strange code if we need to handle that ourselves:

try {
    $slug = $this->sluggerWithEmojiSupport->slug($part, '-', $languageCode);
} catch (\IntlException $e) {
    $slug = $this->sluggerWithoutEmojiSupport->slug($part, '-', $languageCode);
}

/cc @lyrixx What do you think?
@alexander-schranz alexander-schranz mentioned this issue Nov 28, 2022
Merged
@lyrixx
Copy link
Member

lyrixx commented Nov 28, 2022

Indeed, this is not really DX friendly!

@nicolas-grekas any recommendation here?

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Nov 28, 2022 •
edited
Loading

Catch exception in fallback to normal slug for not supported locale.

I suppose this is the way?

Alternatively, we might want to use a fallback locale?

Another option would be to remove the emojis from the slug. I know @fancyweb is considering this. But maybe that'd be solved by the fallback-locale option, by specifying a special skip locale?

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Nov 28, 2022 •
edited
Loading

Let's make withEmoji() final if we anticipate that we might need to change its signature?
6.2.0 is around the corner.

@derrabus derrabus changed the title [Slugger] Slugger with Emoji throws IntlException for not supported locale [String] Slugger with Emoji throws IntlException for not supported locale Nov 28, 2022
@fancyweb
Copy link
Contributor

I think we expect a locale with _ and not with -. Shouldn't we first fall back to the "parent locale" de?

@fancyweb fancyweb mentioned this issue Nov 29, 2022
Merged
nicolas-grekas added a commit that referenced this issue Nov 30, 2022
@nicolas-grekas
bug #48395 [String] Fix AsciiSlugger with emojis (fancyweb)
aa79198 
This PR was merged into the 6.2 branch.

Discussion
----------

[String] Fix AsciiSlugger with emojis

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | #48364
| License       | MIT
| Doc PR        | -

First fix: let's fall back to parent locales if the emoji map does not exist for the full locale? (like we do for symbols map)

Second fix: let's just ignore the emoji transliterator for totally unsupported locales, the behavior of the slugger will just strip the emojis like it already does.

Commits
-------

5f320e5 [String] Fix AsciiSlugger with emojis
@fancyweb fancyweb closed this as completed Dec 2, 2022
@alexander-schranz
Copy link
Contributor Author

alexander-schranz commented Dec 7, 2022 •
edited
Loading

I think we will need todo also the str_replace('-', '_', 'de-at') that the fallback works. I need to recheck sometime the whole locale handling and how it works in symfony.

Mostly we have urls like /de-at and but I think a normalizer would make sense here translator and other kind of services get it as de_AT which I think is the more standardized format. Symfony does I think not do any normalization with the request locale?

PS: Thx for the quick fix 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Status: Needs Review String
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@nicolas-grekas @lyrixx @derrabus @alexander-schranz @fancyweb @carsonbot