Built-in commands should make use of the proper outputs.
As a feature on master, considering that people may actually rely on stdout and the fact commands have been changed a lot since 2.7, I think it's not worth doing this change on lower branches.

Please see also#20586 which adds aSymfonyStyle::getErrorStyle() shortcut for easily switching to stderr.

carsonbot added Status: Needs Review FrameworkBundle Bug labels

Nov 25, 2016

chalasr mentioned this pull request

Nov 25, 2016

[Console] Ease writing to stderr using SymfonyStyle#20586

Merged

chalasr force-pushed theframework/use_stderr_in_commands branch from1eac2be tob7af8c5Compare

November 25, 2016 11:45

Copy link

Contributor

ogizanagi commentedNov 25, 2016

Isn't there a lot more than those 4 commands? What aboutLintCommand for instance? :)

(I guess keeping this in#20586 would be less painful to update as you target master too for this bug fix 😅)

chalasr force-pushed theframework/use_stderr_in_commands branch fromb7af8c5 tob2920e7Compare

November 25, 2016 18:28

Copy link

MemberAuthor

chalasr commentedNov 25, 2016

@ogizanagi I made the changes only where it makes sense in my opinion, which exclude commands where the output give no relevant information at all.

I just applied it on theTwigLintCommand which makes use ofSymfonyStyle::error, because the output is intended to be redirected, but at the moment I don't see more commands to update.

I guess keeping this in#20586 would be less painful to update

Not that much, each one can be merged independently and, since the subject of#20586 was "Add getErrorIo AND use stderr in existing commands", I do think it should be done in two PRs :)

chalasr force-pushed theframework/use_stderr_in_commands branch fromb2920e7 to2207831Compare

November 25, 2016 18:41

Copy link

MemberAuthor

chalasr commentedNov 25, 2016

Note that when exceptions are thrown (which is the case in lot of commands including lint ones), the output goes to stderr naturally.

ogizanagi mentioned this pull request

Nov 28, 2016

[FrameworkBundle][SecurityBundle] Use stderr for some other commands#20674

Closed

Copy link

Contributor

ogizanagi commentedNov 28, 2016

@chalasr : I may have found some other places where it'll be relevant. See#20674

Copy link

MemberAuthor

chalasr commentedNov 28, 2016

Oh, I could have did it here if you just said me.. but ok then

Copy link

Contributor

ogizanagi commentedNov 28, 2016•
edited
Loading

I had to check relevant places myself so it was simpler to submit it 😅

ogizanagi reviewed

Nov 28, 2016

View reviewed changes

src/Symfony/Bridge/Twig/Command/LintCommand.php Outdated

		if (null ===$twig =$this->getTwigEnvironment()) {
		$io->error('The Twig environment needs to be set.');
		$errorIo =$outputinstanceof ConsoleOutputInterface ?newSymfonyStyle($input,$output->getErrorOutput()) :$io;
		$errorIo->error('The Twig environment needs to be set.');

Copy link

Contributor

ogizanagiNov 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I'd challenge this one in order to simply throw a\RuntimeException instead. WDYT?

Copy link

Contributor

ogizanagiNov 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

BTW, I've reverted a similar change in my previous PR thinking it was the same, but actually it's another place:https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/Twig/Command/DebugCommand.php#L90

Copy link

MemberAuthor

chalasrNov 28, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Sure 👍 but should it be done here?

Copy link

Contributor

ogizanagiNov 28, 2016•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

As it slightly changes the output and is done mainly in the aim to output it to stderr more straightforwardly, I'd say yes.

Copy link

Contributor

ogizanagiJan 7, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

The Twig Bridge DebugCommand update is still missing here, even in case the suggestion about throwing an exception instead is not considered. ^^

Copy link

MemberAuthor

chalasrJan 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Both debug/lint commands have been updated :)

Copy link

MemberAuthor

chalasr commentedNov 28, 2016

Having two places to discuss/review/merge about the same thing looks not simpler to me, as you wish.

Copy link

Contributor

ogizanagi commentedNov 28, 2016

Apply the patch then and I'll close mine.

Copy link

MemberAuthor

chalasr commentedNov 28, 2016

Done2af000a

nicolas-grekas added this to the3.x milestone

Dec 6, 2016

nicolas-grekas added Feature and removed Bug labels

Dec 6, 2016

Copy link

Member

nicolas-grekas commentedJan 6, 2017

👍

chalasr force-pushed theframework/use_stderr_in_commands branch 6 times, most recently from8ddb044 to6eaaf28Compare

January 6, 2017 21:42

Copy link

MemberAuthor

chalasr commentedJan 6, 2017•
edited
Loading

Updated for usinggetErrorStyle() in the FrameworkBundle (as it requires the console to be~3.3) and fixed the WebServerBundle commands.

xabbuh reviewed

Jan 7, 2017

View reviewed changes

src/Symfony/Bridge/Twig/Command/LintCommand.php Outdated


		if (null ===$twig =$this->getTwigEnvironment()) {
		$io->error('The Twig environment needs to be set.');
		$io->getErrorStyle()->error('The Twig environment needs to be set.');

Copy link

Member

xabbuhJan 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This will fail when the bridge is used with older versions (before 3.3) of the Console component which is completely valid.

Copy link

MemberAuthor

chalasrJan 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Reverted

xabbuh reviewed

Jan 7, 2017

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/Command/ConfigDebugCommand.php

		protectedfunctionexecute(InputInterface$input,OutputInterface$output)
		{
		$io =newSymfonyStyle($input,$output);
		$errorIo =$io->getErrorStyle();

Copy link

Member

xabbuhJan 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

same here

Copy link

MemberAuthor

chalasrJan 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Reverted everywhere

Copy link

MemberAuthor

chalasrJan 12, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@xabbuh The framework now has a conflict forsymfony/console: <3.3 so it should be fine

xabbuh reviewed

Jan 7, 2017

View reviewed changes

src/Symfony/Bundle/SecurityBundle/Command/UserPasswordEncoderCommand.php Outdated

		protectedfunctionexecute(InputInterface$input,OutputInterface$output)
		{
		$io =newSymfonyStyle($input,$output);
		$errorIo =$io->getErrorStyle();

Copy link

Member

xabbuhJan 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

and the same also applies to the SecurityBundle

Copy link

MemberAuthor

chalasrJan 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Would it work (and make sense) to add a conflict with older versions of the Console?

chalasr force-pushed theframework/use_stderr_in_commands branch 5 times, most recently frombebd602 tob64fe2dCompare

January 8, 2017 10:37

Copy link

MemberAuthor

chalasr commentedJan 8, 2017

Build failure unrelated (see#21198)

chalasr force-pushed theframework/use_stderr_in_commands branch fromb64fe2d to462a6f5Compare

January 10, 2017 17:16

Copy link

MemberAuthor

chalasr commentedJan 10, 2017•
edited
Loading

Rebased, tests are green. This is ready

chalasr force-pushed theframework/use_stderr_in_commands branch 5 times, most recently from1b327f5 to201bb4aCompare

January 12, 2017 17:33

chalasrand others added3 commits

January 12, 2017 21:28

[FrameworkBundle] Make use of stderr for non reliable output

1ee48bf

Use stderr for some other commands

9a3a568

[FrameworkBundle] Use getErrorStyle() when relevant

7b262d8

chalasr force-pushed theframework/use_stderr_in_commands branch from201bb4a to7b262d8Compare

January 12, 2017 20:28

Copy link

MemberAuthor

chalasr commentedJan 30, 2017

ping

xabbuh approved these changes

Jan 30, 2017

View reviewed changes

Copy link

Member

xabbuh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

👍

Copy link

Member

fabpot commentedFeb 19, 2017

Thank you@chalasr.

fabpot merged commit7b262d8 intosymfony:master

Feb 19, 2017

fabpot added a commit that referenced this pull request

Feb 19, 2017

feature#20632[FrameworkBundle] Make use of stderr for non reliable …

4abd0c6

…output (chalasr, ogizanagi)This PR was merged into the 3.3-dev branch.Discussion----------[FrameworkBundle] Make use of stderr for non reliable output| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | n/a| License       | MIT| Doc PR        | n/aBuilt-in commands should make use of the proper outputs.As a feature on master, considering that people may actually rely on stdout and the fact commands have been changed a lot since 2.7, I think it's not worth doing this change on lower branches.Please see also#20586 which adds a `SymfonyStyle::getErrorStyle()` shortcut for easily switching to stderr.Commits-------7b262d8 [FrameworkBundle] Use getErrorStyle() when relevant9a3a568 Use stderr for some other commands1ee48bf [FrameworkBundle] Make use of stderr for non reliable output

chalasr deleted the framework/use_stderr_in_commands branch

February 20, 2017 05:55

nicolas-grekas modified the milestones:3.x,3.3

Mar 24, 2017

fabpot mentioned this pull request

May 1, 2017

Release v3.3.0-BETA1#22603

Merged

Labels

Feature FrameworkBundle Status: Needs Review

Movatterモバイル変換

Uh oh!

[FrameworkBundle] Make use of stderr for non reliable output#20632

[FrameworkBundle] Make use of stderr for non reliable output#20632

Uh oh!

Conversation

chalasr commentedNov 25, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

ogizanagi commentedNov 25, 2016

Uh oh!

chalasr commentedNov 25, 2016

Uh oh!

chalasr commentedNov 25, 2016

Uh oh!

ogizanagi commentedNov 28, 2016

Uh oh!

chalasr commentedNov 28, 2016

Uh oh!

ogizanagi commentedNov 28, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogizanagiNov 28, 2016• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogizanagiJan 7, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalasr commentedNov 28, 2016

Uh oh!

ogizanagi commentedNov 28, 2016

Uh oh!

chalasr commentedNov 28, 2016

Uh oh!

nicolas-grekas commentedJan 6, 2017

Uh oh!

chalasr commentedJan 6, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalasrJan 12, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalasr commentedJan 8, 2017

Uh oh!

chalasr commentedJan 10, 2017• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

chalasr commentedJan 30, 2017

Uh oh!

xabbuh left a comment

Choose a reason for hiding this comment

Uh oh!

fabpot commentedFeb 19, 2017

Uh oh!

Reviewers

Assignees

Labels

chalasr commentedNov 25, 2016•
edited
Loading

ogizanagi commentedNov 28, 2016•
edited
Loading

ogizanagiNov 28, 2016•
edited
Loading

ogizanagiJan 7, 2017•
edited
Loading

chalasr commentedJan 6, 2017•
edited
Loading

chalasrJan 12, 2017•
edited
Loading

chalasr commentedJan 10, 2017•
edited
Loading