Skip to content

Commit d356956

Browse files
jrfnljrfnl
committed
Ruleset: configs in higher level ruleset(s) overrule configs set in included ruleset(s)
This is a completely different solution to issue squizlabs/PHP_CodeSniffer 2197, compared to the original solution which was previously committed to the old 4.0 branch. The "old' solution was to process rulesets "top to bottom", i.e. every directive is processed when "read" while recursing through the rulesets. I've evaluated that solution carefully and found it counter-intuitive, which means that I expect that it would raise lots of support questions. I also expect that solution to break way more rulesets than is justified to solve issue 2197. The original solution would require ruleset maintainers to have a high awareness of what configs are being set in included rulesets. Included rulesets are often PHPCS native standards or external standards, which are both subject to change in every new release. With ruleset processing being top-to-bottom, any "must have" config directives would have to be set _before_ any include which could possibly also have that config directive set. This also means that an external standard _adding_ a config directive to one of their rulesets, would become a potentially breaking change, as a "consumer" ruleset may not realize they were relying on a default value for a "must have" config and that value being changed in an included ruleset could now break their CS scans. So... having said that, this commit contains an alternative, far more specific, and far less invasive, solution for the originally posed problem. --- This commit changes the processing of `config` directives to be based on the inclusion depth, with the highest level ruleset setting a config value overruling the value for that config as set in lower level (included) rulesets. When two rulesets at the same "level" both set the same `config` directive, the last one included "wins". To illustrate: ``` File: phpcs.xml.dist Sets config `testVersion` to `7.2-` Includes CompanyRulesetA File CompanyRulesetA/ruleset.xml Sets config `testVersion` to `8.1-` ``` In 3.x: `testVersion` would be set to `8.1-` In 4.x: `testVersion` will be set to `7.2-` (higher level ruleset wins) ``` File: phpcs.xml.dist Includes CompanyRulesetA and AnotherRuleset File CompanyRulesetA/ruleset.xml Sets config `testVersion` to `8.2-` File AnotherRuleset/ruleset.xml Sets config `testVersion` to `5.6-` ``` In 3.x: `testVersion` would be set to `5.6-` In 4.x: `testVersion` will be set to `5.6-` (CompanyRulesetA and AnotherRuleset are at the same inclusion depth, last one wins) Includes ample tests. Includes minor tweak to improve the debug information and only display that something was set when it actually has been set. Related to squizlabs/PHP_CodeSniffer 1821 - point 2 Fixes squizlabs/PHP_CodeSniffer 2197
1 parent bb4b406 commit d356956

8 files changed

+307
-4
lines changed

‎src/Ruleset.php‎

Lines changed: 31 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,14 @@ class Ruleset
123123
*/
124124
private $config = null;
125125

126+
/**
127+
* The `<config>` directives which have been applied.
128+
*
129+
* @var array<string, int> Key is the name of the config. Value is the ruleset depth
130+
* at which this config was applied (if not overruled from the CLI).
131+
*/
132+
private $configDirectivesApplied = [];
133+
126134
/**
127135
* An array of the names of sniffs which have been marked as deprecated.
128136
*
@@ -574,17 +582,36 @@ public function processRuleset($rulesetPath, $depth=0)
574582
}//end foreach
575583

576584
// Process custom sniff config settings.
585+
// Processing rules:
586+
// - Highest level ruleset take precedence.
587+
// - If the same config is being set from two rulesets at the same level, *last* one "wins".
577588
foreach ($ruleset->{'config'} as $config) {
578589
if ($this->shouldProcessElement($config) === false) {
579590
continue;
580591
}
581592

582-
$this->config->setConfigData((string) $config['name'], (string) $config['value'], true);
583-
if (PHP_CODESNIFFER_VERBOSITY > 1) {
593+
$name = (string) $config['name'];
594+
595+
if (isset($this->configDirectivesApplied[$name]) === true
596+
&& $this->configDirectivesApplied[$name] < $depth
597+
) {
598+
// Ignore this config. A higher level ruleset has already set a value for this directive.
599+
if (PHP_CODESNIFFER_VERBOSITY > 1) {
600+
echo str_repeat("\t", $depth);
601+
echo "\t=> ignoring config value ".$name.': '.(string) $config['value'].' => already changed by a higher level ruleset '.PHP_EOL;
602+
}
603+
604+
continue;
605+
}
606+
607+
$this->configDirectivesApplied[$name] = $depth;
608+
609+
$applied = $this->config->setConfigData($name, (string) $config['value'], true);
610+
if ($applied === true && PHP_CODESNIFFER_VERBOSITY > 1) {
584611
echo str_repeat("\t", $depth);
585-
echo "\t=> set config value ".(string) $config['name'].': '.(string) $config['value'].PHP_EOL;
612+
echo "\t=> set config value ".$name.': '.(string) $config['value'].PHP_EOL;
586613
}
587-
}
614+
}//end foreach
588615

589616
foreach ($ruleset->rule as $rule) {
590617
if (isset($rule['ref']) === false

‎tests/Core/Ruleset/Fixtures/ProcessRulesetConfigDirectivesTest/ProcessRulesetConfigDirectivesATest.xml‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
<?xml version="1.0"?>
2+
<ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" name="ProcessRulesetConfigDirectivesTest" xsi:noNamespaceSchemaLocation="https://raw.githubusercontent.com/PHPCSStandards/PHP_CodeSniffer/master/phpcs.xsd">
3+
4+
<!-- This ruleset sets two config directives.
5+
All included rulesets will try to override these and should fail.
6+
Whether the config is set before, or after, the sub-rulesets are included, should be irrelevant.
7+
-->
8+
<config name="expectValueFromParentATestSetBeforeIncludes" value="parent A"/>
9+
10+
<!-- Include the overrides. -->
11+
<rule ref="./ProcessRulesetConfigDirectivesIncludeATest.xml"/>
12+
<rule ref="./ProcessRulesetConfigDirectivesIncludeBTest.xml"/>
13+
14+
<config name="expectValueFromParentATestSetAfterIncludes" value="parent A"/>
15+
16+
<!-- This directive should not be applied. Last included ruleset (parent B) should win. -->
17+
<config name="expectValueFromParentBTest" value="parent A"/>
18+
19+
<!-- Prevent a "no sniffs were registered" error. -->
20+
<rule ref="Generic.PHP.BacktickOperator"/>
21+
</ruleset>

‎tests/Core/Ruleset/Fixtures/ProcessRulesetConfigDirectivesTest/ProcessRulesetConfigDirectivesBTest.xml‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
<?xml version="1.0"?>
2+
<ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" name="ProcessRulesetConfigDirectivesTest" xsi:noNamespaceSchemaLocation="https://raw.githubusercontent.com/PHPCSStandards/PHP_CodeSniffer/master/phpcs.xsd">
3+
4+
<!-- For this config directive, this is the "highest" level, but the directive is also set in the "parent A"
5+
ruleset, which is at the same "level".
6+
In that case, the value which "wins" is determined by the order in which the rulesets are included.
7+
In this case, that means that "parent B" should win as "parent A" is included _before_ parent B (via the CLI args),
8+
so the value from "parent B" overwrites the value from the earlier read "parent A".
9+
-->
10+
<config name="expectValueFromParentBTest" value="parent B"/>
11+
12+
<!-- For this config directive, this is the "highest" level, but the directive is also set in the "grandchild B" ruleset.
13+
Highest level should win, independently of whether the grandchild is a child of this ruleset, or of another ruleset.
14+
-->
15+
<config name="expectValueFromParentBTestAlsoInGrandChildB" value="parent B"/>
16+
17+
</ruleset>

‎tests/Core/Ruleset/Fixtures/ProcessRulesetConfigDirectivesTest/ProcessRulesetConfigDirectivesIncludeATest.xml‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
<?xml version="1.0"?>
2+
<ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" name="ProcessRulesetConfigDirectivesTest" xsi:noNamespaceSchemaLocation="https://raw.githubusercontent.com/PHPCSStandards/PHP_CodeSniffer/master/phpcs.xsd">
3+
4+
<!-- This config directive should not be applied. Root should win. -->
5+
<config name="expectValueFromParentATestSetBeforeIncludes" value="child A"/>
6+
7+
<!-- For this config directive, this is the "highest" level, so this one should be applied.
8+
The included (grand)child ruleset will try to override this.
9+
-->
10+
<config name="expectValueFromChildATestSetBeforeIncludes" value="child A"/>
11+
12+
<!-- This directive should not be applied. Last included ruleset (child B) should win. -->
13+
<config name="expectValueFromChildBTestInChildAAndChildB" value="child A"/>
14+
15+
<!-- Include the overrides. -->
16+
<rule ref="./ProcessRulesetConfigDirectivesSubIncludeATest.xml"/>
17+
<rule ref="./ProcessRulesetConfigDirectivesSubIncludeBTest.xml"/>
18+
19+
<!-- For this config directive, this is the "highest" level, so this one should be applied.
20+
The included (grand)child ruleset will try to override this.
21+
-->
22+
<config name="expectValueFromChildATestSetAfterIncludes" value="child A"/>
23+
24+
<!-- This config directive should not be applied. Root should win. -->
25+
<config name="expectValueFromParentATestSetAfterIncludes" value="child A"/>
26+
27+
</ruleset>

‎tests/Core/Ruleset/Fixtures/ProcessRulesetConfigDirectivesTest/ProcessRulesetConfigDirectivesIncludeBTest.xml‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
<?xml version="1.0"?>
2+
<ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" name="ProcessRulesetConfigDirectivesTest" xsi:noNamespaceSchemaLocation="https://raw.githubusercontent.com/PHPCSStandards/PHP_CodeSniffer/master/phpcs.xsd">
3+
4+
<!-- These config directives should not be applied. Root should win. -->
5+
<config name="expectValueFromParentATestSetBeforeIncludes" value="child B"/>
6+
<config name="expectValueFromParentATestSetAfterIncludes" value="child B"/>
7+
8+
<!-- For this config directive, this is the "highest" level, but the directive is also set in the "child A"
9+
ruleset, which is at the same "level".
10+
In that case, the value which "wins" is determined by the order in which the rulesets are included.
11+
In this case, that means that "child B" should win as "child A" is included _before_ child B (from the root ruleset),
12+
so the value from "child B" overwrites the value from the earlier read "child A".
13+
-->
14+
<config name="expectValueFromChildBTestInChildAAndChildB" value="child B"/>
15+
16+
<!-- For this config directive, this is the "highest" level, but the directive is also set in the "grandchild A" ruleset.
17+
Highest level should win, independently of whether the grandchild is a child of this ruleset, or of another ruleset.
18+
-->
19+
<config name="expectValueFromChildBTestAlsoInGrandChildA" value="child B"/>
20+
21+
<!-- This directive is only set in this ruleset, so should be applied. -->
22+
<config name="expectValueFromChildBTest" value="child B"/>
23+
24+
</ruleset>

‎tests/Core/Ruleset/Fixtures/ProcessRulesetConfigDirectivesTest/ProcessRulesetConfigDirectivesSubIncludeATest.xml‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
<?xml version="1.0"?>
2+
<ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" name="ProcessRulesetConfigDirectivesTest" xsi:noNamespaceSchemaLocation="https://raw.githubusercontent.com/PHPCSStandards/PHP_CodeSniffer/master/phpcs.xsd">
3+
4+
<!-- These config directives should not be applied. Root should win. -->
5+
<config name="expectValueFromParentATestSetBeforeIncludes" value="grandchild A"/>
6+
<config name="expectValueFromParentATestSetAfterIncludes" value="grandchild A"/>
7+
8+
<!-- This directive should not be applied. Child A should win. -->
9+
<config name="expectValueFromChildATestSetBeforeIncludes" value="grandchild A"/>
10+
11+
<!-- This directive should not be applied. Child B should win. -->
12+
<config name="expectValueFromChildBTestAlsoInGrandChildA" value="grandchild A"/>
13+
14+
<!-- This directive is only set in this ruleset, so should be applied. -->
15+
<config name="expectValueFromGrandChildATest" value="grandchild A"/>
16+
17+
</ruleset>

‎tests/Core/Ruleset/Fixtures/ProcessRulesetConfigDirectivesTest/ProcessRulesetConfigDirectivesSubIncludeBTest.xml‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
<?xml version="1.0"?>
2+
<ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" name="ProcessRulesetConfigDirectivesTest" xsi:noNamespaceSchemaLocation="https://raw.githubusercontent.com/PHPCSStandards/PHP_CodeSniffer/master/phpcs.xsd">
3+
4+
<!-- These config directives should not be applied. Root should win. -->
5+
<config name="expectValueFromParentATestSetBeforeIncludes" value="grandchild B"/>
6+
<config name="expectValueFromParentATestSetAfterIncludes" value="grandchild B"/>
7+
8+
<!-- This directive should not be applied. Child A should win. -->
9+
<config name="expectValueFromChildATestSetAfterIncludes" value="grandchild B"/>
10+
11+
<!-- This directive should not be applied. Parent B should win. -->
12+
<config name="expectValueFromParentBTestAlsoInGrandChildB" value="grandchild B"/>
13+
14+
<!-- This directive is only set in this ruleset, so should be applied. -->
15+
<config name="expectValueFromGrandChildBTest" value="grandchild B"/>
16+
17+
</ruleset>

‎tests/Core/Ruleset/ProcessRulesetConfigDirectivesTest.php‎

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,153 @@
1+
<?php
2+
/**
3+
* Tests for the \PHP_CodeSniffer\Ruleset class.
4+
*
5+
* @copyright 2025 PHPCSStandards and contributors
6+
* @license https://github.com/PHPCSStandards/PHP_CodeSniffer/blob/master/licence.txt BSD Licence
7+
*/
8+
9+
namespace PHP_CodeSniffer\Tests\Core\Ruleset;
10+
11+
use PHP_CodeSniffer\Ruleset;
12+
use PHP_CodeSniffer\Tests\ConfigDouble;
13+
use PHPUnit\Framework\TestCase;
14+
15+
/**
16+
* Tests for the \PHP_CodeSniffer\Ruleset class.
17+
*
18+
* @covers \PHP_CodeSniffer\Ruleset::processRuleset
19+
*/
20+
final class ProcessRulesetConfigDirectivesTest extends TestCase
21+
{
22+
23+
/**
24+
* Directory in which the XML fixtures for this test can be found (without trailing slash).
25+
*
26+
* @var string
27+
*/
28+
private const FIXTURE_DIR = __DIR__.'/Fixtures/ProcessRulesetConfigDirectivesTest';
29+
30+
/**
31+
* The Config object.
32+
*
33+
* @var \PHP_CodeSniffer\Config
34+
*/
35+
private static $config;
36+
37+
38+
/**
39+
* Initialize the config and ruleset objects for this test only once (but do allow recording code coverage).
40+
*
41+
* @return void
42+
*/
43+
protected function setUp(): void
44+
{
45+
if (isset(self::$config) === false) {
46+
// Set up the ruleset.
47+
$standardA = self::FIXTURE_DIR.'/ProcessRulesetConfigDirectivesATest.xml';
48+
$standardB = self::FIXTURE_DIR.'/ProcessRulesetConfigDirectivesBTest.xml';
49+
self::$config = new ConfigDouble(["--standard=$standardA,$standardB"]);
50+
new Ruleset(self::$config);
51+
}
52+
53+
}//end setUp()
54+
55+
56+
/**
57+
* Make sure the Config object is destroyed.
58+
*
59+
* @return void
60+
*/
61+
public static function tearDownAfterClass(): void
62+
{
63+
// Explicitly trigger __destruct() on the ConfigDouble to reset the Config statics.
64+
// The explicit method call prevents potential stray test-local references to the $config object
65+
// preventing the destructor from running the clean up (which without stray references would be
66+
// automagically triggered when this object is destroyed, but we can't definitively rely on that).
67+
if (isset(self::$config) === true) {
68+
self::$config->__destruct();
69+
}
70+
71+
}//end tearDownAfterClass()
72+
73+
74+
/**
75+
* Verify the config directives are set based on the nesting level of the ruleset.
76+
*
77+
* - Highest level ruleset (root) should win over lower level (included) ruleset.
78+
* - When two rulesets at the same "level" both set the same config, last included ruleset should win.
79+
* - But if no higher level ruleset sets the value, the values from lowel levels should be applied.
80+
* - The order of includes versus config directives in a ruleset file is deliberately irrelevant.
81+
*
82+
* @param string $name The name of the config setting we're checking.
83+
* @param string $expected The expected value for that config setting.
84+
*
85+
* @dataProvider dataConfigDirectives
86+
*
87+
* @return void
88+
*/
89+
public function testConfigDirectives($name, $expected)
90+
{
91+
$this->assertSame($expected, self::$config->getConfigData($name));
92+
93+
}//end testConfigDirectives()
94+
95+
96+
/**
97+
* Data provider.
98+
*
99+
* @return array<string, array<string, string>>
100+
*/
101+
public static function dataConfigDirectives()
102+
{
103+
return [
104+
'Config set in parent A before includes; all includes try to overrule; parent A should win' => [
105+
'name' => 'expectValueFromParentATestSetBeforeIncludes',
106+
'expected' => 'parent A',
107+
],
108+
'Config set in parent A after includes; all includes try to overrule; parent A should win' => [
109+
'name' => 'expectValueFromParentATestSetAfterIncludes',
110+
'expected' => 'parent A',
111+
],
112+
'Config set in both parent A and parent B; B is included last via CLI; parent B should win' => [
113+
'name' => 'expectValueFromParentBTest',
114+
'expected' => 'parent B',
115+
],
116+
'Config set in parent B; also set in non-direct grandchild B; parent B should win' => [
117+
'name' => 'expectValueFromParentBTestAlsoInGrandChildB',
118+
'expected' => 'parent B',
119+
],
120+
'Config set in child A before includes; also set in grandchild A; child A should win' => [
121+
'name' => 'expectValueFromChildATestSetBeforeIncludes',
122+
'expected' => 'child A',
123+
],
124+
'Config set in child A after includes; also set in grandchild B; child A should win' => [
125+
'name' => 'expectValueFromChildATestSetAfterIncludes',
126+
'expected' => 'child A',
127+
],
128+
'Config set in both child A and child B; B is included last via ruleset includes; child B should win' => [
129+
'name' => 'expectValueFromChildBTestInChildAAndChildB',
130+
'expected' => 'child B',
131+
],
132+
'Config set in child B; also set in non-direct grandchild A, child B should win' => [
133+
'name' => 'expectValueFromChildBTestAlsoInGrandChildA',
134+
'expected' => 'child B',
135+
],
136+
'Config set in child B - no overrules' => [
137+
'name' => 'expectValueFromChildBTest',
138+
'expected' => 'child B',
139+
],
140+
'Config set in grandchild A ruleset - no overrules' => [
141+
'name' => 'expectValueFromGrandChildATest',
142+
'expected' => 'grandchild A',
143+
],
144+
'Config set in grandchild B ruleset - no overrules' => [
145+
'name' => 'expectValueFromGrandChildBTest',
146+
'expected' => 'grandchild B',
147+
],
148+
];
149+
150+
}//end dataConfigDirectives()
151+
152+
153+
}//end class

0 commit comments

Comments
 (0)