Specifying the progress option in phpcs.xml breaks linter

[rabbitfang]

Adding <arg value="p" /> to the phpcs.xml file causes the following error message to display:

Error parsing PHPCS response
Something went wrong attempting to parse the PHPCS output.

[rabbitfang]

When the progress option is specified, the progress meter is printed regardless of the report format specified. A possible fix would be to skip lines output by phpcs until the first line that begins with an {, but that might be a little hack-ish.

[Arcanemagus]

I'm going to mark this as closed only because this really should be handled in phpcs. We are asking it to output JSON, if it is giving anything but valid JSON back to us that is a bug on its part.

If you file an issue over there a link back here would be nice to follow the issue!

[nelson6e65]

Maybe related: It should ignore <file></file> nodes, because throws same error if you include default directories/files in phpcs.xml file, like:

<!-- ... -->
<file>./src</file>
<!-- ... -->

[Arcanemagus]

Looks like it's definitely related... we are asking it to lint text from stdin, if it's not ignoring things like that in the configuration then it's a bug in phpcs 😞.

[nelson6e65]

By the way, using -s ("Show sniff codes in all reports") before -p argument it will not throws this error in linter if there is issues only:

<!--<arg value="p"/>-->
<arg value="sp"/>

If not, the same error is throws.

I tried and I thought I had found a work-around for this, but it was just an illusion. 😞

[weirdan]

@Arcanemagus, use '-q' switch when running the phpcs. It disables progress, while still outputting the report in json format:

-q            Quiet mode; disables progress and verbose output

This option is available since PHPCS 2.6.2
See also #144

[jaredatch]

Can you think of any reason why -q wouldn't run? I'm on v 2.9.1

Normally in my phpcs.xml.dist I have

<arg name="report" value="full"/>
	<arg name="report" value="summary"/>
	<arg name="report" value="source"/>

Only way PHPCS will run without throwing the output error is to comment that out. No big deal, but from my understanding the updates above should squash that.

[Arcanemagus]

@jaredatch Does it work properly when you run it manually on the CLI?

[jaredatch]

Correct, CLI has no issues. Running phpcs --report=json myfile.php generates the expected JSON only output. It overrides the reports defined in my project's config.

When in atom, its acting like --report=json isn't running or applied at all (https://cl.ly/3U3L2c0W3m1G).

[weirdan]

@jaredatch

When in atom, its acting like --report=json isn't running or applied at all

Your screenshot shows that both json and full+summary+source reports are in efffect. json output is above those tables.

Apparently --report argument is added to those defined in phpcs.xml[.dist], instead of overriding. It's also version-dependent:

❯ cat phpcs.xml | grep report
	<arg name="report" value="full"/>

❯ vendor/bin/phpcs -q --report=json < src/Swagger/src/ConfigProvider.php # <<<<<< PHPCS 2.9.1
{"totals":{"errors":1,"warnings":0,"fixable":1},"files":{"STDIN":{"errors":1,"warnings":0,"messages":[{"message":"Expected 0 spaces before opening brace; 8 found","source":"PSR2.Classes.ClassDeclaration.SpaceBeforeBrace","severity":5,"type":"ERROR","line":11,"column":9,"fixable":true}]}}}
FILE: STDIN
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 11 | ERROR | [x] Expected 0 spaces before opening brace; 8 found
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 48ms; Memory: 4Mb

❯ phpcs -q --report=json < src/Swagger/src/ConfigProvider.php # <<<<<< PHPCS 3.0.2
{"totals":{"errors":1,"warnings":0,"fixable":1},"files":{"STDIN":{"errors":1,"warnings":0,"messages":[{"message":"Expected 0 spaces before opening brace; 8 found","source":"PSR2.Classes.ClassDeclaration.SpaceBeforeBrace","severity":5,"type":"ERROR","line":11,"column":9,"fixable":true}]}}}

Correct, CLI has no issues. Running phpcs --report=json myfile.php generates the expected JSON only output. It overrides the reports defined in my project's config.

You might actually be running two different versions of PHPCS. linter-phpcs searches for vendor/bin/phpcs and will use that if found, and shell would use whatever is in $PATH. If that's the case you could use the linter option to force PHPCS executable.

[weirdan]

@Arcanemagus as for overriding those settings for older (pre-3.x) versions, here's what I tried (so you don't have to repeat it yourself):

1. setting --report-full=/dev/null - this doesn't work for neither of full, summary, source
2. setting --report-full (and others) to a temporary file - that didn't work either.

So, what else could be done?

1. Redirecting json report output with --report-json to a temporary file (another fd (like in /proc/fd), socket, etc) - this works, but could be cumbersome to implement (files needs to be cleaned up, sockets needs to be opened, closed and monitored for activity, etc).

   TMPFILE=`tempfile`; vendor/bin/phpcs -q --report=json --report-json=$TMPFILE < src/Swagger/src/ConfigProvider.php >/dev/null 2>&1 ; cat $TMPFILE; unlink $TMPFILE # <<<<<< PHPCS 2.9.1
   {"totals":{"errors":1,"warnings":0,"fixable":1},"files":{"STDIN":{"errors":1,"warnings":0,"messages":[{"message":"Expected 0 spaces before opening brace; 8 found","source":"PSR2.Classes.ClassDeclaration.SpaceBeforeBrace","severity":5,"type":"ERROR","line":11,"column":9,"fixable":true}]}}}
   

2. Stripping out things that are not json on stdout - sounds extremely fragile
3. Inspecting phpcs.xml[.dist] for options that are known to break phpcs integration, and asking users to provide alternative, compatible ruleset file - sounds doable.
4. Same as previous option, but creating that file automatically (might be a temporary file, or semi-permanent, stored in workspace storage (if Atom has that kind of thing)) - might be a bit harder, but offers better user experience.

[jaredatch]

You might actually be running two different versions of PHPCS. linter-phpcs searches for vendor/bin/phpcs and will use that if found, and shell would use whatever is in $PATH. If that's the case you could use the linter option to force PHPCS executable.

In my settings I have the executable path explicitly defined and the Search for executables option unchecked, so I don't think thats at play here (but could certainly be wrong).

The screenshot does indeed show the json and full+summary+source being output, the later of course is what breaks things.

If I run phpcs --report=json foo.php, the phpcs config is still used, however the report parameter correctly overrides the arguments in the config, and only json is returned. I've confirmed I'm using the same phpcs as defined in package config via which phpcs.

So what is weird is that if I specify the report via CLI it does indeed override the other items defined in the config, but that's not what is happening when it gets triggered inside Atom.

[weirdan]

@jaredatch can you click that main.js [sm] link in the console, set the breakpoint on that line (click the line number), then edit the file and, once debugger kicks in, show what locals (should be shown to the right of the extension source code) look like (be sure to expand parameters variable)?
@Arcanemagus it probably makes sense to add this information to debug output.

[Arcanemagus]

@jaredatch I'm betting that the difference is due to you passing the filename in directly, while we pas the file contents in via stdin. If you run cat foo.php | phpcs --report=json --stdin-path=foo.php - does it still work?

---

@weirdan Thanks for the investigation in #95 (comment). This is quite the mess...

Option 2 definitely sounds fragile to me, I'd rather avoid that route if possible.
Option 3 I'd like to avoid as well, since the entire goal of this is that it should be the same as running phpcs from the CLI.

Options 1 seems like it is viable, if extremely annoying to implement.

Option 4 could work, but I worry about:

• Issues of placement of this new config file, it might need to be in the project directory?
• Potential bugs in phpcs's ability to specify a configuration file override.

@Arcanemagus it probably makes sense to add this information to debug output.

The full output received should be in the console, nobody ever seems to check there though 😛.

[weirdan]

The full output received should be in the console, nobody ever seems to check there though.

Received, yes, it's there, but I was talking about the 'command line' executed (executable + parameters + exec options actually). That could be useful to reproduce things in CLI, to debug version-dependent arguments passing, etc.

Issues of placement of this new config file, it might need to be in the project directory?

Only if PHPCS uses dirname($xmlPath) for something like computing absolute paths for include patterns.

There's ~/.atom folder on my machine, which contains atom-specific things (the path could be obtained from atom.getConfigDirPath()). phpcs configs for atom-specific extension sounds like a good fit. You'd use something like ~/.atom/linter-phpcs/${encodeUri(originalXmlPath)}.xml to allow multiple processed xmls to be stored.

Potential bugs in phpcs's ability to specify a configuration file override.

Then people would complain about linter-phpcs.codeStandardOrConfigFile setting not working - did they?

Option 3 I'd like to avoid as well, since the entire goal of this is that it should be the same as running phpcs from the CLI.

Isn't this extension long past the point where it was true? I've just checked - linter-phpcs effectively runs this with my phpcs version:

cd /home/weirdan/src/project/
vendor/bin/phpcs \ 
  --report=json \
  --encoding=UTF-8 \
  --standard=/home/weirdan/src/project/phpcs.xml \
  --warning-severity=1 \
  -s \
  --stdin-path=/home/weirdan/src/project/src/path/to/file.php \
  < /home/weirdan/src/project/src/path/to/file.php

— far from what I would run:

cd /home/weirdan/src/project/
phpcs

[Arcanemagus]

Only if PHPCS uses dirname($xmlPath) for something like computing absolute paths for include patterns.

I'm pretty sure config files can reference custom rules with relative paths, meaning the config will likely have to be in the same directory as the current config.

---

Then people would complain about linter-phpcs.codeStandardOrConfigFile setting not working - did they?

Ha, when I wrote that I was just assuming that like most other options that was broken at some point in PHPCS's past, looks like we handle that the same for all versions so we are probably okay here.

far from what I would run

Maybe, but it's certainly possible to run it like that.

Note that --report=json, --encoding=UTF-8, and --stdin-path are "interal" to get JSON output, and because we are passing in the content via STDIN (you missed the trailing - option btw).
--warning-severity is actually the default value, the logic around that could be improved to just not send it if so.
-s is to show the source, so if you wanted that you would need to be specifying it on the CLI as well.

That just leaves --standard, which some versions of PHPCS didn't find automatically, while for most the one we specify is the same it would find automatically.

So although you may not type all that out manually, the only major differences there are --report=json and -s.