Skip to content

[Console] Added standalone PSR-3 compliant logger#10194

Merged
fabpot merged 1 commit intosymfony:masterfrom
dunglas:console_logger
Mar 19, 2014
Merged

[Console] Added standalone PSR-3 compliant logger#10194
fabpot merged 1 commit intosymfony:masterfrom
dunglas:console_logger

Conversation

@dunglas
Copy link
Copy Markdown
Member

@dunglas dunglas commented Feb 3, 2014

This PR adds a standalone, PSR-3 compliant, logger to the Console component. It logs all messages on the console output. Messages of DEBUG, INFO and NOTICE levels are displayed using the info format (default to green). Higher levels are displayed using the error formatter (default to red).

This logger is similar to the Monolog's Console Handler but does not have any external dependency (except php-fig/log). This is useful for console applications and commands needing a lightweight PSR-3 compliant logger (e.g. required by a dependency or to display basic informations to the user).

An usage example is available here: https://github.com/dunglas/php-schema.org-model/blob/master/src/SchemaOrgModel/Command/GenerateEntitiesCommand.php#L71

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR symfony/symfony-docs#3696

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it should be an optional dependency

@stof
Copy link
Copy Markdown
Member

stof commented Feb 3, 2014

the message should not always been written IMO. A message at the debug level should not be written in normal verbosity (see how the Monolog ConsoleHandler handles it)

@dunglas
Copy link
Copy Markdown
Member Author

dunglas commented Feb 3, 2014

@stof Thanks for your review. I agree with the verbosity handling.

@dunglas
Copy link
Copy Markdown
Member Author

dunglas commented Feb 3, 2014

All reports of @stof closed and verbosity system added.

I'm not sure about this map: https://github.com/symfony/symfony/pull/10194/files#diff-84e574ab0bb629e124c5752bb81259ccR37
Hints are welcome.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

to be consistent with the Monolog ConsoleHandler in the bridge, this should be at the normal verbosity

@dunglas
Copy link
Copy Markdown
Member Author

dunglas commented Feb 4, 2014

Why array_replace is better @GromNaN? According to http://stackoverflow.com/questions/8537384/array-replace-vs-union-operator-in-php and http://www.nyamsprod.com/blog/2013/array_merge-vs-array_replace-vs-union/ it is similar with array_merge but the + (union) operator is better for performance (I'll use this one).

@dunglas
Copy link
Copy Markdown
Member Author

dunglas commented Feb 4, 2014

Changes done.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Using !isset($this->verbosityLevelMap[$level]) would require less computation.

@dunglas
Copy link
Copy Markdown
Member Author

dunglas commented Feb 4, 2014

Added a way to configure the format (similar to how configuring the verbosity works).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

shouldn't this be an OutOfBoundsException ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The InvalidArgumentException in defined in PSR-3

@stof
Copy link
Copy Markdown
Member

stof commented Feb 4, 2014

@dunglas To be exact, array_merge and array_replace are equivalent only when you have string keys in your arrays. When you have numeric keys, the output will not be the same anymore

@dunglas
Copy link
Copy Markdown
Member Author

dunglas commented Feb 4, 2014

@stof yes I would "say equivalent" here because keys are strings.

@dunglas
Copy link
Copy Markdown
Member Author

dunglas commented Feb 11, 2014

Is this PR ready to be merged? Can I start working on the PR in the doc?

@jakzal jakzal added the Console label Feb 11, 2014
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This class should be moved on the Fixtures namespace?

@dunglas
Copy link
Copy Markdown
Member Author

dunglas commented Feb 15, 2014

DummyOutput class put in the Fixtures namespace and PHPDoc fixed.

@fabpot
Copy link
Copy Markdown
Member

fabpot commented Feb 24, 2014

@dunglas We need the PR for the documentation before merging the PR.

@dunglas
Copy link
Copy Markdown
Member Author

dunglas commented Feb 24, 2014

Fine. I'll do it ASAP.

@fabpot
Copy link
Copy Markdown
Member

fabpot commented Mar 19, 2014

@dunglas Any news on the docs? We need to finish this ASAP to be able to merge this PR before 2.5.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

better leave line breaks here?

@cordoval
Copy link
Copy Markdown
Contributor

nice contribution, this is exactly what i need, if you work on a documentation PR maybe it would be good to do a cookbook? 👶, what do you think about wrapping your ConsoleLogger into a helper, or it is not important?

@dunglas
Copy link
Copy Markdown
Member Author

dunglas commented Mar 19, 2014

@fabpot I start working on the doc PR.
@cordoval what kind of helper?

@fabpot
Copy link
Copy Markdown
Member

fabpot commented Mar 19, 2014

@dunglas As soon as you can create the PR on the docs, please reference it here and we will be good to merge.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants