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] block input stream if needed #50429

Merged
merged 1 commit into from
May 25, 2023

Conversation

joelwurtz
Copy link
Contributor

@joelwurtz joelwurtz commented May 25, 2023

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? n
Tickets /
License MIT
Doc PR /

When the input stream used in the question helper is not blocking, the default value is always used as the stream return false instead of waiting. In order to fix that, we force the stream to be in blocking state and go back to the old state after so other logic is not impacted by this change.

I don't think making a test for that is possible without writing a lot of codes, as we need to have a stream that is not ready for this test, and make it ready later, given there is no async possible. it could be done with a fork or another process writing to a file after the question was asked so we do not block following tests when there is no failure, however i'm not sure if this is wanted ?.
If you have a clean way to make a reproducible test for this bug i'm open to write it.

@carsonbot
Copy link

Hey!

Thanks for your PR. You are targeting branch "5.4" but it seems your PR description refers to branch "5.4,".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

Copy link
Member

@lyrixx lyrixx left a comment

Choose a reason for hiding this comment

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

Good catch. Thanks 👍🏼

When the input stream used in the question helper is not blocking, the default value is always used as the stream return false.
In order to fix that, we force the stream to be in blocking state and go back to the old state after so other logic is not impacted
by this change
Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

Works for me as-is

@nicolas-grekas
Copy link
Member

Good catch, thanks @joelwurtz.

@nicolas-grekas nicolas-grekas merged commit 1263180 into symfony:5.4 May 25, 2023
10 of 11 checks passed
@joelwurtz joelwurtz deleted the fix/input-not-blocked branch May 25, 2023 15:52
This was referenced May 27, 2023
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

5 participants