[Translator] Mark the translator as nullable

[SanderVerkuil]

Q	A
Bug fix?	yes
New feature?	no
Issues	Fix #2056
License	MIT

Apparently, the translator service is not available by default, so removing the cache warmer if the container doesn't have the definition breaks in newer versions. In order to allow this to still work when the translator service is not defined, but there is a translator defined in the container, the behaviour is updated to be nullOnInvalid and in the warmup it verifies whether the TranslatorBagInterface is null or not.

I've added the logger to be able to add a warning when the translator is not valid, which should not cause any issues.

[kbond]

I'm not entirely sure this will solve the issue - I think it's possible translator exists but not an instance of TranslatorBagInterface.

[SanderVerkuil]

I'm not entirely sure this will solve the issue - I think it's possible translator exists but not an instance of TranslatorBagInterface.

The issue I originally had is defined in #1962 and indeed mentions that the translator exists, and is an instance of the TranslatorInterface, but not the TranslatorBagInterface. By updating the service('translator') to be ->nullOnInvalid(), I expect that when the translator service does not implement the TranslatorBagInterface, the service injector will inject null, which in turn solves the issue.

Unless I'm making assumptions that I couldn't have made. I'd be willing to add new test cases, but I'm unsure how I can configure the Kernel to be comparable with what I had locally, because it depended on the framework being present, and disabling the translator in the test environment, like so:

when@test:
  framework:
    translator:
      enabled: false

When the translator is disabled, the translator becomes the IdentityTranslator, which does not implement the TranslatorBagInterface, which caused the aforementioned bug.

[kbond]

Oh ok so nulloninvalid will pass null even if the service exists but doesn't implement the correct interface? I didn't realize this was the case.

[SanderVerkuil]

I just managed to test and verify it locally, unfortunately it doesn't fix it. Will see whether I can make a test scenario.

Apparently the test scenario already tests this scenario, and indeed, the test is failing, but not in the checks, which is weird.

[SanderVerkuil]

Locally the test was indeed failing, not sure why the test was succeeding in the checks though. I've used union types to either allow null, allow TranslatorInterface, or allow the TranslatorBagInterface. This should hopefully fix the issues.

[kbond]

I've reverted the original PR just to get the test suite passing. We've also made some adjustments to prevent regressions in the future.

I still want the original issue fixed though but just wanted some time to properly fix the issue.

[SanderVerkuil]

I've reverted the original PR just to get the test suite passing. We've also made some adjustments to prevent regressions in the future.

I still want the original issue fixed though but just wanted some time to properly fix the issue.

No worries, its great that more pressing bugs are fixed faster, where my main guess is that most people don't disable the translator in the test environment 😅.

I've made this MR up-to-date with 2.x and I've re-added the test.

[smnandre]

Wondering, if you disable the translator in test environment, could you disable the UXTranslator bundle too ?

[SanderVerkuil]

Wondering, if you disable the translator in test environment, could you disable the UXTranslator bundle too ?

I think that might also work, yeah. But he main issue is, I think, that the TranslationCacheWarmer needs the TranslatorBag, and that the Translatorservice is injected. The service namedtranslatorwill (I think) always be an instance of theTranslatorInterface, but not all implementations of the TranslatorInterfacealso implement theTranslatorBag, so if someone chooses a different Translator, one that happens to not implement the TranslatorBag`, it would crash with the same issue.

To be fair, if they choose a different Translator, one that doesn't implement the TranslatorBagInterface, the Symfony-UX/TranslatorBundle also won't work or do what it is meant to do, so in those cases they could also choose to disable or not use the symfony-ux/translator package 😅.

In either case, should there be a hard crash when the translations can't be dumped, or can it fail silently? I'd also be happy to make it a hard crash with a better error message (rather than 'Unable to create the cache warmer, expected an instance of TranslatorDumper, got IdentityTranslator'). There might be some more/better documentation, that for in order to make sure this bundle works, the Translator that is being used also needs to implement the TranslatorBagInterface, so translations can be dumped properly.

[smnandre]

This was a genuine question ;)

So i checked the code of the FrameworkBundle and the documentation of translation

Standard case

Documentation

Translation of text is done through the translator service (Translator).
https://symfony.com/doc/current/translation.html#basic-translation

Service names / definitions

• The translator service is a public alias of the translator.default service.
• The translator.default service is an instance of [Translator](https://github.com/symfony/symfony/blob/7.1/src/Symfony/Component/Translation/Translator.php) (the concrete class)
• The TranslatorInterface service is an alias to the translator service.

Based on

• https://github.com/symfony/symfony/blob/7.1/src/Symfony/Bundle/FrameworkBundle/Resources/config/translation.php
• https://github.com/symfony/symfony/blob/7.1/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

This mean the translator is a Translator object.

---

Identify Translator

The identity translator is configured when the translator is not enabled and used instead of the previous one.

// A translator must always be registered (as support is included by
// default in the Form and Validator component). If disabled, an identity
// translator will be used and everything will still work as expected.

And then you are right the translator is then not a TranslatorBag.

But none of those component need to extract and list translations. They just need a TranslatorInterface.

---

UXTranslator

UXTranslator need the original translator. How could the UX Translator work without translation catalogs ?

In the bundle extension, we define the CacheWarmer service and ask to inject the 'translator' service, so i'm thinking this is ok.
To be precise: a Translator instance, and we type-hint it as TranslatorInterface but we could use TranslatorBag here directly and still be correct.

The UX Translator bundle needs the translator service to be available to work.

And a file must be generated by the CacheWarmer or the front-end scripts will not be available / import map will not work.

---

So...

So i don't see how any solution other than "disable the UX Translator" could work here.

I think the ignoreOnInvalid is not a good idea here as it would only delay problems...
... and should in fact trigger an exception as the service is needed to work.

Same goes for any "silent" / ignoring "crashes" :/

We 100% could improve the DX and dependency injection configuration (maybe allow some enable/disable configuration here), and any suggestion here would be welcomed! 😄

I also agree we could add a paragraph in the documentation to explain both the requirements and their reasons. And probably one to explain how to solve it (whatever way we find).

[smnandre]

Oh and I found why the bug !!

The code i suggested originally (from the DataCollectorTranslatorPass)

        if (!$container->has('translator')) {
            return;
        }

https://github.com/symfony/symfony/blob/7.2/src/Symfony/Component/Translation/DependencyInjection/DataCollectorTranslatorPass.php

The code we merged from the PR

        if (!$containerBuilder->hasDefinition('translator')) {
            return false;
        }

https://github.com/SanderVerkuil/symfony-ux/blob/b75e5d5f21fedcc28a23ba70c385bc137d9f8bf6/src/Translator/src/DependencyInjection/TranslatorCompilerPass.php

This explains the bug in production and the hotfix: the has() method checks definitions and aliases too, when hasDefinition() only check definitions.

This is not a critique, and I take full responsibility for it, as I did not check carefully enough.

[Kocal]

Oh, that's tricky as hell 😅

Good catch tho!

[SanderVerkuil]

Oh wow, didn't know that the has would follow aliases, wheres the hasDefinition would not follow these aliases. An alias is theoretically a definition as well, right?

Anyways, I could make throw an error in the constructor if the injected Translator is not a TranslatorBag. That way we can inform users what to do and how they can resolve it, instead of silently failing.

As the 'only' purpose of this bundle is to 'map' varous catalogues to JS files, I think that it's fair to assume that when the translator is not being used, users should disable the TranslatorBundle. If there's proper documentation with that, I'm perfectly fine with that. @Kocal and/or @smnandre, what do you think about this?

[smnandre]

Anyways, I could make throw an error in the constructor if the injected Translator is not a TranslatorBag. That way we can inform users what to do and how they can resolve it, instead of silently failing.

The injected Translator is type-hinted TranslatorBagInterface in the constructor

So you cannot do this in the constructor, as PHP would throw a TypeError before.

[smnandre]

Oh wow, didn't know that the has would follow aliases, wheres the hasDefinition would not follow these aliases. An alias is theoretically a definition as well, right?

We both learned something here :)

[kbond]

So, what about accepting TranslatorInterface in the constructor so we don't get the type error, then throwing a helpful exception if not an instance of TranslatorBagInterface?

One thing I'm not sure of: if we disable the ux translator bundle, will asset mapper still try and load var/translations?

[smnandre]

So, what about accepting TranslatorInterface in the constructor so we don't get the type error, then throwing a helpful exception if not an instance of TranslatorBagInterface?

I really don't think this should be done at runtime.. better make some check during compiler no ?

And not a big fan to expand the interface "just" to deal with injection problems to be honest... that does not feel like a clean way (but if we don't have much let's do this)

One thing I'm not sure of: if we disable the ux translator bundle, will asset mapper still try and load var/translations?

That's a good question. And we should be able to offer the "activation/deactivation" of the component like others (e.g. serializer, validator, workflows, etc.)

[kbond]

I really don't think this should be done at runtime.. better make some check during compiler no ?

The cache warmer certainly isn't on the hot path but I get your point. I'm not a fan either.

[smnandre]

One thing I'm not sure of: if we disable the ux translator bundle, will asset mapper still try and load var/translations?

So...

Yes !
The ImportMapGenerator will try to load it, with other translator files declared the importmap.php..

... and No
But this will only happen if you run call it (meaning a importmap() call in Twig)

So to run API tests, unit tests, it would work.

For functional test it probably would crash, and E2E logically too.
You could also generate your assets first (once) and then run the functional tests with them i suppose

Solutions
In the end, we could create some custom dumper to replace the one we use (a bit like symfony does with the translator if form/validator are enabled and the translator service does not exist)
But as today it works with constant imports, it would break any front-end script.

So i don't really see any solution easy to implement and that would not break anything ... @Kocal any idea here ?

[Kocal]

By any chance, do you have a reproducer that can help me to work on?

Thanks