PHP_CodeSniffer: STDIN fails on slow input

I noticed that PHP CodeSniffer will fail if receiving the file on STDIN and the input on STDIN is slow.

I discovered this using gulp-phpcs but I’m able to reproduce the issue just by piping the input (and using pv(1) to slow down the pipe).

This might be related to (some of) #993.

Reproducing We’ll use the coding standard from PHP CodeSniffer itself and we’ll also check a file from PHP CodeSniffer. The problem seems to appear at file sizes above 8192 bytes. Reporter.php is ~11.7 kB.

$ wget https://github.com/squizlabs/PHP_CodeSniffer/raw/master/phpcs.xml.dist
$ wget https://github.com/squizlabs/PHP_CodeSniffer/raw/master/src/Reporter.php

Just cat’ing the file on standard in succeeds as expected:

$ cat Reporter.php | phpcs -
.

Time: 147ms; Memory: 4Mb

Making the pipe slow using pv(1) will make the check fail.

$ pv -qL 3k Reporter.php | phpcs -
E


FILE: STDIN
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 12 | ERROR | [x] Expected 1 newline at end of file; 0 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 69ms; Memory: 4Mb

I have reproduced this both on my Macbook and a linux server.

I’m using “PHP_CodeSniffer version 3.0.0 (stable) by Squiz (http://www.squiz.net).” and also tried with current master (5847603911d5b49cf562b9a7368ec0f87a609139) with same result.

About this issue

  • Original URL
  • State: closed
  • Created 7 years ago
  • Reactions: 1
  • Comments: 28 (23 by maintainers)

Commits related to this issue

Most upvoted comments

It’s been a couple of weeks, so I’m going to close this as fixed. Thanks @synthetiv for testing. I’ve also tested this internally with a wrapper script we use for another product and it is working there as well.

I’m running master now (as of a few minutes ago); so far so good! I’ll post here if I start having issues again.

Sounds good to me! I was on vacation last week & wasn’t doing any PHP sniffing, but as of today it’s still working well.

Are more releases planned from the 2.9 branch? If yes I will try to backport my fix from #1490.

Sorry, I missed this.

I’ll only do serious bug fix releases on 2.9, so I wouldn’t merge a core change like this in.

Sure, I’ll try it out later – right now the patched version I’m using is based on PHPCS version 2.9.1, because I mostly use the WordPress coding standards and they aren’t compatible with 3.0 yet.

The patch I’m running now is basically a dumbed-down version of your fix:

diff --git a/CodeSniffer/CLI.php b/CodeSniffer/CLI.php
index 29403cf..ab2a885 100644
--- a/CodeSniffer/CLI.php
+++ b/CodeSniffer/CLI.php
@@ -404,18 +404,11 @@ public function getCommandLineValues()

         // Check for content on STDIN.
         $handle = fopen('php://stdin', 'r');
-        if (stream_set_blocking($handle, false) === true) {
-            $fileContents = '';
-            while (($line = fgets($handle)) !== false) {
-                $fileContents .= $line;
-                usleep(10);
-            }
-
-            stream_set_blocking($handle, true);
-            fclose($handle);
-            if (trim($fileContents) !== '') {
-                $this->values['stdin'] = $fileContents;
-            }
+        stream_set_blocking($handle, true);
+        $fileContents = stream_get_contents($handle);
+        fclose($handle);
+        if (trim($fileContents) !== '') {
+            $this->values['stdin'] = $fileContents;
         }

         return $this->values;

This always waits for STDIN, even if you specify a filename, so it’s not a good general-purpose solution, but it’s working fine for me.

Thank you for writing up this issue and coming up with a fix for it – it was really driving me crazy.

Just want to chime in that I’ve also been running into this issue using Atom and the linter-phpcs package. The common denominator here seems to be Node, so that’s interesting, I guess. Currently I’m working around this issue by using a patched copy of PHPCS.

I can confirm this is an issue, and has come up across several projects (also using gulp-phpcs). Any workaround for 2.x that doesn’t require forking would be much appreciated.

[ Repost from the PR ]

I don’t want people to have to use - just to pipe content into PHPCS.

This was the easy way out I could have taken, but it forces people to have to know an obscure CLI arg to do what I consider pretty basic stuff. Having PHPCS sit and wait for STDIN while someone types content on the screen is a serious edge case, so I added a switch for that.

What I want to do is find a way to, without any special options, have people able to use PHPCS with commands like echo, cat, and xargs for checking content.

The way to do this is possibly to change the behaviour so that STDIN is only ever looked at if PHPCS can’t find anything to check on the command line. This gets more complex because the coding standard itself (normally inside phpcs.xml) might define files and folders to check, so there are times when PHPCS has things to check, but they are all sourced from a ruleset. It would need to wait a short time to see if anything can be found on STDIN before deciding to go ahead and process whatever the ruleset said to process. That “short time” is obviously going to be seen as “lag”, so it would need to be really short.

I’m going to close this PR because this isn’t how I want PHPCS to work. It would be better to have a discussion on the main issue (issue #1472), so I’ll repost my comment there as well.

I’m pretty sure that the only way to allow for slow STDIN is to increase the wait time between reads, which is here: https://github.com/squizlabs/PHP_CodeSniffer/blob/master/src/Config.php#L363

You can try tweaking that value if you want, but I have no idea how high it would need to be for your specific case. If you find a value and it happens to slow down normal STDIN usage, I could make that an option.

Any chance you could try tweaking that value?