Skip to content

[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

Merged
merged 1 commit into from
Sep 21, 2024

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Jan 25, 2024

Q A
Branch? 7.2
Bug fix? no
New feature? yes
Deprecations? yes
Issues Fix #52777
License MIT
Original PR description

Alternative to #53126

In Symfony 2.8, we decided to still show exceptions/errors when running a command with --quiet or SHELL_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 in Output. In the end, I've decided for the last to avoid bug reports from people silently expecting a ConsoleOutputInterface 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.

Copy link
Member

@chalasr chalasr left a 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?

@wouterj
Copy link
Member Author

wouterj commented Jan 27, 2024

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?

@Seldaek
Copy link
Member

Seldaek commented Jan 28, 2024

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.

@Seldaek
Copy link
Member

Seldaek commented Jan 28, 2024

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.

@wouterj
Copy link
Member Author

wouterj commented Jan 30, 2024

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 chronic bash utility for this in our CI. And you can also use bash one-liners like output=`mycommand 2>&1` || echo $output to accomplish this.

@Seldaek
Copy link
Member

Seldaek commented Jan 31, 2024

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?

@nicolas-grekas nicolas-grekas modified the milestones: 7.1, 7.2 May 16, 2024
@nicolas-grekas
Copy link
Member

Let's resume this PR?

@wouterj
Copy link
Member Author

wouterj commented Sep 6, 2024

Yes please :)

We need to make a decision (@symfony/mergers):

A) Make --quiet completely silent, also for errors (the current state of this PR).
This is a bit troubling for applications currently relying on seeing errors when running with quiet verbosity.
It however is much more compatible with 99% of the CLI tools in the world. Only getting output on error is still possible using the techniques I shared before.

B) Add a new verbosity (e.g. SHELL_VERBOSITY=-2) that disables error output.
No change of existing behavior, but introduces more work (and also requires discoverability) to get truly silent output like you get with other CLI tools.
Optionally, we might add some code in the bin/console recipe to default to -2 when using --quiet to remove this extra barrier for upgraded applications.

@stof
Copy link
Member

stof commented Sep 6, 2024

@wouterj using 2 > &1 and then complaining that it screws the processing of the output is void, as the screwing comes from the 2 > &1, not from using stderr propertly in the first place.

Thus, cases that want to process the command output programmatically are probably not using --quiet anyway (as there would be not output to process)

@javiereguiluz
Copy link
Member

@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:

Optionally, we might add some code in the bin/console recipe to default to -2 when using --quiet to remove this extra barrier for upgraded applications.

Having --quiet silently associated to a new verbosity different than quiet might look convenient but it might bite us in the future.

About possible naming of this new constant, AI suggested silent because "quiet" means that makes little or no noise and "silent" means that all noise is suppressed:

Console option SHELL_VERBOSITY value Equivalent PHP constant
--silent -2 OutputInterface::VERBOSITY_SILENT
-q or --quiet -1 OutputInterface::VERBOSITY_QUIET
(none) 0 OutputInterface::VERBOSITY_NORMAL
-v 1 OutputInterface::VERBOSITY_VERBOSE
-vv 2 OutputInterface::VERBOSITY_VERY_VERBOSE
-vvv 3 OutputInterface::VERBOSITY_DEBUG

@kbond
Copy link
Member

kbond commented Sep 18, 2024

About possible naming of this new constant, AI suggested silent because "quiet" means that makes little or no noise and "silent" means that all noise is suppressed:

This makes sense to me. +1 for --silent from me.

@fabpot
Copy link
Member

fabpot commented Sep 19, 2024

Funnily enough, I encountered this issue yesterday. I was annoyed by the fact --quiet still displays the error. I wanted to remove all output and just rely on $?/$status to understand if the command failed.

Having a new --silent flag sounds like a good compromise. I'm 👍

@OskarStark
Copy link
Contributor

silent 🤫 is better wording 👌🏻

@wouterj wouterj force-pushed the issue-52777/no-errors-when-quiet branch from 1c76cfb to 5994816 Compare September 19, 2024 12:29
@wouterj wouterj changed the title [Console] Hide errors during quiet verbosity [Console] Add silent verbosity suppressing all output, including errors Sep 19, 2024
@wouterj wouterj force-pushed the issue-52777/no-errors-when-quiet branch from 5994816 to 5a16a22 Compare September 19, 2024 12:37
@wouterj
Copy link
Member Author

wouterj commented Sep 19, 2024

Thanks for the feedback everyone!

PR is now updated with a new --silent option. Decided to not add a shortcut (-s), as this can be quite conflicting with shortcut options defined by commands (and Symfony itself already uses it in the CompleteCommand). Besides, when reading this discussion, the new mode is a special programmatic case anyways and we recommend people running commands manually to keep using -q instead.

@wouterj wouterj force-pushed the issue-52777/no-errors-when-quiet branch 2 times, most recently from 76991c3 to cd21a93 Compare September 19, 2024 12:57
@wouterj wouterj force-pushed the issue-52777/no-errors-when-quiet branch 2 times, most recently from dbc370c to 51e2856 Compare September 19, 2024 13:23
@wouterj
Copy link
Member Author

wouterj commented Sep 19, 2024

Low deps failure is unrelated, flipped test failure will be fixed by #58320 if I understand correctly.

Copy link
Member

@kbond kbond left a 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?

@wouterj
Copy link
Member Author

wouterj commented Sep 19, 2024

@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).
I consider it a "minor backwards compatibility" that we allow in minor versions, but I thought it was good to flag it as BC break in the UPGRADE guide for better visibility.

@wouterj wouterj force-pushed the issue-52777/no-errors-when-quiet branch from 51e2856 to e7ebf8e Compare September 19, 2024 14:45
fabpot added a commit that referenced this pull request Sep 21, 2024
…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
@wouterj wouterj force-pushed the issue-52777/no-errors-when-quiet branch from e7ebf8e to 57fe0bd Compare September 21, 2024 09:40
@fabpot
Copy link
Member

fabpot commented Sep 21, 2024

Thank you @wouterj.

@fabpot fabpot merged commit a18ed8f into symfony:7.2 Sep 21, 2024
9 of 10 checks passed
@wouterj wouterj deleted the issue-52777/no-errors-when-quiet branch September 21, 2024 12:55
@fabpot fabpot mentioned this pull request Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow disabling exception rendering
10 participants