[Console] Display errors in quiet mode #18781
Conversation
- map VERBOSITY_QUIET normally, rather than suppressing all output without override - ensure that we do write to the output if we've determined (via verbosityLevelMap) that we should
javiereguiluz
changed the title
| // (integration test for the issue #18767 fix) | ||
| $levelName = Logger::getLevelName($level); | ||
|
|
||
| $realOutput = $this->getMock('Symfony\Component\Console\Output\Output', ['doWrite']); |
|
@multi-io it would be cool if you could also look at fixing ConsoleLogger.php: let's not leave a bug that someone else will hit and spend time on, whereas it's cheap to fix now that we know about it :) thanks |
|
@nicolas-grekas ok I've applied the fix to ConsoleLogger too. I've added a ConsoleLogger-specific test that would reproduce the fixed bug. The "if" query condition in ConsoleLogger's output pipeline that compares input and output verbosity to determine whether the output should take place or not is actually redundant because the output will do the exact same thing internally. I guess it still makes sense to leave it in because it short-cuts and avoids formatting the output message in the case where we're not going to write it out anyway. But I've added a comment that at least documents this. We might want to remove the if query instead if we regard the small win in code clarity higher than the performance gain. :) It may be advisable to change the default $verbosityLevelMap of ConsoleLogger so that errors and emergencies are output even to a VERBOSITY_QUIET output (which is used e.g. when running a Symfony command with -q -- quite a common thing, and you may not want to suppress all output there). But I'm not sure about Symfony's actual policy regarding this (if any), and it would technically be a breaking change, so I haven't done it. |
Previously, ConsoleLogger would always output everything to its backing output at the default verbosity (VERBOSITY_NORMAL), which meant that if the output had its verbosity setting set to VERBOSITY_QUIET, nothing would ever be output to it, even if the ConsoleLogger's verbosityMap specified that some logging levels should be output even to a VERBOSITY_QUIET output.
nicolas-grekas
changed the title
|
👍, as a feature to me |
| @@ -55,12 +55,36 @@ public function testVerbosityMapping($verbosity, $level, $isHandling, array $map | |||
| $this->assertSame($isHandling, $handler->isHandling(array('level' => $level)), | |||
| '->isHandling returns correct value depending on console verbosity and log level' | |||
| ); | |||
|
|
|||
| //check that the handler actually outputs the record iff it handles it | |||
|
👍 for master |
|
Thanks for the review. If you don't mind, I'll rewrite the branch history to avoid multiple tiny style fix commits. |
|
@multi-io You can squash the commits if you like but just to let you know, we can do it automatically when merging. |
|
@fabpot ah, ok. I'll do style commits then. |
|
Fixes pushed. |
| ); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
A carriage return must be added after }
|
Sorry, I guess I misunderstood your earlier description. This should be the right fix now. |
|
Thank you @multi-io. |
without override
verbosityLevelMap) that we should
About the second point, it seems ConsoleLogger has the same issue (https://github.com/symfony/symfony/blob/2.8/src/Symfony/Component/Console/Logger/ConsoleLogger.php#L92), but since we're not using that, I haven't checked it.