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

mpdude · 2015-05-03T21:49:22Z

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

We would like the Translator to be able to perform various optimizations on the MessageCatalogues it holds internally. One particular example is working with reduced catalogues for fallback locales that only contain the messages not present in one of the more preferred locales, or possibly do a lazy-loading of catalogues for fallback locales.

These optimizations are not possible as long as clients are able to retrieve the MessageCatalogue instances used inside the Translator by means of the TranslatorBagInterface and expect them to be full-fledged and complete catalogue instances (so the initial attempt had to be reverted in #14315, also see #14380 for a discussion).

In other words, TranslatorBagInterface breaks a boundary of encapsulation important for optimizations.

The two clients of the interface inside Symfony are not really interested in the catalogues per se, but only need them to iterate along the fallback chain in order to figure out the locale ultimately used. This information can now be obtained through the new FallbackLocaleAwareInterface.

To be discussed:

In 3.0, should FallbackLocaleAwareInterface remain a dedicated interface or become part of TranslatorInterface?
Shall we keep the TranslatorBagInterface in the future, especially if Translator does not implement it?

mpdude · 2015-05-03T21:56:55Z

src/Symfony/Component/Translation/Translator.php

@@ -506,4 +522,19 @@ private function getConfigCacheFactory()

        return $this->configCacheFactory;
    }
+
+    private function getCatalogueInternal($locale = null)


We could rename this back to getCatalogue() and make it private once we do no longer implement TranslatorBagInterface (in 3.0).

can you add this note above.

aitboudad · 2015-05-03T22:06:18Z

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

@@ -213,54 +213,6 @@ public function testDifferentCacheFilesAreUsedForDifferentSetsOfFallbackLocales(
        $this->assertEquals('bar', $translator->trans('bar'));
    }

-    public function testPrimaryAndFallbackCataloguesContainTheSameMessagesRegardlessOfCaching()


it shoud be removed in 3.0 rename it into testLegacyPrimaryA.. instead and add

$this->iniSet('error_reporting', -1 & ~E_USER_DEPRECATED);

aitboudad · 2015-05-03T22:13:58Z

src/Symfony/Component/Translation/FallbackLocaleAwareInterface.php

+ *
+ * @author Matthias Pigulla <mp@webfactory.de>
+ */
+interface FallbackLocaleAwareInterface


TranslatorLocaleAwareInterface ?

stof · 2015-05-04T13:17:26Z

src/Symfony/Component/Translation/DataCollectorTranslator.php

@@ -35,8 +35,8 @@ class DataCollectorTranslator implements TranslatorInterface, TranslatorBagInter
     */
    public function __construct(TranslatorInterface $translator)
    {
-        if (!$translator instanceof TranslatorBagInterface) {
-            throw new \InvalidArgumentException(sprintf('The Translator "%s" must implement TranslatorInterface and TranslatorBagInterface.', get_class($translator)));
+        if (!($translator instanceof TranslatorBagInterface && $translator instanceof FallbackLocaleAwareInterface)) {


requiring it to implement both is actually a BC break as not implementing FallbackLocaleAwareInterface was allowed previously. You are restricting the supported argument, and the BC promise only allows to widen it

the right solution would be to provide a Translator implementation which would implement FallbackLocaleAwareInterface on top of TranslatorBagInterface and wrap the translator into this implementation when it implements TranslatorBagInterface but not FallbackLocaleAwareInterface. This would make things BC, avoid duplication between the DataCollectorTranslator and the LoggingTranslator and not have performance penalty for the normal case (given that the default translator implements the new interface)

stof · 2015-05-04T13:53:20Z

@mpdude I just thought about another case where TranslatorBagInterface could be useful: https://github.com/willdurand/BazingaJsTranslationBundle/ is exposing translation catalogues to the JS code. Currently, it does that by reimplementing the loading itself (it gets all translation loaders from Symfony and gets all resources as well, all of which is done by the Translator) because TranslatorBagInterface was not available at that time. What would be the clean way toimplement such your case in your new architecture (duplicating the loading is not the clean way in the 2.6 architecture IMO, but it was probably the only way in the 2.3 architecture, which is why it is used)

aitboudad · 2015-05-04T14:13:01Z

well I think we need to keep getCatalogue and the optimize caching should be done by optimizing MessageCatalogue see #14526

mpdude · 2015-05-04T14:19:04Z

@stof In the case of messages available in both the primary and in fallback catalogues, which set of messages do you think this bundle would need?

The problem is that if we burden Translator with general tasks like providing complete MessageCatalogue chains, it gets harder to optimize the data structures used internally for the "find best translation available" use case (the trans() and transChoice() methods).

We could, of course, internally use optimized structures for these methods plus a "complete" set of MessageCatalogue chains for the TranslatorBagInterface-based use cases, but that would clearly be a nightmare to maintain and would not make a good example for the SRP.

So, what if we had two different classes (or services in the case of the full-stack framework):

The first one is where the resources and loaders are registered. You tell it your preferred order of locales, it will return a full-fledged MessageCatalogue chain (expensive operation!).

The second one is the Translator. It will initially obtain the MessageCatalogue from the first class, but then optimize/tweak things as it sees fit for the trans() and transChoice() methods. These optimized structures can be kept in the cache and will not be exposed to the outside world.

stof · 2015-05-04T15:17:58Z

@mpdude This bundle would need anything needed to be able to reimplement trans and transChoice in JS. So it would need the full catalogues for any active locales (it currently supports activating multiple locales, in case the locale choice need to be done on the JS side, or to keep a single translation file for all locales to simplify the loading of the JS), but only fallback messages might be enough for other locales.

And I agree that delegating all the loading to a separate class might be a better architecture. 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).

This is something I already thought about in the past. The hard part is the Translator class is currently designed to be extendable through inheritance. Extracting this logic into separate collaborators in a BC way will likely be very hard.

aitboudad · 2015-05-04T15:22:44Z

@stof FYI I work on delegating all the loading :)

stof · 2015-05-04T15:24:39Z

@aitboudad have you found a way to keep BC for people overwriting protected methods of the Translator to customize the Translator through its current extension points ?

aitboudad · 2015-05-04T19:31:35Z

@stof see an initial work for DelegationLoader I'll complete it soon, I think we need to deprecate Symfony\Bundle\FrameworkBundle\Translation\Translator.
To avoid BC we will create a new translator service and deprecate the default one.

aitboudad · 2015-05-05T10:28:05Z

@mpdude why we can't probably not move that responsibility out into another class ? until now
I have only one issue about initializeCatalogue, I'll see how to deal with it.

@stof should we move handling fallback catalogues into DelegationLoader or by composition instead ?

fabpot · 2015-09-24T15:10:03Z

What is the status of this PR? ping @aitboudad

mpdude · 2015-09-24T21:28:01Z

I rebased this branch and fixed the various issues from the first review by @aitboudad and @stof.

This won't take us to the ultimate goal of a better and more efficient Translator (#13948) because we'd probably also need to factor loading out of the Translator and prohibit inheritance (this and that comment and maybe something like the #14622 or #14671 PRs?)

Anyway, this IMO improves our situation because we can (in 3.0) remove an interface that definetly is a show-stopper.

mpdude · 2015-09-25T04:36:34Z

src/Symfony/Component/Translation/DataCollectorTranslator.php

@@ -39,6 +44,12 @@ public function __construct(TranslatorInterface $translator)
            throw new \InvalidArgumentException(sprintf('The Translator "%s" must implement TranslatorInterface and TranslatorBagInterface.', get_class($translator)));


Need to fix that. The interface is deprecated and must not be required.

aitboudad · 2015-09-25T10:19:42Z

@fabpot ll try to finish #14622 today as I think it's more efficient than this one(ideally we should have something like https://github.com/openl10n/openl10n-translation/tree/master/src/Openl10n/Component/Translation/MessageCatalogue)

…or a translation. That way, clients that need this information need no longer obtain the MessageCatalogues through the TranslatorBagInterface. This is a first step towards better encapsulation. Our final goal is to be able to use optimized implementations of the MessageCatalogue without leaking those to our clients.

…e on top of TranslatorBagInterface

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

@symfony/deciders I would like to ask for your vote on this.

I believe that removing the TranslatorBagInterface is one important prerequisite for more efficient caching and/or loading inside the Translator because it improves encapsulation.

With the current state of this PR, one important thing to note is that we leave clients without a way to retrieve the message catalogue from the translator, a feature added in 2.6 "by accident" (it would not have been necessary for the feature we added back then).

To provide an alternate means of retrieving the catalogue is an on-going effort in #14622 and #14671, but with the internal structure of Translator (different code paths, mixing several concerns, protected methods and @api) I am unsure if we can make it.

So, the decision you need to make is whether the improvement this PR brings is worth this possible annoyance for users of the TranslatorBagInterface in case that no other replacement for it can be provided.

Note that @stof refers to one third-party bundle that works without this interface by implementing the loading itself.

(The failing test is for PHP 7 is completely unrelated.)

nicolas-grekas · 2017-02-14T13:28:28Z

I think we don't need this one: optimized caching is already dealt with on PHP7, where static arrays and static strings are far superior optimizing strategies. Thus, no need to do anything more on our side to me.

jakzal · 2017-02-28T13:15:31Z

@mpdude any thoughts on what Nicolas said?

nicolas-grekas · 2017-07-11T14:42:32Z

Closing as explained. Of course, please reopen if you don't agree.

mpdude · 2017-07-12T07:33:24Z

I opened this a while ago when I was working on a project where we had issues due to a large number of languages supported and comprehensive catalogs.

We found other ways to work around this and I cannot tell whether the problem would still appear on PHP7.

So, closing this is fine for me. Will reopen it or make a new case should I run into it again.

mpdude reviewed May 3, 2015
View reviewed changes

mpdude mentioned this pull request May 3, 2015

[RFC][Translator] Deprecate fallback catalogues and remove them in 3.0 #14380

Closed

aitboudad reviewed May 3, 2015
View reviewed changes

mpdude mentioned this pull request May 3, 2015

[Translation] Translator::getMessages() is not part of an interface #14529

Closed

aitboudad reviewed May 3, 2015
View reviewed changes

aitboudad added the Translation label May 3, 2015

stof reviewed May 4, 2015
View reviewed changes

mpdude mentioned this pull request May 12, 2015

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

Closed

7 tasks

aitboudad mentioned this pull request May 17, 2015

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

Closed

mpdude reviewed Sep 25, 2015
View reviewed changes

mpdude changed the title ~~[RFC] [Translation] Remove TranslatorBagInterface to allow for optimized caching in 3.0~~ [Translation] Remove TranslatorBagInterface to allow for optimized caching in 3.0 Oct 3, 2015

mpdude added 14 commits October 3, 2015 11:01

Keep test methods around as long as we still implement the interface

2688b09

Add note for future me.

3077d80

For BC, use an adapter class to implement FallbackLocaleAwareInterfac…

fa055ec

…e on top of TranslatorBagInterface

Add deprecation notices to docblocks

e4bba37

Avoid unnecessary code change

372c247

use strict comparison

98b1a74

Use if instead of elseif because the if returns

89df6f5

Update legacy tests

24f0830

Improve deprecation notice

1364559

Improve docblock

c0f6bf0

Fabbot

4df5021

We must not require clients to implement a deprecated interface

2a72a09

Typo pointed out by FabBot

ad53737

javiereguiluz added the Enhancement label Feb 1, 2016

nicolas-grekas added this to the 3.x milestone Dec 6, 2016

nicolas-grekas closed this Jul 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mpdude commented May 3, 2015

mpdude May 3, 2015

aitboudad May 3, 2015

aitboudad May 3, 2015

aitboudad May 3, 2015

stof May 4, 2015

stof May 4, 2015

stof commented May 4, 2015

aitboudad commented May 4, 2015

mpdude commented May 4, 2015

stof commented May 4, 2015

aitboudad commented May 4, 2015

stof commented May 4, 2015

aitboudad commented May 4, 2015

aitboudad commented May 5, 2015

fabpot commented Sep 24, 2015

mpdude commented Sep 24, 2015

mpdude Sep 25, 2015

aitboudad commented Sep 25, 2015

mpdude commented Oct 3, 2015

nicolas-grekas commented Feb 14, 2017

jakzal commented Feb 28, 2017

nicolas-grekas commented Jul 11, 2017

mpdude commented Jul 12, 2017

		@@ -39,6 +44,12 @@ public function __construct(TranslatorInterface $translator)
		throw new \InvalidArgumentException(sprintf('The Translator "%s" must implement TranslatorInterface and TranslatorBagInterface.', get_class($translator)));

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

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

Conversation

mpdude commented May 3, 2015

mpdude May 3, 2015

Choose a reason for hiding this comment

aitboudad May 3, 2015

Choose a reason for hiding this comment

aitboudad May 3, 2015

Choose a reason for hiding this comment

aitboudad May 3, 2015

Choose a reason for hiding this comment

stof May 4, 2015

Choose a reason for hiding this comment

stof May 4, 2015

Choose a reason for hiding this comment

stof commented May 4, 2015

aitboudad commented May 4, 2015

mpdude commented May 4, 2015

stof commented May 4, 2015

aitboudad commented May 4, 2015

stof commented May 4, 2015

aitboudad commented May 4, 2015

aitboudad commented May 5, 2015

fabpot commented Sep 24, 2015

mpdude commented Sep 24, 2015

mpdude Sep 25, 2015

Choose a reason for hiding this comment

aitboudad commented Sep 25, 2015

mpdude commented Oct 3, 2015

nicolas-grekas commented Feb 14, 2017

jakzal commented Feb 28, 2017

nicolas-grekas commented Jul 11, 2017

mpdude commented Jul 12, 2017