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 Stringable objects in addition to strings ?

@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!

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.

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.

This includes userland code but also Symfony's own components.

As pointed out already \Symfony\Component\Validator\ConstraintViolation::__construct() uses the translator and permits stringables.

@alexpott that's only one usage though.

I think that this issue points to the fact that maybe the FIG standard needs to be changed.

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.

Stringables are very useful in code that is meant to be interoperable.

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.

the library can be completely ignorant of it.

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.

TranslatorInterface defines the behavior that the Symfony components expect from a translator. Why would you want to implement that contract and then immediately violate it? That does not make sense.

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 SymfonyCompatibleTranslator that decorates the Drupal Translator using the Symfony Translator contract (for usage with the Validator component).
From my knowledge, this is quite a common approach in the modern interop world (see e.g. the multiple interop decorator classes in the HttpClient component).

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 json_encode() (https://3v4l.org/9TdNd), which is a common use of the Translator (we even use it like this in the Symfony code).
This makes me also worry a lot more about adding this suggestion.

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.

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 ExecutionContextInterface and ConstraintViolationBuilderInterface that are able to interface with your translator. Have you tried to do so?

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"

See https://www.drupal.org/project/drupal/issues/3231683

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:

• The Validator requiring a class that implements TranslatorInterface

• Various Validator classes (with even interface methods marked @internal, so not replaceable without warnings from the debug classloader) calling the ::trans() method, which is defined on that interface?

@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.

@catch56

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. Psr16Cache and Psr18Client.

@tgalopin

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.

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.
I'm 👍 personally here as I don't see any reason to reject this and it makes the abstraction more powerful/useful.

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.
Lazy strings are a really legit concern, strategy and design to solve some problems.
We use them eg for autowiring errors and I'm glad PHP 8 didn't add the string type on the Exception::$message precisely because we told them this would break our design around that. Not all PHP has to be idiomatic.

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?
And on Symfony's side, if we reject this change, what can we do to improve the abstraction to allow lazy strings?

Add a StringableTranslatableInterface and have TranslatableInterface extend StringableTranslatableInterface?

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 TranslatableMarkup is a translatable, not a translator. Which is precisely the concept Symfony provides for lazy-translated-strings: https://symfony.com/blog/new-in-symfony-5-2-translatable-objects

I'd explore that contract first :)

Sorry, i missed TranslatableInterface was also patched.

Now im confused, what exactly is blocking Drupal from doing TranslatableMarkup implements TranslatableInterface and then leverage their own translator.

(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:

1. Accept this PR (Drupal continues to do exactly what it's been doing for the past couple of years).
2. Add a new interface like StringableTranslatableInterface, and adapt the Validator component to accept either interface. (should be very straightforward on our end)
3. In Validator, remove the @internal declaration from ExecutionContext::buildViolation() (quite a lot of forked code for us to maintain just to avoid (1) or (2), and I've no idea if it'd be acceptable to remove the @internal there).

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 TranslatableMarkup implements TranslatableInterface, not decorating the translator

We could sell string|Stringable as a feature for providing self-translating-strings (to be casted lazy, rather than invoking the translator lazy). In that sense, im only worried widening the type is too costly for the general case.

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 string|Stringable rather than string|TranslatableInterface. At this point we may solve #44619, #36784 as well.

Thanks for the details @catch56, all those options are reasonable to me, and I agree with your analysis of the 3 options. I proposed StringableTranslatableInterface but if I were to rewind in time, I'd prefer not adding this interface since from a design pov, it strange to me to split those two possible return types in two interfaces. Aka I don't see why we'd benefit from interface segregation here. So my preference would be merging this. But I'm not alone to decide so I just try to weight the point for #1 :)

It sounds like the issue is actually Validator supporting string|Stringable rather than string|TranslatableInterface

We can make it accept string|Stringable|TranslatableInterface but that'd be for 6.1 I suppose.

@catch56: would @ro0NL's proposal make sense for you?

As I understand @ro0NL's proposal, it's to make e.g. ExecutionContextInterface::buildViolation() accept Translatable alongside string|Stringable for the $message param - and then have different code paths (i.e. don't call ::trans() on it, change methods like getParameters() to extract parameters from the message instead of what was passed in etc.).

There appear to be two main problems with this:

1. ConstraintViolationBuilderInterface::setParameter|s() (and probably some other methods) won't make sense if $message is a Translatable - would it be a no-op in that case? Throw an exception? Seems like an API change if it starts throwing an exception, and potentially confusing if it doesn't.

2. All of Drupal's constraints would have to change to providing Translatables instead of strings as $message, meaning a downstream API change from us since we'd need to enforce using Translatable on dependent code (or at least any messages with substitutions).

Understood. My first concern are Symfony users, who would seem to benefit more from string|Stringable|Translatable support in the Validator component. It's a missing piece IMHO.

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 string|Stringable for the $id param still? 😅

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);
})

Understood. My first concern are Symfony users, who would seem to benefit more from string|Stringable|Translatable support in the Validator component. It's a missing piece IMHO.

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.

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"

See https://www.drupal.org/project/drupal/issues/3231683

Are you saying

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.
I hope this won't be a hard blocker on your side.
Let us know if it becomes one.