Allow translators to return stringables #44911
Conversation
|
This is inline with \Symfony\Component\Validator\ConstraintViolation::__construct() which accepts a \Stringable for $message. |
|
What is the use-case here? |
|
@derrabus Drupal only actually translates strings if they are rendered to string. This change allows our implementation of TranslatorInterface to continue to return Drupal's TranslatableMarkup objects as it does currently - see https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Validation%21DrupalTranslator.php/function/DrupalTranslator%3A%3Atrans/9.3.x |
|
It also allows the messages to contain HTML and not be auto-escaped by Twig. |
|
Isn't this a BC-break since now consumers of the interface will have to deal with |
|
@jvasseur no, because PHP allows for more precise return type hints: https://3v4l.org/O8aeY#v8.0.14 |
|
@tgalopin the comment was talking about consumers, not about implementers |
|
Ah sorry! |
There was a problem hiding this comment.
I really don't feel comfortable with this change, for multiple reasons.
As already mentioned, a consumer of this interface would currently not expect an implementation to return anything but a string. This includes userland code but also Symfony's own components.
Secondly, the FIG is currently in the progress of standardizing interfaces for translators and I don't expect those interfaces to allow a Stringable return either. It would be unfortunate if merging this change would prevent us from adopting the FIG standard in the future.
My suggestion: Introduce another public method to you translator with the return type you're suggesting. Consumers that can handle stringable objects may call your method and you could implement the Symfony contract without violating it.
I think that this issue points to the fact that maybe the FIG standard needs to be changed. Stringables are very useful in code that is meant to be interoperable. It allows consumers of libraries to decorate strings with additional information that the require and the library can be completely ignorant of it. |
As pointed out already \Symfony\Component\Validator\ConstraintViolation::__construct() uses the translator and permits stringables. |
|
@alexpott that's only one usage though. |
Maybe, I don't know. This standard is still a work in progress. If you want to make sure that it includes stringable objects, please get in contact with the FIG.
That may very well be the case and I'm pretty sure you're doing useful stuff with them. But that does not change the fact that our current translation contract guarantees that a translator implementation returns an actual string. None of Symfony's components is tested with stringable objects as translated strings and downstream projects certainly won't expect them either.
If that were the case, we would not have this discussion. Strings and stringable objects might behave similar mostly, but the very fact that you feel urged to change the return type proves that in the end they're just not the same. Supporting stringable objects creates little edge cases everywhere.
I've made a suggestion in my previous answer. Can you please elaborate why that suggestion would not work for you? |
|
Drupal has its own translation component, the only reason we have Symfony's as a dependency is because we use the Validation component, which requires the interface. Symfony\Component\Validator::getMessage() (for example) returns String|Stringable and this is what we're relying on. ConstraintViolationBuilder::addViolation() and ExecutionContext::addViolation() both call the trans() method, so we can't just add our own method to the translator, we'd have to fork those classes too to call it. |
|
Wouldn't it make sense for Drupal to have its own Translator contract (as it already has its own Translator implementation, which from the sounds of it is quite different from Symfony's). Drupal would then implement a |
|
For illustration, I've been made aware of some of the edge-case bugs with stringable objects. They really act as an object, not as a string. E.g. when using them in combination with |
Okay, and you need your stringable objects to appear in those violation lists? Yes, in that case you would indeed need provide your own implementations of We could also think about adding a new contract that supports stringable and that the validator can consume instead of the current one. |
|
Drupal previously did have custom implementations of these interfaces, but we tried to move to using Symfony's classes directly - mainly because some parts of the validator component including methods on ExecutionContextInterface are tagged @internal which triggered warnings: "Used by the validator engine. Should not be called by user code. It may change without further notice. You should not extend it" |
|
Indeed. Building your own execution context is probably not the best idea I had today. 🙈 The validator is not very extensible when it comes to rendering violation messages. Maybe this is something we could change? |
|
I think having a translator decorator is by far the easiest solution. This way, Drupal can create their own contract fulfilling their own wishes. And they can use the decorator whenever they need to connect their translator to a Symfony component (such as the validator). |
|
@wouterj how would a decorator resolve the issue of:
|
|
@catch56 if you don't expect to receive back from the validator the Stringable objects you passed in (ie if you're fine with the validator turning them into strings), a decorator could work as @wouterj said. If you wish to keep the Stringable object for later display in the application, the decorator indeed won't work (as it would necessarily transform Stringables to strings). In such case, I agree with @derrabus : we could introduce a new version of the contract, as a new interface, and progressively add support for such interface in Symfony components/end-user libraries. That would avoid the hard BC break on consumers of the interface, while providing a migration path for most of them. |
interface DrupalTranslatorInterface
{
public function trans(string $id, array $parameters): \Stringable|string;
}
class SymfonyTranslator implements SymfonyTranslatorInterface
{
public function __construct(
private DrupalTranslatorInterface $translator
) {}
public function trans(string $id, array $parameters): string
{
return (string) $this->translator->trans($id, $parameters);
}
}
$drupalTranslator = new DrupalTranslator(...);
$validator = new Validator(new SymfonyTranslator($drupalTranslator));This is a pattern that Symfony uses a lot to provide interop with many standards in PHP. E.g. |
We've already implemented that approach in https://www.drupal.org/project/drupal/issues/3255245 (casting the translated strings early, only for validation messages), but it's not ideal (see discussion in the issue), which is why this PR is open to try to find an alternative to doing that. Also the question of whether we'll end up needing to do similar in more places over time if TranslatorInterface starts being used in other components at any point. |
|
On the other end, listening to feedback from users is a nice way to make the best possible decision. |
|
Forcing everybody to cast is not worth it IMHO. |
|
This PR is providing continuity with what was already possible in practice one month ago. It's not bringing anything new to me. |
|
Im pretty sure nobody's expecting this: Where implicit casting as a side-effect may occur ..., i have no idea :) What bugs me more, is at this point the abstraction is broken, as we dont know the Stringable object is an actual translated string. I tend to believe Drupal's issues are a) poor design/integrations, or b) tied to Validator (#44911 (comment)) |
|
Saying other's design is broken is too easy, at least for my own satisfaction. Here, I feel like we closed a door without realizing it, and one of our advanced users is affected unintentionally. I'd like to know what Drupal's folks think about possible alternatives? Add a |
|
Looking at the code from https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Validation%21DrupalTranslator.php/function/DrupalTranslator%3A%3Atrans/9.3.x Drupal's I'd explore that contract first :) |
|
Sorry, i missed Now im confused, what exactly is blocking Drupal from doing (Or use the Symfony translator if needed, but handle TranslatableMarkup state accordingly still) |
|
The suggestion of decorating Translator (to cast TranslatableMarkup objects to string in the ::trans() method), is something we already have implemented in an PR, but would prefer not to do - i.e. we opened this PR specifically to see if we could avoid having to do that. Of the alternative things that Symfony could do, trying to summarise what I think they are:
For me personally, (1) seems like it's correcting an oversight, (2) would be fine for us but likely to end up requiring more changes in Symfony components overall (since they generally seem to handle stringable already), (3) feels like it's punting things and could result in different problems later on, but still preferable to the early cast workaround for me. |
|
Im suggesting We could sell But as mentioned, SF by default can invoke the translator pretty lazy (eg. at templating level) if you work with translatables instead. It sounds like the issue is actually Validator supporting |
|
Thanks for the details @catch56, all those options are reasonable to me, and I agree with your analysis of the 3 options. I proposed
We can make it accept |
|
As I understand @ro0NL's proposal, it's to make e.g. There appear to be two main problems with this:
|
|
Understood. My first concern are Symfony users, who would seem to benefit more from This looks quite an API challenge yes :) but if Translatable is the extension point, then Validator its own API (getParameter et al) could be up for deprecation IMHO, rather than bridging it somehow and complexifying things. Nevertheless, if Drupal is truly stuck, then sure why not :) Maybe in the future Symfony will benefit from self-translating-strings too 👍 Should we also discuss |
|
What i'd ultimately like to achieve is something like https://3v4l.org/21rjj#v8.1.1 :) |
|
It looks like I'm not going to be able to convince other core team members. Now I'm wondering: can't you turn a translatable into a stringable by using a decorator like this one? use Symfony\Component\String\LazyString;
$stringable = LazyString::fromCallable(static function () use ($translatable, $translator, $locale) {
return $translatable->trans($translator, $locale);
}) |
This shouldn't need pointing out, but Drupal is a Symfony user. |
|
About making Validator accept TranslatableInterface, I had a look at the code and it doesn't make sense to me. Stringable fills the need for lazy string on this component IMHO. @alexpott @catch56 what about my previous comment? #44911 (comment) |
|
@nicolas-grekas as per the PR, stringable is enough for us, the problem is it not being accepted consistently everywhere. |
Are you saying symfony/src/Symfony/Component/Validator/Context/ExecutionContextInterface.php Lines 125 to 126 in 6b9fafb I tend to believe it's the way for Drupal, or put different widening the Translator type has least preference for me. |
|
I'm closing because we don't have a consensus here. |
Extends the scope of possible return values from \Symfony\Contracts\Translation\TranslatableInterface::trans() and \Symfony\Contracts\Translation\TranslatorInterface::trans() to aid Symfony 6 adoption by Drupal 10. This change should not result in an changes to existing code as returning a string is still valid.