squizlabs / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
10.66k stars 1.48k forks source link

phpcs is giving errors while phpcbf is saying everything is fine #2303

Closed linaori closed 5 years ago

linaori commented 5 years ago

Output from phpcs: PHPCBF CAN FIX THE 87 MARKED SNIFF VIOLATIONS AUTOMATICALLY (note that this is taken from 1 of the files, there are more files with violations).

Output from phpcbf: No fixable errors were found

When running phpcbf for the whole file, it works and it will try to fix the errors inside. I've removed the cache file inbetween but no luck.

<?xml version="1.0"?>
<ruleset>
    <description>The coding standard for SRS.</description>

    <arg name="basepath" value="."/>
    <arg name="extensions" value="php"/>
    <arg name="parallel" value="80"/>
    <arg name="cache" value=".phpcs-cache"/>
    <arg name="colors"/>

    <!-- Ignore warnings, show progress of the run and show sniff names -->
    <arg value="nps"/>

    <!-- Directories to be checked -->
    <file>src</file>

    <!-- Include full Doctrine Coding Standard -->
    <rule ref="Doctrine"/>
</ruleset>
gsherwood commented 5 years ago

This will be impossible to replicate without having a sample file that I can use to replicate the issue. Are you able to provide a small code snippet where running PHPCS on that file shows fixable errors but running PHPCBF does not? I'd also need to see the actual PHPCS report with error codes (--report=full -s) to see if the fixable errors are from core sniffs or Doctrine sniffs.

linaori commented 5 years ago

I currently do not. However, trying to fix it manually on the big file, did result in segfaults and I noticed that one of the sniffs created code that was not executable by PHP. As they are pretty big classes (that desperately need cleaning) I'll have to make some time to see if I can reproduce the issue. I'm suspecting it's related to nesting levels as well as the files would receive an "unable to fix" at times.

I also notice that in a lot of cases phpcbf won't be able to fix anything, but doesn't tell you this. phpcs will spew hundreds of (unfixable by phpcbf) errors, and then phpcbf says it's all fine. I think it would be nice if phpcbf would indicate that there are unfixable errors in that file. Should I open a feature request for this?

gsherwood commented 5 years ago

I think it would be nice if phpcbf would indicate that there are unfixable errors in that file. Should I open a feature request for this?

If you want to. It's not a priority of PHPCBF to show counts and lists of unfixable errors as this is what PHPCS does, so I'm not sure I'd put that on the roadmap for PHPCBF, but it doesn't hurt to open an issue and see if more support pops up for it.

linaori commented 5 years ago

I'll open it just in case. Regarding my other issue, I have not encountered it again nor do I have time to fully research it at the moment. I can close this issue and re-open (or create a new one) if I encounter it again. If you wish to leave it open, that's also fine by me

linaori commented 5 years ago

When making my other issue, I was suggested a similar issue by github (awesome feature!): #1518 this could be related to one of my issues where running it didn't fix anything. Sadly phpcbf only reports that it failed fixing things, would there be a possibility to show which sniffs failed at which lines? Or there an option to show this? I've tried running with verbosity, but wasn't giving me information I found of use.

gsherwood commented 5 years ago

would there be a possibility to show which sniffs failed at which lines?

No, it has no idea what has gone wrong. PHPCBF just tries to fix a file up to 50 times and if, after the 50th time, there are still fixable errors being reported, then it gives up and says the file can't be fixed.

PHPCS is able to give you quite good debug information about this using --report=diff -vv (or -vvv if you want to see how the file looks after each fix) but the output is massive and should only be used for debugging.

jrfnl commented 5 years ago

Hopefully not happening here, but just to keep in mind - I came across a few sniffs recently in an external repo which used the addFixableError() method (which is how errors/warnings are marked as fixable), but didn't actually contain fixers.... In that case, that's a clear bug in the sniffs and should be reported to the developers of the external standard with the advice to use addError()/addWarning() instead when the sniff is not capable of fixing things.

linaori commented 5 years ago

Thanks, that should be a good pointer next time I encounter this!