[Console] default to stderr in the console helpers #15794

alcohol · 2015-09-14T16:21:11Z

Interactive input/output and informational output such as progress should go to stderr if available.

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

See #13730 also for previous discussion.

If someone explicitly wants to use stdout, they can simply pass $output->getStream() instead of $output in most use-cases.

Tobion · 2015-09-14T17:01:28Z

src/Symfony/Component/Console/Helper/DialogHelper.php

@@ -454,6 +467,10 @@ private function validateAttempts($interviewer, OutputInterface $output, $valida
    {
        $e = null;
        while (false === $attempts || $attempts--) {
+            if ($output instanceof ConsoleOutputInterface) {
+                $output = $output->getErrorOutput();


this should be outside the loop

Tobion · 2015-09-14T17:02:26Z

Could you create another PR against 2.7 that changes ProgressBar and QuestionHelper accordingly please?

alcohol · 2015-09-14T17:04:57Z

Yes already working on a PR for 2.7.

Are ProgressBar and QuestionHelper the only ones eligible?
There is also a SymfonyQuestionHelper. I take it that is for internal stuff only then?

Tobion · 2015-09-14T17:05:46Z

SymfonyQuestionHelper extends QuestionHelper and thus should be convered already

alcohol · 2015-09-14T17:24:02Z

Please let me know if #15795 covers it or if any amendments need to be made.

alcohol · 2015-09-14T17:24:54Z

@Seldaek here you go (again).

Tobion · 2015-09-14T19:16:35Z

👍

Status: Reviewed

stof · 2015-09-15T09:44:48Z

Tests covering this are missing

alcohol · 2015-09-15T09:48:42Z

What would be an acceptable test scenario in your opinion, to validate these changes?

stof · 2015-09-15T10:09:17Z

@alcohol Creating the helper with a ConsoleOutputInterface, and asserting that it writes its output to the error output.

alcohol · 2015-09-15T10:26:30Z

Allright. Will implement something later today. Thanks for the feedback.

Tobion · 2015-09-20T14:55:47Z

Status: Needs Work
Tests

alcohol · 2015-09-21T21:09:48Z

@Tobion got any tips?

I tried the following for the DialogHelper for example, but yeah obviously it doesn't work as expected.

    public function testAskOnStderrWithConsoleOutput()
    {
        if (!$this->hasSttyAvailable()) {
            $this->markTestSkipped('`stderr` is required to test stderr output functionality');
        }

        $dialog = new DialogHelper();

        $dialog->setInputStream($this->getInputStream("\nnot stdout\n"));

        $this->assertEquals('stderr', $dialog->ask(new ConsoleOutput(), 'Where should output go?', 'stderr'));
        $this->assertEquals('not stdout', $dialog->ask($output = new ConsoleOutput(), 'Where should output go?', 'stderr'));

        rewind($output->getErrorOutput()->getStream());
        $this->assertEquals('Where should output go?', stream_get_contents($output->getErrorOutput()->getStream()));
    }

/srv/git/github/symfony (2.3-default-to-stderr*) $ ./phpunit src/Symfony/Component/Console/Tests/Helper/DialogHelperTest.php
PHPUnit 4.8.9 by Sebastian Bergmann and contributors.

..F....

Time: 236 ms, Memory: 6.00Mb

There was 1 failure:

1) Symfony\Component\Console\Tests\Helper\DialogHelperTest::testAskOnStderrWithConsoleOutput
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'Where should output go?'
+''

/srv/git/github/symfony/src/Symfony/Component/Console/Tests/Helper/DialogHelperTest.php:81

FAILURES!
Tests: 7, Assertions: 35, Failures: 1.
Where should output go?Where should output go?KO src/Symfony/Component/Console/Tests/Helper/DialogHelperTest.php

I noticed all the tests use php://memory for output. Nothing actually ever tests stdout or stderr.

alcohol · 2015-09-21T21:40:41Z

@Tobion, @stof, can you let me know if the above added tests are in line with what you in mind? I'll add more in that case.

… (alcohol) This PR was merged into the 2.7 branch. Discussion ---------- [Console] Default to stderr for the console helpers (2.7+) Interactive input/output and informational output such as progress should go to `stderr` if available. | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Only merge if #15794 is merged. If someone explicitly wants to use `stdout`, they can simply pass `$output->getStream()` instead of `$output` in most use-cases. Commits ------- 90c2a96 Default to stderr for console helpers (only merge if #15794 gets merged)

* 2.7: [Config] Fix enum default value in Yaml dumper Finnish translation fix [CssSelector] Optimize regexs matching simple selectors Fix the phpdoc in the CssSelector TranslatorInterface [Console] Add clock mock to fix transient test on HHVM [DomCrawler] Optimize the regex used to find namespace prefixes [EventDispatcher] skip one lazy loading call [EventDispatcher] fix memory leak in a getListeners Default to stderr for console helpers (only merge if #15794 gets merged)

* 2.8: Added the right revision date for status code registry [Config] Fix enum default value in Yaml dumper fixed typo. [Translation][File dumper] allow get file content without writing in file. Finnish translation fix [CssSelector] Optimize regexs matching simple selectors Fix the phpdoc in the CssSelector TranslatorInterface [Console] Add clock mock to fix transient test on HHVM [DomCrawler] Optimize the regex used to find namespace prefixes [VarDumper] Add EnumStub for dumping virtual collections with casters [Finder] Deprecate adapters and related classes [EventDispatcher] skip one lazy loading call [EventDispatcher] fix memory leak in a getListeners [WebProfilerBundle] added btn-link. Remove duplication of the handling of regex filters in the Finder Default to stderr for console helpers (only merge if #15794 gets merged) Conflicts: src/Symfony/Component/Console/Tests/Helper/LegacyProgressHelperTest.php src/Symfony/Component/EventDispatcher/EventDispatcher.php src/Symfony/Component/VarDumper/Tests/CliDumperTest.php src/Symfony/Component/VarDumper/Tests/HtmlDumperTest.php

keradus · 2016-02-06T17:39:23Z

Hi everyone !
Whan is the state of this PR ?

alcohol · 2016-02-07T23:15:45Z

@keradus not sure, getting dusty? :d

fabpot · 2016-03-04T09:21:17Z

Thank you @alcohol.

This PR was squashed before being merged into the 2.3 branch (closes #15794). Discussion ---------- [Console] default to stderr in the console helpers Interactive input/output and informational output such as progress should go to `stderr` if available. | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | See #13730 also for previous discussion. If someone explicitly wants to use `stdout`, they can simply pass `$output->getStream()` instead of `$output` in most use-cases. Commits ------- 3d4e95e [Console] default to stderr in the console helpers

* 2.8: Added the right revision date for status code registry [Config] Fix enum default value in Yaml dumper fixed typo. [Translation][File dumper] allow get file content without writing in file. Finnish translation fix [CssSelector] Optimize regexs matching simple selectors Fix the phpdoc in the CssSelector TranslatorInterface [Console] Add clock mock to fix transient test on HHVM [DomCrawler] Optimize the regex used to find namespace prefixes [VarDumper] Add EnumStub for dumping virtual collections with casters [Finder] Deprecate adapters and related classes [EventDispatcher] skip one lazy loading call [EventDispatcher] fix memory leak in a getListeners [WebProfilerBundle] added btn-link. Remove duplication of the handling of regex filters in the Finder Default to stderr for console helpers (only merge if symfony#15794 gets merged) Conflicts: src/Symfony/Component/Console/Tests/Helper/LegacyProgressHelperTest.php src/Symfony/Component/EventDispatcher/EventDispatcher.php src/Symfony/Component/VarDumper/Tests/CliDumperTest.php src/Symfony/Component/VarDumper/Tests/HtmlDumperTest.php

carsonbot added the Status: Needs Review label Sep 14, 2015

Tobion reviewed Sep 14, 2015
View reviewed changes

alcohol mentioned this pull request Sep 14, 2015

[Console] Default to stderr for the console helpers (2.7+) #15795

Merged

carsonbot added Status: Reviewed and removed Status: Needs Review labels Sep 14, 2015

carsonbot added Status: Needs Work and removed Status: Reviewed labels Sep 20, 2015

alcohol added 2 commits September 21, 2015 23:39

Default to stderr in the console helpers

9bf55bb

add stderr tests

b6e0724

javiereguiluz added Bug Console labels Mar 3, 2016

fabpot closed this Mar 4, 2016

keradus mentioned this pull request Mar 4, 2016

Split output to stderr and stdout PHP-CS-Fixer/PHP-CS-Fixer#1654

Merged

fabpot mentioned this pull request Mar 13, 2016

Release v2.3.39 #18153

Merged

This was referenced Mar 25, 2016

Release v2.7.11 #18315

Merged

Release v2.8.4 #18327

Closed

Release v2.8.4 #18329

Merged

Release v3.0.4 #18363

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Console] default to stderr in the console helpers #15794

[Console] default to stderr in the console helpers #15794

alcohol commented Sep 14, 2015

Tobion Sep 14, 2015

Tobion commented Sep 14, 2015

alcohol commented Sep 14, 2015

Tobion commented Sep 14, 2015

alcohol commented Sep 14, 2015

alcohol commented Sep 14, 2015

Tobion commented Sep 14, 2015

stof commented Sep 15, 2015

alcohol commented Sep 15, 2015

stof commented Sep 15, 2015

alcohol commented Sep 15, 2015

Tobion commented Sep 20, 2015

alcohol commented Sep 21, 2015

alcohol commented Sep 21, 2015

keradus commented Feb 6, 2016

alcohol commented Feb 7, 2016

fabpot commented Mar 4, 2016

[Console] default to stderr in the console helpers #15794

[Console] default to stderr in the console helpers #15794

Conversation

alcohol commented Sep 14, 2015

Tobion Sep 14, 2015

Choose a reason for hiding this comment

Tobion commented Sep 14, 2015

alcohol commented Sep 14, 2015

Tobion commented Sep 14, 2015

alcohol commented Sep 14, 2015

alcohol commented Sep 14, 2015

Tobion commented Sep 14, 2015

stof commented Sep 15, 2015

alcohol commented Sep 15, 2015

stof commented Sep 15, 2015

alcohol commented Sep 15, 2015

Tobion commented Sep 20, 2015

alcohol commented Sep 21, 2015

alcohol commented Sep 21, 2015

keradus commented Feb 6, 2016

alcohol commented Feb 7, 2016

fabpot commented Mar 4, 2016