Page MenuHomePhabricator

PHPCS supports reading from stdin.
ClosedPublic

Authored by joshuaspence on May 10 2014, 4:05 AM.

Details

Summary

PHPCS does actually support reading data from stdin, so let's make ArcanistExternalLinter aware of this.

Test Plan

arc unit.

Diff Detail

Repository
rARC Arcanist
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

joshuaspence retitled this revision from to PHPCS supports reading from stdin..
joshuaspence updated this object.
joshuaspence edited the test plan for this revision. (Show Details)
joshuaspence added a reviewer: epriestley.
epriestley accepted this revision.May 10 2014, 8:50 AM
epriestley edited edge metadata.
This revision is now accepted and ready to land.May 10 2014, 8:50 AM
epriestley closed this revision.May 10 2014, 8:50 AM
epriestley updated this revision to Diff 21467.

Closed by commit rARC3d5aa332fe6d (authored by @joshuaspence, committed by @epriestley).

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.

aik099 added a subscriber: aik099.Dec 5 2014, 9:13 AM

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.)

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.

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.

aik099 added a comment.EditedJan 29 2015, 9:12 AM

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.

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.

aik099 added a comment.EditedMay 13 2015, 2:26 PM

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