[Mailer][TwigBridge] Add support for translatable subject #59967
Conversation
norkunas
commented
Mar 13, 2025
| Q | A |
|---|---|
| Branch? | 7.3 |
| Bug fix? | no |
| New feature? | yes |
| Deprecations? | no |
| Issues | Fix #54203 |
| License | MIT |
aefcbb8 to
a9b69b4
Compare
|
@stof can you review this? |
So the trans method would act as a subject getter? |
|
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;
}🤔 |
|
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? |
|
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 |
a9b69b4 to
fb59ac0
Compare
Done. |
src/Symfony/Component/Mailer/Tests/EventListener/MessageListenerTest.php
Outdated
Show resolved
Hide resolved
fb59ac0 to
97eb49b
Compare
|
Thank you @norkunas. |
| @@ -58,9 +58,9 @@ class Email extends Message | |||
| /** | |||
| * @return $this | |||
| */ | |||
| public function subject(string $subject): static | |||
| public function subject(string|\Stringable $subject): static | |||
There was a problem hiding this comment.
This may be a breaking change for apps that extend this class and override this method (https://3v4l.org/uuJj9)
There was a problem hiding this comment.
Supporting a TranslatableInterface is absolutely not the same than supporting Stringable. There is no requirement for TranslatableInterface to be Stringable.
There was a problem hiding this comment.
Yeah, my idea was off the cuff. Needs to be thought out more.
There was a problem hiding this comment.
btw, even for the core TranslatableMessage (that is stringable), casting it to string does not translate it. It returns the translation key.
There was a problem hiding this comment.
Right, I thought that'd be fine as that's the behavior of using keys that don't have a translation.
There was a problem hiding this comment.
My thinking was to allow the subject to be any stringable object allowing a listener to intercept. If none does, it would just be cast to a string.
There was a problem hiding this comment.
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)
This does not make sense. The return value of |
|
@norkunas, without this pr, you could achieve subject translation in the template I think? {% do email.setSubject(...translate...) %} |
This looks like a workaround :) |
…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 review f638a6a Revert "[Mailer][TwigBridge] Add support for translatable subject" e51607b Revert "fix compatibility with TwigBridge < 7.3"