symfony / ux

Symfony UX initiative: a JavaScript ecosystem for Symfony
https://ux.symfony.com/
MIT License
828 stars 299 forks source link

[Translator] Warming up the cache fails when the translator is disabled. #1962

Closed SanderVerkuil closed 2 months ago

SanderVerkuil commented 3 months ago

When performing acceptance or unit tests, we disable the translator so the translated values will be the translation keys. This makes assertions easier, as we don't have to update the tests when the translations have been altered.

Right now, I'm facing the following issue:

TypeError {#46989
  #message: "Symfony\UX\Translator\CacheWarmer\TranslationsCacheWarmer::__construct(): Argument #1 ($translatorBag) must be of type Symfony\Component\Translation\TranslatorBagInterface, Symfony\Component\Translation\IdentityTranslator given, called in /srv/app/var/cache/test/ContainerWZcqQ3o/getUx_Translator_CacheWarmer_TranslationsCacheWarmerService.php on line 24"
  #code: 0
  #file: "./vendor/symfony/ux-translator/src/CacheWarmer/TranslationsCacheWarmer.php"
  #line: 25
  trace: {
    ./vendor/symfony/ux-translator/src/CacheWarmer/TranslationsCacheWarmer.php:25 { …}
    ./var/cache/test/ContainerWZcqQ3o/getUx_Translator_CacheWarmer_TranslationsCacheWarmerService.php:24 {
      ContainerWZcqQ3o\getUx_Translator_CacheWarmer_TranslationsCacheWarmerService::do($container, $lazyLoad = true)^
      › 
      ›     return $container->privates['ux.translator.cache_warmer.translations_cache_warmer'] = new \Symfony\UX\Translator\CacheWarmer\TranslationsCacheWarmer(($container->privates['translator'] ??= new \Symfony\Component\Translation\IdentityTranslator()), ($container->privates['ux.translator.translations_dumper'] ?? $container->load('getUx_Translator_TranslationsDumperService')));
      › }
    }
    ./var/cache/test/ContainerWZcqQ3o/KernelTestDebugContainer.php:1995 { …}
    ./var/cache/test/ContainerWZcqQ3o/getCacheWarmerService.php:37 { …}
    ./vendor/symfony/http-kernel/CacheWarmer/CacheWarmerAggregate.php:99 { …}
    ./vendor/symfony/http-kernel/Kernel.php:545 { …}
    ./vendor/symfony/http-kernel/Kernel.php:763 { …}
    ./vendor/symfony/http-kernel/Kernel.php:126 { …}
    ./src/Kernel.php:34 { …}
    ./vendor/symfony/framework-bundle/Console/Application.php:190 { …}
    ./vendor/symfony/framework-bundle/Console/Application.php:72 { …}
    ./vendor/symfony/console/Application.php:175 { …}
    ./vendor/symfony/runtime/Runner/Symfony/ConsoleApplicationRunner.php:49 { …}
    ./vendor/autoload_runtime.php:29 { …}
    ./bin/console:11 { …}
  }
}

What would the proper and recommended way be to handle this?

smnandre commented 3 months ago

We probably could do something like this to remove the definiton / disable the package in those scenarios..

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

But you won't be able to use UX Translator at all then, as it need a TranslatorBag to generate the translations

SanderVerkuil commented 3 months ago

Yeah, I indeed understand that it would not be able to work. Right now, I'm enabling the translator in CI/CD, which I'm hoping will work. Otherwise, I'll have to write a custom compiler pass to remove/disable that definition myself.

It would also be great to hear how others handle with translations in a test environment and acceptance tests/assertions.

smnandre commented 3 months ago

If you're ok with the "disable" thing, would you try writing a PR about this ?

SanderVerkuil commented 3 months ago

@smnandre Unfortunately only disabling it if the translator is not enabled didn't work. When the framework is enabled, the forms (or validator, or some other bundles that require the translator) are enabled and the translator is disabled, a translator is added which is the IdentityTranslator. In this case, I verify whether the translator is a subclass of the TranslatorBag, as that is actually the element that is required.

Because the TranslationsCacheWarmer is optional, would it also be a possibility to mark the translator as nullable, and rely on autowiring instead of 'hoping' that the translator happens to implement the TranslatorBag interface?

smnandre commented 3 months ago

In this case, I verify whether the translator is a subclass of the TranslatorBag, as that is actually the element that is required.

Yes, this is what i had in mind when mentionning this file : https://github.com/symfony/symfony/blob/7.2/src/Symfony/Component/Translation/DependencyInjection/DataCollectorTranslatorPass.php

I think we could, in the same manner, remove the TranslationCacheWarmer (and a lot of other things probably) in a dedicated compiler pass.

After, i wonder in term of DX ... that could be a bit hard to explain this.

Could we add a "canBeDisabled" in the config, so for instance you would disable it when needed ? Would this solve your use case ?

Because in fact TranslationCacheWarmer is not really optional... without it the JS files would not be generated :|