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

[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

Conversation

@Simperfit
Copy link
Contributor

@SimperfitSimperfit commentedNov 1, 2017
edited
Loading

QA
Branch?4.1
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?no
Fixed tickets#23146
LicenseMIT
Doc PRtodo.

@SimperfitSimperfitforce-pushed thefeature/made-the-translator-logging-to-false-by-default-and-when-enabling-the-profiler branch 3 times, most recently from7c8c7e3 toc6a5316CompareNovember 1, 2017 14:38
return;
}
if ($container->has('profiler')) {
$container->hasParameter('translator.logging') &&$container->setParameter('translator.logging',false);

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.

Simperfit reacted with thumbs up emoji
Copy link
ContributorAuthor

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-grekasnicolas-grekas added this to the4.1 milestoneNov 1, 2017
@SimperfitSimperfitforce-pushed thefeature/made-the-translator-logging-to-false-by-default-and-when-enabling-the-profiler branch fromc6a5316 to1df6d5dCompareNovember 1, 2017 18:20
@SimperfitSimperfit changed the base branch from3.3 tomasterNovember 1, 2017 18:23
@SimperfitSimperfitforce-pushed thefeature/made-the-translator-logging-to-false-by-default-and-when-enabling-the-profiler branch from1df6d5d toffc6e9bCompareNovember 1, 2017 18:23
@Simperfit
Copy link
ContributorAuthor

@nicolas-grekas comments done and PR rebased

@SimperfitSimperfitforce-pushed thefeature/made-the-translator-logging-to-false-by-default-and-when-enabling-the-profiler branch 2 times, most recently from0fe56df to1dd741eCompareNovember 1, 2017 18:40
@ogizanagi
Copy link
Contributor

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

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

Simperfit reacted with thumbs up emoji

@Simperfit
Copy link
ContributorAuthor

I may have misunderstood what we wanted here, I understood that we wanted to enforce it atfalse 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 theLoggingTranslator 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 thetranslation 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.

@SimperfitSimperfitforce-pushed thefeature/made-the-translator-logging-to-false-by-default-and-when-enabling-the-profiler branch from1dd741e to7b803d2CompareNovember 1, 2017 19:08
@SimperfitSimperfitforce-pushed thefeature/made-the-translator-logging-to-false-by-default-and-when-enabling-the-profiler branch from7b803d2 to0252830CompareNovember 1, 2017 19:09
@Simperfit
Copy link
ContributorAuthor

@ogizanagi changes done.

Copy link
Contributor

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

Thank you@Simperfit.

@fabpotfabpot merged commit0252830 intosymfony:masterDec 1, 2017
fabpot added a commit that referenced this pull requestDec 1, 2017
…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
@fabpotfabpot mentioned this pull requestMay 7, 2018
cmfcmf added a commit to cmfcmf/AdventureLookup that referenced this pull requestJun 19, 2020
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@dunglasdunglasdunglas approved these changes

@stofstofstof approved these changes

+2 more reviewers

@ogizanagiogizanagiogizanagi approved these changes

@20uf20uf20uf approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

[8]ページ先頭

©2009-2025 Movatter.jp