Skip to content

[Console] non interactive confirmation question #30796

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

Conversation

antonch1989
Copy link
Contributor

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

@antonch1989
Copy link
Contributor Author

Hope to get some suggestions to find better and non native way to access STDIN in purpose to make this solution more testable

@chalasr chalasr added this to the next milestone Mar 31, 2019
@antonch1989 antonch1989 changed the title non interactive confirmation question [Console] non interactive confirmation question Apr 2, 2019
@Simperfit
Copy link
Contributor

The test seems to fail

cc @chalasr any idea ?

@antonch1989
Copy link
Contributor Author

this failing test looks unrelated

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.

I think I like it

@@ -63,6 +64,13 @@ public function ask(InputInterface $input, OutputInterface $output, Question $qu
}
}

if ($question instanceof ConfirmationQuestion) {
$value = trim(fgets(STDIN));
if ($value === 'y') {
Copy link
Member

@chalasr chalasr Aug 26, 2019

Choose a reason for hiding this comment

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

Any true-ish value should be considered according to

$this->trueAnswerRegex = $trueAnswerRegex;

I think it should read more bytes from the input stream (as much as in doAsk()) and, if it's not false, return the value after it has been passed to Question::getNormalizer()/getValidator().

Also, couldn't this work for any question instead of only confirmation questions?

@@ -63,6 +64,13 @@ public function ask(InputInterface $input, OutputInterface $output, Question $qu
}
}

if ($question instanceof ConfirmationQuestion) {
$value = trim(fgets(STDIN));
Copy link
Member

Choose a reason for hiding this comment

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

It should use $input->getStream() and STDIN as fallback, which implies to move up the intialization of $this->inputStream below

@chalasr
Copy link
Member

chalasr commented Aug 26, 2019

Some important changes have been made to this code area since you opened your PR, please rebase and retarget this to the 4.4 branch, that should help addressing my comments.

@fabpot
Copy link
Member

fabpot commented Feb 3, 2020

Closing as the PR looks stale.

@fabpot fabpot closed this Feb 3, 2020
nicolas-grekas added a commit that referenced this pull request Feb 7, 2020
This PR was submitted for the master branch but it was merged into the 4.4 branch instead.

Discussion
----------

[Console] Consider STDIN interactive

| Q             | A
| ------------- | ---
| Branch?       |4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #30726, supersedes #30796
| License       | MIT
| Doc PR        | -

As demonstrated with `yes | bin/console foo` in #30726, original assumption made in #1699 was wrong. Then, #8452 was merged which solved bug #8079 -> this was a use case when application hangs with `--no-interaction` flag - nobody probably realized that application can be in "non-interactive" mode, without using this flag and not hang. Then, there was #14102 which was poor man's fix for problem caused by this. So already plenty issues this behaviour causes. Looks like a mess to me. Application should be considered non-interactive only when explicitly specified so (--no-interactive flag), otherwise it doesn't hang.

### What this change means?
It only changes one case: When doing `echo foo | bin/console bar`, `yes | bin/console bar`, `bin/console bar < foo`, etc. Redirecting stdout is not affected, as application in that case was considered interactive before too. With stdin, this opens possibility to control symfony/console based application by default via stdin, including via `proc_open`.

Additionally, not only it allows to control the input for questions, it also makes the question and answers to display on screen. So before, user had no idea what questions are happening and what answers (defaults) are being used.

### About a BC break
I'm not really aware of a valid use case this can break. Can you help find any?

1. Since symfony/console components were NOT interactive with stdin before, stdin couldn't be used to control them - so there this change breaks nothing, because it didn't make sense to pass stdin there instead of specifying -n flag.
1. If application uses internal logic where it relies on STDIN disregarding `Output::isInteractive` flag, this doesn't change anything for these either - they will keep using STDIN disregarding result of this flag.
1. What if application uses internal logic for stdin AND console components like QuestionHelper? To me, that doesn't make much sense, because with previous behaviour, such questions would result always into defaults. It might make sense in case application supports both modes - either stdin, or user supplied input and just use default answers with stdin. But I cannot figure out example of such use - what would be the case where application allows user to control something via stdin, but at the same time forbids them to set certain aspects (answers to questions given)?
1. What about `SHELL_INTERACTIVE` env variable? Only way to utilize it was to force enable interactive mode, but since it will be interactive now by default, it will do nothing and no behaviour changes.
1. Preventing stdin control was much bigger potential BC break. Despite that, it was disallowed in minor Symfony version. And as far as I can see, I saw no backlash.

Finally, this targets Symfony 5.0 to be extra sure anyways, so I think it's ok, but feel free to suggest documenting this in upgrade guide or changelog. I would even target 4.4, but chose 5.0 as it's easier to push through there.

Commits
-------

ef157d5 [Console] Consider STDIN interactive
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
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.

6 participants