[Console] Make DialogHelper respect interaction settings #8452
Conversation
| @@ -895,6 +895,10 @@ protected function configureIO(InputInterface $input, OutputInterface $output) | |||
| */ | |||
| protected function doRunCommand(Command $command, InputInterface $input, OutputInterface $output) | |||
| { | |||
| if ($command->getHelperSet()->has('dialog')) { | |||
| $command->getHelperSet()->get('dialog')->setInput($input); | |||
There was a problem hiding this comment.
wouldn't it be better to allow other helpers to require the input as well, with a dedicated interface and an instanceof switch ?
There was a problem hiding this comment.
you mean something like an InputAwareInterface and something like:
foreach ($command->getHelperSet() as $helper) {
if ($helper instanceof InputAwareInterface) {
$helper->setInput($input);
}
}I like that!
|
@wouterj sorry, no infinite loop. I missed the fact that |
| * | ||
| * @return InputInterface | ||
| */ | ||
| public function getInput() |
There was a problem hiding this comment.
I would remove this getter as you should never rely on a helper to get the input instance.
There was a problem hiding this comment.
agree, I had removed it but something did go wrong when rebasing. I'm fixing it now
| * | ||
| * @param InputInterface $input | ||
| */ | ||
| public function setInput(InputInterface $input) |
There was a problem hiding this comment.
I'm not a big fan of the injection of the input here as it means that the helper becomes stateful. I know that because of BC, we don't have many choices, but another possibility would be to just inject the value of the interaction flag. It would still be stateful, but "less".
While writing this comment, I realize that injection the interaction flag is probably not a good idea, but let's discuss this possibility.
There was a problem hiding this comment.
What about injecting the Application and adding a getInput method to the application?
This means that the application is statefull of the input, but it is already quite statefull of the current command (Application::$runningCommand).
And it means that the helpers are statefull of the application, but as the application keeps the same during a request we can call it stateless imo.
| namespace Symfony\Component\Console\Input; | ||
|
|
||
| /** | ||
| * InputAwareInterface should be implemented by classes that depends on the |
|
What about |
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
This is based on @cordoval's #8366, but it tries to not break BC and to be a little more userfriendly.
@stof I can't seem to follow the infinite loop you talked about in #8366 .
DialogHelper::askwill return the default, which isnull, that breaks the while loop and it returns the default.