Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Profiler][Translation] Logging false by default and desactivated when using the profiler#24785
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
7c8c7e3 toc6a5316Compare| return; | ||
| } | ||
| if ($container->has('profiler')) { | ||
| $container->hasParameter('translator.logging') &&$container->setParameter('translator.logging',false); |
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&& is like an "if" - that's uncommon syntax in SF code base, isn't it?
the "hasParameter" check should be moved in the "if" instead.
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.
it's not common in SF you are right.
nicolas-grekas commentedNov 1, 2017
new features are for 4.1 (master) only |
c6a5316 to1df6d5dCompare1df6d5d toffc6e9bCompareSimperfit commentedNov 1, 2017
@nicolas-grekas comments done and PR rebased |
0fe56df to1dd741eCompareogizanagi commentedNov 1, 2017
With these changes, it's not possible at all to set But IMHO, changing the default value to |
Simperfit commentedNov 1, 2017
I may have misunderstood what we wanted here, I understood that we wanted to enforce it at I don't think it would be considered as BC too. |
ogizanagi commentedNov 1, 2017
To clarify: from dev point of view, there is no apparent reason we want the logging + translator panel being enabled at the same time, as the info is redundant. The point is that in most case the info is just redundant and disabling this feature when the profiler is enabled (or at all) is a better default. |
1dd741e to7b803d2Compare…n using the profiler
7b803d2 to0252830CompareSimperfit commentedNov 5, 2017
@ogizanagi changes done. |
ogizanagi left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
As explained, it seems right to me to do this change, unless we want strict BC (then see#24785 (comment))
fabpot commentedDec 1, 2017
Thank you@Simperfit. |
…esactivated when using the profiler (Simperfit)This PR was merged into the 4.1-dev branch.Discussion----------[Profiler][Translation] Logging false by default and desactivated when using the profiler| Q | A| ------------- | ---| Branch? | 4.1| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | no| Fixed tickets |#23146| License | MIT| Doc PR | todo.Commits-------0252830 [Profiler][Translation] Logging false by default and desactivated when using the profiler
This is fixed in Symfony 4.x, seesymfony/symfony#24785
Uh oh!
There was an error while loading.Please reload this page.