[Translation] Add TranslatorFallbackInterface, fixes #25356 #25549

Warxcell · Dec 19, 2017

Q	A
Branch?	master
Bug fix?	no
New feature?	kind of
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#25356
License	MIT
Doc PR	no

Simperfit · Dec 20, 2017

Could you please squash all the commits ? You should link the email you comitted with to your github account.

Warxcell · Dec 20, 2017

@Simperfit done.

Simperfit · Dec 20, 2017

src/Symfony/Component/Translation/TranslatorFallbackInterface.php

+namespace Symfony\Component\Translation;
+
+/**
+ * TranslatorFallbackInterface.


this is not needed

Simperfit · Dec 20, 2017

src/Symfony/Component/Translation/TranslatorFallbackInterface.php

+interface TranslatorFallbackInterface extends TranslatorInterface
+{
+    /**
+     * Gets the fallback locales.


the method name is self explanatory.

I used description as-is from Translator.php

Simperfit · Dec 20, 2017

src/Symfony/Component/Translation/TranslatorFallbackInterface.php

+    public function getFallbackLocales();
+
+    /**
+     * Sets the fallback locales.


the method name is self explanatory.

I used description as-is from Translator.php

You can remove this IMHO.

Everywhere else there is a description. This method is also self-explanatory, but it has description.

/** * Returns the current locale. * * @return string The locale */ public function getLocale();

I think it should stay.

I understand the point but the fact is this should be cleaned : #24931

Simperfit · Dec 20, 2017

src/Symfony/Component/Translation/IdentityTranslator.php

@@ -18,7 +18,14 @@
 */
 class IdentityTranslator implements TranslatorInterface
 {
+    /**
+     * @var MessageSelector


to be removed

Why? I think it's good to typehint variables in docblock?

It does not add anything because we already know this with the constructor.

Simperfit · Dec 20, 2017

src/Symfony/Component/Translation/IdentityTranslator.php

    private $selector;
+
+    /**
+     * @var string


to be removed

Simperfit · Dec 20, 2017

Since it's a new feature could you please target master ?

Also you are missing some test ;)

Simperfit · Dec 20, 2017

src/Symfony/Component/Translation/DataCollectorTranslator.php

+    public function setFallbackLocales(array $locales)
+    {
+        if (!$this->translator instanceof TranslatorFallbackInterface) {
+            throw new \BadMethodCallException('The original translator does not support fallback locales');


missing dot at te end

Simperfit · Dec 20, 2017

src/Symfony/Component/Translation/LoggingTranslator.php

+    public function setFallbackLocales(array $locales)
+    {
+        if (!$this->translator instanceof TranslatorFallbackInterface) {
+            throw new \BadMethodCallException('The original translator does not support fallback locales');


missing dot at the end

Warxcell · Dec 20, 2017

@Simperfit I'm not sure if it's whole new feature, because the code still looks and acts like before.
The same reason I think tests are enough. I could only assert all fallback tests that Translator is fallback translator ?

Simperfit · Dec 20, 2017

For the test you need to assert that an exception is thrown when the translator does not implement the new interface.

IMHO it's a new interface, it should be in master but I may be wrong. @xabbuh WDYT ?

Warxcell · Dec 20, 2017

@Simperfit I added some tests. You can see them.

nicolas-grekas

adding a new interface is definitely a new feature to me, so this should be for master, isn't it?

nicolas-grekas · Dec 20, 2017

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

@@ -134,6 +136,7 @@ public function testSetFallbackLocalesMultiple()
        // force catalogue loading
        $translator->trans('bar');

+        $this->assertTrue($translator instanceof TranslatorFallbackInterface);


you should use assertInstanceOf everywhere

Warxcell · Dec 20, 2017

Removed redundant descriptions and typehints.

Replace assertTrue with assertInstanceOf

stof · Dec 20, 2017

this must indeed go in master

stof · Dec 20, 2017

src/Symfony/Component/Translation/TranslatorFallbackInterface.php

+     *
+     * @throws \InvalidArgumentException If a locale contains invalid characters
+     */
+    public function setFallbackLocales(array $locales);


I'm not sure this method needs to be in the interface. Configuring the instance does not need to belong in the API.

Then why TranslatorInterface has ability to setLocale ?

I'm just following the same pattern. Indeed I agree that if the Translation Component does not need to configure Translator, it should be only inside implementation.

The locale can change throughout the request. But I agree with @stof that this is probably not needed for the fallback locales.

Warxcell · Dec 20, 2017

Rebased from master.

src/Symfony/Component/Translation/DataCollectorTranslator.php

src/Symfony/Component/Translation/LoggingTranslator.php

Warxcell · Dec 21, 2017

@xabbuh I made the requested changes. I just don't get what's the problem with appveyor?

Warxcell · Dec 21, 2017

Actually I still believe that setFallbackLocales should be in API, as it's used by framework bundle.

https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php#L888

What will happens if translator is replaced in some project with translator that does not support fallback locales?

xabbuh · Dec 22, 2017

The failure on AppVeyor is not related to these changes.

Warxcell · Dec 25, 2017

Let's say I also want to build something like "listener". A callback - when locale/fallback locales is/are changed inside translator - something to happen?

nicolas-grekas · Sep 21, 2018

I think I'm 👎 here: getFallbackLocales only fits reflection purposes in the code base. We'd better deprecate it actually and find another way for data collectors and debug:translation. Let's reduce the complexity instead of increasing it.

Warxcell · Sep 21, 2018

@nicolas-grekas actually I'm not using that for debug purposes. I have built Entity Translation Bundle (for translating objects, mainly entities) which can be coupled with Symfony Translator (share same locale and fallback locales) instead of setting twice same thing.

Is there way to acomplish that with data collector?

nicolas-grekas · Sep 21, 2018

Setting the same options/locales/fallbacks when configuring your translator seems the best option to me.

nicolas-grekas · Sep 24, 2018

Closing in favor of #28579

carsonbot added Status: Needs Review Translation Feature labels Dec 19, 2017

Simperfit reviewed Dec 20, 2017

View reviewed changes

src/Symfony/Component/Translation/IdentityTranslator.php Outdated

private $selector;

/**

* @var string

Copy link

Contributor

Simperfit Dec 20, 2017

Choose a reason for hiding this comment

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

to be removed

Simperfit reviewed Dec 20, 2017

View reviewed changes

nicolas-grekas added this to the 2.7 milestone Dec 20, 2017

nicolas-grekas reviewed Dec 20, 2017

View reviewed changes

Warxcell changed the base branch from 2.7 to master December 20, 2017 10:36

stof reviewed Dec 20, 2017

View reviewed changes

xabbuh modified the milestones: 2.7, 4.1 Dec 20, 2017

xabbuh requested changes Dec 20, 2017

View reviewed changes

src/Symfony/Component/Translation/DataCollectorTranslator.php Outdated Show resolved Hide resolved

src/Symfony/Component/Translation/DataCollectorTranslator.php Outdated Show resolved Hide resolved

src/Symfony/Component/Translation/LoggingTranslator.php Outdated Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels Dec 20, 2017

[Translation] fixes #25356, add TranslatorFallbackInterface

6ca51c3

nicolas-grekas modified the milestones: 4.1, next Apr 20, 2018

nicolas-grekas mentioned this pull request Sep 24, 2018

Deprecate Translation::getFallbackLocales() #28579

Closed

nicolas-grekas closed this Sep 24, 2018

nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018

Search code, repositories, users, issues, pull requests...

Uh oh!

[Translation] Add TranslatorFallbackInterface, fixes #25356 #25549

[Translation] Add TranslatorFallbackInterface, fixes #25356 #25549

Uh oh!

Conversation

Warxcell commented Dec 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Simperfit commented Dec 20, 2017

Uh oh!

Warxcell commented Dec 20, 2017

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!

Warxcell Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Simperfit Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Simperfit commented Dec 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Warxcell commented Dec 20, 2017

Uh oh!

Simperfit commented Dec 20, 2017

Uh oh!

Warxcell commented Dec 20, 2017

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Warxcell commented Dec 20, 2017

Uh oh!

stof commented Dec 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Warxcell Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Warxcell commented Dec 20, 2017

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Warxcell commented Dec 21, 2017

Uh oh!

Warxcell commented Dec 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Warxcell commented Dec 19, 2017 •

edited

Loading

Warxcell Dec 20, 2017 •

edited

Loading

Simperfit Dec 20, 2017 •

edited

Loading

Warxcell Dec 20, 2017 •

edited

Loading

Warxcell commented Dec 21, 2017 •

edited

Loading

Warxcell commented Sep 21, 2018 •

edited

Loading