Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Console] Consider STDIN interactive#33897

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@ostrolucky
Copy link
Contributor

@ostroluckyostrolucky commentedOct 7, 2019
edited by nicolas-grekas
Loading

QA
Branch?4.4
Bug fix?yes
New feature?no
Deprecations?no
TicketsFix#30726, supersedes#30796
LicenseMIT
Doc PR-

As demonstrated withyes | 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 doingecho 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 viaproc_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.
  2. If application uses internal logic where it relies on STDIN disregardingOutput::isInteractive flag, this doesn't change anything for these either - they will keep using STDIN disregarding result of this flag.
  3. 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)?
  4. What aboutSHELL_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.
  5. 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.

@stof
Copy link
Member

stof commentedOct 8, 2019

AFAICT, this could now make command actually hang when they are running in a place where they are not interactive but you don't explicitly add the flag, for instance on CI.

Should we instead add a flag to explicit force interactive mode for case where you want to pipe your own input, rather than never detecting anything and breaking things for cases which were relying on this detection to disable interaction ?

Without such detection, adding a question in an existing command becomes a BC break, as it becomes interactive and will hang.

@ostrolucky
Copy link
ContributorAuthor

I don't think it will hang with this patch. What's the reproduction scenario for that?

@chalasrchalasr modified the milestones:5.0,nextDec 10, 2019
@nicolas-grekas
Copy link
Member

nicolas-grekas commentedFeb 3, 2020
edited
Loading

I'm 👎 for this change - this will break background jobs that are fine with the default choice.
Instead, I propose we add a new--interactive global flag.
PR welcome.

@ostrolucky
Copy link
ContributorAuthor

No it won't, what's reproducer for that? I already asked for that (and didn't get answer) and explained in detail why nothing important breaks in PR description.

@nicolas-grekas
Copy link
Member

I'm reopening and canceling my downvote: I checked out this locally and can't find a situation where the removed logic was useful. Good for 5.0 on my side, could be good for 4.4 also, maybe 3.4.

ostrolucky reacted with thumbs up emoji

@nicolas-grekasnicolas-grekas changed the base branch frommaster to4.4February 7, 2020 08:29
@nicolas-grekas
Copy link
Member

Thank you@ostrolucky.

nicolas-grekas added a commit that referenced this pull requestFeb 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 breakI'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-grekasnicolas-grekas merged commitef157d5 intosymfony:4.4Feb 7, 2020
This was referencedFeb 29, 2020
@ElisDN
Copy link
Contributor

It broke allcomposer app ... calls.

@ostrolucky
Copy link
ContributorAuthor

Would be worth separate issue and more investigation. Does composer pass in empty stdin? 🤔

@chalasr
Copy link
Member

See#35988

@bendavies
Copy link
Contributor

bendavies commentedMar 11, 2020
edited
Loading

this also breaks docker run.

this should not be interactive:

docker run --rm IMAGE bin/consolecommand

this should be interactive:

docker run --rm -it IMAGE bin/consolecommand

with this PR, both the above commands make the console interactive.

removingposix_isatty just breaks tty detection completely.

consider revert?

mbrodala, teohhanhui, and Mediagone reacted with thumbs up emoji

@ostrolucky
Copy link
ContributorAuthor

ostrolucky commentedMar 11, 2020
edited
Loading

Let's stop talking about interactive flag. Console component needs to accept stdin input for controlling its commands, in similar fashion as happens when doingdocker run --rm ubuntu bash -c 'echo wat | (read a; echo $a)'. It didn't and that was fixed.

Changing interactive flag itself was just easiest solution to achieve that. Not using it for this purpose would loose its point. Ideally console component shouldn't have it at all. In the end change of semantics of this flag wouldn't make a difference in your case if component stopped using it.

Solution here might be to change interactive flag to false before (or during, if not otherwise possible + repeat) first related exception happens (Aborted. message). That's when it's clear no stdin input is coming to save the day.

teohhanhui reacted with thumbs down emojiteohhanhui reacted with confused emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

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

7 participants

@ostrolucky@stof@nicolas-grekas@ElisDN@chalasr@bendavies@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp