Skip to content

[Translation] added LoggingTranslator. #10887

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 10 commits into from
Closed
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler;

use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;

/**
* @author Abdellatif Ait boudad <a.aitboudad@gmail.com>
*/
class LoggingTranslatorPass implements CompilerPassInterface
{
public function process(ContainerBuilder $container)
{
if (!$container->hasAlias('logger')) {
return;
}

if ($container->getParameter('translator.logging')) {
Copy link
Member

Choose a reason for hiding this comment

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

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"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I opened a PR: #12023

$translatorAlias = $container->getAlias('translator');
$definition = $container->getDefinition((string) $translatorAlias);
$class = $container->getParameterBag()->resolveValue($definition->getClass());

$refClass = new \ReflectionClass($class);
if ($refClass->implementsInterface('Symfony\Component\Translation\TranslatorInterface') && $refClass->implementsInterface('Symfony\Component\Translation\TranslatorBagInterface')) {
$container->getDefinition('translator.logging')->setDecoratedService('translator');
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,16 @@
*/
class Configuration implements ConfigurationInterface
{
private $debug;

/**
* @param bool $debug Whether debugging is enabled or not
*/
public function __construct($debug)
{
$this->debug = (bool) $debug;
}

/**
* Generates the configuration tree builder.
*
Expand Down Expand Up @@ -441,6 +451,7 @@ private function addTranslatorSection(ArrayNodeDefinition $rootNode)
->canBeEnabled()
->children()
->scalarNode('fallback')->defaultValue('en')->end()
->booleanNode('logging')->defaultValue($this->debug)->end()
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not such a fan of dynamic defaults as it means you get different output from config:dump-reference depending on the environment. then again I guess its the "honest" thing because if you default to false here and then overwrite the false in dev mode, then it is confusing as well.

->end()
->end()
->end()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,14 @@ public function load(array $configs, ContainerBuilder $container)
));
}

/**
* {@inheritdoc}
*/
public function getConfiguration(array $config, ContainerBuilder $container)
{
return new Configuration($container->getParameter('kernel.debug'));
}

/**
* Loads Form configuration.
*
Expand Down Expand Up @@ -627,6 +635,8 @@ private function registerTranslatorConfiguration(array $config, ContainerBuilder
}
$translator->addMethodCall('setFallbackLocales', array($config['fallback']));

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

// Discover translation directories
$dirs = array();
if (class_exists('Symfony\Component\Validator\Validator')) {
Expand Down
2 changes: 2 additions & 0 deletions src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\RoutingResolverPass;
use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\ProfilerPass;
use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\TranslatorPass;
use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\LoggingTranslatorPass;
use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\AddCacheWarmerPass;
use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\AddCacheClearerPass;
use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\ContainerBuilderDebugDumpPass;
Expand Down Expand Up @@ -77,6 +78,7 @@ public function build(ContainerBuilder $container)
$container->addCompilerPass(new AddConsoleCommandPass());
$container->addCompilerPass(new FormPass());
$container->addCompilerPass(new TranslatorPass());
$container->addCompilerPass(new LoggingTranslatorPass());
$container->addCompilerPass(new AddCacheWarmerPass());
$container->addCompilerPass(new AddCacheClearerPass());
$container->addCompilerPass(new TranslationExtractorPass());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

<parameters>
<parameter key="translator.class">Symfony\Bundle\FrameworkBundle\Translation\Translator</parameter>
<parameter key="translator.logging.class">Symfony\Component\Translation\LoggingTranslator</parameter>
Copy link
Contributor

Choose a reason for hiding this comment

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

As of 2.6 this won't be done anymore. Can you move the class name to the service definition?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iltar Agreed for an application-specific but not for a reusable bundles ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

@aitboudad, this is not good for re-usability as you cannot change the constructor. You can use service decoration for this. I suggest you read upon the issue Fabien made: #11881

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iltar 👍

<parameter key="translator.identity.class">Symfony\Component\Translation\IdentityTranslator</parameter>
<parameter key="translator.selector.class">Symfony\Component\Translation\MessageSelector</parameter>
<parameter key="translation.loader.php.class">Symfony\Component\Translation\Loader\PhpFileLoader</parameter>
Expand Down Expand Up @@ -46,6 +47,12 @@
</argument>
</service>

<service id="translator.logging" class="%translator.logging.class%" public="false">
<argument type="service" id="translator.logging.inner" />
<argument type="service" id="logger" />
<tag name="monolog.logger" channel="translation" />
Copy link
Contributor

Choose a reason for hiding this comment

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

A translator is not a loger, is it?

Copy link
Member

Choose a reason for hiding this comment

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

monolog.logger is not about telling the service is a logger. It is aout telling MonologBundle that it should inject a logger for the given channel rather than the default one

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

</service>

<service id="translator" class="%translator.identity.class%">
<argument type="service" id="translator.selector" />
</service>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Bundle\FrameworkBundle\Tests\DependencyInjection\Compiler;

use Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler\LoggingTranslatorPass;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Definition;

class LoggingTranslatorPassTest extends \PHPUnit_Framework_TestCase
{
public function testProcess()
{
$definition = $this->getMock('Symfony\Component\DependencyInjection\Definition');
$container = $this->getMock('Symfony\Component\DependencyInjection\ContainerBuilder');
$parameterBag = $this->getMock('Symfony\Component\DependencyInjection\ParameterBag\ParameterBagInterface');

$container->expects($this->once())
->method('hasAlias')
->will($this->returnValue(true));

$container->expects($this->once())
->method('getParameter')
->will($this->returnValue(true));

$container->expects($this->once())
->method('getAlias')
->will($this->returnValue('translation.default'));

$container->expects($this->exactly(2))
->method('getDefinition')
->will($this->returnValue($definition));

$definition->expects($this->once())
->method('getClass')
->will($this->returnValue("%translator.class%"));

$parameterBag->expects($this->once())
->method('resolveValue')
->will($this->returnValue("Symfony\Bundle\FrameworkBundle\Translation\Translator"));

$container->expects($this->once())
->method('getParameterBag')
->will($this->returnValue($parameterBag));

$pass = new LoggingTranslatorPass();
$pass->process($container);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}

public function testThatCompilerPassIsIgnoredIfThereIsNotLoggerDefinition()
{
$container = $this->getMock('Symfony\Component\DependencyInjection\ContainerBuilder');
$container->expects($this->once())
->method('hasAlias')
->will($this->returnValue(false));

$pass = new LoggingTranslatorPass();
$pass->process($container);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ class ConfigurationTest extends \PHPUnit_Framework_TestCase
public function testDefaultConfig()
{
$processor = new Processor();
$config = $processor->processConfiguration(new Configuration(), array(array('secret' => 's3cr3t')));
$config = $processor->processConfiguration(new Configuration(true), array(array('secret' => 's3cr3t')));

$this->assertEquals(
array_merge(array('secret' => 's3cr3t', 'trusted_hosts' => array()), self::getBundleDefaultConfig()),
Expand All @@ -33,7 +33,7 @@ public function testDefaultConfig()
public function testValidTrustedProxies($trustedProxies, $processedProxies)
{
$processor = new Processor();
$configuration = new Configuration();
$configuration = new Configuration(true);
$config = $processor->processConfiguration($configuration, array(array(
'secret' => 's3cr3t',
'trusted_proxies' => $trustedProxies,
Expand Down Expand Up @@ -62,7 +62,7 @@ public function getTestValidTrustedProxiesData()
public function testInvalidTypeTrustedProxies()
{
$processor = new Processor();
$configuration = new Configuration();
$configuration = new Configuration(true);
$processor->processConfiguration($configuration, array(
array(
'secret' => 's3cr3t',
Expand All @@ -77,7 +77,7 @@ public function testInvalidTypeTrustedProxies()
public function testInvalidValueTrustedProxies()
{
$processor = new Processor();
$configuration = new Configuration();
$configuration = new Configuration(true);
$processor->processConfiguration($configuration, array(
array(
'secret' => 's3cr3t',
Expand Down Expand Up @@ -123,6 +123,7 @@ protected static function getBundleDefaultConfig()
'translator' => array(
'enabled' => false,
'fallback' => 'en',
'logging' => true,
),
'validation' => array(
'enabled' => false,
Expand Down
2 changes: 2 additions & 0 deletions src/Symfony/Component/Translation/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ CHANGELOG
-----

* added possibility to cache catalogues
* added TranslatorBagInterface
* added LoggingTranslator

2.5.0
-----
Expand Down
124 changes: 124 additions & 0 deletions src/Symfony/Component/Translation/LoggingTranslator.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\Translation;

use Psr\Log\LoggerInterface;

/**
* @author Abdellatif Ait boudad <a.aitboudad@gmail.com>
*/
class LoggingTranslator implements TranslatorInterface
{
/**
* @var TranslatorInterface
*/
private $translator;

/**
* @var LoggerInterface
*/
private $logger;

/**
* @param Translator $translator
* @param LoggerInterface $logger
*/
public function __construct($translator, LoggerInterface $logger)
{
if (!($translator instanceof TranslatorInterface && $translator instanceof TranslatorBagInterface)) {
throw new \InvalidArgumentException(sprintf('The Translator "%s" must implements TranslatorInterface and TranslatorBagInterface.', get_class($translator)));
}

$this->translator = $translator;
$this->logger = $logger;
}

/**
* {@inheritdoc}
*/
public function trans($id, array $parameters = array(), $domain = null, $locale = null)
{
$trans = $this->translator->trans($id, $parameters , $domain , $locale);
$this->log($id, $domain, $locale);

return $trans;
}

/**
* {@inheritdoc}
*/
public function transChoice($id, $number, array $parameters = array(), $domain = null, $locale = null)
{
$trans = $this->translator->transChoice($id, $number, $parameters, $domain, $locale);
$this->log($id, $domain, $locale);

return $trans;
}

/**
* {@inheritdoc}
*
* @api
*/
public function setLocale($locale)
{
$this->translator->setLocale($locale);
}

/**
* {@inheritdoc}
*
* @api
*/
public function getLocale()
{
return $this->translator->getLocale();
}

/**
* Passes through all unknown calls onto the translator object.
*/
public function __call($method, $args)
{
return call_user_func_array(array($this->translator, $method), $args);
}

/**
* Logs for missing translations.
*
* @param string $id
* @param string|null $domain
* @param string|null $locale
*/
private function log($id, $domain, $locale)
{
if (null === $locale) {
$locale = $this->getLocale();
}

if (null === $domain) {
$domain = 'messages';
}

$id = (string) $id;
$catalogue = $this->translator->getCatalogue($locale);
if ($catalogue->defines($id, $domain)) {
return;
}

if ($catalogue->has($id, $domain)) {
$this->logger->debug('Translation use fallback catalogue.', array('id' => $id, 'domain' => $domain, 'locale' => $catalogue->getLocale()));
} else {
$this->logger->warning('Translation not found.', array('id' => $id, 'domain' => $domain, 'locale' => $catalogue->getLocale()));
}
}
}
Loading