8000 [Console] Separate formatter for stdout/stderr by alcohol · Pull Request #13661 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Console] Separate formatter for stdout/stderr #13661

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

[Console] Separate formatter for stdout/stderr #13661

wants to merge 4 commits into from

Conversation

alcohol
Copy link
Contributor
@alcohol alcohol commented Feb 11, 2015

[Console]

When piping output from a console command through | more for example, stderr is still considered to have color support although stdout does not. When stdout and stderr share the same formatter, this results in color codes being output while this is not desired behaviour.

E.g.

# stdout.php
<?php
require_once 'vendor/autoload.php';
$out = new Symfony\Component\Console\Output\ConsoleOutput();
$out->writeln('<info>OK</info>');

without patch

$ php stdout.php | more
ESC[32mOKESC[39m

$ php stdout.php 2>/dev/null | more
OK

with patch

$ php stdout.php | more
OK

The cleanest solution I could think of, was simply cloning the given formatter rather than sharing the same instance.

Partial credit goes to @Seldaek as well for discovering and debugging this unexpected behaviour with me.

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

@Seldaek
Copy link
Member
Seldaek commented Feb 11, 2015

👍

This was referenced Feb 11, 2015
Merged
Closed
Closed
@giosh94mhz
Copy link
Contributor

for reference: 👎 see #13449 (comment)

@alcohol
Copy link
Contributor Author
alcohol commented Feb 12, 2015

While I can understand the concern for non-BC in the case of not "clone-able" Formatters, I think the case of actually running into such a scenario is so highly unlikely that it's negligible.

@giosh94mhz
Copy link
Contributor

I agree with you that it's a remote scenario, but still is a BC since put more restriction on the constructor; also cloning a DI instance smells to me.

So, I don't say this PR should not be merged, but probably is not a good idea to merge it in 2.3. 2.7 it's almost here and may be a perfect opportunity. :)

@alcohol
Copy link
Contributor Author
alcohol commented Jul 22, 2015

@fabpot @xabbuh @Tobion do any of you have any feedback on this PR?

@xabbuh
Copy link
Member
xabbuh commented Jul 26, 2015

The fix looks good, but I would prefer if we added an extra $errorFormatter argument to the constructor which defaults to null. The user would then be able to pass their own formatter for stderr and therefore being able to reference it from the outside (like they can do for stdout). Cloning can be the default behaviour then when the formatter for stderr is omitted.

@alcohol
Copy link
Contributor Author
alcohol commented Jul 26, 2015

@xabbuh one additional question then, should calls to setDecorated, setFormatter and setVerbosity still also proxy to stderr when called on a stdout wrapper?

alcohol added 3 commits July 26, 2015 12:34
@alcohol
added argument for stderr formatter
18b4c57
@alcohol
Merge branch '2.3' of github.com:symfony/symfony into separate-format…
064ceb4 
…ter-per-stdout-stderr

* '2.3' of github.com:symfony/symfony: (273 commits)
  #15331 add infos about deprecated classes to UPGRADE-3.0
  [Security] removed useless else condition in SwitchUserListener class.
  [travis] Tests deps=low with PHP 5.6
  [Console] Fix console output with closed stdout
  [Security] fix check for empty usernames
  [Form] updated exception message of ButtonBuilder::setRequestHandler()
  [travis] Fix deps=high jobs
  [HttpFoundation] [PSR-7] Allow to use resources as content body and to return resources from string content
  [DependencyInjection] Remove unused code in XmlFileLoader
  [HttpFoundation] Behaviour change in PHP7 for substr
  bumped Symfony version to 2.3.32
  updated VERSION for 2.3.31
  update CONTRIBUTORS for 2.3.31
  updated CHANGELOG for 2.3.31
  fixed some tests
  Remove excess whitespace
  Added 'default' color
  [HttpFoundation] Reload the session after regenerating its id
  [HttpFoundation] Add a test case to confirm a bug in session migration
  [Finder] Command::addAtIndex() fails with Command instance argument
  ...
@alcohol
aligned docblock param descriptions
139211e
@xabbuh
Copy link
Member
xabbuh commented Aug 1, 2015

@alcohol Maybe we should check in if both formatters are the same and only in this case change the stderr related stuff too.

@alcohol alcohol closed this Sep 5, 2015
@Seldaek Seldaek mentioned this pull request Sep 12, 2015
Merged
fabpot added a commit that referenced this pull request Sep 13, 2015
@fabpot
bug #15771 [Console] Ensure the console output is only detected as de…
e2156d7 
…corated when both stderr and stdout support colors (Seldaek)

This PR was merged into the 2.3 branch.

Discussion
----------

[Console] Ensure the console output is only detected as decorated when both stderr and stdout support colors

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

This is a simplified version of #13661 which does not create any issues with having two decorators but merely ensures that if either STDERR **or** STDOUT has colors disabled, then both will have decoration disabled. It's not a perfect solution but it's better than having both enabled as this breaks things. And I don't think we can come to a better solution without api changes.

Commits
-------

f3d8444 [Console] Ensure the console output is only detected as decorated when both stderr and stdout support colors
@wouterj wouterj mentioned this pull request Nov 4, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Console
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alcohol @Seldaek @giosh94mhz @xabbuh
0