Skip to content

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

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 1, 2017
Merged

[Profiler][Translation] Logging false by default and desactivated when using the profiler #24785

merged 1 commit into from
Dec 1, 2017

Conversation

Simperfit
Copy link
Contributor

@Simperfit Simperfit commented Nov 1, 2017 •
edited
Loading

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.

@Simperfit Simperfit force-pushed the feature/made-the-translator-logging-to-false-by-default-and-when-enabling-the-profiler branch 3 times, most recently from 7c8c7e3 to c6a5316 Compare November 1, 2017 14:38
nicolas-grekas
nicolas-grekas reviewed Nov 1, 2017
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/LoggingTranslatorPass.php Outdated
@@ -28,6 +28,9 @@ public function process(ContainerBuilder $container)
if (!$container->hasAlias('logger') || !$container->hasAlias('translator')) {
return;
}
if ($container->has('profiler')) {
$container->hasParameter('translator.logging') && $container->setParameter('translator.logging', false);
Copy link
Member

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.

Copy link
Contributor Author

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

new features are for 4.1 (master) only

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Nov 1, 2017
@Simperfit Simperfit force-pushed the feature/made-the-translator-logging-to-false-by-default-and-when-enabling-the-profiler branch from c6a5316 to 1df6d5d Compare November 1, 2017 18:20
@Simperfit Simperfit changed the base branch from 3.3 to master November 1, 2017 18:23
@Simperfit Simperfit force-pushed the feature/made-the-translator-logging-to-false-by-default-and-when-enabling-the-profiler branch from 1df6d5d to ffc6e9b Compare November 1, 2017 18:23
@Simperfit
Copy link
Contributor Author

@nicolas-grekas comments done and PR rebased

@Simperfit Simperfit force-pushed the feature/made-the-translator-logging-to-false-by-default-and-when-enabling-the-profiler branch 2 times, most recently from 0fe56df to 1dd741e Compare November 1, 2017 18:40
@ogizanagi
Copy link
Contributor

With these changes, it's not possible at all to set framework.translator.logging to true with profiler enabled, as it'll behave like if you set false, right?
Instead, what about updating the Fwb Configuration class only to normalize the value to false if no value was set explicitly and the profiler is enabled, $this->debug if profiler is disabled?

But IMHO, changing the default value to false probably is enough. Anyone can opt-in if they need it for whatever reason. Not sure it would be considered a BC break.

@Simperfit
Copy link
Contributor Author

I may have misunderstood what we wanted here, I understood that we wanted to enforce it at false whenever the profiler is enabled. I guess we could just change the default value in here and in the recipe and it would be ok.

I don't think it would be considered as BC too.

@ogizanagi
Copy link
Contributor

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.
But that doesn't mean we must enforce the logging to be disabled when the profiler is enabled, as you might have use cases for using the LoggingTranslator despite this. For instance by using it to collect missing translations and get a report at the end of the day thanks to a dedicated monolog handler using the translation channel. Not quite common surely, but may exist.

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.

@Simperfit Simperfit force-pushed the feature/made-the-translator-logging-to-false-by-default-and-when-enabling-the-profiler branch from 1dd741e to 7b803d2 Compare November 1, 2017 19:08
@Simperfit Simperfit force-pushed the feature/made-the-translator-logging-to-false-by-default-and-when-enabling-the-profiler branch from 7b803d2 to 0252830 Compare November 1, 2017 19:09
@Simperfit
Copy link
Contributor Author

@ogizanagi changes done.

ogizanagi
ogizanagi approved these changes Nov 5, 2017
View reviewed changes
Copy link
Contributor

@ogizanagi ogizanagi left a comment •
edited
Loading

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

fabpot commented Dec 1, 2017

Thank you @Simperfit.

@fabpot fabpot merged commit 0252830 into symfony:master Dec 1, 2017
fabpot added a commit that referenced this pull request Dec 1, 2017
@fabpot
feature #24785 [Profiler][Translation] Logging false by default and d…
424cbcc 
…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
@fabpot fabpot mentioned this pull request May 7, 2018
Merged
cmfcmf added a commit to cmfcmf/AdventureLookup that referenced this pull request Jun 19, 2020
@cmfcmf
Don't log missing translations twice.
f6ef143 
This is fixed in Symfony 4.x, see
symfony/symfony#24785
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@dunglas dunglas dunglas approved these changes

@stof stof stof approved these changes

@ogizanagi ogizanagi ogizanagi approved these changes

@20uf 20uf 20uf approved these changes

Assignees
No one assigned
Labels
Feature Status: Reviewed Translation WebProfilerBundle
Projects
None yet
Milestone
4.1
Development

Successfully merging this pull request may close these issues.
8 participants
@Simperfit @nicolas-grekas @ogizanagi @fabpot @dunglas @stof @20uf @carsonbot