[Bridge/Monolog] Enhance the Console Handler #21705

lyrixx · 2017-02-21T16:03:19Z

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.

    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:

After:

greg0ire · 2017-02-21T16:22:58Z

src/Symfony/Bridge/Monolog/Formatter/ConsoleFormatter.php

+            }
+            if (isset($args[1])) {
+                $options['date_format'] = $args[1];
+            }


What about the two last arguments?

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

shouldn't $allowInlineLineBreaks map to multiline ?

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

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.

greg0ire · 2017-02-21T16:23:15Z

src/Symfony/Bridge/Monolog/Formatter/ConsoleFormatter.php

    {
-        parent::__construct($format, $dateFormat, $allowInlineLineBreaks, $ignoreEmptyContextAndExtra);
+        // BC Layer


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

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

greg0ire · 2017-02-21T16:24:05Z

src/Symfony/Bridge/Monolog/Formatter/ConsoleFormatter.php

+            'multiline' => false,
+        ), $options);
+
+        $this->cloner = new VarCloner();


No DI here or line 83?

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.

lyrixx · 2017-02-21T20:04:24Z

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

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

greg0ire · 2017-02-22T10:17:59Z

src/Symfony/Bridge/Monolog/Formatter/ConsoleFormatter.php

+     */
+    public function __construct($options = array())
+    {
+        // BC Layer, old constructor:


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

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

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 · 2017-02-22T10:23:58Z

src/Symfony/Bridge/Monolog/Formatter/ConsoleFormatter.php

+    {
+        // 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);


insteand => instead

Thanks. Fixed.

greg0ire · 2017-02-22T10:28:51Z

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 · 2017-02-22T13:16:34Z

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

This PR is now ready for the review ;)

romainneutron

👍

fabpot · 2017-02-22T16:10:02Z

src/Symfony/Bridge/Monolog/Formatter/ConsoleFormatter.php

+    {
+        // 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);


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

are deprecated since 3.3

Thanks; fixed.

fabpot · 2017-02-22T16:11:06Z

src/Symfony/Bridge/Monolog/Formatter/ConsoleFormatter.php

+        ), $options);
+
+        if (!class_exists(VarCloner::class)) {
+            throw new \RuntimeException('To use the ConsoleFormatter you must install the symfony/var-dumper component.');


This could be moved to the top, right?

Thanks. Fixed.

fabpot · 2017-02-22T16:35:22Z

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

lyrixx · 2017-02-22T16:52:33Z

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 the ConsoleFormatter does not extend the LineFormatter anymore. It just implements the interface.

We can create two ConsoleFormatter implementation (the new and the old one). Then the ConsoleHandler 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?

jderusse · 2017-02-22T17:56:07Z

src/Symfony/Bridge/Monolog/Formatter/ConsoleFormatter.php

+    const SIMPLE_DATE = 'H:i:s';
+
+    private static $levelColorMap = array(
+        100 => 'fg=white',


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

because the map will not be aligned ;) just kidding !

PSR contant with int level does not exist

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

fabpot · 2017-03-01T14:36:23Z

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

lyrixx · 2017-03-06T16:23:54Z

@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 to keep 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.

fabpot · 2017-03-06T16:37:38Z

src/Symfony/Bridge/Monolog/Formatter/ConsoleFormatter.php

+    private $dumper;
+
+    /**
+     * {@inheritdoc}


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

Thanks. I updated the PHP Doc.

fabpot · 2017-03-06T17:07:47Z

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

Basically, the formatter now uses the VarDumper & uses more significant colors and has a more compact / readable format (IMHO).

lyrixx · 2017-03-06T17:10:48Z

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

Done ;)

fabpot · 2017-03-06T17:12:20Z

Thank you @lyrixx.

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

Related to symfony/symfony#21705 Note: It's not an issue to let monolog doing it, but if monolog do it symfony can not do it. The best experience is when symfony do it. So let's disable it.

…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 to symfony/symfony#21705 Note: It's not an issue to let monolog doing it, but if monolog do it symfony 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 · 2017-03-09T10:05:48Z

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 · 2017-03-09T10:28:04Z

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

Tobion · 2017-05-20T10:19:52Z

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

…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

carsonbot added Status: Needs Review Feature Deprecation labels Feb 21, 2017

lyrixx added the MonologBridge label Feb 21, 2017

lyrixx force-pushed the log-command-refacto branch 2 times, most recently from 3ae3f06 to 4103b61 Compare February 21, 2017 16:22

greg0ire reviewed Feb 21, 2017

View reviewed changes

lyrixx force-pushed the log-command-refacto branch from 4103b61 to 5fe4af5 Compare February 21, 2017 17:25

lyrixx force-pushed the log-command-refacto branch from 5fe4af5 to 6823479 Compare February 21, 2017 20:23

greg0ire reviewed Feb 22, 2017

View reviewed changes

lyrixx force-pushed the log-command-refacto branch from 6823479 to 2f01f9f Compare February 22, 2017 10:25

lyrixx force-pushed the log-command-refacto branch 2 times, most recently from b516ef7 to 1ba3fcd Compare February 22, 2017 10:37

greg0ire approved these changes Feb 22, 2017

View reviewed changes

lyrixx force-pushed the log-command-refacto branch from 1ba3fcd to 7a2de64 Compare February 22, 2017 13:14

romainneutron approved these changes Feb 22, 2017

View reviewed changes

fabpot reviewed Feb 22, 2017

View reviewed changes

lyrixx force-pushed the log-command-refacto branch from 7a2de64 to df24afc Compare February 22, 2017 16:29

jderusse reviewed Feb 22, 2017

View reviewed changes

lyrixx force-pushed the log-command-refacto branch from df24afc to 5fa5041 Compare February 22, 2017 18:17

nicolas-grekas added this to the 3.3 milestone Feb 25, 2017

lyrixx force-pushed the log-command-refacto branch from 5fa5041 to 4487f99 Compare March 6, 2017 16:19

lyrixx mentioned this pull request Mar 6, 2017

[FrameworkBundle][Monolog] Added a new way to follow logs #21080

Merged

fabpot approved these changes Mar 6, 2017

View reviewed changes

lyrixx force-pushed the log-command-refacto branch 2 times, most recently from a11d586 to 69ec738 Compare March 6, 2017 17:01

[Bridge/Monolog] Enhanced the Console Handler

b663ab5

Basically, the formatter now uses the VarDumper & uses more significant colors and has a more compact / readable format (IMHO).

lyrixx force-pushed the log-command-refacto branch from 69ec738 to b663ab5 Compare March 6, 2017 17:10

fabpot merged commit b663ab5 into symfony:master Mar 6, 2017

lyrixx deleted the log-command-refacto branch March 6, 2017 17:12

lyrixx mentioned this pull request Mar 7, 2017

Do not process psr_3 log messages, as it's now done by symfony. symfony/symfony-standard#1054

Merged

mvrhov mentioned this pull request Mar 9, 2017

[Console] Enhanced stack trace memory problem #21943

Closed

javiereguiluz mentioned this pull request Mar 26, 2017

[Bridge/Monolog] Enhance the Console Handler symfony/symfony-docs#7689

Closed

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

lyrixx mentioned this pull request Oct 19, 2017

[VarDumper] Code excerpts and trailing commas #24581

Closed

This was referenced Sep 14, 2018

[MonologBridge] Ignore empty context/extra by default in the ConsoleFormatter #11496

Merged

[MonologBridge] Re-add option option to ignore empty context and extra data #28471

Merged

[Bridge/Monolog] Enhance the Console Handler #21705

[Bridge/Monolog] Enhance the Console Handler #21705

Conversation

lyrixx commented Feb 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpdude Sep 14, 2018 • edited Loading

Choose a reason for hiding this comment

greg0ire Feb 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire Feb 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyrixx commented Feb 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Feb 22, 2017

lyrixx commented Feb 22, 2017

romainneutron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Feb 22, 2017

lyrixx commented Feb 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Mar 1, 2017

lyrixx commented Mar 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Mar 6, 2017

lyrixx commented Mar 6, 2017

fabpot commented Mar 6, 2017

mvrhov commented Mar 9, 2017 • edited Loading

lyrixx commented Mar 9, 2017

Tobion commented May 20, 2017

lyrixx commented Feb 21, 2017 •

edited

Loading

mpdude Sep 14, 2018 •

edited

Loading

greg0ire Feb 21, 2017 •

edited

Loading

greg0ire Feb 21, 2017 •

edited

Loading

mvrhov commented Mar 9, 2017 •

edited

Loading