10000 [Console] Fallback to default answers when unable to read input by ostrolucky · Pull Request #36031 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 16, 2020
Merged

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

merged 1 commit into from
Mar 16, 2020

Conversation

ostrolucky
Copy link
Contributor
@ostrolucky ostrolucky commented Mar 11, 2020 •
edited
Loading
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.

@ostrolucky ostrolucky force-pushed the fallback-from-stdin branch 2 times, most recently from c24c696 to 7248bb0 Compare March 11, 2020 18:55
@nicolas-grekas nicolas-grekas added this to the 4.4 milestone Mar 12, 2020
@bendavies
Copy link
Contributor
bendavies commented Mar 12, 2020 •
edited
Loading

what about the fact that there is lots of code out there making decisions on $input->isInteractive() without using the question helper at all?
The behavior of all that code is different now.

@ostrolucky ostrolucky force-pushed the fallback-from-stdin branch from 7248bb0 to 8ddaa20 Compare March 12, 2020 20:13
@ostrolucky
Copy link
Contributor Author

Valid point. Can you link some?

@teohhanhui
Copy link
Contributor

Same argument I made in #36027 (comment)

@bendavies
Copy link
Contributor

Valid point. Can you link some?

There are quite a few.
https://github.com/symfony/symfony/search?q=isInteractive&type=

I was just greping my vendor folder and there will obviously be third party instances of this too.

@ostrolucky
Copy link
Contributor Author

There are quite a few.
symfony/symfony/search?q=isInteractive&type=

Thanks for the list. I checked it and none of those commands should break:

  • password encoder: Interactive check is run only when not providing password via argument. Running command in non-interactive shell without providing password doesn't make sense in the first place, but even if you do, result is error, similarly as before.
  • debug:container: it uses question helper as fallback
  • messenger:failed:retry: Again fallback. It doesn't make sense to run this command in non-interactive shell without providing id
  • debug:form : it uses question helper as fallback

@fabpot
Copy link
Member
fabpot commented Mar 16, 2020

Thank you @ostrolucky.

@fabpot fabpot merged commit bd1aaf1 into symfony:4.4 Mar 16, 2020
@nicolas-grekas nicolas-grekas mentioned this pull request Mar 19, 2020
Closed
nicolas-grekas referenced this pull request in symfony/console Mar 19, 2020
@ostrolucky @nicolas-grekas
[Console] Consider STDIN interactive
55b6ebf
@danepowell
Copy link
Contributor
danepowell commented Mar 19, 2020 •
edited
Loading

Just attaching a patch for folks wanting to test this via a composer patch to v4.4.5.
symfony-console-36031.txt

(this patch removes conflicting test files that would otherwise be in the diff)

@ostrolucky
Copy link
Contributor Author

No need, you may just suffix PR url with .patch https://github.com/symfony/symfony/pull/36031.patch

@danepowell
Copy link
Contributor
danepowell commented Mar 19, 2020 •
edited
Loading

I tried that but it doesn't apply cleanly due to conflicting changes in the 4.4.x branch. My patch removes the conflicting test files.

@danepowell
Copy link
Contributor

I've confirmed that this PR fixes the regression on my project, thanks.

This was referenced Mar 27, 2020
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Bug Console Status: Reviewed
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
7 participants
@ostrolucky @bendavies @teohhanhui @fabpot @danepowell @nicolas-grekas @carsonbot
0