Add emoji support for url slugger #6916
Conversation
nozarashi20
force-pushed
the
feat/emoji-support
branch
from
1815058
to
feb87f2
Compare
nozarashi20
force-pushed
the
feat/emoji-support
branch
from
5cf08d2
to
bca1334
Compare
There was a problem hiding this comment.
@nozarashi20 Really thank you for working on this 👍 . I think the IntlException should not happen in case of the slugger. In which case it does happen? I think we should create an issue on symfony side for this. If it is resolved we could maybe move the whole withEmoji into the service definition if possible.
| $slug = $this->slugger->slug($part, '-', $languageCode); | ||
| try { | ||
| $slug = $this->slugger->slug($part, '-', $languageCode); | ||
| } catch (\IntlException $e) { // handle unknown emoji transliterator id |
There was a problem hiding this comment.
This sounds like something which should be reported to symfony itself. I think the slugger should just ignore if it does not know something.
There was a problem hiding this comment.
@alexander-schranz , this is the condition that triggers the IntlException.
It happens when there is no mapping file for a locale. For instance, there is no emoji-de_at.php file; which leads to this error
There was a problem hiding this comment.
Thank you for detailed debugging here. Let see if symfony will change here something. Created an issue here: symfony/symfony#48364
Else I think we should just in the constructor created a $this->sluggerWithEmoji and $this->sluggerWithoutEmoji to avoid creating new objects always inside the loop.
There was a problem hiding this comment.
Think we not longer need the sluggerWithEmoji? Since symfony/symfony#48395?
Also I think we should do str_replace('-', '_', $languageCode) to convert it into a valid intl locale. @nozarashi20 What do you think?
There was a problem hiding this comment.
Great @alexander-schranz , the code will be much simpler.
And I agree on replacing dashes with underscore. We'll then be able to handle codes like en-US.
Have you already encounter this case ?
There was a problem hiding this comment.
On other cases we did also just output the language this way explode('-', $locale)[0] but as it could be that the slugger support en-us and en-uk I would give the whole intl locale into it, if possible.
I think for general handling we should create an own issue how we maybe normalize the locale better in future. So maybe all things are handle by intl based format like en_UK problem is that most website don't want to have domain.com/en_UK´ instead domain.com/en-uk` in it. But as said that is an own issue by itself, so first concentrate on the slugger case.
There was a problem hiding this comment.
Updated code and tests
…underscore in language code
nozarashi20
force-pushed
the
feat/emoji-support
branch
from
a3e805a
to
fbeeebc
Compare
|
@nozarashi20 Thank you! Good work 👍 |
What's in this PR?
Add emoji support for url slugger
Why?
Currently emojis are just removed and not replaced by the name when slugging a title with emoji for it.