Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Silence stream_isatty and posix_isatty#37122
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
It was actually silence before (and still is in 3.4), but we removed this later, thinking it's not needed anymore.Without this, user will get `XX bytes of buffered data lost during stream conversion` errorsSimple reproducer:```php(new Application()) ->register('app') ->setCode(function(InputInterface $input, OutputInterface $output) { (new QuestionHelper())->ask($input, $output, (new Question('Foo?'))->setValidator(function () { throw new InvalidArgumentException('Foo!'); })); }) ->getApplication() ->setDefaultCommand('app', true) ->run();```echo "foo\n" | php console-app.phpnicolas-grekas commentedJun 6, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Coincidentally, we discussed this with@weaverryan yesterday. The warning is legit: data lost during stream conversion means we're ignoring part of the user input. The best fix I can think of is to do this instead: -$attempts = $question->getMaxAttempts();+$attempts = $question->getMaxAttempts() ?? 100; |
ostrolucky commentedJun 6, 2020
Wouldn't that reopen problem 2 that was fixed in#36590? In stdin mode, only one attempt to display question should happen. |
nicolas-grekas commentedJun 6, 2020
We have little options here, having to choose between losing user input and asking more that one time... |
ostrolucky commentedJun 6, 2020
As mentioned in PR description, I can't help further if I don't understand issue that was fixed in#36696. Because after reverting that patch I cannot reproduce the issue patch is meant to fix.
No, stdout is same in both cases of |
nicolas-grekas commentedJun 6, 2020
The patch in#36696 fixes exactly the same situation as reported here, but when answers are correct: when the response is correct, we don't care about tty or not. That's what the patch does: dong the tty check only when the answer fails. Now, we have the same problem but only when a wrong answer is given. |
ostrolucky commentedJun 6, 2020
Yes you did say that inhttps://github.com/symfony/symfony/pull/36696/files#r420063328 and I said in this PR description that according that description code like such symfony/src/Symfony/Component/Console/Tests/phpt/uses_stdin_as_interactive_input.phpt Lines 18 to 27 in8ddaa20
Can you reproduce it in attached project? Composer.json is setup in a way that it doesn't contain#36696 but does contain#36590. I am running it with with |
| privatefunctionisTty():bool | ||
| { | ||
| $inputStream = !$this->inputStream &&\defined('STDIN') ?STDIN :$this->inputStream; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I just discovered that this is enough to fix the warning:$inputStream = null === $this->inputStream || (\defined('STDIN') && STDIN === $this->inputStream) ? fopen('php://input', 'r') : $this->inputStream;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Unfortunately, this trades one error for another. This DOES fix the "bytes of buffered data lost during stream conversion" error. But now, as soon as an answer fails validation, instead of asking again, it ALWAYS fails immediately - it falls out of the loop and hits thethrow at the end
| throw$error; |
Here is a SUPER simple reproduced -https://github.com/weaverryan/console-question-reproducer - I tried the current code on that and your proposed patch. But this PR doesn't fix the issue either - I think this PR sort of fixes it, but then I think we're still hitting#37046.
The only thing that fully works is reverting back to 5.0.8 - you can try all of this pretty quickly with the reproducer.
nicolas-grekas commentedJun 18, 2020
We settled on another way, isn't it? |
ostrolucky commentedJun 18, 2020
yep |
After#36696, I've got report that users get
XX bytes of buffered data lost during stream conversionerrors when validation of their first question fails. This can be seen athttps://travis-ci.org/github/symfony/maker-bundle/jobs/693200010#L332Simple reproducer:
Runnning as:
echo "foo\n" | php console-app.phpThese calls were actually silenced before already (and still are in 3.4), but we removed this later, thinking it's not needed anymore.
I would actually prefer to fix this differently if possible, but I would have to be able to reproduce issue mentioned inhttps://github.com/symfony/symfony/pull/36696/files#r420063328 first. I was unable to reproduce that issue with code similar as inhttps://github.com/symfony/symfony/blob/8ddaa20b29caa7aeb505681f12e86a7a444ae8b4/src/Symfony/Component/Console/Tests/phpt/uses_stdin_as_interactive_input.phpt, which should match scenario that referenced comment describes
cc@weaverryan@nicolas-grekas