-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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} | ||
*/ | ||
public function warmUp($cacheDir) | ||
{ | ||
if ($this->translator instanceof WarmableInterface) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would rather type hint the constructor argument as There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
---|---|---|
|
@@ -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" /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it should be translator.default
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in prod we use translator There was a problem hiding this comment. Choose a reason for hiding this commentThe 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')); There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 😃 |
||
<tag name="kernel.cache_warmer" /> | ||
</service> | ||
</services> | ||
</container> |
There was a problem hiding this comment.
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):