The problem with this diff is that it actually breaks some functionality within phpcs ruleset, e.g. it no longer can follow exclude-pattern setting for specific standards. Maybe it would make sense to use configuration to define should phpcs be used with stdin, or not? Currently the only workaround I see for this is setting every rule in .arclint with different flags instead of using usual ruleset.xml, but that would be a mess.

I completely agree with @aurelijus. @epriestley please revert this change.

Now the linter does also fail when you have an empty php file. Instead of showing the correct message

arc lint foo.php
>>> Lint for foo.php:


   Warning  (PHPCS.W.Internal.NoCodeFound)
    No PHP code was found in this file and short open tags are not allowed by
    this install of PHP. This file may be using short open tags but PHP does
    not allow them.

    >>>        1

it shows

arc lint foo.php
Exception
Some linters failed:
    - ArcanistPhpcsLinter: CommandException: Command failed with error #2!
      COMMAND
      'vendor/bin/phpcs' '--report=xml' '--standard=PSR2'

      STDOUT
      ERROR: You must supply at least one file or directory to process.

      Usage: phpcs [-nwlsaepvi] [-d key[=value]] [--colors] [--no-colors]
          [--report=<report>] [--report-file=<reportFile>] [--report-<report>=<reportFile>] ...
          [--report-width=<reportWidth>] [--generator=<generator>] [--tab-width=<tabWidth>]
          [--severity=<severity>] [--error-severity=<severity>] [--warning-severity=<severity>]
          [--runtime-set key value] [--config-set key value] [--config-delete key] [--config-show]
          [--standard=<standard>] [--sniffs=<sniffs>] [--encoding=<encoding>]
          [--extensions=<extensions>] [--ignore=<patterns>] <file> ...
                            Set runtime value (see --config-set)
              -n            Do not print warnings (shortcut for --warning-severity=0)
              -w            Print both warnings and errors (this is the default)
              -l            Local directory only, no recursion
              -s            Show sniff codes in all reports
              -a            Run interactively
         ... (2,049 more bytes) ...

      STDERR
      (empty)
(Run with `--trace` for a full exception trace.)

In D9034#68749, @aurelijus wrote:

The problem with this diff is that it actually breaks some functionality within phpcs ruleset, e.g. it no longer can follow exclude-pattern setting for specific standards. Maybe it would make sense to use configuration to define should phpcs be used with stdin, or not? Currently the only workaround I see for this is setting every rule in .arclint with different flags instead of using usual ruleset.xml, but that would be a mess.

Sorry about this... if you have a ticket I can fix it. I'm not entirely sure how to replicate it at the moment.

Fix is reverting this commit. As noted above when STDIN not used the rulesets which require knowledge of checked filename will be working again.

In D9034#109839, @aik099 wrote:

Fix is reverting this commit. As noted above when STDIN not used the rulesets which require knowledge of checked filename will be working again.

It seems that it is also possible to prepend the file contents with phpcs_input_file: $path\n, but I'm not sure which approach is more preferable.

What I don't know is why STDIN support was enabled for this linter. If it was enabled just because PHPCS supports STDIN, then surely commit must be reverted because in STDIN mode PHPCS capabilities are limited compared to mode, when you provide file path to it.

This change has made it apparently impossible to write a PHPCS Lint check that accesses the filename (as it now always appears to be STDIN).

See T7674.

In D9034#131628, @epriestley wrote:

See T7674.

From that I can get that making PHPCS accept input over STDIN was made to satisfy hooks at some point. But since commit hooks are now removed then we can change that sniff behavior to accept data from files as it should.

Ah, I've totally missed that ArcanistExternalLinter::supportsReadDataFromStdin method now returns false by default instead of true and it should work as expected.