Skip to content

Commit 3d2c0a5

Browse files
jrfnljrfnl
committed
Ruleset: (CLI) <arg> settings in higher level ruleset(s) overrule args set in included ruleset(s)
Again, this is a completely different solution to issues squizlabs/PHP_CodeSniffer 2395, 2597, 2602, compared to the original solution which was previously committed to the old 4.0 branch in response to issue 2197. 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 these issues. The original solution would require ruleset maintainers to have a high awareness of what CLI args 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" `<arg>` settings would have to be set _before_ any include which could possibly also have that directive set. This also means that an external standard _adding_ a CLI arg (like `tab-width`) 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" CLI flag 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 `arg` directives to be based on the inclusion depth, with the highest level ruleset setting a CLI arg value overruling the value for that arg as set in lower level (included) rulesets. When two rulesets at the same "level" both set the same `arg` directive, the first one included "wins". :point_right: Note: this is different from `<config>` directives where the **_last_** included ruleset wins. However, it means that for two rulesets at the same "level", the behaviour is unchanged, which makes the BC-break smaller. To illustrate: ``` File: phpcs.xml.dist Sets arg `tab-width` to `4` Includes CompanyRulesetA File CompanyRulesetA/ruleset.xml Sets arg `tab-width` to `2` ``` In 3.x: `Config::$tabWidth` would be set to `2` In 4.x: `Config::$tabWidth` will be set to `4` (higher level ruleset wins) ``` File: phpcs.xml.dist Includes CompanyRulesetA and AnotherRuleset File CompanyRulesetA/ruleset.xml Sets arg `extensions` to `php,module,phpt`. File AnotherRuleset/ruleset.xml Sets arg `extensions` to `php,module,profile,test` ``` In 3.x: `Config::$extensions` would be set to `php,module,phpt` In 4.x: `Config::$extensions` will be set to `php,module,phpt` (CompanyRulesetA and AnotherRuleset are at the same inclusion depth, first one wins) The solution as implemented takes CLI flags which can be set using multiple different CLI arguments into account. What this means is as follows: ``` File: phpcs.xml.dist Sets arg `no-colors` Includes CompanyRulesetA File CompanyRulesetA/ruleset.xml Sets arg `colors` ``` In 3.x: `Config::$colors` would be set to `true` In 4.x: `Config::$colors` will be set to `false` (higher level ruleset wins) Includes ample tests. Note: there are no tests covering the `cache` setting, while by rights, there should be, but this is (yet again) something where the global `PHP_CODESNIFFER_IN_TESTS` is checked from within the runtime code, which means that the `cache` directive cannot be set/changed from within the test suite. Yes, this should be changed, but that is outside of the scope of this PR. For now, this has been manually tested and found working. Includes minor tweak to improve the debug information and only display that something was set when it actually has been set. Fixes squizlabs/PHP_CodeSniffer 2395 Fixes squizlabs/PHP_CodeSniffer 2597 Fixes squizlabs/PHP_CodeSniffer 2602
1 parent b6abb91 commit 3d2c0a5

File tree

8 files changed

+424
-24
lines changed

8 files changed

+424
-24
lines changed

‎src/Config.php

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -104,6 +104,23 @@ class Config
104104
*/
105105
const DEFAULT_REPORT_WIDTH = 80;
106106

107+
/**
108+
* Translation table for config settings which can be changed via multiple CLI flags.
109+
*
110+
* If the flag name matches the setting name, there is no need to add it to this translation table.
111+
* Similarly, if there is only one flag which can change a setting, there is no need to include
112+
* it in this table, even if the flag name and the setting name don't match.
113+
*
114+
* @var array<string, string> Key is the CLI flag name, value the corresponding config setting name.
115+
*/
116+
public const CLI_FLAGS_TO_SETTING_NAME = [
117+
'n' => 'warningSeverity',
118+
'w' => 'warningSeverity',
119+
'warning-severity' => 'warningSeverity',
120+
'no-colors' => 'colors',
121+
'no-cache' => 'cache',
122+
];
123+
107124
/**
108125
* An array of settings that PHPCS and PHPCBF accept.
109126
*

‎src/Ruleset.php

Lines changed: 94 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,14 @@ class Ruleset
131131
*/
132132
private $configDirectivesApplied = [];
133133

134+
/**
135+
* The `<arg>` directives which have been applied.
136+
*
137+
* @var array<string, int> Key is the name of the setting. Value is the ruleset depth
138+
* at which this setting was applied (if not overruled from the CLI).
139+
*/
140+
private $cliSettingsApplied = [];
141+
134142
/**
135143
* An array of the names of sniffs which have been marked as deprecated.
136144
*
@@ -613,6 +621,92 @@ public function processRuleset($rulesetPath, $depth=0)
613621
}
614622
}//end foreach
615623

624+
// Process custom command line arguments.
625+
// Processing rules:
626+
// - Highest level ruleset take precedence.
627+
// - If the same CLI flag is being set from two rulesets at the same level, *first* one "wins".
628+
$cliArgs = [];
629+
foreach ($ruleset->{'arg'} as $arg) {
630+
if ($this->shouldProcessElement($arg) === false) {
631+
continue;
632+
}
633+
634+
// "Long" CLI argument. Arg is in the format `<arg name="name" [value="value"]/>`.
635+
if (isset($arg['name']) === true) {
636+
$name = (string) $arg['name'];
637+
$cliSettingName = $name;
638+
if (isset($this->config::CLI_FLAGS_TO_SETTING_NAME[$name]) === true) {
639+
$cliSettingName = $this->config::CLI_FLAGS_TO_SETTING_NAME[$name];
640+
}
641+
642+
if (isset($this->cliSettingsApplied[$cliSettingName]) === true
643+
&& $this->cliSettingsApplied[$cliSettingName] < $depth
644+
) {
645+
// Ignore this CLI flag. A higher level ruleset has already set a value for this setting.
646+
if (PHP_CODESNIFFER_VERBOSITY > 1) {
647+
$statusMessage = str_repeat("\t", $depth);
648+
$statusMessage .= "\t=> ignoring command line arg --".$name;
649+
if (isset($arg['value']) === true) {
650+
$statusMessage .= '='.(string) $arg['value'];
651+
}
652+
653+
echo $statusMessage.' => already changed by a higher level ruleset '.PHP_EOL;
654+
}
655+
656+
continue;
657+
}
658+
659+
// Remember which settings we've seen.
660+
$this->cliSettingsApplied[$cliSettingName] = $depth;
661+
662+
$argString = '--'.$name;
663+
if (isset($arg['value']) === true) {
664+
$argString .= '='.(string) $arg['value'];
665+
}
666+
} else {
667+
// "Short" CLI flag. Arg is in the format `<arg value="value"/>` and
668+
// value can contain multiple flags, like `<arg value="ps"/>`.
669+
$cleanedValue = '';
670+
$cliFlagsAsArray = str_split((string) $arg['value']);
671+
foreach ($cliFlagsAsArray as $flag) {
672+
$cliSettingName = $flag;
673+
if (isset($this->config::CLI_FLAGS_TO_SETTING_NAME[$flag]) === true) {
674+
$cliSettingName = $this->config::CLI_FLAGS_TO_SETTING_NAME[$flag];
675+
}
676+
677+
if (isset($this->cliSettingsApplied[$cliSettingName]) === true
678+
&& $this->cliSettingsApplied[$cliSettingName] < $depth
679+
) {
680+
// Ignore this CLI flag. A higher level ruleset has already set a value for this setting.
681+
if (PHP_CODESNIFFER_VERBOSITY > 1) {
682+
echo str_repeat("\t", $depth);
683+
echo "\t=> ignoring command line flag -".$flag.' => already changed by a higher level ruleset '.PHP_EOL;
684+
}
685+
686+
continue;
687+
}
688+
689+
// Remember which settings we've seen.
690+
$cleanedValue .= $flag;
691+
$this->cliSettingsApplied[$cliSettingName] = $depth;
692+
}//end foreach
693+
694+
if ($cleanedValue === '') {
695+
// No flags found which should be applied.
696+
continue;
697+
}
698+
699+
$argString = '-'.$cleanedValue;
700+
}//end if
701+
702+
$cliArgs[] = $argString;
703+
704+
if (PHP_CODESNIFFER_VERBOSITY > 1) {
705+
echo str_repeat("\t", $depth);
706+
echo "\t=> set command line value $argString".PHP_EOL;
707+
}
708+
}//end foreach
709+
616710
foreach ($ruleset->rule as $rule) {
617711
if (isset($rule['ref']) === false
618712
|| $this->shouldProcessElement($rule) === false
@@ -706,30 +800,6 @@ public function processRuleset($rulesetPath, $depth=0)
706800
$this->processRule($rule, $newSniffs, $depth);
707801
}//end foreach
708802

709-
// Process custom command line arguments.
710-
$cliArgs = [];
711-
foreach ($ruleset->{'arg'} as $arg) {
712-
if ($this->shouldProcessElement($arg) === false) {
713-
continue;
714-
}
715-
716-
if (isset($arg['name']) === true) {
717-
$argString = '--'.(string) $arg['name'];
718-
if (isset($arg['value']) === true) {
719-
$argString .= '='.(string) $arg['value'];
720-
}
721-
} else {
722-
$argString = '-'.(string) $arg['value'];
723-
}
724-
725-
$cliArgs[] = $argString;
726-
727-
if (PHP_CODESNIFFER_VERBOSITY > 1) {
728-
echo str_repeat("\t", $depth);
729-
echo "\t=> set command line value $argString".PHP_EOL;
730-
}
731-
}//end foreach
732-
733803
// Set custom php ini values as CLI args.
734804
foreach ($ruleset->{'ini'} as $arg) {
735805
if ($this->shouldProcessElement($arg) === false) {

‎tests/Core/Ruleset/Fixtures/ProcessRulesetCliArgsTest/.phpcs.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="ProcessRulesetCliArgsTest" xsi:noNamespaceSchemaLocation="https://raw.githubusercontent.com/PHPCSStandards/PHP_CodeSniffer/master/phpcs.xsd">
3+
4+
<!-- Issues squizlabs/PHP_CodeSniffer#2597 + squizlabs/PHP_CodeSniffer#2602: allow overruling CLI args when they have a single CLI flag names.
5+
One value is set before the includes, one after to safeguard that the order in the file does not influence the results.
6+
-->
7+
<arg name="extensions" value="inc,install,module,php,profile,test,theme"/>
8+
9+
<!-- Issue squizlabs/PHP_CodeSniffer#2395: allow overruling CLI args when they have different CLI flag names. -->
10+
<arg name="no-colors"/>
11+
12+
<!-- Miscellaneous other settings. -->
13+
<arg name="report" value="full,summary,source"/>
14+
15+
<!-- Include the overrides. -->
16+
<rule ref="./phpcs.xml.dist"/>
17+
<rule ref="./config/phpcs/include-overrules.xml"/>
18+
19+
<!-- Issues squizlabs/PHP_CodeSniffer#2597 + squizlabs/PHP_CodeSniffer#2602: second test. -->
20+
<arg name="tab-width" value="3"/>
21+
22+
<!-- Issue squizlabs/PHP_CodeSniffer#2395: second test. -->
23+
<arg value="w"/>
24+
25+
<!-- Prevent a "no sniffs were registered" error. -->
26+
<rule ref="Generic.PHP.BacktickOperator"/>
27+
</ruleset>

‎tests/Core/Ruleset/Fixtures/ProcessRulesetCliArgsTest/config/phpcs/include-overrules.xml

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
<?xml version="1.0"?>
2+
<ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" name="ProcessRulesetCliArgsTest" xsi:noNamespaceSchemaLocation="https://raw.githubusercontent.com/PHPCSStandards/PHP_CodeSniffer/master/phpcs.xsd">
3+
4+
<!-- Issues squizlabs/PHP_CodeSniffer#2597 + squizlabs/PHP_CodeSniffer#2602: allow overruling CLI args when they have the CLI flag names.
5+
The value from the parent ruleset should be applied, not the one from the child.
6+
-->
7+
<arg name="tab-width" value="5"/>
8+
9+
<!-- Paths must be resolved based on the ruleset location which included the directive. -->
10+
<arg name="report-file" value="./report.txt"/>
11+
12+
<!-- Miscellaneous other settings. -->
13+
<arg name="report-width" value="60"/>
14+
15+
</ruleset>

‎tests/Core/Ruleset/Fixtures/ProcessRulesetCliArgsTest/phpcs.xml.dist

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
<?xml version="1.0"?>
2+
<ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" name="ProcessRulesetCliArgsTest" xsi:noNamespaceSchemaLocation="https://raw.githubusercontent.com/PHPCSStandards/PHP_CodeSniffer/master/phpcs.xsd">
3+
4+
<!-- Issue squizlabs/PHP_CodeSniffer#2395: allow overruling CLI args when they have different CLI flag names.
5+
The values from the parent ruleset should be applied, not those from the child.
6+
-->
7+
<arg name="colors"/>
8+
<arg value="qn"/>
9+
10+
<!-- Paths must be resolved based on the ruleset location which included the directive. -->
11+
<arg name="basepath" value="./"/>
12+
13+
<!-- Miscellaneous other settings. -->
14+
<arg name="parallel" value="10"/>
15+
16+
<!-- Include the overrides. -->
17+
<rule ref="./vendor/OrgName/ExtStandard/ruleset.xml"/>
18+
19+
<!-- Issues squizlabs/PHP_CodeSniffer#2597 + squizlabs/PHP_CodeSniffer#2602: allow overruling CLI args when they have the CLI flag names.
20+
The values from the parent ruleset should be applied, not those from the child.
21+
-->
22+
<arg name="extensions" value="inc,php"/>
23+
<arg name="tab-width" value="4"/>
24+
25+
<!-- Miscellaneous other settings. -->
26+
<arg name="report-width" value="120"/>
27+
28+
</ruleset>

‎tests/Core/Ruleset/Fixtures/ProcessRulesetCliArgsTest/vendor/OrgName/ExtStandard/bootstrap.php

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
<?php
2+
3+
// Do nothing. This is just a placeholder file for the "bootstrap" CLI flag test.

‎tests/Core/Ruleset/Fixtures/ProcessRulesetCliArgsTest/vendor/OrgName/ExtStandard/ruleset.xml

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
<?xml version="1.0"?>
2+
<ruleset xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" name="ProcessRulesetCliArgsTest" xsi:noNamespaceSchemaLocation="https://raw.githubusercontent.com/PHPCSStandards/PHP_CodeSniffer/master/phpcs.xsd">
3+
4+
<!-- Issue squizlabs/PHP_CodeSniffer#2395. -->
5+
<arg name="warning-severity" value="3"/>
6+
<arg value="p"/>
7+
8+
<!-- Paths must be resolved based on the ruleset location which included the directive. -->
9+
<arg name="bootstrap" value="./bootstrap.php"/>
10+
11+
<!-- Miscellaneous other settings. -->
12+
<arg name="parallel" value="2"/>
13+
<arg name="report" value="code,summary"/>
14+
15+
</ruleset>

0 commit comments

Comments
 (0)