[Translator] move loading catalogue out of Translator. #14671

aitboudad · 2015-05-17T18:12:21Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Fixed tickets	~
Tests pass?	yes
License	MIT

#14530 (comment)
It could also simplify a bunch of things:

lazy-loading of loaders could be implemented by implementing this interface in a lazy way (or relying of the DI feature to build the lazy implementation) instead of extending the Translator in FrameworkBundle, which would also make the lazy-loading available for people using DI+Translator outside Symfony

caching of catalogues could be implemented by composition instead of being in the main class dealing with loaders (which does not forbid caching the internal structures of the Translator class, which is a separate topic).

Before

$translator = new Translator('fr');
$translator->addLoader('array', new ArrayLoader());
$translator->addResource('array', array('Hello World!' => 'Bonjour'), 'fr');

echo $translator->trans('Hello World!')."\n";

After

$resourceCatalogue = new ResourceMessageCatalogueProvider();
$resourceCatalogue->addLoader('array', new ArrayLoader());
$resourceCatalogue->addResource('array', array('Hello World!' => 'Bonjour'), 'fr');
$translator = new Translator('fr', $resourceCatalogue);

echo $translator->trans('Hello World!')."\n";

mpdude · 2015-05-18T05:56:47Z

src/Symfony/Component/Translation/CacheTranslatorBag.php

+    /**
+     * {@inheritdoc}
+     */
+    public function getCatalogue($locale = null)


$locale may be null, which probably does not work for the other methods you're calling from here.

hmm I know about it, right now I'm not sure how to deal with it :), any thought ?
in 3.0 I would like to remove the default value:

public function getCatalogue($locale)

fixed, I removed the default value of locale argument in TranslatorBagInterface

mpdude · 2015-05-18T22:24:07Z

src/Symfony/Component/Translation/Translator.php

    protected function computeFallbackLocales($locale)
    {
+        trigger_error('The '.__METHOD__.' method is deprecated since version 2.8 and will be removed in 3.0. Rely on FallbackTranslatorBag instead.', E_USER_DEPRECATED);


Two things –

Is that the right way to deprecate protected functions? I mean, people might override this method without calling the parent class and never get the deprecation notice. (@stof?)

How can people provide their own implementation for this? Currently this is possible. That also applies to other protected methods as well.

I think so, if someone override public method too he can't get the deprecation notice.

It depends, for example to override computeFallbackLocales method they must use custom FallbackTranslatorBag.

MattKetmo · 2015-07-15T16:10:15Z

Hello,

I just have a quick look on this PR. It seems I also did a custom implementation of the Translator to split the different responsibilities. What bothered me was the fact that the Translator class does to much thing (file caching, resource loader, locale fallback) and I just wanted to change the cache mecanism (didn't want to cache them in php files in order to easily update them in production).

If you're insterested to have a look, here is the implementation I wrote: openl10n-translation/SimpleTranslator.php

I introduced a new MessageCatalogueLoader interface and use decoration, which is basically the equivalent of your usage of TranslatorBagInterface.

I'll have a closer look to your PR but I'm 👍 for this improvement.

MattKetmo · 2015-08-03T13:03:28Z

src/Symfony/Component/Translation/TranslatorBag.php

+ *
+ * @author Abdellatif Ait boudad <a.aitboudad@gmail.com>
+ */
+class TranslatorBag implements TranslatorBagInterface, TranslatorBagIdentifiable


Others TranslatorBag have a named (Cache, Fallback) except this one.
I would have renamed it to ResourceTranslatorBag.

sound good to me 👍 , done

aitboudad · 2015-09-25T22:42:11Z

ping @symfony/deciders

fabpot · 2015-09-30T11:51:28Z

Also, tests do not pass.

fabpot · 2015-09-30T11:51:54Z

ping @symfony/deciders

aitboudad · 2015-09-30T21:54:41Z

I added another PR #16036 which improve the performances and allows using custom provider.

aitboudad · 2015-09-30T21:56:19Z

The failure tests is not related to this PR.

mpdude · 2015-10-01T13:58:53Z

Honestly, I have spent quite some time reviewing this and I am not sure I have completely understood this change and all its implications. My gut feeling is that this is not ready, but I find it hard to give precise technical reasons for that.

I am not in a position to decide on this, but I would really recommend to get in-depth reviews from some more other people.

aitboudad · 2015-10-01T19:28:52Z

@mpdude what part do you think should be improved ?

mpdude · 2015-10-01T19:33:24Z

Once again, I am currently working through your code :) I'll let you know!

mpdude · 2015-10-03T10:08:39Z

This is a little mess because there are two code paths (with and without caching) through Translator, with several protected methods along the way and @api for all this.

@fabpot Would entirely deprecating the current Translator and adding a completely new implementation (with the same interface, of course) be an acceptable strategy?

Do we absolutely have to provide a flex/extension point for every conceivable tweak users could have made through protected methods in the past?

And, by the way, do we still have time or is 2.8 already feature-frozen?

aitboudad · 2015-10-11T12:09:41Z

Honestly I don't see what going wrong with the current implementation. Right now I guess 2.8 already feature-frozen, so I'm going to close.

MattKetmo · 2015-10-11T12:15:59Z

Oh, seriously! All this work for nothing?
I'd really have liked to see this merged.
I will keep my custom implemtation then.
😞

aitboudad · 2015-10-12T11:38:11Z

@MattKetmo I haven't abandoned, I must have at least one vote from deciders to be able to merge it.
Anyway I'll try to propose it for the next release.

aitboudad · 2015-10-19T11:09:45Z

reopened in #16286

aitboudad mentioned this pull request May 17, 2015

[RFC] Factor loading and caching out of Translator #14622

Closed

7 tasks

aitboudad added Feature Translation labels May 17, 2015

aitboudad force-pushed the translation_loader_resolver branch 3 times, most recently from 7527f65 to 557537f Compare May 17, 2015 20:01

mpdude reviewed May 18, 2015
View reviewed changes

aitboudad mentioned this pull request May 18, 2015

[2.8][Translation] added message cache. #14526

Closed

mpdude reviewed May 18, 2015
View reviewed changes

aitboudad force-pushed the translation_loader_resolver branch 6 times, most recently from b5c28e5 to 65361ff Compare May 23, 2015 11:38

aitboudad changed the title ~~[WIP][Translator] move loading catalogue out of Translator.~~ [Translator] move loading catalogue out of Translator. Jun 1, 2015

MattKetmo reviewed Aug 3, 2015
View reviewed changes

aitboudad force-pushed the translation_loader_resolver branch 2 times, most recently from a646aa7 to 0a2363e Compare August 7, 2015 17:59

aitboudad added the Status: Needs Review label Aug 11, 2015

mpdude mentioned this pull request Sep 24, 2015

[Translation] Remove TranslatorBagInterface to allow for optimized caching in 3.0 #14530

Closed

aitboudad force-pushed the translation_loader_resolver branch 4 times, most recently from 21431e3 to 70d0b85 Compare September 25, 2015 22:41

aitboudad force-pushed the translation_loader_resolver branch 4 times, most recently from d6d9ee1 to 07a4775 Compare September 30, 2015 18:36

aitboudad mentioned this pull request Sep 30, 2015

[2.8][Translation] added DoctrineMessageCatalogue. #16036

Closed

aitboudad added Status: Reviewed and removed Status: Needs Review labels Sep 30, 2015

aitboudad force-pushed the translation_loader_resolver branch 4 times, most recently from a874893 to e53217d Compare October 1, 2015 11:52

[Translator] move loading catalogue out of Translator.

95e23d2

aitboudad force-pushed the translation_loader_resolver branch from e53217d to 95e23d2 Compare October 11, 2015 11:46

aitboudad closed this Oct 11, 2015

aitboudad deleted the translation_loader_resolver branch October 11, 2015 12:09

aitboudad restored the translation_loader_resolver branch October 14, 2015 18:52

mpdude mentioned this pull request Oct 20, 2015

[Translator] move loading catalogue out of Translator. #16286

Closed

aitboudad mentioned this pull request Apr 3, 2017

[Translator] move loading catalogue out of Translator. #17563

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Translator] move loading catalogue out of Translator. #14671

[Translator] move loading catalogue out of Translator. #14671

aitboudad commented May 17, 2015

mpdude May 18, 2015

aitboudad May 18, 2015

aitboudad May 18, 2015

mpdude May 18, 2015

aitboudad May 19, 2015

MattKetmo commented Jul 15, 2015

MattKetmo Aug 3, 2015

aitboudad Aug 7, 2015

aitboudad commented Sep 25, 2015

fabpot commented Sep 30, 2015

fabpot commented Sep 30, 2015

aitboudad commented Sep 30, 2015

aitboudad commented Sep 30, 2015

mpdude commented Oct 1, 2015

aitboudad commented Oct 1, 2015

mpdude commented Oct 1, 2015

mpdude commented Oct 3, 2015

aitboudad commented Oct 11, 2015

MattKetmo commented Oct 11, 2015

aitboudad commented Oct 12, 2015

aitboudad commented Oct 19, 2015

[Translator] move loading catalogue out of Translator. #14671

[Translator] move loading catalogue out of Translator. #14671

Conversation

aitboudad commented May 17, 2015

Before

After

mpdude May 18, 2015

Choose a reason for hiding this comment

aitboudad May 18, 2015

Choose a reason for hiding this comment

aitboudad May 18, 2015

Choose a reason for hiding this comment

mpdude May 18, 2015

Choose a reason for hiding this comment

aitboudad May 19, 2015

Choose a reason for hiding this comment

MattKetmo commented Jul 15, 2015

MattKetmo Aug 3, 2015

Choose a reason for hiding this comment

aitboudad Aug 7, 2015

Choose a reason for hiding this comment

aitboudad commented Sep 25, 2015

fabpot commented Sep 30, 2015

fabpot commented Sep 30, 2015

aitboudad commented Sep 30, 2015

aitboudad commented Sep 30, 2015

mpdude commented Oct 1, 2015

aitboudad commented Oct 1, 2015

mpdude commented Oct 1, 2015

mpdude commented Oct 3, 2015

aitboudad commented Oct 11, 2015

MattKetmo commented Oct 11, 2015

aitboudad commented Oct 12, 2015

aitboudad commented Oct 19, 2015