Skip to content

Excluding a class of rules via <exclude name="xxx"/>, inside a ruleset ... globally excludes them ... #1081

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
EliW opened this issue May 6, 2025 · 4 comments

Comments

@EliW
Copy link

EliW commented May 6, 2025

Describe the bug

I have been attempting to use the exclude functionality as shown in the documentation here:
https://github.com/PHPCSStandards/PHP_CodeSniffer/wiki/Annotated-Ruleset

Wherein I exclude a class of rules inside of the section that is pulling in a ruleset. Specifically the part of documentation/example here:

<!--
    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>

As it says, it will 'include all of Squiz but remove any from Squiz that come from Generic"

This is not working for me, as (for above case), it would be completely removing ALL Generic.* rules, even if included by another ruleset. Example below:

Custom ruleset

<?xml version="1.0"?>
<ruleset name="My Custom Standard">
	<description>Coding Standards</description>

	<rule ref="WordPress-Extra">
		<!-- Include the WordPress-Extra ruleset (from a separate package, not important here though) -->
		<exclude name="PSR2"/>
	</rule>
	<rule ref="PSR12">
		<!-- Right, now declare PSR-12 please -->
	</rule>

</ruleset>

To reproduce

  1. Create a standard similar to above.
  2. Issue a phpcs -s -e
  3. See that all PSR2 rules are removed, even if added by PSR12

Expected behavior

Only declarations for PSR2 rules that exist in 'WordPress-Extra' and it's sub-rule-sets it includes ... would be excluded. But since later PSR12 is added in as another ruleset to include enmass ... PSR2 and PSR1 rules, as configured by the PSR12 file(s) ... would apply ... and therefore a -s -e would show them.

Right now, since you exclude them in the 'WordPress-Extra' context ... they are completely excluded, even if added in by PSR12 ruleset later.

Versions (please complete the following information)

Operating System MacOS 15.4.1
PHP version 8.1
PHP_CodeSniffer version 3.12.2
Standard doesn't matter
Install type Composer (local)

Additional context

n/a

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.
  • [x ] I have verified the issue still exists in the master branch of PHP_CodeSniffer.
@jrfnl
Copy link
Member

jrfnl commented May 6, 2025

This is actually the way the ruleset reading was designed and changing this would be a huge breaking change.

I do agree it can be counter-intuitive and I'd like to have a think at some point about how this experience can be improved, but that will have to wait for a future major release.

In the mean time, changing the <exclude name="PSR2"/> to <exclude name="PSR2.Namespaces.NamespaceDeclaration"/> should get you the desired result anyway.

WordPress-Extra and PSR12 include nearly the same sniffs from PSR2, with two exceptions, the PSR2.Namespaces.NamespaceDeclaration being included by WP Extra, but not by PSR12 and PSR2.Methods.FunctionCallSignature being included by PSR12 and not by WP Extra.
So, excluding the PSR2.Namespaces.NamespaceDeclaration sniff should get things working as you intended.

@jrfnl
Copy link
Member

jrfnl commented May 6, 2025

Oh.. one caveats and my suggestion above actually solves that problem too - using <exclude name="PSR2"/> will read the whole PSR2 ruleset and exclude everything from that ruleset, not just the PSR2 sniffs, but also all other sniffs PSR2 included.
So, using a <exclude name="CompleteStandard"/> directive is generally not a good idea.

@EliW
Copy link
Author

EliW commented May 7, 2025

@jrfnl Thanks for the reply! I agree completely that excluding an entire ruleset is ... problematic. Might I suggest that at least the documentation page (really 'example page' vs real docs) be updated?

It implies that what I did, based upon what it says, would work. IE using the logic:

I want the 'standard' WordPress-Extras ruleset as default configured. But don't include any PSR2 rules that it tries to apply inside of it.

Because I then want to apply PSR12 rules, which include PSR2 rules. And I want 'those defaults' to apply.

(What I posted was a much simpler example obviously. My real file was basically doing a bunch of WP rule exclusions, and then also realizing that the WP ruleset, added a bunch of universal/etc rules that I didn't want. And also-also that it added PSR2 rules, for example, and CONFIGURED THEM to be differently config than PSR12 does ... For example it does:

	<rule ref="PSR2.Classes.PropertyDeclaration.Underscore">
		<severity>5</severity>
	</rule>

And I wanted the 'PSR12 standard' version of that to be in place at default severity.

In my own case, I solved this by instead of trying to be cute and saying "include this ruleset, but ignore all 'these parts of it'" ... I just figured out what the parts I wanted were used -s -e to make a list, and copy-pasted that in and included JUST those rules.

So it works. But, IMO, the docs are claiming something that isn't true.

Actually, as written/functional. Shouldn't the not be allowed 'inside' of a ruleset anyway?

IE: if an exclude is going to exclude that rule from all included later rulesets. Why not move the exclude 'outside' of the rules and even not allow it inside, since it's truly global?

#2cents ...

Regardless, thank you again for the response.

@jrfnl
Copy link
Member

jrfnl commented May 8, 2025

@jrfnl Thanks for the reply! I agree completely that excluding an entire ruleset is ... problematic. Might I suggest that at least the documentation page (really 'example page' vs real docs) be updated?

I took a look to do just that and I'm a bit in doubt how to update the page as for the particular example used on the page, the problem doesn't apply.

This is the snippet from the "Annotated Ruleset" page I'm looking at:

<!--
    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>

The things, Generic isn't a really a standard, but more of a "sniff collection", which also means it doesn't include sniffs from other standards, nor makes customizations in the ruleset, so in this case, the exclude should work as expected.

That also highlights, in part, why changing the Ruleset behaviour for when complete standards are excluded is going to be a challenge: it touches on a fundamental design principle in PHP_CodeSniffer: every sniff needs to be part of a standard, but the rulesets for standards are also used to include other rules/make sniff customizations etc.

This is not an issue when sniff libraries are set up as sniff collections with a basically empty ruleset.xml file (like the PHPCS native Generic, all the PHPCSExtra rulesets, the SlevomatCodingStandard or PHPCompatibility), but it is an issue for every single standard which provides its own sniffs AND has a detailed ruleset.xml file pulling in sniffs from other standards and making customizations.

The problem also only really comes into play when using an <exclude name="StandardName"/> directive.

I seem to remember support for this was added in PHPCS 3.0.0 and, to be honest, I'm kind of tempted to prohibit complete standard excludes to bypass the problem, rather than to "solve" it as there will be so many different opinions on what "solving" this should look like.
Some people will find the current behaviour desirable, others would expect just the sniffs starting with StandardName to be excluded, yet others would expect it to work based on the nesting etc.

It's something I've been thinking about for a while already, but I haven't been able to come up with an elegant solution which would not lead to tons of support requests (yet).

There is also another issue with the rulesets which I find counter-intuitive: a previous <exclude ...>s (possibly from an included external standards) cannot be overruled by a new include <rule ref...> and even including the specific error code and setting the <severity>5</severity> to re-enable the rule doesn't always work either.

All in all, don't expect a solution any time soon - 5.0 at the earliest as any change in ruleset interpretation must go into a new major.
Sorry about that, but there's a lot of legacy decisions to content with, very few tests for the PHPCS framework and a very large userbase, so these kind of changes are not that straight-forward to make.

For now, if you have any suggestions on how the docs can be improved for the current situation, let me know and I'll update the wiki.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants