Console ApplicationTester does not allow multiple prompts on invalid Question input

[weierophinney]

Symfony version(s) affected: 5.0.0, 5.1.0

Description

When using the QuestionHelper and testing via an ApplicationTester, if $maxAttempts has not been set, the question will stop prompting for input after the first question response, regardless of whether or not more input are available.

How to reproduce

use Symfony\Component\Console\Application;
use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Helper\QuestionHelper;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;
use Symfony\Component\Console\Question\Question;
use Symfony\Component\Console\Tester\ApplicationTester;

require './vendor/autoload.php';

class QuestionCommand extends Command
{
    protected function configure(): void
    {
        $this->setDescription('A command that asks a question');
    }

    protected function execute(InputInterface $input, OutputInterface $output): int
    {
        $question = new Question('This is a promptable question');
        $question->setValidator(function ($value) {
            if (! preg_match('/^[A-Z][A-Za-z0-9_]+$/', $value)) {
                throw new RuntimeException('Question requires a valid class name');
            }
            return $value;
        });

        /** @var QuestionHelper $helper */
        $helper = $this->getHelper('question');
        $class = $helper->ask($input, $output, $question);

        $output->writeln(sprintf('<info>%s</info>', $class));

        return 0;
    }
}

$application = new Application('test');
$application->add(new QuestionCommand('question'));
$application->setAutoExit(false);

$tester = new ApplicationTester($application);
$tester->setInputs(['', 'not-a-class', 'also not a class', 'FinallyAClass']);

$statusCode = $tester->run(
    ['command' => 'question'],
    ['interactive' => true]
);

printf("Status code: %d\n", $statusCode);

Expected result:

Status code: 0

Actual result:

Status code: 1

Possible Solution

In both version 4 and version 5 code, QuestionHelper::validateAttempts() has the following loop defined:

while (null === $attempts || $attempts--)

In version 5, the following line is added at the end of the loop:

$attempts = $attempts ?? -(int) $this->isTty();

This means that you will always run the loop exactly 1 time when $attempts is set to null, as isTty() always returns false when run under the ApplicationTester (which uses a PHP memory stream to represent the TTY input stream).

An $attempts value of null is meant to indicate no maximum, and to prompt indefinitely; more importantly, it is the default value.

In order to preserve previous (and documented) behavior, I think this should likely read:

$attempts = $attempts === null ? null : -(int) $this->isTtty();

[chalasr]

Same as symfony/maker-bundle#629. I agree with the proposed solution.
ping @nicolas-grekas

[nicolas-grekas]

symfony/maker-bundle#629 is different and is going to be fixed by my patch suggestion on #37122 IIUC.

Here, we're running into a conflict with #36590

I agree that the meaning of null is meant to be "forever". If the change in #36590 leads to a BC break, we might need to consider an alternative approach instead of #36590.

@ostrolucky WDYT?

[ostrolucky]

I don't think #36590 is the reason here, but #36696. In #36590, $attempts is set to 1 only for non-tty session, while #36696 modifies $attempts in either way. And ApplicationTester doesn't use stdin.

edit: Taking it back, it is indeed caused by #36590

[ostrolucky]

Problem here is that ApplicationTester uses php://memory as input stream and this is of course not considered to be a tty. Since we introduced QuestionHelper::isTty function only so we can prevent infinite loops (which shouldn't happen with ApplicationTester or via similar methods), I would think it would be fair enough to remove $this->inputStream to be a factor there. Then, this behaviour will activate only when application is running under actual non-interactive STDIN. WDYT @nicolas-grekas?

[nicolas-grekas]

So, the first line of the isTty() method would be just:
$inputStream = fopen('php://input', 'r');

For the application tester, this might mean that the behavior changes when running in a CI that exposes a TTY. Do they all do? What about Windows?

[ostrolucky]

#37160 should help answer this question