-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Console] Add silent verbosity suppressing all output, including errors #53632
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
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.
Makes sense to me. Should we document it as a BC break given the behavior change is not opt-in?
I'm not sure, I don't think we want to call this a BC break (or tag it as such) to give the wrong impression that command output is considered within the BC promise. Let's also friendly ping @Seldaek who implemented the existing logic of showing errors during quiet verbosity. Do you remember a use-case for this? (something with Composer maybe?) Is there something we should take in mind before merging this PR? |
IMO it is better to keep displaying it at least if you have a ConsoleOutputInterface where you can output on stderr. Any machine readable output would generally be expected on stdout. By default it will output to stderr so it won't disturb anything. So I personally would close this, because yes when running quiet in some script/CI and something major breaks and you do not see it at all, it sucks. But that's just my view of course. |
Note: I see the point about logging duplicating things, that's not great indeed. I am not sure how else this could be mitigated. I don't really see a way to safely do this, and IMO duplicating uncaught exception sure not great but still better as not seeing them at all. |
I get your point about stderr, however both stderr and stdout of processes are redirected to a container's output. So in hosting relying on containers, this is still an issue. There are utils/techniques you can apply to hide the output of a command, but still get errors (based on exit codes). E.g. in the past, I've used the little |
Ok, then I guess all I can ask is to maybe make this optional so that apps that are pure CLI apps like Composer and not 12-factor style symfony envs with full logger config could maybe retain the quiet-level output? |
Let's resume this PR? |
Yes please :) We need to make a decision (@symfony/mergers): A) Make B) Add a new verbosity (e.g. |
@wouterj using Thus, cases that want to process the command output programmatically are probably not using |
@wouter I looked in other projects and some commands use special options for this:
About your proposals, I like this one --> Add a new verbosity (e.g. SHELL_VERBOSITY=-2) that disables error output But, I wouldn't do this:
Having About possible naming of this new constant, AI suggested
|
This makes sense to me. +1 for |
Funnily enough, I encountered this issue yesterday. I was annoyed by the fact Having a new |
silent 🤫 is better wording 👌🏻 |
1c76cfb
to
5994816
Compare
5994816
to
5a16a22
Compare
Thanks for the feedback everyone! PR is now updated with a new |
76991c3
to
cd21a93
Compare
dbc370c
to
51e2856
Compare
Low deps failure is unrelated, flipped test failure will be fixed by #58320 if I understand correctly. |
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.
This is still considered a BC break?
@kbond adding a new global option is always a BC break, as it'll break commands that defined this option before (you'll get an exception when registering that command). |
51e2856
to
e7ebf8e
Compare
…troduced in Symfony 7.2 (wouterj) This PR was merged into the 6.4 branch. Discussion ---------- [Runtime] Adapt test to support the `--silent` option introduced in Symfony 7.2 | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | - | License | MIT Prepare tests for #53632 Commits ------- bc5ae53 [Runtime] Adapt test to support the --silent option introduced in Symfony 7.2
e7ebf8e
to
57fe0bd
Compare
Thank you @wouterj. |
Original PR description
Alternative to #53126
In Symfony 2.8, we decided to still show exceptions/errors when running a command with
--quiet
orSHELL_VERBOSITY=-1
(#15680).Since that time, we've introduced the ConsoleLogger and 12-factor app logic. In todays landscape, it's more common to run commands that only output computer-readable text (e.g. JSON), which is ingested and displayed by services like Datadog and Kibana.
Our decision from 2.8 breaks this, as the JSON is interrupted by human-readable exception output that users can't disable. At the same time, this information is duplicated as errors are always logged (and thus shown as JSON).
I think we should revert the 2.8 decision, rather than adding a new "extremely quiet" verbosity mode. As far as I'm aware, this is also more consistent with normal unix commands which also don't output anything when passing
--quiet
.I don't think this warrants as a BC break, as the errors are human readable and we don't promise BC on human readable console output (afaik, we don't promise BC on any console output, but I think we should be careful when changing e.g. the JSON logging).
This updated PR adds a new
--silent
verbosity mode that suppresses all output, including exceptions caught by the Application.I went back and forth between simply passing
NullOutput
to the command in silent mode or not ignoring everything written to silent mode inOutput
. In the end, I've decided for the last to avoid bug reports from people silently expecting aConsoleOutputInterface
in their commands (e.g. for sections) without properly guarding it.The new
isSilent()
methods can be used by commands to e.g. write important messages to the logger instead of command output when running in silent mode.