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

Fix lint commands frozen on empty stdin #33523

Open
wants to merge 1 commit into
base: 4.4
from

Conversation

@chalasr
Copy link
Member

commented Sep 9, 2019

Q A
Branch? 4.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

Running e.g. lint:yaml - with no piped content makes the command hangs currently, this makes it fail instead. Also fixes the command help which we forgot to update

@chalasr chalasr added this to the next milestone Sep 9, 2019

@chalasr chalasr force-pushed the chalasr:lint-layer branch 2 times, most recently from 8daae93 to 06702ed Sep 9, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

This is wrong, hanging is normal behavior, the command waits for EOF, which is CTRL+D. That's exactly the same with any Unix command that reads from stdin.

The fact it works is relying on a race condition. Don't be fooled.

For 4.4 only, master shouldn't rely on ftell()

@chalasr

This comment has been minimized.

Copy link
Member Author

commented Sep 9, 2019

Sure, but it's a behavior change which might give false positives in a CI. Take e.g:

cat unexisting | bin/console lint:yaml

On 4.3 the output is an error message and exit code is 1. On 4.4-dev it succeeds with code 0.

@nicolas-grekas
Copy link
Member

left a comment

Oh I thought you submitted that again master. For 4.4 it works for me!

src/Symfony/Bridge/Twig/Command/LintCommand.php Outdated Show resolved Hide resolved

@chalasr chalasr force-pushed the chalasr:lint-layer branch 4 times, most recently from 5f1ac9d to 6472304 Sep 10, 2019

@yceruto
Copy link
Member

left a comment

Much better imo, thanks!

@chalasr chalasr force-pushed the chalasr:lint-layer branch from 6472304 to fd83289 Sep 11, 2019

@chalasr chalasr force-pushed the chalasr:lint-layer branch from fd83289 to b60e0c1 Sep 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.