[Translation] added LoggingTranslator. #10887

aitboudad · 2014-05-10T22:45:52Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#3015, #2435
License	MIT
Doc PR	symfony/symfony-docs/pull/4050

jakzal · 2014-05-10T23:08:28Z

src/Symfony/Component/Translation/Translator.php

+        }
+
+        return $this->catalogues[$locale];
+    }


This would fix #2435. Might be worth to cover it by test.

jakzal · 2014-05-10T23:10:01Z

This is a much better approach than previous attempts to solve the problem.

jakzal · 2014-05-10T23:10:56Z

src/Symfony/Component/Translation/LoggableTranslator.php

+use Psr\Log\LoggerInterface;
+
+/**
+ * Translator.


Remove please.

stof · 2014-05-10T23:48:00Z

src/Symfony/Component/Translation/LoggableTranslator.php

+     * @param TranslatorInterface $translator The translator
+     * @param LoggerInterface     $logger     A logger instance
+     */
+    public function __construct(TranslatorInterface $translator, LoggerInterface $logger = null)


the typehint is wrong. It expects getCatalogue on the translator, which is not part of the interface

aitboudad · 2014-06-08T11:52:35Z

To enable logger in config (default value is %kernel.debug%):

framework:
    #esi:             ~
    translator:      { fallback: "%locale%", logging: true }

Koc · 2014-06-08T12:05:18Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

@@ -428,6 +428,7 @@ private function addTranslatorSection(ArrayNodeDefinition $rootNode)
                    ->canBeEnabled()
                    ->children()
                        ->scalarNode('fallback')->defaultValue('en')->end()
+                        ->booleanNode('enable_logging')->defaultValue('%kernel.debug%')->end()


This doesn't work. Take a look at the documentation http://symfony.com/doc/current/cookbook/configuration/using_parameters_in_dic.html

Also let's name it just logging as in Swiftmailer/Doctrine other bundles.

fabpot · 2014-06-16T08:26:44Z

👍

stof · 2014-06-16T08:32:40Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -610,6 +610,8 @@ private function registerTranslatorConfiguration(array $config, ContainerBuilder
        }
        $translator->addMethodCall('setFallbackLocales', array($config['fallback']));

+        $container->setParameter('translator.logging', $container->getParameterBag()->resolveValue($config['logging']));


I don't see the point of resolving it here. Given that other places could overwrite the parameter anyway, this does not ensure it is resolved when reaching the compiler pass. The resolution should be moved to the compiler pass

Why use non-usual way? Let's do on same manner like in Swiftmailer/Doctrine other bundles and how documentation suggests (provide argument $debug for the configuration class.

fabpot · 2014-07-25T09:48:48Z

👍

…e LoggableTranslatorPass.

…terface and TranslatorBagInterface.

fabpot · 2014-09-24T08:37:16Z

Thank you @aitboudad.

This PR was squashed before being merged into the 2.6-dev branch (closes #10887). Discussion ---------- [Translation] added LoggingTranslator. | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #3015, #2435 | License | MIT | Doc PR | symfony/symfony-docs/pull/4050 Commits ------- b7770bc [Translation] added LoggingTranslator.

lyrixx · 2014-09-24T17:03:37Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/LoggingTranslatorPass.php

+            return;
+        }
+
+        if ($container->getParameter('translator.logging')) {


This broke my symfony-master

ParameterNotFoundException in ParameterBag.php line 106: You have requested a non-existent parameter "translator.logging". Did you mean this: "translator.logging.class"?

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

I opened a PR: #12023

tvogt · 2014-11-10T10:51:46Z

This addition is currently breaking my install:

Catchable Fatal Error: Argument 1 passed to BM2\SiteBundle\Twig\GeographyExtension::__construct() must be an instance of Symfony\Component\Translation\Translator, instance of Symfony\Component\Translation\LoggingTranslator given, called in /home/maf/symfony/app/cache/test/appTestDebugProjectContainer.php on line 4885 and defined

The funny part is that changing GeographyExtension leads to the inverse error - now it expects LoggingTranslator, but is given Translator.

Koc · 2014-11-10T11:33:11Z

@tvogt Have you tryed typehint using TranslatorInterface instead of concrete implementation?

rvanlaak · 2014-11-14T17:54:07Z

Using the TranslatorInterface as @Koc mentioned also solved a broken FormType after the upgrade for me.

->setAllowedTypes(array(
    'translator' => 'Symfony\Bundle\FrameworkBundle\Translation\Translator',
));

should be

->setAllowedTypes(array(
    'translator' => 'Symfony\Component\Translation\TranslatorInterface',
));

It seems like the default framework Translator has changed, so maybe mention something about the changes of the Translator component in UPGRADE-2.6.md?

stof · 2014-11-15T11:10:36Z

@rvanlaak any code typehinting the concrete implementation rather than the interface will suffers from issues as soon as a bundle extends the functionalities of service through composition (which is a much better way than doing it through inheritance). You should also rely on the interface for the typehint rather than on concrete implementations

aitboudad · 2014-11-15T12:05:18Z

@stof, @fabian to avoid BC, what do you think to set logging as false by default instead of debug mode ?

rvanlaak · 2014-11-15T12:29:14Z

@stof I totally agree with you, but couldn't find something about it in the changelog

@aitboudad +1 for enabling the logging by default, it is a great feature!

linaori · 2014-11-15T12:31:19Z

...fony/Bundle/FrameworkBundle/Tests/DependencyInjection/Compiler/LoggingTranslatorPassTest.php

+            ->will($this->returnValue($parameterBag));
+
+        $pass = new LoggingTranslatorPass();
+        $pass->process($container);


The container is just a data container, why don't you just test if the data you expect to be added in the container is there rather than writing tests to see if the container gets called the way you think it gets called?

It will make the test easier to update and less fragile, as the inner workings may change over time while the input and output stay the same.

alcalyn · 2014-11-20T10:38:56Z

I just update to 2.6@dev.

Note that the translator (service translator) refers now to LoggingTranslator

I got it from php app/console container:debug :

translator            Symfony\Component\Translation\LoggingTranslator                                            
translator.default    Symfony\Bundle\FrameworkBundle\Translation\Translator

Edit: translator refers to LoggingTranslator only if debug mode is on.
So if like me you set Translator as type hint, juste change to TranslatorInterface ;)

linaori · 2014-11-20T10:45:04Z

In both debug and non-debug?

alcalyn · 2014-11-20T11:04:46Z

No, I got Symfony\Bundle\FrameworkBundle\Translation\Translator from translator when debug mode is disabled. So it's ok

tvogt · 2014-11-21T16:27:47Z

@Koc yes, of course.

…lator (derrabus) This PR was merged into the 2.6 branch. Discussion ---------- [Translation] [2.6] Upgrade information for LoggingTranslator | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | none | License | MIT | Doc PR | none When upgrading a Symfony 2.5 project to the 2.6 branch, I noticed that the `@translator` service was changed. In the affected project, we've had a service that depends on the `@translator` service and uses a type hint to ensure that a `Translator` instance is passed to the constructor. With the introduction of the `LoggingTranslator` class (PR #10887), this type hint now fails. I have added a small note to ` UPGRADE-2.6.md` in case more people stumble across this changed behavior. Commits ------- cd55a81 Upgrade information for the Translation component regarding the new LoggingTranslator class.

This PR was submitted for the master branch but it was merged into the 2.6 branch instead (closes #4050). Discussion ---------- [Translation] added logging capability. | Q | A | ------------- | --- | Doc fix? | no | New docs? | yes(symfony/symfony/pull/10887) | Applies to | 2.6+ |Fixed tickets | - Commits ------- e8e50fa added logging to translator.

jakzal reviewed May 10, 2014
View reviewed changes

src/Symfony/Component/Translation/LoggableTranslator.php

use Psr\Log\LoggerInterface;

/**

* Translator.

Copy link

Contributor

jakzal May 10, 2014

Choose a reason for hiding this comment

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

Remove please.

jakzal added the Translator label May 10, 2014

stof reviewed May 10, 2014
View reviewed changes

Koc reviewed Jun 8, 2014
View reviewed changes

stof reviewed Jun 16, 2014
View reviewed changes

aitboudad mentioned this pull request Jul 23, 2014

[Translation] added logging capability. symfony/symfony-docs#4050

Closed

aitboudad added 8 commits September 24, 2014 09:10

[FrameworkBundle] [Translation] use LoggableTranslator in mode debug.

f543258

[Translation] added test GetCatalogue

8fdd235

[Translation][LoggableTranslator] make logger required with Minor fix.

0f2294d

[FrameworkBundle] [Translation] Added config to enable logging and us…

ecea954

…e LoggableTranslatorPass.

fixed typo.

38fb884

added TranslatorBagInterface.

071880b

Use LoggableTranslator only if the translator implements TranslatorIn…

8af5171

…terface and TranslatorBagInterface.

rename LoggableTranslator to LoggingTranslator + minor fixes.

c7ee300

aitboudad force-pushed the ticket_3015 branch from b862638 to c7ee300 Compare September 24, 2014 08:15

fabpot closed this Sep 24, 2014

aitboudad deleted the ticket_3015 branch September 24, 2014 08:37

lyrixx reviewed Sep 24, 2014
View reviewed changes

linaori reviewed Nov 15, 2014
View reviewed changes

derrabus mentioned this pull request Nov 26, 2014

[Translation] [2.6] Upgrade information for LoggingTranslator #12579

Merged

bdujon mentioned this pull request Apr 6, 2015

Controllers in Symfony 2.6.0 slower than 2.5.x? #12790

Closed

Uh oh!

[Translation] added LoggingTranslator. #10887

[Translation] added LoggingTranslator. #10887

Uh oh!

Conversation

aitboudad commented May 10, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakzal commented May 10, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aitboudad commented Jun 8, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Jun 16, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Jul 25, 2014

Uh oh!

fabpot commented Sep 24, 2014

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!

tvogt commented Nov 10, 2014

Uh oh!

Koc commented Nov 10, 2014

Uh oh!

rvanlaak commented Nov 14, 2014

Uh oh!

stof commented Nov 15, 2014

Uh oh!

aitboudad commented Nov 15, 2014

Uh oh!

rvanlaak commented Nov 15, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alcalyn commented Nov 20, 2014

Uh oh!

linaori commented Nov 20, 2014

Uh oh!

alcalyn commented Nov 20, 2014

Uh oh!

tvogt commented Nov 21, 2014

Uh oh!

Uh oh!