phpcs-only attribute is not always respected

[fredden]

Describe the bug

When using the phpcs-only or phpcbf-only attributes within a ruleset, the reports from phpcs do not look as expected.

Code sample

<?php




function main() {
  return 1;
  return 2;
}

Custom ruleset

<?xml version="1.0"?>
<ruleset>
    <file>test.php</file>

    <arg name="basepath" value="."/>
    <arg name="colors"/>
    <arg value="s"/>

    <rule ref="Squiz.WhiteSpace.FunctionSpacing" phpcs-only="true"/>

    <rule ref="Squiz.PHP.NonExecutableCode" />
</ruleset>

To reproduce

Steps to reproduce the behavior:

1. Create a file called test.php with the code sample above.
2. Create a file called phpcs.xml with the custom ruleset above.
3. Run phpcs
4. See the output displayed

FILE: test.php
--------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
--------------------------------------------------------------------------------------------------------------------------
6 | ERROR   | [x] Expected 2 blank lines before function; 4 found (Squiz.WhiteSpace.FunctionSpacing.Before)
8 | WARNING | [ ] Code after the RETURN statement on line 7 cannot be executed (Squiz.PHP.NonExecutableCode.Unreachable)
--------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------------------------------------------------

Time: 18ms; Memory: 6MB

5. Run phpcbf
6. See the output displayed

No fixable errors were found

Time: 17ms; Memory: 6MB

7. Note the discrepancy between output from phpcs and phpcbf regarding "fixable" error.

Expected behaviour

The errors reported by phpcs as fixable should be fixed by phpcbf.

Versions (please complete the following information)

Operating System	Debian Stable
PHP version	8.3
PHP_CodeSniffer version	3.10.2 (also tested with a3d11a9 which is the current master branch at time of writing)
Standard	custom
Install type	Composer (global for v3.10.2, local for master branch)

Additional context

There are some words describing how the phpcs-only and phpcbf-only attributes should work here: https://github.com/PHPCSStandards/PHP_CodeSniffer/wiki/Annotated-Ruleset#selectively-applying-rules

Please confirm

• I have searched the issue list and am not opening a duplicate issue.
• I have read the Contribution Guidelines and this is not a support question.
• I confirm that this bug is a bug in PHP_CodeSniffer and not in one of the external standards.
• I have verified the issue still exists in the master branch of PHP_CodeSniffer.

[jrfnl]

@fredden Thanks for opening this issue. I agree this is confusing, but I don't think the functionality is incorrectly handled - phpcs-only is being respected.

I think the issue is more a UI one and that a phpcs run should not mark an error/warning as fixable (via the X) when the standard/category/sniff/error code was flagged to be phpcs-only.

Do you agree ?

[jrfnl]

Been thinking about this one some more now that I've studied the Ruleset class more closely while writing the tests.

The problem is that with the current architecture, the Ruleset is deterministic. What I mean by that is that it only contains the end-result of the rules in the XML file(s), not the decision information which determined what the end-result should look like.

Or to put it in more practical terms: the Ruleset doesn't store whether a sniff was loaded because it had the phpcs-only flag set.
Additionally, the shouldProcessElement() method used to determine whether to process a rule is agnostic to the XML element on which it is used and, again, is deterministic (bool return value).

The only way I can think of to solve the issue described above, would be to introduce logic to determine for rule elements (only) whether the phpcs-only attribute is being applied AND PHPCS is in CS mode and then to store those sniff codes/error codes in a new (private) property with a getter method which can be passed an error code and can then check if that error code aligns with any of the stored "CS mode only" codes.
This getter could then be called in the File::addMessage() and update the $fixable flag before it is used in the method.

What do you think ?

[fredden]

I discussed this somewhat with @jrfnl on a call today. It would be good to understand how these two attributes are used in the wild, as there may be scope for removing them completely if they aren't being used, or replacing them with something more suited to the use-cases identified. Next steps for this would be to do a search of public code using these attributes, and understand why they were introduced to PHP_CodeSniffer in the first place.