Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Translation] AddStaticMessage#60935
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This comment has been minimized.
This comment has been minimized.
nicolas-grekas left a comment
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.
can you please add a line in the changelog ?
mdeboer commentedJun 29, 2025
Just out of curiosity, do you have any examples where you have aTranslatable message, that shouldn't be translated? Don't get me wrong but to me it really smells. ATranslatable object should inherently betranslatable. Setting the But maybe some examples would clear things up for me why this path was chosen. Thanks in advance! 🙏🏻 |
VincentLanglet commentedJun 29, 2025
Let's just answer this point first@mdeboer, it's the purpose of the
Sure, for instance Maybe the right behavior would be:
but:
I find it simpler to allow |
false as domain fortranslatableMessagefalse as domain fortranslatableMessagefalse as domain forTranslatableMessagemdeboer commentedJun 29, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thanks for your reply@VincentLanglet I understand that in case of EasyAdmin it is easier to allow this change, but imho this is a design flaw in EasyAdmin. Why should the framework bend the rules and accommodate this bad design pattern? Wouldn't something like Rector be of use to refactor EasyAdmin? Or maybe introduce something like an |
07d269e tocdd516bCompareVincentLanglet commentedJun 29, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
No, because
And while it could be considered as a design flaw, be aware that
For instance, form labels are translated by default by Symfony, and they allow a false translation domain now, but you could use the same argument to say it shouldn't translate and wait for TranslateMessage in order to produce a translation, or you should pass the label already-translated.
Might be better indeed (cc@nicolas-grekas if you want to re-review since the whole PR changed). |
false as domain forTranslatableMessageUntranslatableMessageUntranslatableMessageNonTranslatableMessage
mdeboer left a comment
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.
Nice, looks good to me 👍🏻 Now it is a clear indicator that the message should not be translatedand it fixes the original problem.
Uh oh!
There was an error while loading.Please reload this page.
NonTranslatableMessageNonTranslatableMessagexabbuh commentedJun 30, 2025
I am not convinced that this really is the way to go. Having something passed around that implements an interface called |
VincentLanglet commentedJun 30, 2025
What alternative do you propose then for#60935 (comment) ?@xabbuh Moreover this is still an implementation which can be done in the userland code, but it would be useful to provide it by default, in the same way symfony provides IdentityTranslator which is described with the comment: |
xabbuh commentedJun 30, 2025
The issue you outline in#60935 (comment) should be adressed in EasyAdminBundle and if it's simply shipping the |
VincentLanglet commentedJun 30, 2025
So instead of creating a NonTranslatableMessage in Symfony, with a really generic behavior of "Keeping the message as if even if someone try to translate it later", you recommend to add this class in every bundle with this need. That seems odd to me :/. EasyAdminBundle is not the only one bundle with the Seems like a classic behavior of Symfony to me when providing an interface to often
If the That could also allows to simplify code for some libs when you currently have to support |
nicolas-grekas left a comment
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.
I'm fine with adding this personally - that's a valid implementation of the interface to me (each one their specific behavior after all).
I'm wondering about the naming: you can calltrans() so technically this is translatable. It's just that the translation is fixed.
LiteralMessage? RawMessage? StaticMessage?
VincentLanglet commentedJul 7, 2025
I'm fine with all of three. Any preference@nicolas-grekas ? |
VincentLanglet commentedJul 10, 2025
I renamed it to StaticMessage then@OskarStark@nicolas-grekas |
NonTranslatableMessageStaticMessageOskarStark commentedJul 12, 2025
PR body would need an update |
…ng` (VincentLanglet)This PR was squashed before being merged into the 7.4 branch.Discussion----------[Translation] Deprecate `TranslatableMessage::__toString`| Q | A| ------------- | ---| Branch? | 7.4| Bug fix? | no| New feature? | no| Deprecations? | yes| Issues | Fix #... <!-- prefix each issue number with "Fix #"; no need to create an issue if none exists, explain below -->| License | MITResolve the following discussion#60935 (comment)cc `@stof` `@xabbuh`Commits-------916befb [Translation] Deprecate `TranslatableMessage::__toString`
92e35ca tob75e752CompareVincentLanglet commentedJul 17, 2025
Uh oh!
There was an error while loading.Please reload this page.
0fb2b74 tob329818Comparenicolas-grekas commentedJul 24, 2025
Thank you@VincentLanglet. |
3686363 intosymfony:7.4Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
When working with translation, it's "classic" to have library with the following logic:
Then if the user doesn't want the text to be translated, he cannot.
A useful and simple way would be to support
new TranslatableMessage($text, domain: false)but as recommended, it might be better to introduce a special class for this, the StaticMessage