Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Console] don't check tty on stdin, it breaks with "data lost during stream conversion" #36696

Merged
merged 1 commit into from May 5, 2020

Conversation

nicolas-grekas
Copy link
Member

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

/cc @ostrolucky FYI

@nicolas-grekas nicolas-grekas added this to the 4.4 milestone May 5, 2020
@nicolas-grekas nicolas-grekas changed the title [Console] fix "stream_isatty(): 52 bytes of buffered data lost during stream conversion" [Console] dont check tty on stdin, it break with "data lost during stream conversion" May 5, 2020
@nicolas-grekas nicolas-grekas changed the title [Console] dont check tty on stdin, it break with "data lost during stream conversion" [Console] don't check tty on stdin, it breaks with "data lost during stream conversion" May 5, 2020
@ostrolucky
Copy link
Contributor

What's the reproduction scenario for this? There is an phpt integration test using stdin and question helper, I'm wondering why it didn't fail. Code I used is inspired by former code in Application class and there was no sapi_windows_vt100_support used there. It was also passing stream to those functions disregarding if output is instance of streaminterface

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented May 5, 2020

echo "$several_answers" | the-command

@fabpot
Copy link
Member

fabpot commented May 5, 2020

Thank you @nicolas-grekas.

@fabpot fabpot merged commit cf6e499 into symfony:4.4 May 5, 2020
@fabpot fabpot deleted the console-q branch May 5, 2020 11:57
@@ -477,6 +473,8 @@ private function validateAttempts(callable $interviewer, OutputInterface $output
throw $e;
} catch (\Exception $error) {
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was the motivation to move this inside a loop because isTty() result may change between iterations? I wonder how can that happen

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

echo "$several_answers" | the-command breaks in real life, with stream_isatty() emitting PHP warnings "data lost during stream conversion".

Moving after reading the response means we never hit this line when correct responses are provided. Which is the case in the described situation (programmatic filling of answers.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants