[Translation] Added logging capability to translator #9770

aitboudad · 2013-12-14T13:55:27Z

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

stloyd · 2013-12-14T14:02:32Z

src/Symfony/Component/Translation/Translator.php

@@ -185,6 +191,8 @@ public function trans($id, array $parameters = array(), $domain = null, $locale
            $this->loadCatalogue($locale);
        }

+        $catalogue = $this->filterCatalogueForId($this->catalogues[$locale], $id, $domain);


This $catalogue variable is not used anywhere.

stloyd · 2013-12-14T14:04:05Z

You should add dev dependency for logger in composer.json.

cordoval · 2013-12-14T14:17:16Z

src/Symfony/Component/Translation/Translator.php

+            if ($catalogue->has($id, $domain)) {
+                $this->logger->info('Translator: using fallback catalogue.', array('id' => $id, 'domain' => $domain));
+            } else {
+                $this->logger->warn('Translator: translation not found.', array('id' => $id, 'domain' => $domain));


be compliant with PRS logger interface -> warning

Tobion · 2013-12-14T21:16:45Z

src/Symfony/Component/Translation/Translator.php

+     * @param  string                    $domain
+     * @return MessageCatalogueInterface
+     */
+    protected function filterCatalogueForId(MessageCatalogueInterface $catalogue, $id, $domain)


we don't use protected by default but private. also this method doesnt make much sense since it only returns $catalogue that is passed to it.

I think its better to only call this method when there is an actual logger, as calling a method which does nothing (in production) adds up the method-calls. And the trans(Choice) method is potentially called 20+ times or so. So any performance gain (even how small is) is a big plus.

fabpot · 2013-12-15T17:08:00Z

src/Symfony/Component/Translation/Translator.php

@@ -54,6 +55,11 @@ class Translator implements TranslatorInterface
    private $selector;

    /**
+     * @var LoggerInterface|null
+     */
+    protected $logger;


should be private

Tobion · 2013-12-15T20:15:49Z

src/Symfony/Component/Translation/Translator.php

@@ -244,6 +271,22 @@ protected function loadCatalogue($locale)
        $this->loadFallbackCatalogues($locale);
    }

+    /**
+     * Extension point for handling catalogue misses or fallbacks.


its not an "extension point"

also the method name is quite strange. it does not "filter" anything. why not simply call it log

stof · 2013-12-16T17:34:15Z

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

@@ -398,6 +398,7 @@ private function addTranslatorSection(ArrayNodeDefinition $rootNode)
                    ->canBeEnabled()
                    ->children()
                        ->scalarNode('fallback')->defaultValue('en')->end()
+                        ->scalarNode('logger_id')->defaultNull()->end()


There is no point in providing the logger id IMO. It is totally inconsistent with all other places in Symfony. You should inject the logger service instead, and use a dedicated Monolog channel through the tag.
the logging configuration should be a boolean to enable/disable it if you want it

@stof #9770 (diff)

well, using a different channel should be done in core, like for other services supporting logging.

Thus, most services don't even ask you whether they should enable logging or no. they just enable it all the time as soon as the logger service is available.

@stof I will delete logger_id, and add channel 'translation' in core.

are you agree :) ?

yeah, looks good

fabpot · 2013-12-29T15:00:12Z

Can you rebase on current master?

fabpot · 2013-12-29T15:04:20Z

Actually, as the trans and transChoice methods are called a lot on a typical website, I'd like to see how the new code impact the performance of these methods (with a logger that does nothing for the added channel as it will probably be the most common use case). So, I'm -1 on this PR.

aitboudad · 2014-01-05T22:47:13Z

Well what do you think about injecting a logger only on mode debug ??

fabpot · 2014-01-06T16:23:35Z

Injection the logger only in debug more is probably a better idea.

fabpot · 2014-01-06T16:23:53Z

src/Symfony/Component/Translation/Translator.php

+    private function log(MessageCatalogueInterface $catalogue, $id, $domain)
+    {
+        if ($catalogue->has($id, $domain)) {
+            $this->logger->debug('Translation use fallback catalogue.', array('id' => $id, 'domain' => $domain, 'locale' => $catalogue->getLocale()));


logger can be null here

It's checked in if (null !== $this->logger && !$catalogue->defines($id, $domain)), but maybe it'll be a good idea to move checking if logger exists inside the method.

fabpot · 2014-01-06T16:24:06Z

src/Symfony/Component/Translation/Translator.php

@@ -223,6 +240,16 @@ public function transChoice($id, $number, array $parameters = array(), $domain =
    }

    /**
+     * Set the logger.


fabpot · 2014-01-06T16:24:36Z

You should also adde some unit tests for this new feature and a CHANGELOG entry (in the component dir)

aitboudad · 2014-01-13T23:14:16Z

closed in favor of #10014

aitboudad added 2 commits December 14, 2013 13:39

[Translation] added logger

681cb60

[FrameworkBundle] added logging capabilities to translator

945fba7

stloyd reviewed Dec 14, 2013
View reviewed changes

cordoval reviewed Dec 14, 2013
View reviewed changes

aitboudad added 3 commits December 14, 2013 14:20

[Translation][composer] added psr/log

78c124f

[Translation] logger fixed bugs

95234e7

[FrameworkBundle][Tests] Fixed failing test

1967081

Tobion reviewed Dec 14, 2013
View reviewed changes

aitboudad added 3 commits December 15, 2013 12:38

[Translation][Composer] added missing text

147b6f1

[FrameworkBundle] [Translation] allow injecting a logger.

ee9da6e

[Translation] call filterCatalogueForId when logger is not null

f07beca

fabpot reviewed Dec 15, 2013
View reviewed changes

[Translation] [logger] change protected to private

ff5e3c0

Tobion reviewed Dec 15, 2013
View reviewed changes

[translation] [logger] added locale

9497aa8

stof reviewed Dec 16, 2013
View reviewed changes

aitboudad added 3 commits December 16, 2013 20:42

[Translation] [logger] use Monolog channel

56c6bb3

[translation][logger] use debug instead of info

f4f99c6

delete logger_id

d625d07

fabpot reviewed Jan 6, 2014
View reviewed changes

aitboudad closed this Jan 13, 2014

aitboudad deleted the ticket_3015 branch January 13, 2014 23:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Translation] Added logging capability to translator #9770

[Translation] Added logging capability to translator #9770

aitboudad commented Dec 14, 2013

stloyd Dec 14, 2013

stloyd commented Dec 14, 2013

cordoval Dec 14, 2013

Tobion Dec 14, 2013

sstok Dec 15, 2013

fabpot Dec 15, 2013

Tobion Dec 15, 2013

Tobion Dec 15, 2013

stof Dec 16, 2013

aitboudad Dec 16, 2013

stof Dec 16, 2013

aitboudad Dec 16, 2013

stof Dec 16, 2013

fabpot commented Dec 29, 2013

fabpot commented Dec 29, 2013

aitboudad commented Jan 5, 2014

fabpot commented Jan 6, 2014

fabpot Jan 6, 2014

piotrpasich Jan 7, 2014

fabpot Jan 6, 2014

fabpot commented Jan 6, 2014

aitboudad commented Jan 13, 2014

[Translation] Added logging capability to translator #9770

[Translation] Added logging capability to translator #9770

Conversation

aitboudad commented Dec 14, 2013

Choose a reason for hiding this comment

stloyd commented Dec 14, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Dec 29, 2013

fabpot commented Dec 29, 2013

aitboudad commented Jan 5, 2014

fabpot commented Jan 6, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Jan 6, 2014

aitboudad commented Jan 13, 2014