10000 [Console] RFC for Symfony 3.0: Show errors even when --quiet is present · Issue #15680 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[Console] RFC for Symfony 3.0: Show errors even when --quiet is present #15680

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

Closed
Seldaek opened this issue Sep 3, 2015 · 9 comments
Closed

Comments

@Seldaek
Copy link
Member
Seldaek commented Sep 3, 2015

I'd like to discuss this (sort of BC breaking) change for 3.0. The problem is in most cases people use --quiet / -q when running a console app, they don't want any output but still expect output/details in case an error occurs.

I see a few options to handle this:

  1. Check whether there is an tag in the message and if so output no matter if verbosity is quiet or not
  2. Only output exceptions that are caught by the Application, i.e. "uncaught" exception, regardless of the verbosity.
  3. Add an optional parameter to OutputInterface::write/writeln, that would be defined as $verbosity = self::VERBOSITY_NORMAL. That would allow any write call to contain information about which verbosity level it should be written to. I think that would be quite nice because it also enables dropping a lot of conditionals like if ($output->isVeryVerbose()) { $output->writeln(...); }, those would become $output->writeln('foo', OutputInterface::OUTPUT_NORMAL, OutputInterface::VERBOSITY_VERY_VERBOSE); which reads a bit better, except for the constants being long as hell and the OUTPUT type sitting in the middle (not sure if that'd be too much of a BC break to add the new arg as second argument, I've never seen the output type being used in the wild, or maybe both could be cobbled together as a bitwise flag).

Point 3 would be nice either way I think, and it would allow some messages to pass through the quiet mode, so if you do a write with VERBOSITY_QUIET it would always be sent, but VERBOSITY_NORMAL and above wouldn't be sent in quiet mode. Similarly we could change the exception writing to be written on the quiet verbosity.

If anyone has another idea or comments that'd be appreciated.

@fabpot
Copy link
Member
fabpot commented Sep 3, 2015

Point 3 sounds the best option.

@Seldaek
Copy link
Member Author
Seldaek commented Sep 3, 2015

@fabpot I think so too. I've discussed this a bit on IRC with Iltar and we thought it could also go into 2.8 if we turn the $type arg into a $flags arg that takes bit flags for either OUTPUT_* or VERBOSITY_*. That'd solve the long function call issue, but it requires changing the values of the constants. I don't know if that's acceptable in 2.8 or not. IMO people should know better than use the values, and I've never seen this arg used anywhere, so I would say it's ok but not sure what others think.

@fabpot
Copy link
Member
fabpot commented Sep 3, 2015

Changing the values is fine by me, if not, defining constants is useless.

@javiereguiluz
Copy link
Member

@Seldaek if I understand your proposal correctly, I think it's a bit contradictory:

Problem: if I run a command with --quiet I don't want to see any message.
Solution: define VERBOSITY_QUIET to allow commands to display messages when running them with --quiet

@wouterj
Copy link
Member
wouterj commented Sep 6, 2015

@javiereguiluz --quiet means that you don't want to have all informational messages (like "Clearing cache for dev"). You still want to see the error messages (e.g. "Unable to clear cache: No permission to remove dir dev_old").

@javiereguiluz
Copy link
Member

@wouterj yes, but I think these two things are different:

  1. Let's change --quiet behavior to supress all messages, except error messages.
  2. Let's change some things to allow commands to show messages even in --quiet mode and change errors to always appear in quiet too.

I think the first point makes sense. But I don't understand the second one.

@wouterj
Copy link
Member
wouterj commented Sep 6, 2015

@javiereguiluz I think this is to be more flexible (e.g. allow errors that are not exceptions).

Btw, point (3) of this issue (and your second point) is not only about writing messages in quiet mode, it's also about improving to conditionally write in certain modes. E.g:

-if ($output->isVerbose()) {
-    $this->writeln('Some verbose message');
-}
+$this->writeln('Some verbose message', default, OutputInterface::VERBOSITY_VERBOSE);

@javiereguiluz
Copy link
Member

@wouterj the new code would be:

$output->writeln('Some verbose message', OutputInterface::OUTPUT_NORMAL, OutputInterface::VERBOSITY_VERY_VERBOSE);

(which by the way is 114 chars long and the previous if() is just 71 chars)

My concern is that the new behavior would allow commands to display any message even when running in --quiet ... which is the exact same behavior as today.

@Seldaek
Copy link
Member Author
Seldaek commented Sep 6, 2015

@javiereguiluz the new code if we change $type to be a bitmask of options would be $output->writeln('Some verbose message', OutputInterface::VERBOSITY_VERY_VERBOSE); which is maybe still longer in characters but only one line and disturbs the code less with output conditionals that aren't really relevant to the code.

Right now, in --quiet mode AFAIK it's impossible to output anything at all. I'd just like to change that because it's usually not what people mean by quiet, they just don't want any output if things go well. If you get an error in a CI build or something and you used --quiet to keep things lean in the output, then it fails but you don't know what happened retroactively which sucks. It's been requested a few times by Composer users.

If you really don't want any output ever, >/dev/null works fine..

fabpot added a commit that referenced this issue Sep 13, 2015
…erbosity can be passed in as well (Seldaek)

This PR was merged into the 2.8 branch.

Discussion
----------

Convert Output::write's type to an options arg where verbosity can be passed in as well

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #15680
| License       | MIT
| Doc PR        |

Commits
-------

749fba5 Convert Output::write's type to an options arg where verbosity can be passed in as well
@fabpot fabpot closed this as completed Sep 13, 2015
fabpot added a commit that referenced this issue Sep 13, 2015
…ixes #15680 (Seldaek)

This PR was merged into the 2.8 branch.

Discussion
----------

Make the exception output visible even in quiet mode, fixes #15680

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #15680
| License       | MIT
| Doc PR        |

This builds upon #15772 and sets the exception output in Application::renderException to use VERBOSITY_QUIET so that exceptions are always visible. IMO it's a good change but I can see that it is potentially controversial.

Commits
-------

e9ee8f5 Make the exception output visible even in quiet mode, fixes #15680
fabpot added a commit that referenced this issue Sep 14, 2015
* 2.8: (31 commits)
  [DomCrawler] Invalid uri created from forms if base tag present
  [VarDumper] Add caster for OuterIterator objects
  [Console] update param type phpdoc for StreamOutput
  [Console] fix typo in OutputInterface
  Use stderr by default when a specific output is not injected
  fixed bad merge
  [Debug] Fix case mismatch detection
  [HttpKernel] Add entry point to more easily create/configure the DI extension
  [DX] Added a logout link in the security panel of the web debug toolbar
  [HttpKernel] fix broken multiline <esi:remove>
  [DoctrineBridge] Fixed #14840
  [FrameworkBundle] add a suggest for the serializer component
  fixed CS
  removed non-working tests
  [WIP] #15502 Make template shortcuts be usable without Templating component
  Redesigned the Symfony Profiler
  [Yaml] Fix the parsing of float keys
  Make the exception output visible even in quiet mode, fixes #15680
  Convert Output::write's type to an options arg where verbosity can be passed in as well
  [Console] Ensure the console output is only detected as decorated when both stderr and stdout support colors
  ...
weaverryan added a commit to symfony/symfony-docs that referenced this issue Jul 10, 2016
This PR was submitted for the 2.8 branch but it was merged into the 2.7 branch instead (closes #5672).

Discussion
----------

Add constants to BC promise

| Q | A
| --- | ---
| Doc fix? | no
| New docs? | yes
| Applies to | all
| Fixed tickets | #5224

This does things a little bit different than proposed in #5224, based on [this comment by @fabpot](symfony/symfony#15680 (comment)) and the fact that one uses constants, to allow to change the value without breaking the code.

/cc @webmozart @stof @weaverryan @xabbuh please review this & give your opinion on this.

Commits
-------

d25c28d Add constants to BC promise
ostrolucky pushed a commit to ostrolucky/symfony that referenced this issue Mar 25, 2018
* 2.8: (31 commits)
  [DomCrawler] Invalid uri created from forms if base tag present
  [VarDumper] Add caster for OuterIterator objects
  [Console] update param type phpdoc for StreamOutput
  [Console] fix typo in OutputInterface
  Use stderr by default when a specific output is not injected
  fixed bad merge
  [Debug] Fix case mismatch detection
  [HttpKernel] Add entry point to more easily create/configure the DI extension
  [DX] Added a logout link in the security panel of the web debug toolbar
  [HttpKernel] fix broken multiline <esi:remove>
  [DoctrineBridge] Fixed symfony#14840
  [FrameworkBundle] add a suggest for the serializer component
  fixed CS
  removed non-working tests
  [WIP] symfony#15502 Make template shortcuts be usable without Templating component
  Redesigned the Symfony Profiler
  [Yaml] Fix the parsing of float keys
  Make the exception output visible even in quiet mode, fixes symfony#15680
  Convert Output::write's type to an options arg where verbosity can be passed in as well
  [Console] Ensure the console output is only detected as decorated when both stderr and stdout support colors
  ...
fabpot added a commit that referenced this issue Sep 21, 2024
… including errors (wouterj)

This PR was merged into the 7.2 branch.

Discussion
----------

[Console] Add silent verbosity suppressing all output, including errors

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | yes
| Issues        | Fix #52777
| License       | MIT

<details>
<summary>Original PR description</summary>

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).

</details>

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.

Commits
-------

57fe0bd [Console] Add silent verbosity mode suppressing all output, including errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants
0