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

[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

Merged

Conversation

@VincentLanglet
Copy link
Contributor

@VincentLangletVincentLanglet commentedJun 28, 2025
edited
Loading

QA
Branch?7.4
Bug fix?no
New feature?yes
Deprecations?no
IssuesFix #...
LicenseMIT

When working with translation, it's "classic" to have library with the following logic:

if ($messageinstanceof Translatable) {$message->trans($translator);}else {$translator->trans($message, domain:'OpenSourceBundle');}

Then if the user doesn't want the text to be translated, he cannot.

A useful and simple way would be to supportnew TranslatableMessage($text, domain: false) but as recommended, it might be better to introduce a special class for this, the StaticMessage

@carsonbot

This comment has been minimized.

@VincentLangletVincentLanglet marked this pull request as ready for reviewJune 28, 2025 07:15
@carsonbotcarsonbot added this to the7.4 milestoneJun 28, 2025
@carsonbotcarsonbot changed the titleAllow to use false as domain for translatableMessage[Translation] Allow to use false as domain for translatableMessageJun 29, 2025
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.

can you please add a line in the changelog ?

@mdeboer
Copy link
Contributor

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 thedomain to false does not really tell me that it shouldn't be translated, rather I would expect a "default" domain to be used like "messages".

But maybe some examples would clear things up for me why this path was chosen.

Thanks in advance! 🙏🏻

@VincentLanglet
Copy link
ContributorAuthor

Setting thedomain to false does not really tell me that it shouldn't be translated, rather I would expect a "default" domain to be used like "messages".

Let's just answer this point first@mdeboer, it's the purpose of theNULL domain, you can find it in a lot of "domain" options likehttps://symfony.com/doc/current/reference/forms/types/form.html#translation-domain

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.

Sure, for instanceEasyAdmin always translate his labels, title, flash message, and more
https://github.com/EasyCorp/EasyAdminBundle/blob/4.x/templates/flash_messages.html.twig#L12
It use messages or EasyAdminBundle as default translation domains based on the context, but support usingTranslatable object in order to use another domain but doesn't support disabling the translation.

Maybe the right behavior would be:

  • to not translate by default
  • to ask users to pass translatableMessage instead

but:

  • it's a big BC break
  • when 99% of labels asked to be translated it ask addingt() everywhere

I find it simpler to allowfalse as translation_domain for such library ; especially because it's not the first library I saw with this "translate by default"...

@OskarStarkOskarStark changed the title[Translation] Allow to use false as domain for translatableMessage[Translation] Allow to usefalse as domain fortranslatableMessageJun 29, 2025
@OskarStarkOskarStark changed the title[Translation] Allow to usefalse as domain fortranslatableMessage[Translation] Allow to usefalse as domain forTranslatableMessageJun 29, 2025
@mdeboer
Copy link
Contributor

mdeboer commentedJun 29, 2025
edited
Loading

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 anUntranslatableMessage?

rvanlaak reacted with thumbs up emoji

@VincentLanglet
Copy link
ContributorAuthor

VincentLanglet commentedJun 29, 2025
edited
Loading

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?

No, because

  • it's mainly translated in the view which are twig templates
  • such change would be an HARD BC-break

And while it could be considered as a design flaw, be aware that

  • such libs was implemented before the introduction of translatable interface
  • it could also be considered as a useful shortcut to automatically translate text when it's the case in 99%.

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.

Or maybe introduce something like anUntranslatableMessage?

Might be better indeed
I update the PR with aNonTranslatableMessage@mdeboer

(cc@nicolas-grekas if you want to re-review since the whole PR changed).

mdeboer reacted with heart emoji

@VincentLangletVincentLanglet changed the title[Translation] Allow to usefalse as domain forTranslatableMessage[Translation] IntroduceUntranslatableMessageJun 29, 2025
@VincentLangletVincentLanglet changed the title[Translation] IntroduceUntranslatableMessage[Translation] IntroduceNonTranslatableMessageJun 29, 2025
Copy link
Contributor

@mdeboermdeboer left a 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.

OskarStark reacted with thumbs up emoji
@OskarStarkOskarStark changed the title[Translation] IntroduceNonTranslatableMessage[Translation] AddNonTranslatableMessageJun 30, 2025
@xabbuh
Copy link
Member

I am not convinced that this really is the way to go. Having something passed around that implements an interface calledTranslatableInterface which then ends up not being translated is unexpected behaviour to me. Thus I am 👎 on adding thisNonTranslatableMessage class.

@VincentLanglet
Copy link
ContributorAuthor

I am not convinced that this really is the way to go. Having something passed around that implements an interface calledTranslatableInterface which then ends up not being translated is unexpected behaviour to me. Thus I am 👎 on adding thisNonTranslatableMessage class.

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:

IdentityTranslator does not translate anything.

@xabbuh
Copy link
Member

The issue you outline in#60935 (comment) should be adressed in EasyAdminBundle and if it's simply shipping theNonTranslatableMessage class IMO.

@VincentLanglet
Copy link
ContributorAuthor

The issue you outline in#60935 (comment) should be adressed in EasyAdminBundle and if it's simply shipping theNonTranslatableMessage class IMO.

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 thetranslate by default behavior.

Seems like a classic behavior of Symfony to me when providing an interface to often

  • Give the real implementation
  • Provide a stub/fixed/basic implementation

If theNonTranslatableMessage does not makes sens, the IdentityTranslator shouldn't have a lot more.

That could also allows to simplify code for some libs when you currently have to supportstring|TranslatableInterface now you could only supportTranslatableInterface...

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 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
Copy link
ContributorAuthor

LiteralMessage? RawMessage? StaticMessage?

I'm fine with all of three.
Can also be IdentityMessage (since we have an IdentityTranslator) or FixedMessage.

Any preference@nicolas-grekas ?

@VincentLanglet
Copy link
ContributorAuthor

I likeStaticMessage, but just my 2 cents

I renamed it to StaticMessage then@OskarStark@nicolas-grekas

@VincentLangletVincentLanglet changed the title[Translation] AddNonTranslatableMessage[Translation] AddStaticMessageJul 11, 2025
@OskarStark
Copy link
Contributor

PR body would need an update

VincentLanglet reacted with thumbs up emoji

nicolas-grekas added a commit that referenced this pull requestJul 17, 2025
…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`
@VincentLangletVincentLangletforce-pushed thetranslatableFalse branch 3 times, most recently from92e35ca tob75e752CompareJuly 17, 2025 22:05
@VincentLanglet
Copy link
ContributorAuthor

Ready for review@xabbuh@stof

@nicolas-grekas
Copy link
Member

Thank you@VincentLanglet.

@nicolas-grekasnicolas-grekas merged commit3686363 intosymfony:7.4Jul 24, 2025
11 checks passed
This was referencedOct 27, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@stofstofstof approved these changes

@welcoMatticwelcoMatticwelcoMattic approved these changes

@OskarStarkOskarStarkOskarStark approved these changes

@xabbuhxabbuhAwaiting requested review from xabbuh

+1 more reviewer

@mdeboermdeboermdeboer approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

7.4

Development

Successfully merging this pull request may close these issues.

8 participants

@VincentLanglet@carsonbot@mdeboer@xabbuh@stof@OskarStark@nicolas-grekas@welcoMattic

[8]ページ先頭

©2009-2025 Movatter.jp