Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

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

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
aitboudad wants to merge10 commits intosymfony:masterfromaitboudad:ticket_3015

Conversation

aitboudad
Copy link
Contributor

QA
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#3015,#2435
LicenseMIT
Doc PRsymfony/symfony-docs/pull/4050

}

return $this->catalogues[$locale];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

done

@jakzal
Copy link
Contributor

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

use Psr\Log\LoggerInterface;

/**
* Translator.
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove please.

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

Choose a reason for hiding this comment

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

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

@aitboudad
Copy link
ContributorAuthor

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@fabpot
Copy link
Member

👍

@@ -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']));
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Member

👍

@fabpot
Copy link
Member

Thank you@aitboudad.

@fabpotfabpot closed thisSep 24, 2014
fabpot added a commit that referenced this pull requestSep 24, 2014
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/4050Commits-------b7770bc [Translation] added LoggingTranslator.
@aitboudadaitboudad deleted the ticket_3015 branchSeptember 24, 2014 08:37
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
ContributorAuthor

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

@tvogt
Copy link

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
Copy link
Contributor

Koc commentedNov 10, 2014

@tvogt Have you tryed typehint usingTranslatorInterface instead of concrete implementation?

@rvanlaak
Copy link
Contributor

Using theTranslatorInterface as@Koc mentioned also solved a brokenFormType 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 frameworkTranslator has changed, so maybe mention something about the changes of the Translator component inUPGRADE-2.6.md?

@stof
Copy link
Member

@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
Copy link
ContributorAuthor

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

@rvanlaak
Copy link
Contributor

@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!

->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.

@alcalyn
Copy link
Contributor

I just update to 2.6@dev.

Note that the translator (servicetranslator) 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 setTranslator as type hint, juste change toTranslatorInterface ;)

@linaori
Copy link
Contributor

In both debug and non-debug?

@alcalyn
Copy link
Contributor

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

@tvogt
Copy link

@Koc yes, of course.

fabpot added a commit that referenced this pull requestNov 28, 2014
…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        | noneWhen 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.
weaverryan added a commit to symfony/symfony-docs that referenced this pull requestDec 7, 2014
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.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

15 participants
@aitboudad@jakzal@fabpot@romainneutron@Tobion@tvogt@Koc@rvanlaak@stof@alcalyn@linaori@lsmith77@cordoval@lyrixx@wouterj

[8]ページ先頭

©2009-2025 Movatter.jp