[FrameworkBundle][Translation] moved cache to Translation component (new PR) #11373

OwlyCode · 2014-07-11T11:46:08Z

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

This supersedes the PR #11197 (while including the changes made by it). I removed the $options argument for Symfony\Component\Translation\Translator and replaced it by a public method enableCache($cacheDir, $debug = false). It aims to solve what @fabpot said about passing an array of options in #11197 ~~while not modifying the existing constructors~~.

ClementGautier · 2014-07-11T13:37:25Z

@OwlyCode Squash your commits please

jderusse · 2014-07-16T06:04:41Z

Given the security issue: http://symfony.com/blog/security-releases-cve-2014-4931-symfony-2-3-18-2-4-8-and-2-5-2-released, you have to rebase this PR and re-apply modifications on Translator: 06a80fb#diff-abaa70edd88fdf9fbf5f2ab72ab89891R100

OwlyCode · 2014-07-16T12:21:25Z

I rebased the commits on master so they benefit from the security release and are ready to be merged. All tests on Translator are green on my machine. @jeremy-derusse is it ok for you ?

jderusse · 2014-07-16T13:14:36Z

@OwlyCode sounds good.
I think, you can also copy new tests into TranslatorCacheTest.php (06a80fb#diff-990df0e8238847f8ae54e8227f295c22R48)
to avoid regression on #11093

And fix the Coding Standard: http://fabbot.io/report/symfony/symfony/11373/4022cb2c12d8ef7a24773e09430e200459a9bdb2

Tobion · 2014-07-17T23:39:56Z

src/Symfony/Component/Translation/Translator.php

     * Constructor.
     *
     * @param string               $locale   The locale
     * @param MessageSelector|null $selector The message selector for pluralization
+     * @param string               $cacheDir The directory to use for the cache


string|null

Tobion · 2014-07-17T23:41:10Z

If there is a cache within a component it should be a generic CacheInterface injected and one implementation by using the filesystem. As it has been refactored in twig.
See fabpot/Twig#1421

fabpot · 2014-07-18T05:35:57Z

The cache only makes sense if this a PHP file that PHP itself is able to "optimize". There is indeed a PR on Twig, but I'm still not in favor of it. I can understand some usages, but people should not use it blindly.

So, we can indeed provide a cache interface, but we need to make it clear that we will only ever support the PHP implementation and document the limitation of the other possible implementations.

jderusse · 2014-07-18T05:55:36Z

src/Symfony/Component/Translation/Tests/TranslatorCacheTest.php

+    {
+        $translator = $this->getTranslator($this->getLoader());
+        $translator->setLocale('fr');
+        $translator->setFallbackLocales(array('en', 'es', 'pt-PT', 'pt_BR'));


Can you add tests for locales with . and @ from FrameworkBundle? https://github.com/symfony/symfony/pull/11403/files#diff-990df0e8238847f8ae54e8227f295c22R48

fabpot · 2014-09-05T06:57:33Z

Can you rebase this PR on current master. Be careful as we made several security issue fixes.

[Translation][Cache] removed accessors for options.

OwlyCode · 2014-09-05T08:47:18Z

I rebased master, the security fixes didn't conflict but the assertValidLocale call added by 9e1bc22 did. This is fixed and tested, but I left assertValidLocale as protected even if not needed anymore to avoid BC break.

fabpot · 2014-09-05T08:48:50Z

@jeremy-derusse Can you review this one?

fabpot · 2014-09-05T08:49:31Z

src/Symfony/Component/Translation/CHANGELOG.md

@@ -1,6 +1,10 @@
 CHANGELOG
 =========

+2.6.0
+-----


missing blank line below

jderusse · 2014-09-05T09:00:26Z

Can you add (or move) new tests added on https://github.com/symfony/symfony/pull/11830/files ?

OwlyCode · 2014-09-05T11:08:41Z

I fixed the issues reported by @fabpot and @jeremy-derusse on this PR.

…ator Fixed phpdoc Aligned variables and description Removed enableCache and added cache setup in constructor Added tests for locales with . and @ with caching

fabpot · 2014-09-05T13:22:09Z

Is it mergeable now?

stof · 2014-09-05T13:29:22Z

👍

fabpot · 2014-09-05T13:31:14Z

@OwlyCode Can you fix the CS issue reported by fabbot?

OwlyCode · 2014-09-05T13:37:27Z

I'm not sure on the way to solve this as the use statement causing the CS issue is inside an heredoc string (it is the content of a generated file).

stof · 2014-09-05T13:40:25Z

Indeed. It looks like a false positive of the PHP-CS-Fixer here. I suspect fabbot is running the stable release for now, while the dev version should be fixed for this case

fabpot · 2014-09-05T13:43:46Z

👍

fabpot · 2014-09-05T13:45:10Z

Thank you @OwlyCode.

@fabpot

…ion component (new PR) (aitboudad, OwlyCode) This PR was merged into the 2.6-dev branch. Discussion ---------- [FrameworkBundle][Translation] moved cache to Translation component (new PR) | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #10628 | License | MIT | Doc PR | This supersedes the PR #11197 (while including the changes made by it). I removed the `$options` argument for `Symfony\Component\Translation\Translator` and replaced it by a public method `enableCache($cacheDir, $debug = false)`. It aims to solve what @fabpot said about passing an array of options in #11197 <s>while not modifying the existing constructors</s>. Commits ------- 30fed6a [Translation][Cache] Removed the options from the arguments of Translator 8b2d9a8 [FrameworkBundle][Translation] moved cache to Translation component

romainneutron added the FrameworkBundle label Jul 17, 2014

aitboudad mentioned this pull request Jul 17, 2014

[FrameworkBundle][Translation] moved cache to Translation component #11197

Closed

Tobion reviewed Jul 17, 2014
View reviewed changes

jderusse reviewed Jul 18, 2014
View reviewed changes

[FrameworkBundle][Translation] moved cache to Translation component

8b2d9a8

[Translation][Cache] removed accessors for options.

fabpot reviewed Sep 5, 2014
View reviewed changes

src/Symfony/Component/Translation/CHANGELOG.md

@@ -1,6 +1,10 @@

CHANGELOG

=========

2.6.0

-----

Copy link

Member

fabpot Sep 5, 2014

Choose a reason for hiding this comment

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

missing blank line below

[Translation][Cache] Removed the options from the arguments of Transl…

30fed6a

…ator Fixed phpdoc Aligned variables and description Removed enableCache and added cache setup in constructor Added tests for locales with . and @ with caching

fabpot merged commit 30fed6a into symfony:master Sep 5, 2014

xabbuh mentioned this pull request Apr 1, 2015

[RFC] [Config] Use a Factory for ConfigCache instances #7781

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FrameworkBundle][Translation] moved cache to Translation component (new PR) #11373

[FrameworkBundle][Translation] moved cache to Translation component (new PR) #11373

OwlyCode commented Jul 11, 2014

ClementGautier commented Jul 11, 2014

jderusse commented Jul 16, 2014

OwlyCode commented Jul 16, 2014

jderusse commented Jul 16, 2014

Tobion Jul 17, 2014

Tobion commented Jul 17, 2014

fabpot commented Jul 18, 2014

jderusse Jul 18, 2014

fabpot commented Sep 5, 2014

OwlyCode commented Sep 5, 2014

fabpot commented Sep 5, 2014

fabpot Sep 5, 2014

jderusse commented Sep 5, 2014

OwlyCode commented Sep 5, 2014

fabpot commented Sep 5, 2014

stof commented Sep 5, 2014

fabpot commented Sep 5, 2014

OwlyCode commented Sep 5, 2014

stof commented Sep 5, 2014

fabpot commented Sep 5, 2014

fabpot commented Sep 5, 2014

[FrameworkBundle][Translation] moved cache to Translation component (new PR) #11373

[FrameworkBundle][Translation] moved cache to Translation component (new PR) #11373

Conversation

OwlyCode commented Jul 11, 2014

ClementGautier commented Jul 11, 2014

jderusse commented Jul 16, 2014

OwlyCode commented Jul 16, 2014

jderusse commented Jul 16, 2014

Tobion Jul 17, 2014

Choose a reason for hiding this comment

Tobion commented Jul 17, 2014

fabpot commented Jul 18, 2014

jderusse Jul 18, 2014

Choose a reason for hiding this comment

fabpot commented Sep 5, 2014

OwlyCode commented Sep 5, 2014

fabpot commented Sep 5, 2014

fabpot Sep 5, 2014

Choose a reason for hiding this comment

jderusse commented Sep 5, 2014

OwlyCode commented Sep 5, 2014

fabpot commented Sep 5, 2014

stof commented Sep 5, 2014

fabpot commented Sep 5, 2014

OwlyCode commented Sep 5, 2014

stof commented Sep 5, 2014

fabpot commented Sep 5, 2014

fabpot commented Sep 5, 2014