Enhanced support for nested rulesets

[shvlv]

Is your feature request related to a problem?

The use use-case we are trying to implement is described here - inpsyde/php-coding-standards#81.

In short, we want to introduce template-specific sniffs. It's easy to do with the include-pattern property:

<rule ref="InpsydeTemplates">
    <include-pattern>*/templates/*</include-pattern>
</rule>

But during the discussion, we decided to make InpsydeTemplates extend the existing Inpsyde standard but several sniffs. So the InpsydeTemplates config is:

<rule ref="Inpsyde">
        <exclude name="Inpsyde.CodeQuality.NoElse" />
</rule>

But it seems it's not possible to do what we want to provide proper setup at the consuming package. There are several options:

Option 1

<rule ref="Inpsyde">
        <exclude-pattern>*/templates/*</exclude-pattern>
</rule>

<rule ref="InpsydeTemplates">
     <include-pattern>*/templates/*</include-pattern>
</rule>

It does not work because no Inpsyde specific and imported sniffs are applied to templates directory.

Option 2

<rule ref="Inpsyde" />

<rule ref="Inpsyde">
    <exclude-pattern>*/templates/*</exclude-pattern>
</rule>

<rule ref="InpsydeTemplates">
    <include-pattern>*/templates/*</include-pattern>
</rule>

Imported Inpsyde rules are applied to templates but custom sniffs not.

Option 3

<rule ref="Inpsyde" />

<rule ref="InpsydeTemplates">
    <include-pattern>*/templates/*</include-pattern>
</rule>

It's very close to what we need. The single problem is Inpsyde.CodeQuality.NoElse is still applied to templates what we want to avoid.

Option 3a

The consumer config is the same as above.

The InpsydeTemplates config is:

<rule ref="Inpsyde" />
<rule ref="Inpsyde.CodeQuality.NoElse">
    <severity>0</severity>
</rule>

Inpsyde.CodeQuality.NoElse rule is disabled for whole package.

Describe the solution you'd like

Make the include-pattern (and maybe exclude-pattern) directive more sophisticated. We can use include-pattern to exclude/include sniffs or to define different sniff severity per-directory (regexp).

Additional context (optional)

I briefly checked the PHPCS codebase and saw it's impossible now and may require much effort. After processing all rulesets, we get independent maps of include/ignore patterns and severity per sniff. If so, maybe we can just stick with Option 3. But maybe I missed something.

Thanks a lot for maintaining such an important project!

• I intend to create a pull request to implement this feature.

[jrfnl]

This sounds loosely related to #126, though more than anything, it feels that this is about understanding how rulesets work.

I can see two solutions here:

1. Don't use a separate ruleset. If this is only about excluding one sniff for a certain path, do that instead.

<rule ref="Inpsyde"/>
<rule ref="Inpsyde.CodeQuality.NoElse">
        <exclude-pattern>*/templates/*</exclude-pattern>
</rule>

2. Set things up more in a more modular fashion where the ruleset names don't overlap with sniff names.
   This would look something like this:
   • Inpsyde - a sniff collection which holds the sniffs only. The ruleset for this would look like:

     <ruleset name="Inpsyde">
     </ruleset>

   • InpsydeCS - ruleset only, no sniffs, this would look something like:

     <ruleset name="InpsydeCS">
         <rule ref="Inpsyde"/>
         ... whatever other rules you want to include ...
     </ruleset>

   • InpsydeTemplates - ruleset only, no sniffs, this would look something like:

     <ruleset name="InpsydeTemplates">
         <rule ref="InpsydeCS"/>
         <rule ref="Inpsyde.CodeQuality.NoElse">
             <severity>0</severity>
         </rule>
         ... whatever other template specific configuration you want ...
     </ruleset>

   You should then able to do something like this in the consumer packages:

   <rule ref="InpsydeCS">
       <exclude-pattern>*/templates/*</exclude-pattern>
   </rule>
   <rule ref="InpsydeTemplates">
       <include-pattern>*/templates/*</include-pattern>
   </rule>

Note: while looking at this, I checked out your repo and there are more problems with it, including the standards not complying with the naming conventions set out by PHPCS, which can cause problems with the autoloading and configuration.
I'd recommend reading up on this and fixing this up (or hiring someone who knows what they are doing to do this for you).

[shvlv]

Thanks a lot for your fast feedback!

Don't use a separate ruleset. If this is only about excluding one sniff for a certain path, do that instead.

Perhaps it's the only way to go.

Set things up more in a more modular fashion where the ruleset names don't overlap with sniff names

Unfortunately, it doesn't work. I tested your suggestion, and it works exactly as my Option 1 (even a bit worse because of using severity).

This is the debug output from PHP_CodeSniffer\Runner::$ruleset.

ignorePatterns

I.e. all InpsydeCS rules will be ignored. And since we use ignore first (

PHP_CodeSniffer/src/Files/File.php

Lines 464 to 480 in 8b83793

	// If the file path matches one of our ignore patterns, skip it.
	// While there is support for a type of each pattern
	// (absolute or relative) we don't actually support it here.
	foreach ($listenerData['ignore'] as $pattern) {
	// We assume a / directory separator, as do the exclude rules
	// most developers write, so we need a special case for any system
	// that is different.
	if (DIRECTORY_SEPARATOR === '\\') {
	$pattern = str_replace('/', '\\\\', $pattern);
	}
	$pattern = '`'.$pattern.'`i';
	if (preg_match($pattern, $this->path) === 1) {
	$this->ignoredListeners[$class] = true;
	continue(2);
	}
	}

) and data comes from ignorePatterns property all those sniffs will be ignored for templates directory.

includePatterns

It works as expected.

ruleset

It means severity is always assigned globally. So Inpsyde.CodeQuality.NoElse is not run for every part of the package.

So as I can see, it's impossible to exclude and include rules simultaneously. And severity is always global regardless of the include/exclude configuration.

Note: while looking at this, I checked out your repo and there are more problems with it, including the standards not complying with the naming conventions set out by PHPCS, which can cause problems with the autoloading and configuration

Thanks for the hint. We considering refactoring now 😄 .

[jrfnl]

@shvlv Sorry, my bad. I'll have another think about this.