Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Console] Display errors in quiet mode#18781
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
Uh oh!
There was an error while loading.Please reload this page.
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
| // (integration test for the issue #18767 fix) | ||
| $levelName = Logger::getLevelName($level); | ||
| $realOutput =$this->getMock('Symfony\Component\Console\Output\Output', ['doWrite']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
array('doWrite')
nicolas-grekas commentedMay 19, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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 |
multi-io commentedMay 21, 2016
@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 thinginternally. 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. :) Itmay 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 itsbacking output at the default verbosity (VERBOSITY_NORMAL), whichmeant that if the output had its verbosity setting set toVERBOSITY_QUIET, nothing would ever be output to it, even if theConsoleLogger's verbosityMap specified that some logging levels shouldbe output even to a VERBOSITY_QUIET output.
nicolas-grekas commentedJun 8, 2016
👍, as a feature to me |
| '->isHandling returns correct value depending on console verbosity and log level' | ||
| ); | ||
| //check that the handler actually outputs the record iff it handles it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
if instead of iff
fabpot commentedJun 8, 2016
👍 for master |
multi-io commentedJun 9, 2016
Thanks for the review. If you don't mind, I'll rewrite the branch history to avoid multiple tiny style fix commits. |
fabpot commentedJun 9, 2016
@multi-io You can squash the commits if you like but just to let you know, we can do it automatically when merging. |
multi-io commentedJun 9, 2016
@fabpot ah, ok. I'll do style commits then. |
multi-io commentedJun 9, 2016
Fixes pushed. |
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
A carriage return must be added after }
multi-io commentedJun 9, 2016
Sorry, I guess I misunderstood your earlier description. This should be the right fix now. |
fabpot commentedJun 10, 2016
Thank you@multi-io. |
Uh oh!
There was an error while loading.Please reload this page.
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.