Incomplete <exclude> rule causes following rules not to be executed

codemasher commented 1 month ago

Describe the bug

Hey, I just ran into an issue where an incomplete <exclude> rule (a copy/paste leftover) caused following rules in the phpcs.xml not to be executed.

Code sample

I don't have a simple code sample at this time.

Custom ruleset

See https://github.com/chillerlan/php-qrcode/blob/da5bdb82c8755f54de112b271b402aaa8df53269/phpcs.xml.dist#L38

<?xml version="1.0"?>
<ruleset name="My Custom Standard">
    <rule ref="Generic">
        <!-- exclude some rules -->
        <exclude name="Generic.PHP.UpperCaseConstant" />
        <exclude name="Generic.WhiteSpace.DisallowTabIndent" />

        <!-- here's the copy/paste leftover -->
        <exclude name="Generic." />
    </rule>

    <!-- some more rules, rulesets, etc. -->
    <rule ref="PEAR">
        <exclude name="PEAR.Classes" />
    </rule>

</ruleset>

To reproduce

Steps to reproduce the behaviour:

Clone chillerlan/php-qrcode, checkout at tag 5.0.2 (commit da5bdb82c8755f54de112b271b402aaa8df53269) and run composer install
Run phpcs
There will be ca 6 errors reported
Now remove the aforementioned superfluous line
Run phpcs again
The reported errors are now a couple hundred

Expected behaviour

An incomplete or invalid rule should be ignored and error detection should resume normally (maybe emitting a message hinting at the invalid rule), meaning for the above example that the number of reported errors should always be a couple hundred.

Versions (please complete the following information)


Operating System	Windows 10
PHP version	8.3
PHP_CodeSniffer version	3.10.2
Standard	custom (see phpcs.xml.dist)
Install type	Composer

Additional context

The result with the above custom standard will likely be similar in other projects (may save the hassle to clone that specific project).

In the latest version of my library I have completely changed the phpcs.xml to include select rules, rather than using exclude blocks.

Please confirm

[x] I have searched the issue list and am not opening a duplicate issue.
[x] I have read the Contribution Guidelines and this is not a support question.
[x] I confirm that this bug is a bug in PHP_CodeSniffer and not in one of the external standards. (no external standards were used)
[x] I have verified the issue still exists in the master branch of PHP_CodeSniffer. (the latest version was used, main branch is 1 commit ahead as of now)

jrfnl commented 1 month ago

@codemasher Thanks for reporting this, but in my opinion this is not a bug and everything is working as expected.

You can exclude individual error codes, sniffs, categories and even complete standards. In this case, a complete standard is being excluded. The stray . at the end should be tolerated by PHPCS and should not be a reason for PHPCS to change its behaviour.

Excluding Generic from Generic is of course not that useful, as in that case the whole block could be removed, but that's not a concern for the engine itself.

Also see the Annotated ruleset documentation.

<!--
    You can even exclude a whole standard. This example includes
    all sniffs from the Squiz standard, but excludes any that come
    from the Generic standard.
 -->
 <rule ref="Squiz">
  <exclude name="Generic"/>
 </rule>

jrfnl commented 1 month ago

In the latest version of my library I have completely changed the phpcs.xml to include select rules, rather than using exclude blocks.

That's up to you, but it will increase your maintenance burden if you want to use predefined standards as you'll need to keep up with any changes in those standards yourself.

codemasher commented 1 month ago

Hey, thank you for the quick reply!

Excluding Generic from Generic is of course not that useful, as in that case the whole block could be removed, but that's not a concern for the engine itself.

Thanks for clearing that up! What I wonder now is: does that also exclude rules from the other standards that follow the mentioned block that inherit from Generic? That would explain the near zero errors then.

That's up to you, but it will increase your maintenance burden if you want to use predefined standards as you'll need to keep up with any changes in those standards yourself.

Yes, I am aware of that. I have not yet found and ideal route and decided how to proceed in the future. (I'm opposing most of the PSR/PER coding standards, so I have to run custom config)

jrfnl commented 1 month ago

Excluding Generic from Generic is of course not that useful, as in that case the whole block could be removed, but that's not a concern for the engine itself.

Thanks for clearing that up! What I wonder now is: does that also exclude rules from the other standards that follow the mentioned block that inherit from Generic? That would explain the near zero errors then.

Excludes work for the complete ruleset, as otherwise, if you'd include multiple rulesets which include the same sniff and would only exclude that particular sniff from one, you'd still get the rule included, which would then be reported as a bug here again ;-)

You can always check what is included by running phpcs -e (or phpcs --standard=... -e). -e "explains" the ruleset.

If you do that without the errant exclude on the example ruleset you gave above, you would see that the ruleset includes 98 sniffs, 80 from Generic, 17 from PEAR and 1 from Squiz.

With the errant exclude included in the file, this changes to 18 sniffs, 17 from PEAR and 1 from Squiz. (and none from Generic)

That's up to you, but it will increase your maintenance burden if you want to use predefined standards as you'll need to keep up with any changes in those standards yourself.

Yes, I am aware of that. I have not yet found and ideal route and decided how to proceed in the future. (I'm opposing most of the PSR/PER coding standards, so I have to run custom config)

I feel your pain ;-)

codemasher commented 1 month ago

Ok that clears up the confusion! (you can probably imagine my shock when I found that superfluous line and ran phpcs again...)

I feel your pain ;-)

Haha thank you! It's only gotten worse since the PHP 8.4 alpha came out and the abandonment of phan, which is why I started revamping the phpcs config in first place...

Anyway, I think this can be closed then. Again, thank you for your time and work!

jrfnl commented 1 month ago

Haha thank you! It's only gotten worse since the PHP 8.4 alpha came out and the abandonment of phan, which is why I started revamping the phpcs config in first place...

What with phan being abandoned and revamping the PHPCS config, you may want to consider adding PHPCompatibility into the mix as well ;-) (Pro-tip: use dev-develop for now until v 10 has been released)

codemasher commented 1 month ago

What with phan being abandoned and revamping the PHPCS config, you may want to consider adding PHPCompatibility into the mix as well ;-) (Pro-tip: use dev-develop for now until v 10 has been released)

Oh! Thanks for the recommendation, I will definitely look into this!

PHPCSStandards / PHP_CodeSniffer