Skip to content
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

[Console] There is no way to non-interactively change answer of QuestionHelper #30726

Closed
ostrolucky opened this issue Mar 27, 2019 · 9 comments · Fixed by #33897
Closed

[Console] There is no way to non-interactively change answer of QuestionHelper #30726

ostrolucky opened this issue Mar 27, 2019 · 9 comments · Fixed by #33897

Comments

@ostrolucky
Copy link
Contributor

$output->writeln(
    (int) (new QuestionHelper())->ask(
         $input, $output, new ConfirmationQuestion('yes or no?', false)
    )
);

I expect running this as yes | ./bin/console foo will output 1, but outputs 0. Problem is QuestionHelper slaps there always default value when it detects noninteractive mode.

Maybe non interactive mode detection could be improved, since yes command clearly interacts with application.

@antonch1989
Copy link
Contributor

@ostrolucky if you don't plan to implement this, I could work on this issue

@ostrolucky
Copy link
Contributor Author

@antonch1989 feel free to work on this. I would eventually get around working on this, but I don't know when.

@antonch1989
Copy link
Contributor

ok then, I'm on it

nicolas-grekas added a commit that referenced this issue 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
@teohhanhui
Copy link
Contributor

Maybe non interactive mode detection could be improved, since yes command clearly interacts with application.

But that's not what (non-)interactive means. Interactive means the user is able to interactively provide an input, which in the case of a piped-in STDIN is clearly not possible.

@ostrolucky
Copy link
Contributor Author

User and tools like yes are both using stdin, so semantics for considering command interactive needs to be same.

@teohhanhui
Copy link
Contributor

teohhanhui commented Mar 11, 2020

What I mean is, (non-)interactive has nothing to do with whether STDIN is being used or not, but has to do with whether there is a (pseudo-)TTY.

EDIT: Actually more complicated than that: https://www.tldp.org/LDP/abs/html/intandnonint.html

@ostrolucky
Copy link
Contributor Author

That's your definition. I say interaction happens when passing input through stdin as well.

Even if you are right with definition of interactive, it doesn't change the problem that Symfony uses interactive flag for determinig if it accepts stdin input. So we can rollback this flag to old definition, but we then also need to change Console component to stop treating this flag as deciding factor for ignoring stdin input, to go in line with other standard tools such as readline. In the end not much would be changed for users.

@teohhanhui
Copy link
Contributor

Respectfully, this is not my definition. I refer you to

https://www.tldp.org/LDP/abs/html/intandnonint.html

and

https://pubs.opengroup.org/onlinepubs/9699919799/utilities/sh.html

which define quite clearly what's meant by an interactive shell.

it doesn't change the problem that Symfony uses interactive flag for determinig if it accepts stdin input. So we can rollback this flag to old definition, but we then also need to change Console component to stop treating this flag as deciding factor for ignoring stdin input, to go in line with other standard tools such as readline.

Indeed, this bug is still valid. #33897 is not the correct fix because it makes incorrect assumptions, and also because it breaks many things.

@ostrolucky
Copy link
Contributor Author

Let us consider an interactive script to be one that requires input from the user, usually with read statements

read accepts stdin:

$ echo foo | (read a; echo $a)
foo

In second link I didn't see anything about interactive command/script, just interactive shell

fabpot added a commit that referenced this issue Mar 16, 2020
…input (ostrolucky)

This PR was merged into the 4.4 branch.

Discussion
----------

[Console] Fallback to default answers when unable to read input

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

Alternative to #36027.

This fixes linked issues without having to revert fix for #30726. Successfully tested with composer script, `docker run` and `docker run -it`.

Commits
-------

8ddaa20 [Console] Fallback to default answers when unable to read input
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants