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

[Mailer][TwigBridge] Add support for translatable subject#59967

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
fabpot merged 1 commit intosymfony:7.3fromnorkunas:translatable-mail-subject
Mar 26, 2025

Conversation

norkunas
Copy link
Contributor

QA
Branch?7.3
Bug fix?no
New feature?yes
Deprecations?no
IssuesFix#54203
LicenseMIT

@norkunas
Copy link
ContributorAuthor

@stof can you review this?

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.

Instead of this, which adds cross-cutting concerns to the base class (= combinatory explosion), what about allowing Email objects to implement TranslatableInterface?

@norkunas
Copy link
ContributorAuthor

Instead of this, which adds cross-cutting concerns to the base class (= combinatory explosion), what about allowing Email objects to implement TranslatableInterface?

So the trans method would act as a subject getter?

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedMar 17, 2025
edited
Loading

Hum, maybe not, I thing I had something else in mind ("static" instead of "string" as return type):

interface TranslatableInterface{publicfunctiontrans(TranslatorInterface$translator, ?string$locale =null):static;}

🤔

@norkunas
Copy link
ContributorAuthor

For me it doesn't matter if I'd need to call trans or translatableSubject, still it will need to be stored as it's done now, no?

@kbond
Copy link
Member

What about having the existing subject accept a stringable object? Then, in the listener (or a translation specific one), check for an instance of TranslatableMessage and translate?

@norkunas
Copy link
ContributorAuthor

What about having the existing subject accept a stringable object? Then, in the listener (or a translation specific one), check for an instance of TranslatableMessage and translate?

If argument widening is not a BC break then sure

@norkunasnorkunasforce-pushed thetranslatable-mail-subject branch froma9b69b4 tofb59ac0CompareMarch 26, 2025 05:18
@norkunas
Copy link
ContributorAuthor

What about having the existing subject accept a stringable object?

Done.

@norkunasnorkunasforce-pushed thetranslatable-mail-subject branch fromfb59ac0 to97eb49bCompareMarch 26, 2025 08:36
@fabpot
Copy link
Member

Thank you@norkunas.

norkunas reacted with heart emoji

@fabpotfabpot merged commitfcdc0cf intosymfony:7.3Mar 26, 2025
3 of 11 checks passed
@norkunasnorkunas deleted the translatable-mail-subject branchMarch 26, 2025 08:53
@@ -58,9 +58,9 @@ class Email extends Message
/**
* @return $this
*/
public function subject(string $subject): static
public function subject(string|\Stringable $subject): static
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This may be a breaking change for apps that extend this class and override this method (https://3v4l.org/uuJj9)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Supporting a TranslatableInterface is absolutely not the same than supporting Stringable. There is no requirement for TranslatableInterface to be Stringable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yeah, my idea was off the cuff. Needs to be thought out more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

btw, even for the core TranslatableMessage (that is stringable), casting it to string does not translate it. It returns the translation key.

Copy link
Member

@kbondkbondMar 26, 2025
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Right, I thought that'd be fine as that's the behavior of using keys that don't have a translation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

My thinking was to allow the subject to beany stringable object allowing a listener to intercept. If none does, it would just be cast to a string.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Adding support for Stringable and then doing weird magic to support TranslatableMessage while the changelog says it adds support for translatable subject (which would be understood as being the interface by many people) looks wrong to me (and limiting as there are valid use cases for other implementations of TranslatableInterface)

@stof
Copy link
Member

Hum, maybe not, I thing I had something else in mind ("static" instead of "string" as return type):

interface TranslatableInterface{    public function trans(TranslatorInterface $translator, ?string $locale = null): static;}

This does not make sense. The return value ofTranslatableInterface::trans is the translated text, which must be a string, not an instance of the current class (it would be impossible to implement that interface properly)

@kbond
Copy link
Member

@norkunas, without this pr, you could achieve subject translation in the template I think?

{%do email.setSubject(...translate...) %}

@norkunas
Copy link
ContributorAuthor

@norkunas, without this pr, you could achieve subject translation in the template I think?

{%do email.setSubject(...translate...) %}

This looks like a workaround :)

fabpot added a commit that referenced this pull requestMar 26, 2025
…ble objects" (kbond)This PR was squashed before being merged into the 7.3 branch.Discussion----------[Mailer][TwigBridge] Revert "Add support for translatable objects"| Q             | A| ------------- | ---| Branch?       | 7.3| Bug fix?      | no| New feature?  | no| Deprecations? | no| Issues        | Reverts#60047 &#59967| License       | MIT#59967 introduced a BC break so we need to revert and find another solution.Commits-------41cc1d6 code reviewf638a6a Revert "[Mailer][TwigBridge] Add support for translatable subject"e51607b Revert "fix compatibility with TwigBridge < 7.3"
@fabpotfabpot mentioned this pull requestMay 2, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@kbondkbondkbond left review comments

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@stofstofstof left review comments

@ycerutoycerutoyceruto left review comments

@fabpotfabpotfabpot approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.

Subject of TemplatedEmail should also be in a template
7 participants
@norkunas@nicolas-grekas@kbond@fabpot@stof@yceruto@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp