Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
aitboudad commentedMay 10, 2014
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
done
jakzal commentedMay 10, 2014
This is a much better approach than previous attempts to solve the problem. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Remove please.
There was a problem hiding this comment.
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 commentedJun 8, 2014
To enable logger in config (default value is %kernel.debug%): framework:#esi: ~translator:{ fallback: "%locale%", logging: true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This doesn't work. Take a look at the documentationhttp://symfony.com/doc/current/cookbook/configuration/using_parameters_in_dic.html
There was a problem hiding this comment.
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 commentedJun 16, 2014
👍 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 commentedJul 25, 2014
👍 |
…e LoggableTranslatorPass.
…terface and TranslatorBagInterface.
fabpot commentedSep 24, 2014
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/4050Commits-------b7770bc [Translation] added LoggingTranslator.
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There was a problem hiding this comment.
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 commentedNov 10, 2014
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 commentedNov 10, 2014
@tvogt Have you tryed typehint using |
rvanlaak commentedNov 14, 2014
Using the should be It seems like the default framework |
stof commentedNov 15, 2014
@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 commentedNov 15, 2014
rvanlaak commentedNov 15, 2014
@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! |
There was a problem hiding this comment.
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 commentedNov 20, 2014
I just update to 2.6@dev. Note that the translator (service I got it from php app/console container:debug : Edit: |
linaori commentedNov 20, 2014
In both debug and non-debug? |
alcalyn commentedNov 20, 2014
No, I got Symfony\Bundle\FrameworkBundle\Translation\Translator from |
tvogt commentedNov 21, 2014
@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 | 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.
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.