-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
Use stderr by default when a specific output is not injected #15728
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This change broke some monolog bridge tests. |
Sorry, fixed the tests. |
Related #13730 |
@Tobion in retrospect, I think that other one you linked was misguided, it should be given the right output by the user instead of doing automagic. But in this case there is no easy way for the user to do it right, so I think defaulting to the right behavior when setup with the listener is good, and if people want to do it wrong they can always reconfigure the hard way. |
@@ -120,7 +120,11 @@ public function close() | |||
*/ | |||
public function onCommand(ConsoleCommandEvent $event) | |||
{ | |||
$this->setOutput($event->getOutput()); |
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.
For convenience, you could use a temporary variable:
$output = $event->getOutput();
if ($output instanceof ConsoleOutputInterface) {
$output = $output->getErrorOutput();
}
$this->setOutput($output);
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.
I guess that's a bit nicer yes, fixed.
👍 |
$output | ||
->expects($this->any()) | ||
->method('getVerbosity') | ||
->will($this->returnValue(OutputInterface::VERBOSITY_DEBUG)) | ||
; | ||
$output | ||
->expects($this->once()) | ||
->expects($this->any()) |
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.
Why did you change that? It should only be callec once.
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.
Ah that was trying to achieve something else and then I gave up and forgot to change it back. Let me change it back quickly.
👍 |
Thank you @Seldaek. |
…ed (Seldaek) This PR was merged into the 2.7 branch. Discussion ---------- Use stderr by default when a specific output is not injected | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | stderr is really the best place to put logs by default, so unless someone explicitly passes something else we should use it, and especially not depend on the log level to decide where to output. Commits ------- c28796e Use stderr by default when a specific output is not injected
stderr is really the best place to put logs by default, so unless someone explicitly passes something else we should use it, and especially not depend on the log level to decide where to output.