-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Add a MessageFormatter Phrase renderer #22996
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
Add a MessageFormatter Phrase renderer #22996
Conversation
Hi @navarr. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
c07b2ad
to
ffba68e
Compare
This gives us the ability to use ICU MessageFormatter formatting strings (inc. placeholders) for better support of internationalization.
Naive performance testing results (testing only the cost of the formatting in different scenarios - data for the comment) Cost for non-MessageFormatter strings:
Cost for MessageFormatter strings:
"Speed Difference" is MessageFormatter÷strpos or strpos÷MessageFormatter All costs are per 1,000,000 operations. Naive code at: https://gist.github.com/navarr/e4c2b69b0d0fd3ca40b764e771640d11 |
ffba68e
to
7f88d91
Compare
lib/internal/Magento/Framework/Phrase/Renderer/MessageFormatter.php
Outdated
Show resolved
Hide resolved
lib/internal/Magento/Framework/Phrase/Test/Unit/Renderer/MessageFormatterTest.php
Outdated
Show resolved
Hide resolved
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.
Hi @navarr,
Thank you for contribution! Really good job!
Could you review comments from me and from @joni-jones?
lib/internal/Magento/Framework/Phrase/Test/Unit/Renderer/MessageFormatterTest.php
Show resolved
Hide resolved
* Declares strict types properly * @inheritdoc -> @inheritdoc * Simplifies FQNs
I've performed the changes as requested 👍 |
Hi @ihor-sviziev, thank you for the review. |
Hello @navarr Unfortunately, we are stuck on the testing stage for this PR. So, could you please help us with user scenarios/steps and expected result for manual testing? Thank you in andvance |
Hi @sdzhepa, In code we can see examples: Even in the issue description here is Japanese example:
For testing you should use this Japanese locale in your magento store, add ja_JP translation to csv file like this:
And try to execute following translation with code:
IMO we should add few translated phrases to the tests, but looks like it will be really not so easy |
@ihor-sviziev Thanks for explanation |
✔️ QA passed |
Hi @sivaschenko, |
Hi @navarr, thank you for your contribution! |
This gives us the ability to use ICU MessageFormatter formatting strings (inc. placeholders) for better support of internationalization.
MessageFormatter provides this by essentially giving a massive amount of flexibility to translatable strings, for example, consider a phrase describing how many orders there are. In English, you might create:
Or maybe
These work fine for english, but the first one creates three phrases for the same string (and falls apart for languages such as Arabic and Ukranian) - and the second one falls apart in foreign languages.
With MessageFormatter:
This can then be mapped however necessary in foreign languages. Japanese for example:
This is just a small, quick example of the flexibility MessageFormatter provides over the current system. Because of this form of processing, it also adds support for plurals and ordinal selection. It also provides support for locale based date and currency formatting, but Magento's built-ins may be preferred for such functionality.
Contribution checklist (*)