Skip to content

Commit 22607ab

Browse files
jrfnljrfnl
authored
Merge pull request #1003 from PHPCSStandards/phpcs-4.0/feature/sq-2197-ruleset-config-directive-processing
Ruleset: configs in higher level ruleset(s) overrule configs set in included ruleset(s)
2 parents bb4b406 + d356956 commit 22607ab

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)