NotificationsYou must be signed in to change notification settings
Fork9.6k
Star30.4k

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

aitboudad wants to merge1 commit intosymfony:2.8fromaitboudad:translation_loader_resolver

Closed

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

aitboudad wants to merge1 commit intosymfony:2.8fromaitboudad:translation_loader_resolver

Conversation

Copy link

Contributor

aitboudad commentedMay 17, 2015

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 =newTranslator('fr');$translator->addLoader('array',newArrayLoader());$translator->addResource('array',array('Hello World!' =>'Bonjour'),'fr');echo$translator->trans('Hello World!')."\n";

After

$resourceCatalogue =newResourceMessageCatalogueProvider();$resourceCatalogue->addLoader('array',newArrayLoader());$resourceCatalogue->addResource('array',array('Hello World!' =>'Bonjour'),'fr');$translator =newTranslator('fr',$resourceCatalogue);echo$translator->trans('Hello World!')."\n";

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 thetranslation_loader_resolver branch 3 times, most recently from7527f65 to557537fCompare

May 17, 2015 20:01

mpdude reviewed

May 18, 2015

View reviewed changes

src/Symfony/Component/Translation/CacheTranslatorBag.php

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

Copy link

Contributor

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

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

publicfunction getCatalogue($locale)

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

fixed, I removed the default value of locale argument inTranslatorBagInterface

aitboudad mentioned this pull request

May 18, 2015

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

Closed

mpdude reviewed

May 18, 2015

View reviewed changes

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

Copy link

Contributor

Choose a reason for hiding this comment

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

Two things –

Is that the right way to deprecateprotected 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.

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

I think so, if someone override public method too he can't get the deprecation notice.
It depends, for example to overridecomputeFallbackLocales method they must use customFallbackTranslatorBag.

aitboudad force-pushed thetranslation_loader_resolver branch 6 times, most recently fromb5c28e5 to65361ffCompare

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

Copy link

Contributor

MattKetmo commentedJul 15, 2015

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 theTranslator 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 newMessageCatalogueLoader interface and use decoration, which is basically the equivalent of your usage ofTranslatorBagInterface.

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

MattKetmo reviewed

Aug 3, 2015

View reviewed changes

src/Symfony/Component/Translation/TranslatorBag.php

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

Copy link

Contributor

MattKetmoAug 3, 2015

Choose a reason for hiding this comment

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

OthersTranslatorBag have a named (Cache, Fallback) except this one.
I would have renamed it toResourceTranslatorBag.

Copy link

ContributorAuthor

aitboudadAug 7, 2015

Choose a reason for hiding this comment

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

sound good to me 👍 , done

aitboudad force-pushed thetranslation_loader_resolver branch 2 times, most recently froma646aa7 to0a2363eCompare

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 thetranslation_loader_resolver branch 4 times, most recently from21431e3 to70d0b85Compare

September 25, 2015 22:41

Copy link

ContributorAuthor

aitboudad commentedSep 25, 2015

ping @symfony/deciders

Copy link

Member

fabpot commentedSep 30, 2015

Also, tests do not pass.

Copy link

Member

fabpot commentedSep 30, 2015

ping @symfony/deciders

aitboudad force-pushed thetranslation_loader_resolver branch 4 times, most recently fromd6d9ee1 to07a4775Compare

September 30, 2015 18:36

aitboudad mentioned this pull request

Sep 30, 2015

[2.8][Translation] added DoctrineMessageCatalogue.#16036

Closed

Copy link

ContributorAuthor

aitboudad commentedSep 30, 2015

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

Copy link

ContributorAuthor

aitboudad commentedSep 30, 2015

The failure tests is not related to this PR.

aitboudad added Status: Reviewed and removed Status: Needs Review labels

Sep 30, 2015

aitboudad force-pushed thetranslation_loader_resolver branch 4 times, most recently froma874893 toe53217dCompare

October 1, 2015 11:52

Copy link

Contributor

mpdude commentedOct 1, 2015

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.

Copy link

ContributorAuthor

aitboudad commentedOct 1, 2015

@mpdude what part do you think should be improved ?

Copy link

Contributor

mpdude commentedOct 1, 2015

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

Copy link

Contributor

mpdude commentedOct 3, 2015

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

@fabpot Would entirely deprecating the currentTranslator 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 throughprotected methods in the past?

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

[Translator] move loading catalogue out of Translator.

95e23d2

aitboudad force-pushed thetranslation_loader_resolver branch frome53217d to95e23d2Compare

October 11, 2015 11:46

Copy link

ContributorAuthor

aitboudad commentedOct 11, 2015

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.

aitboudad closed this

Oct 11, 2015

aitboudad deleted the translation_loader_resolver branch

October 11, 2015 12:09

Copy link

Contributor

MattKetmo commentedOct 11, 2015

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

Copy link

ContributorAuthor

aitboudad commentedOct 12, 2015

@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.