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

[Bridge/Monolog] Enhance the Console Handler#21705

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

Merged
fabpot merged 1 commit intosymfony:masterfromlyrixx:log-command-refacto
Mar 6, 2017

Conversation

@lyrixx
Copy link
Member

@lyrixxlyrixx commentedFeb 21, 2017•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?yes
Tests pass?-
Fixed tickets-
LicenseMIT
Doc PR-

I extracted and enhanced the formatting logic from#21080.
Basically, The formatter now use the VarDumper & use more significant colors and has a more compact / readable format (IMHO).

I used the following code to generate before/after screenshots.

protectedfunctionexecute(InputInterface$input,OutputInterface$output)    {$logger =$this->getContainer()->get('logger');$filesystem =$this->getContainer()->get('filesystem');$anObject =new \stdClass;$anObject->firstPpt ='foo';$anObject->secondePpt ='bar';$logger->log('notice','Hello {who}', ['who' =>'Wold','an_object' =>$anObject,'file_system' =>$filesystem,        ]);$r =new \ReflectionClass(LogLevel::class);foreach ($r->getConstants()as$level) {$logger =$logger->withName($level);$logger->log($level,'log at level {level}', ['level' =>$level,            ]);        }    }

before:

screenshot12

After:

screenshot11

sstok, ahilles107, mathroc, linaori, hason, ogizanagi, lyrixx, apfelbox, yceruto, maidmaid, and 4 more reacted with heart emoji
}
if (isset($args[1])) {
$options['date_format'] =$args[1];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What about the two last arguments?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

They are not relevant anymore. So I don't need to "map" them.

greg0ire reacted with thumbs up emoji
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't$allowInlineLineBreaks map tomultiline ?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Indeed. I will fix it in the next pr. Thanks.

Copy link
Contributor

@mpdudempdudeSep 14, 2018•
edited
Loading

Choose a reason for hiding this comment

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

So you intentionally removed the option to suppress emtpy context and extra data?

Update: Just saw that this was mentioned in#21705 (comment) as well.

publicfunction__construct($options =array())
{
parent::__construct($format,$dateFormat,$allowInlineLineBreaks,$ignoreEmptyContextAndExtra);
// BC Layer
Copy link
Contributor

@greg0iregreg0ireFeb 21, 2017•
edited
Loading

Choose a reason for hiding this comment

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

You might want to document what the signature you're emulating here looks like

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Sure ;) And I have to add a trigger_error too. I just wait some feedback before.

greg0ire reacted with thumbs up emoji
'multiline' =>false,
),$options);

$this->cloner =newVarCloner();
Copy link
Contributor

@greg0iregreg0ireFeb 21, 2017•
edited
Loading

Choose a reason for hiding this comment

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

No DI here or line 83?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

No indeed. IMHO it's useless and there are other part of the framework where the dumper / cloner is hard-coded like that.

About the line 83, I cannot anyway as I do need a CLIDumper and well configured.

greg0ire reacted with thumbs up emoji
@lyrixx
Copy link
MemberAuthor

I made other attempts to find "the best" formatting. What do you prefer?

screenshot8

A == 👍
B == 😆
C == 🎉
D == ❤️

diskojulien, matks, Taluu, mdeltour, odolbeau, theofidry, apfelbox, ro0NL, and athlonUA reacted with thumbs up emojiDevelle and ro0NL reacted with laugh emojinicolas-grekas, lyrixx, ogizanagi, fabpot, mathroc, sstok, oscherler, T1l3, jeremyb, obense, and 25 more reacted with hooray emoji

*/
publicfunction__construct($options =array())
{
// BC Layer, old constructor:
Copy link
Contributor

Choose a reason for hiding this comment

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

What I meant by "documenting what it looks like" was more sth like :

// BC layer for this signature : __construct($format = null, $dateFormat = null)

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Actually, it's "documented" just bellow in the trigger error message.

For the record, I added what you asked for, then I added the deprecation, then I removed because it was a kind of duplicate. and now I notice I forgot to remove, old constructor:

greg0ire reacted with thumbs up emoji
Copy link
Contributor

Choose a reason for hiding this comment

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

Alright!

{
// BC Layer, old constructor:
if (!is_array($options)) {
@trigger_error(sprintf('The constructor arguments $format, $dateFormat, $allowInlineLineBreaks, $ignoreEmptyContextAndExtra of %s are deprecated and will be removed in 4.0. Use $options insteand.',self::class),E_USER_DEPRECATED);
Copy link
Contributor

Choose a reason for hiding this comment

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

insteand => instead

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Thanks. Fixed.

@greg0ire
Copy link
Contributor

In your commit message :

- Basically, The formatter now use the VarDumper & use more significant…+ Basically, the formatter now uses the VarDumper & uses more significant…
lyrixx reacted with thumbs up emoji

@lyrixxlyrixxforce-pushed thelog-command-refacto branch 2 times, most recently fromb516ef7 to1ba3fcdCompareFebruary 22, 2017 10:37
@lyrixx
Copy link
MemberAuthor

Looks like theC is the favorite one ! ❤️ it's mine too.

This PR is now ready for the review ;)

Copy link
Contributor

@romainneutronromainneutron left a comment

Choose a reason for hiding this comment

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

👍

{
// BC Layer
if (!is_array($options)) {
@trigger_error(sprintf('The constructor arguments $format, $dateFormat, $allowInlineLineBreaks, $ignoreEmptyContextAndExtra of %s are deprecated and will be removed in 4.0. Use $options instead.',self::class),E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

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

Missing quotes around%s (should be"%s")

Copy link
Member

Choose a reason for hiding this comment

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

are deprecated since 3.3

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Thanks; fixed.

),$options);

if (!class_exists(VarCloner::class)) {
thrownew \RuntimeException('To use the ConsoleFormatter you must install the symfony/var-dumper component.');
Copy link
Member

Choose a reason for hiding this comment

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

This could be moved to the top, right?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Thanks. Fixed.

@fabpot
Copy link
Member

Quick question: would it be possible to fallback to the old way when vardumper is not installed?

@lyrixx
Copy link
MemberAuthor

Quick question: would it be possible to fallback to the old way when vardumper is not installed?

It's not really easy (but not impossible) since theConsoleFormatter does not extend theLineFormatter anymore. It just implements the interface.

We can create twoConsoleFormatter implementation (the new and the old one). Then theConsoleHandler can choose and the "best" one. (What about the naming ?)

Another solution is to extends again the LineFormatter.

What is the best option to you?

constSIMPLE_DATE ='H:i:s';

privatestatic$levelColorMap =array(
100 =>'fg=white',
Copy link
Member

Choose a reason for hiding this comment

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

what's about using monolog's (or PSR) constants?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

  1. because the map will not be aligned ;) just kidding !
  2. PSR contant with int level does not exist

I updated the PR to use Monolog\Logger::CONSTANT

@nicolas-grekasnicolas-grekas added this to the3.3 milestoneFeb 25, 2017
@fabpot
Copy link
Member

@lyrixx, I think we still need to support the case wherevar-dumper is not installed as we are trying to limit the required dependencies.

@lyrixx
Copy link
MemberAuthor

@lyrixx, I think we still need to support the case where var-dumper is not installed as we are trying to limit the required dependencies.

I pushed a new version: Now if the var-dumper component is not installed, the context and the extra are not dumped at all.

I chose to do that in order tokeep the code as simple as possible.

I could also extends the NormalizeFormatter, but there is lot of code in the LineFormatter. So I don't think it worth it.

private$dumper;

/**
* {@inheritdoc}
Copy link
Member

Choose a reason for hiding this comment

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

This should be removed as you don't extend anything anymore. Also, the phpdoc should document the available option keys.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Thanks. I updated the PHP Doc.

@lyrixxlyrixxforce-pushed thelog-command-refacto branch 2 times, most recently froma11d586 to69ec738CompareMarch 6, 2017 17:01
@fabpot
Copy link
Member

Last thing, can you add a note in the CHANGELOG?

Basically, the formatter now uses the VarDumper & uses more significantcolors and has a more compact / readable format (IMHO).
@lyrixx
Copy link
MemberAuthor

Last thing, can you add a note in the CHANGELOG?

Done ;)

@fabpot
Copy link
Member

Thank you@lyrixx.

@fabpotfabpot merged commitb663ab5 intosymfony:masterMar 6, 2017
fabpot added a commit that referenced this pull requestMar 6, 2017
This PR was merged into the 3.3-dev branch.Discussion----------[Bridge/Monolog] Enhance the Console Handler| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | yes| Tests pass?   | -| Fixed tickets | -| License       | MIT| Doc PR        | ----I extracted and enhanced the formatting logic from#21080.Basically, The formatter now use the VarDumper & use more significant colors and has a more compact / readable format (IMHO).---I used the following code to generate before/after screenshots.```php    protected function execute(InputInterface $input, OutputInterface $output)    {        $logger = $this->getContainer()->get('logger');        $filesystem = $this->getContainer()->get('filesystem');        $anObject = new \stdClass;        $anObject->firstPpt = 'foo';        $anObject->secondePpt = 'bar';        $logger->log('notice', 'Hello {who}', [            'who' => 'Wold',            'an_object' => $anObject,            'file_system' => $filesystem,        ]);        $r = new \ReflectionClass(LogLevel::class);        foreach ($r->getConstants() as $level) {            $logger = $logger->withName($level);            $logger->log($level, 'log at level {level}', [                'level' => $level,            ]);        }    }```before:![screenshot12](https://cloud.githubusercontent.com/assets/408368/23183075/0bb21f80-f87b-11e6-8123-f6da70f2493b.png)After:![screenshot11](https://cloud.githubusercontent.com/assets/408368/23182971/b4022ed8-f87a-11e6-9d3b-de1a9d4ce9aa.png)Commits-------b663ab5 [Bridge/Monolog] Enhanced the Console Handler
@lyrixxlyrixx deleted the log-command-refacto branchMarch 6, 2017 17:12
lyrixx added a commit to lyrixx/symfony-standard that referenced this pull requestMar 7, 2017
Related tosymfony/symfony#21705Note: It's not an issue to let monolog doing it, but if monolog do itsymfony can not do it. The best experience is when symfony do it.So let's disable it.
fabpot added a commit to symfony/symfony-standard that referenced this pull requestMar 9, 2017
…symfony. (lyrixx)This PR was merged into the 3.3-dev branch.Discussion----------Do not process psr_3 log messages, as it's now done by symfony.Related tosymfony/symfony#21705Note: It's not an issue to let monolog doing it, but if monolog do itsymfony can not do it. The best experience is when symfony do it.So let's disable it.Commits-------ebe2bad Do not process psr_3 log messages, as it's now done by symfony.
@mvrhov
Copy link

mvrhov commentedMar 9, 2017•
edited
Loading

This seems to be a memory hog. At least I was greeted with the php terminating script due to memory exhaustion (128M) instead of getting the stack dump. I have run the command with time to see the top memory consumption and it was around 150M-160M. Running same command with 3.2, the memory used is around 50M

@lyrixx
Copy link
MemberAuthor

@mvrhov Thanks for reporting this, but could you open a new issue and ping me to keep a trace of this issue?

@Tobion
Copy link
Contributor

Does this PR basically revert#11496? I don't see why.

nicolas-grekas added a commit that referenced this pull requestSep 20, 2018
…xt and extra data (mpdude)This PR was squashed before being merged into the 3.4 branch (closes#28471).Discussion----------[MonologBridge] Re-add option option to ignore empty context and extra data| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets || License       | MIT| Doc PR        |In#11496, an option was added to `ConsoleFormatter` to ignore empty context and extra data. This setting was even turned on by default.The `ConsoleHandler` was then overhauled in#21705. During this change, the option got lost.Commits-------d1e7438 [MonologBridge] Re-add option option to ignore empty context and extra data
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@stofstofstof left review comments

@jderussejderussejderusse left review comments

@fabpotfabpotfabpot approved these changes

+3 more reviewers

@mpdudempdudempdude left review comments

@romainneutronromainneutronromainneutron approved these changes

@greg0iregreg0iregreg0ire approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

3.3

Development

Successfully merging this pull request may close these issues.

11 participants

@lyrixx@greg0ire@fabpot@mvrhov@Tobion@romainneutron@stof@jderusse@mpdude@nicolas-grekas@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp