Skip to content

[2.7][Translation] generate translation cache at warmup #13942

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ CHANGELOG
-----

* Added possibility to extract translation messages from a file or files besides extracting from a directory
* Added `TranslationsCacheWarmer` to create catalogues at warmup

2.6.0
-----
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Bundle\FrameworkBundle\CacheWarmer;

use Symfony\Component\HttpKernel\CacheWarmer\CacheWarmerInterface;
use Symfony\Component\HttpKernel\CacheWarmer\WarmableInterface;
use Symfony\Component\Translation\TranslatorInterface;

/**
* Generates the catalogues for translations.
*
* @author Xavier Leune <xavier.leune@gmail.com>
*/
class TranslationsCacheWarmer implements CacheWarmerInterface
{
private $translator;

public function __construct(TranslatorInterface $translator)
{
$this->translator = $translator;
}

/**
* {@inheritdoc}
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to duplicate the docblock here. Just use (same with the warmUp() method):

/**
 * {@inheritdoc}
 */

public function warmUp($cacheDir)
{
if ($this->translator instanceof WarmableInterface) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather type hint the constructor argument as WarmableInterface instead of TranslatorInterface.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hint the constructor argument as WarmableInterface look wrong to me when it's optional TranslationsCacheWarmer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. It's the same thing for RouterCacheWarmer, I used it as a sample.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't understand, why we need check this here when we could change the constructor type hint.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot. Otherwise we forbid decorating the translator with a non-warmable one (or we need to add complex logic in a compiler pass to determine whether we should disable this cache warmer entirely and remove it from the container to avoid being broken)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually already do some reflection stuff in that compiler pass. But yeah, it's probably not worth to put much effort in this though.

$this->translator->warmUp($cacheDir);
}
}

/**
* {@inheritdoc}
*/
public function isOptional()
{
return true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\Reference;

/**
* @author Abdellatif Ait boudad <a.aitboudad@gmail.com>
Expand All @@ -38,6 +39,7 @@ public function process(ContainerBuilder $container)
$refClass = new \ReflectionClass($class);
if ($refClass->implementsInterface('Symfony\Component\Translation\TranslatorInterface') && $refClass->implementsInterface('Symfony\Component\Translation\TranslatorBagInterface')) {
$container->getDefinition('translator.logging')->setDecoratedService('translator');
$container->getDefinition('translation.warmer')->replaceArgument(0, new Reference('translator.logging.inner'));
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -705,12 +705,22 @@ private function registerTranslatorConfiguration(array $config, ContainerBuilder
->in($dirs)
;

$locales = array();
foreach ($finder as $file) {
list($domain, $locale, $format) = explode('.', $file->getBasename(), 3);
$files[] = (string) $file;
if (!isset($files[$locale])) {
$files[$locale] = array();
}

$files[$locale][] = (string) $file;
}

$translator->replaceArgument(4, $files);
$options = array_merge(
$translator->getArgument(3),
array('resource_files' => $files)
);

$translator->replaceArgument(3, $options);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,5 +152,10 @@
<service id="translation.extractor" class="%translation.extractor.class%"/>

<service id="translation.writer" class="%translation.writer.class%"/>

<service id="translation.warmer" class="Symfony\Bundle\FrameworkBundle\CacheWarmer\TranslationsCacheWarmer" public="false">
<argument type="service" id="translator" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be translator.default

 <argument type="service" id="translator.default" />

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why I should use translator.default. If I do that when a dev inject his own service which can be Warmable, he'll need to implement his own warmer but TranslationsCacheWarmer can do the job and is not specific to the class Translator.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use the alias "translator", in dev mode this won't work as the translator return an instance of LoggingTranslator.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LoggingTranslator implements TranslatorInterface but it does not implements WarmableInterface. So the cache file won't be generated but "it will work" as no error will be thrown. It would be sad to force developpers to duplicate code to have some cache in dev.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ops, ok let's keep it as it and inject translator.logging.inner for translation.warmer in https://github.com/symfony/symfony/blob/2.7/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/LoggingTranslatorPass.php#L40

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks ! Now I know I must not check the french version of the documentation. Decorating Services are not in this version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

translator.logging.inner does not exists in prod env. What should I do ? This is useful mainly in prod env.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in prod we use translator

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you didn't understand :D, in translation.xml we inject translator

<service id="translation.warmer" class="Symfony\Bundle\FrameworkBundle\CacheWarmer\TranslationsCacheWarmer" public="false">
    <argument type="service" id="translator" />
    <tag name="kernel.cache_warmer" />
</service>

and in LoggingTranslatorPass.php#L40

$container->getDefinition('translation.warmer')->replaceArgument(0, new Reference('translator.logging.inner'));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You know... Like french people say I quickly understand but you have to explain to me a long time 😃
Anyway I understood and I'll push my update soon. Thks !

<tag name="kernel.cache_warmer" />
</service>
</services>
</container>
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public function testProcess()
->method('getAlias')
->will($this->returnValue('translation.default'));

$container->expects($this->exactly(2))
$container->expects($this->exactly(3))
->method('getDefinition')
->will($this->returnValue($definition));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -223,9 +223,9 @@ public function testTranslator()
$container = $this->createContainerFromFile('full');
$this->assertTrue($container->hasDefinition('translator.default'), '->registerTranslatorConfiguration() loads translation.xml');
$this->assertEquals('translator.default', (string) $container->getAlias('translator'), '->registerTranslatorConfiguration() redefines translator service from identity to real translator');
$resources = $container->getDefinition('translator.default')->getArgument(4);
$options = $container->getDefinition('translator.default')->getArgument(3);

$files = array_map(function ($resource) { return realpath($resource); }, $resources);
$files = array_map(function ($resource) { return realpath($resource); }, $options['resource_files']['en']);
$ref = new \ReflectionClass('Symfony\Component\Validator\Validation');
$this->assertContains(
strtr(dirname($ref->getFileName()).'/Resources/translations/validators.en.xlf', '/', DIRECTORY_SEPARATOR),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ public function testTransWithCaching()
public function testTransWithCachingWithInvalidLocale()
{
$loader = $this->getMock('Symfony\Component\Translation\Loader\LoaderInterface');
$translator = $this->getTranslator($loader, array('cache_dir' => $this->tmpDir), array(), 'loader', '\Symfony\Bundle\FrameworkBundle\Tests\Translation\TranslatorWithInvalidLocale');
$translator = $this->getTranslator($loader, array('cache_dir' => $this->tmpDir), 'loader', '\Symfony\Bundle\FrameworkBundle\Tests\Translation\TranslatorWithInvalidLocale');
$translator->setLocale('invalid locale');

$this->setExpectedException('\InvalidArgumentException');
Expand All @@ -106,23 +106,25 @@ public function testLoadRessourcesWithCaching()
{
$loader = new \Symfony\Component\Translation\Loader\YamlFileLoader();
$resourceFiles = array(
__DIR__.'/../Fixtures/Resources/translations/messages.fr.yml',
'fr' => array(
__DIR__.'/../Fixtures/Resources/translations/messages.fr.yml',
),
);

// prime the cache
$translator = $this->getTranslator($loader, array('cache_dir' => $this->tmpDir), $resourceFiles, 'yml');
$translator = $this->getTranslator($loader, array('cache_dir' => $this->tmpDir, 'resource_files' => $resourceFiles), 'yml');
$translator->setLocale('fr');

$this->assertEquals('répertoire', $translator->trans('folder'));

// do it another time as the cache is primed now
$translator = $this->getTranslator($loader, array('cache_dir' => $this->tmpDir), array(), 'yml');
$translator = $this->getTranslator($loader, array('cache_dir' => $this->tmpDir), 'yml');
$translator->setLocale('fr');

$this->assertEquals('répertoire', $translator->trans('folder'));

// refresh cache when resources is changed in debug mode.
$translator = $this->getTranslator($loader, array('cache_dir' => $this->tmpDir, 'debug' => true), array(), 'yml');
$translator = $this->getTranslator($loader, array('cache_dir' => $this->tmpDir, 'debug' => true), 'yml');
$translator->setLocale('fr');

$this->assertEquals('folder', $translator->trans('folder'));
Expand All @@ -132,10 +134,12 @@ public function testLoadRessourcesWithoutCaching()
{
$loader = new \Symfony\Component\Translation\Loader\YamlFileLoader();
$resourceFiles = array(
__DIR__.'/../Fixtures/Resources/translations/messages.fr.yml',
'fr' => array(
__DIR__.'/../Fixtures/Resources/translations/messages.fr.yml',
),
);

$translator = $this->getTranslator($loader, array(), $resourceFiles, 'yml');
$translator = $this->getTranslator($loader, array('resource_files' => $resourceFiles), 'yml');
$translator->setLocale('fr');

$this->assertEquals('répertoire', $translator->trans('folder'));
Expand Down Expand Up @@ -221,14 +225,13 @@ protected function getContainer($loader)
return $container;
}

public function getTranslator($loader, $options = array(), $resources = array(), $loaderFomat = 'loader', $translatorClass = '\Symfony\Bundle\FrameworkBundle\Translation\Translator')
public function getTranslator($loader, $options = array(), $loaderFomat = 'loader', $translatorClass = '\Symfony\Bundle\FrameworkBundle\Translation\Translator')
{
$translator = new $translatorClass(
$this->getContainer($loader),
new MessageSelector(),
array($loaderFomat => array($loaderFomat)),
$options,
$resources
$options
);

if ('loader' === $loaderFomat) {
Expand All @@ -243,6 +246,22 @@ public function getTranslator($loader, $options = array(), $resources = array(),

return $translator;
}

public function testWarmup()
{
$loader = new \Symfony\Component\Translation\Loader\YamlFileLoader();
$resourceFiles = array(
'fr' => array(
__DIR__.'/../Fixtures/Resources/translations/messages.fr.yml',
),
);

// prime the cache
$translator = $this->getTranslator($loader, array('cache_dir' => $this->tmpDir, 'resource_files' => $resourceFiles), 'yml');
$this->assertFalse(file_exists($this->tmpDir.'/catalogue.fr.php'));
$translator->warmup($this->tmpDir);
$this->assertTrue(file_exists($this->tmpDir.'/catalogue.fr.php'));
}
}

class TranslatorWithInvalidLocale extends Translator
Expand Down
46 changes: 32 additions & 14 deletions src/Symfony/Bundle/FrameworkBundle/Translation/Translator.php
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

namespace Symfony\Bundle\FrameworkBundle\Translation;

use Symfony\Component\HttpKernel\CacheWarmer\WarmableInterface;
use Symfony\Component\Translation\Translator as BaseTranslator;
use Symfony\Component\Translation\MessageSelector;
use Symfony\Component\DependencyInjection\ContainerInterface;
Expand All @@ -20,52 +21,67 @@
*
* @author Fabien Potencier <fabien@symfony.com>
*/
class Translator extends BaseTranslator
class Translator extends BaseTranslator implements WarmableInterface
{
protected $container;
protected $loaderIds;
protected $resourceFiles;

protected $options = array(
'cache_dir' => null,
'debug' => false,
'resource_files' => array(),
);

/**
* @var array
*/
private $resourceLocales;

/**
* Constructor.
*
* Available options:
*
* * cache_dir: The cache directory (or null to disable caching)
* * debug: Whether to enable debugging or not (false by default)
* * resource_files: List of translation resources available grouped by locale.
*
* @param ContainerInterface $container A ContainerInterface instance
* @param MessageSelector $selector The message selector for pluralization
* @param array $loaderIds An array of loader Ids
* @param array $options An array of options
* @param array $resourceFiles An array of resource directories
* @param ContainerInterface $container A ContainerInterface instance
* @param MessageSelector $selector The message selector for pluralization
* @param array $loaderIds An array of loader Ids
* @param array $options An array of options
*
* @throws \InvalidArgumentException
*/
public function __construct(ContainerInterface $container, MessageSelector $selector, $loaderIds = array(), array $options = array(), $resourceFiles = array())
public function __construct(ContainerInterface $container, MessageSelector $selector, $loaderIds = array(), array $options = array())
{
$this->container = $container;
$this->loaderIds = $loaderIds;
$this->resourceFiles = $resourceFiles;

// check option names
if ($diff = array_diff(array_keys($options), array_keys($this->options))) {
throw new \InvalidArgumentException(sprintf('The Translator does not support the following options: \'%s\'.', implode('\', \'', $diff)));
}

$this->options = array_merge($this->options, $options);
$this->resourceLocales = array_keys($this->options['resource_files']);
if (null !== $this->options['cache_dir'] && $this->options['debug']) {
$this->loadResources();
}

parent::__construct(null, $selector, $this->options['cache_dir'], $this->options['debug']);
}

/**
* {@inheritdoc}
*/
public function warmUp($cacheDir)
{
foreach ($this->resourceLocales as $locale) {
$this->loadCatalogue($locale);
}
}

/**
* {@inheritdoc}
*/
Expand All @@ -87,11 +103,13 @@ protected function initialize()

private function loadResources()
{
foreach ($this->resourceFiles as $key => $file) {
// filename is domain.locale.format
list($domain, $locale, $format) = explode('.', basename($file), 3);
$this->addResource($format, $file, $locale, $domain);
unset($this->resourceFiles[$key]);
foreach ($this->options['resource_files'] as $locale => $files) {
foreach ($files as $key => $file) {
// filename is domain.locale.format
list($domain, $locale, $format) = explode('.', basename($file), 3);
$this->addResource($format, $file, $locale, $domain);
unset($this->options['resource_files'][$locale][$key]);
}
}
}
}