symfony / symfony

The Symfony PHP framework
https://symfony.com
MIT License
29.52k stars 9.41k forks source link

[Serializer] `TranslatableNormalizer` should be opt-in #53451

Open priyadi opened 6 months ago

priyadi commented 6 months ago

Symfony version(s) affected

6.4

Description

I have a few classes and enums that implement TranslatableInterface. After 6.4, serializing them gives me the translated string (or the translation ID), instead of the classes' contents. Right now, I have to decorate the TranslatableNormalizer out to make everything normal again.

How to reproduce

class foo implements TranslatableInterface
{
    public function trans(TranslatorInterface $translator, ?string $locale = null): string
    {
        return 'foo';
    }

    public string $bar = 'bar';
}

$serializer->serialize(new foo(), 'json');

With TranslatableNormalizer it outputs "foo". Without TranslatableNormalizer, it gives me {"bar":"bar"}

Possible Solution

I propose making TranslatableNormalizer opt-in, not enabled by default. TranslatableInterface is a general interface that can be implemented by any object, not just those that specifically holding translation IDs.

Or, we can make the class configurable. And by default, we can limit it to operate on classes that specifically contain translation strings, and nothing else, like TranslatableMessage.

Additional Context

This is how I disable TranslatableNormalizer:

use Symfony\Component\DependencyInjection\Attribute\AsDecorator;
use Symfony\Component\Serializer\Normalizer\NormalizerInterface;
use Symfony\Component\Serializer\Normalizer\TranslatableNormalizer;

#[AsDecorator(decorates: 'serializer.normalizer.translatable')]
final class TranslatableNormalizerDecorator implements NormalizerInterface
{
    public function __construct(
        private readonly TranslatableNormalizer $decorated,
    ) {
    }

    public function getSupportedTypes(?string $format): array
    {
        return $this->decorated->getSupportedTypes($format);
    }

    public function normalize(mixed $object, ?string $format = null, array $context = [])
    {
        return $this->decorated->normalize($object, $format, $context);
    }

    public function supportsNormalization(mixed $data, ?string $format = null): bool
    {
        return false;
    }
}
mtarld commented 6 months ago

Indeed, IMHO this normalizer should be opt-in for 6.4. But at the end (in 7.x), I do think that it should be enabled by default and able to be opt-out, WDYT?

priyadi commented 5 months ago

Indeed, IMHO this normalizer should be opt-in for 6.4. But at the end (in 7.x), I do think that it should be enabled by default and able to be opt-out, WDYT?

I still think it should be enabled by default only for TranslatableMessage, not TranslatableInterface. Or, we can enable for TranslatableInterface, but only if the object is not an enum, and has only a single method trans().

I can't believe I'm the only one in the world that has been doing these:

Translatable Enums

use Symfony\Contracts\Translation\TranslatableInterface;
use Symfony\Component\Translation\TranslatableMessage;

enum Gender: string implements TranslatableInterface
{
    case Male = 'male';
    case Female = 'female';
    case Other = 'other';

    public function trans(TranslatorInterface $translator, ?string $locale = null): string
    {
        $translation = match ($this) {
            self::Male => new TranslatableMessage('Male'),
            self::Female => new TranslatableMessage('Female'),
            self::Other => new TranslatableMessage('Other'),
        };

        return $translation->trans($translator, $locale);
    }
}

Example in Twig:

{{ person.gender|trans }}

Translatable Exception

The easiest way to have translatable error messages:

use Symfony\Component\Translation\IdentityTranslator;
use Symfony\Component\Translation\TranslatableMessage;
use Symfony\Contracts\Translation\TranslatableInterface;
use Symfony\Contracts\Translation\TranslatorInterface;

abstract class AbstractTranslatableException extends AbstractDomainException
implements TranslatableInterface
{
    public function __construct(int $code = 0, \Throwable $previous = null)
    {
        parent::__construct($this->getNonTranslatableMessage(), $code, $previous);
    }

    abstract protected function getTranslatableMessage(): TranslatableInterface;

    public function getNonTranslatableMessage(): string
    {
        return $this->getTranslatableMessage()->trans(new IdentityTranslator());
    }

    final public function trans(
        TranslatorInterface $translator,
        ?string $locale = null
    ): string {
        return $this->getTranslatableMessage()->trans($translator, $locale);
    }
}

class SpannerException extends AbstractTranslatableException
{
     protected function getTranslatableMessage(): TranslatableInterface
     {
         return new TranslatableMessage('Someone threw a SpannerException in the works!');
     }
}

Translatable File

use Symfony\Component\Translation\IdentityTranslator;
use Symfony\Component\Translation\TranslatableMessage;
use Symfony\Contracts\Translation\TranslatableInterface;
use Symfony\Contracts\Translation\TranslatorInterface;

class File implements TranslatableInterface, \Stringable
{
    private string $fileName = '';

    public function __toString(): string
    {
        return $this->trans(new IdentityTranslator());
    }

    public function trans(
        TranslatorInterface $translator,
        ?string $locale = null
    ): string {
        if ($this->fileName === '') {
             $message = new TranslatableMessage('(Untitled)');
        } else {
             $message = new TranslatableMessage('{fileName}', ['{fileName}' => $this->fileName]);
        }
        return $message->trans($translator, $locale);
    }
}
nicolas-grekas commented 5 months ago

How would you recommend opting in for this? An attribute on the class? Up to giving it a try?

priyadi commented 5 months ago

How would you recommend opting in for this? An attribute on the class? Up to giving it a try?

I did some thinking & rethinking last week. I think this is more of an architectural problem, not a technical one. This is what I would do instead:

Create a TranslatableMessage under symfony/translation-contracts. So everyone gets a standardized translatable message value object, something similar to DateTime or Money but for translation strings.

Projects with architectural constraints now don't have to reimplement TranslatableMessage only to avoid depending on symfony/translation. It only needs to depend on symfony/translation-contracts. There should be no longer any need to reimplement TranslatableInterface if the only thing we want is something similar to TranslatableMessage. We implement TranslatableInterface only if the object does more than just storing a translatable string.

Then, we can deprecate the TranslatableMessage in symfony/translation, and get everyone migrate to the new one in symfony/translation-contracts.

Now that we have a canonical TranslatableMessage value object, we can change TranslatableNormalizer to target the new TranslatableMessage instead of TranslatableInterface, because now we are confident if all we need is storing translatable string, we would only use TranslatableMessage.

WDYT @nicolas-grekas ?

EDIT: While we are at it, maybe we can also move IdentityTranslator to symfony/translation-contracts, and rename it to NullTranslator.