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

xavierleune · 2015-03-16T17:32:23Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#13919
License	MIT
Doc PR	NA

This PR uses the parameters "locale" and "fallback_locales" to generate the catalogues at warmup, avoiding the creation of files at runtime.

xabbuh · 2015-03-16T22:16:30Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/translation.xml

@@ -33,6 +33,7 @@
        <parameter key="translation.loader.class">Symfony\Bundle\FrameworkBundle\Translation\TranslationLoader</parameter>
        <parameter key="translation.extractor.class">Symfony\Component\Translation\Extractor\ChainExtractor</parameter>
        <parameter key="translation.writer.class">Symfony\Component\Translation\Writer\TranslationWriter</parameter>
+        <parameter key="translation.warmer.class">Symfony\Bundle\FrameworkBundle\CacheWarmer\TranslationsCacheWarmer</parameter>


Please add the class name in the attribute directly below.

I'm not sure to understand. You ask me to put my class name directly in my service declaration, that's right ?
If that's right, can you please explain me (for my personnal knowledge) why should I do it in this particular case ?

Thx,

Symfony used to use class parameters for a long time to make the class names configurable. Though there are better ways to modify services so that it was decided to not promote these parameters anymore and also don't introduce new ones (all services introduced with Symfony 2.6 don't declare them). The existing parameters have to be kept for BC, but they will be removed in Symfony 3.0.

Ok, thank you for your feedback

xavierleune · 2015-03-17T07:53:46Z

Thanks for your feedback @aitboudad, @xabbuh and @fabpot. I wait for the last feedback about $cacheDir before I push a new commit.

aitboudad · 2015-03-17T08:08:24Z

src/Symfony/Component/Translation/Translator.php

+        if ($this->locale !== null) {
+            $this->loadCatalogue($this->locale);
+        }
+        foreach ($this->fallbackLocales as $locale) {


You can avoid this as it's already called in loadCatalogue,
see https://github.com/symfony/symfony/blob/2.7/src/Symfony/Component/Translation/Translator.php#L344

Actually, that won't do the trick. If I try to load cz catalogue and it's not a registered locale, I'll have a cz catalogue with fallback translations (i.e fr) computed in.
It won't generate catalogues for every fallback. Am I right ?

ok, that sounds reasonable

aitboudad · 2015-03-17T08:17:43Z

@xavierleune Could you add some tests as well?

xavierleune · 2015-03-17T08:47:08Z

@aitboudad actually I can't see what should be tested. If I want to check input / output, I don't have any output to test. I could test if catalogues are generated, but that would be a double test. Do you have any idea ?

aitboudad · 2015-03-17T08:55:02Z

@xavierleune you can check if catalogues are generated when locale it's not registered

xavierleune · 2015-03-17T09:50:38Z

@aitboudad I'm not sure that's very relevant

aitboudad · 2015-03-17T09:58:38Z

src/Symfony/Bundle/FrameworkBundle/Translation/Translator.php

+     */
+    public function warmUp($cacheDir)
+    {
+        if ($this->options['cache_dir'] !== null) {


You should write null !== $this->options['cache_dir']

xavierleune · 2015-03-17T10:25:36Z

@aitboudad thanks for your last feedback. For the records, the "Constructor." comment should be removed from RouterCacheWarmer also.

aitboudad · 2015-03-19T16:16:34Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/translation.xml

@@ -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" />


it should be translator.default

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

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.

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

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.

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

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

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

in prod we use translator

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'));

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 !

aitboudad · 2015-03-19T16:25:24Z

@xavierleune I just left 2 comments. this will probably be my last remark :)

aitboudad · 2015-03-23T09:01:25Z

ping @xavierleune

xavierleune · 2015-03-23T09:23:43Z

Yes I'm here... I was making a new PR for some other changes 😄
I'll test your suggestions today.

Thks

dunglas · 2015-03-23T22:52:11Z

👍

fabpot · 2015-03-24T08:05:17Z

src/Symfony/Bundle/FrameworkBundle/Translation/Translator.php

+    public function warmUp($cacheDir)
+    {
+        if (null !== $this->options['cache_dir']) {
+            foreach (array_keys($this->resourceFiles) as $locale) {


Can you update this part as #14022 has been merged to avoid a BC break?

xavierleune · 2015-03-24T13:53:41Z

@fabpot done, thank your for the feedback.

fabpot · 2015-03-24T16:33:58Z

@aitboudad the last word is for you :)

Tobion · 2015-03-24T17:31:04Z

src/Symfony/Bundle/FrameworkBundle/Translation/Translator.php

+    {
+        if (null !== $this->options['cache_dir']) {
+            if (null !== $this->locale) {
+                $this->loadCatalogue($this->locale);


so this will only load the catalogue for the current locale?

The current locale can be not in the fallback locales (if the current is the default one). So I prefer to load the catalogue for the current locale and then for fallback locales.

loading all locales that are be part of $resourceFiles for me here make sense

Actually I rolled back because with last update in 2.7, it was broken. And I think it's weird to use file names to retrieve available locales.

ok sound good :)

@aitboudad actually this is ok for prod environment but in dev, resourceFiles is empty. If that's ok for you I'll add test and push update today.

hi @xavierleune :D, just remove https://github.com/xavierleune/symfony/blob/feature/13919-translations-cache-warmer/src/Symfony/Bundle/FrameworkBundle/Translation/Translator.php#L95 and it will work in dev env.

@aitboudad I guess it was here for a good reason. Seems it avoid a kind of infinite (or very long) loop somewhere.

Results for time php app/console cache:clear in prod or dev environments :

Env Type Before After

prod real 0m6.315s 1m48.515s

user 0m3.224s 1m28.898s

sys 0m0.696s 0m9.677s

---- ---- -------- ---------

dev real 0m3.364s 2m0.263s

user 0m2.228s 1m34.026s

sys 0m0.448s 0m13.129s

I don't have time to investigate more. I'll add some tests and then I'll be done for this PR.

I'll see it

@xavierleune fixed by aitboudad@106d04e can you include my commit into this PR or just allow me to commit into your fork.

aitboudad · 2015-03-24T20:51:30Z

👍, ping @fabpot

xavierleune · 2015-03-30T09:02:18Z

@aitboudad I cherry picked your changes, thx

xavierleune · 2015-03-30T09:41:44Z

@aitboudad can you please take a look to the travis build ? I'm not sur why it's broken. All tests ran fine on my laptop...

aitboudad · 2015-03-30T09:51:56Z

@xavierleune not related to this PR (#14002 need to be merged into master)

fabpot · 2015-03-30T15:36:31Z

@xavierleune Can you rebase to get rid of the merge commit?

aitboudad · 2015-03-30T15:41:06Z

@fabpot just FYI I moved the property resourceFiles into options of Translator

xabbuh · 2015-03-30T20:22:59Z

src/Symfony/Bundle/FrameworkBundle/CacheWarmer/TranslationsCacheWarmer.php

+     */
+    public function warmUp($cacheDir)
+    {
+        if ($this->translator instanceof WarmableInterface) {


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

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)

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.

xavierleune · 2015-03-31T10:23:47Z

@fabpot done
@aitboudad all right, thx

stof · 2015-03-31T10:45:19Z

src/Symfony/Bundle/FrameworkBundle/CacheWarmer/TranslationsCacheWarmer.php

+
+    /**
+     *
+     * @param TranslatorInterface $translator A Translator instance


there is no value at all in this phpdoc. It only duplicates the signature

stof · 2015-03-31T11:38:41Z

src/Symfony/Bundle/FrameworkBundle/Translation/Translator.php

+    /**
+     * {@inheritdoc}
+     */
+    public function warmUp($cacheDir)


public methods should be before protected and private ones

…s at warmup

aitboudad · 2015-04-02T07:04:29Z

ping @fabpot, this is ready for merge.

aitboudad · 2015-04-03T06:31:48Z

Thank you @xavierleune.

…p (xavierleune) This PR was merged into the 2.7 branch. Discussion ---------- [2.7][Translation] generate translation cache at warmup | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #13919 | License | MIT | Doc PR | NA This PR uses the parameters "locale" and "fallback_locales" to generate the catalogues at warmup, avoiding the creation of files at runtime. Commits ------- 94d3876 FIX #13919 added TranslationsCacheWarmer to generate catalogues at warmup

…ot used. (aitboudad) This PR was merged into the 2.7 branch. Discussion ---------- [FrameworkBundle][Translation] skip warmUp when cache is not used. | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Fixed tickets | #13942 | Tests pass? | yes | License | MIT Commits ------- efb10f6 [FrameworkBundle][Translation] skip warmUp when cache is not used.

xabbuh reviewed Mar 16, 2015
View reviewed changes

fabpot changed the title ~~FIX #13919 added TranslationsCacheWarmer to generate catalogues at warmup~~ added TranslationsCacheWarmer to generate catalogues at warmup Mar 17, 2015

aitboudad reviewed Mar 17, 2015
View reviewed changes

xavierleune force-pushed the feature/13919-translations-cache-warmer branch from 050b8b4 to f2c0226 Compare March 17, 2015 09:02

aitboudad reviewed Mar 17, 2015
View reviewed changes

xavierleune force-pushed the feature/13919-translations-cache-warmer branch from f2c0226 to 57cd942 Compare March 17, 2015 10:20

aitboudad added the Translation label Mar 17, 2015

aitboudad reviewed Mar 19, 2015
View reviewed changes

xavierleune force-pushed the feature/13919-translations-cache-warmer branch from 57cd942 to c56d47c Compare March 23, 2015 17:07

fabpot reviewed Mar 24, 2015
View reviewed changes

xavierleune force-pushed the feature/13919-translations-cache-warmer branch from 5d8b58a to 93b0f0c Compare March 24, 2015 13:26

xavierleune force-pushed the feature/13919-translations-cache-warmer branch 2 times, most recently from ebc84cb to df1e752 Compare March 24, 2015 13:52

Tobion reviewed Mar 24, 2015
View reviewed changes

xavierleune force-pushed the feature/13919-translations-cache-warmer branch from df1e752 to a83a7dd Compare March 24, 2015 20:39

xavierleune force-pushed the feature/13919-translations-cache-warmer branch from a83a7dd to 5000800 Compare March 26, 2015 09:56

aitboudad changed the title ~~added TranslationsCacheWarmer to generate catalogues at warmup~~ [2.7][Translation] generate translation cache at warmup Mar 29, 2015

xavierleune force-pushed the feature/13919-translations-cache-warmer branch from 3f82dbe to d1015db Compare March 30, 2015 09:01

xabbuh reviewed Mar 30, 2015
View reviewed changes

xavierleune force-pushed the feature/13919-translations-cache-warmer branch from 4a926f2 to f5f8395 Compare March 31, 2015 10:23

stof reviewed Mar 31, 2015
View reviewed changes

xavierleune force-pushed the feature/13919-translations-cache-warmer branch from f5f8395 to 58799d0 Compare March 31, 2015 10:53

stof reviewed Mar 31, 2015
View reviewed changes

FIX symfony#13919 added TranslationsCacheWarmer to generate catalogue…

94d3876

…s at warmup

xavierleune force-pushed the feature/13919-translations-cache-warmer branch from 58799d0 to 94d3876 Compare March 31, 2015 12:08

aitboudad merged commit 94d3876 into symfony:2.7 Apr 3, 2015

This was referenced Apr 7, 2015

Some ideas to improve performance of the Translator component #13948

Closed

[FrameworkBundle][Translation] skip warmUp when cache is not used. #14301

Merged

Env	Type	Before	After
prod	real	0m6.315s	1m48.515s
	user	0m3.224s	1m28.898s
	sys	0m0.696s	0m9.677s
----	----	--------	---------
dev	real	0m3.364s	2m0.263s
	user	0m2.228s	1m34.026s
	sys	0m0.448s	0m13.129s

Uh oh!

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

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

Uh oh!

Conversation

xavierleune commented Mar 16, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xavierleune commented Mar 17, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aitboudad commented Mar 17, 2015

Uh oh!

xavierleune commented Mar 17, 2015

Uh oh!

aitboudad commented Mar 17, 2015

Uh oh!

xavierleune commented Mar 17, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xavierleune commented Mar 17, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aitboudad commented Mar 19, 2015

Uh oh!

aitboudad commented Mar 23, 2015

Uh oh!

xavierleune commented Mar 23, 2015

Uh oh!

dunglas commented Mar 23, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xavierleune commented Mar 24, 2015

Uh oh!

fabpot commented Mar 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment