Change ruleset processing to be top-to-bottom instead of processing tags in groups

[grappler]

@jrfnl second point (Custom ruleset cannot overrule value in included ruleset) was not fixed when the issue #1821 was closed.

I have done a bit of more debugging. We have the following two rulesets.

<?xml version="1.0"?>
<ruleset name="Company-Ruleset">
	<rule ref="PHPCompatibility"/>
	<config name="testVersion" value="5.6-"/>
</ruleset>

The problem is that the config here does not override the testVersion defined in Company-Ruleset

<?xml version="1.0"?>
<ruleset name="Project-Ruleset">
	<rule ref="Company-Ruleset" />
	<config name="testVersion" value="7.1-"/>
</ruleset>

When running phpcs . -vvv you can see why the config is not being overwritten.

Processing ruleset project/phpcs.xml.dist
	=> set config value testVersion: 7.1-
	Processing rule "Company-Ruleset"
		=> Company-Ruleset/ruleset.xml
		* rule is referencing a standard using ruleset path; processing *
		Processing ruleseCompany-Ruleset/ruleset.xml
			=> set config value testVersion: 5.6-

What I realized I could override the config with another ruleset.

<?xml version="1.0"?>
<ruleset name="Project-Ruleset">
	<rule ref="Company-Ruleset" />
	<config name="testVersion" value="7.1-"/>
	<rule ref="config-override.xml"/>
</ruleset>

The config-override.xml looks like the following

<?xml version="1.0"?>
<ruleset>
	<config name="testVersion" value="7.1-"/>
</ruleset>

When running phpcs . -vvv now the output looks like the following.

Processing ruleset project/phpcs.xml.dist
	=> set config value testVersion: 7.1-
	Processing rule "Company-Ruleset"
		=> Company-Ruleset/ruleset.xml
		* rule is referencing a standard using ruleset path; processing *
		Processing ruleseCompany-Ruleset/ruleset.xml
			=> set config value testVersion: 5.6-
[...]
	Processing rule "config-override.xml"
		* rule is referencing a standard using ruleset path; processing *
		Processing ruleset config-override.xml
			=> set config value testVersion: 7.1-

[dingo-d]

Have the exact same issue 😄 I didn't add another ruleset, but I've just silenced the error thrown by specifying severity to 0 of the specific rule that was causing me issues (which I know won't be an issue in production).

[gsherwood]

This is due to the way ruleset parsing currently happens. All config vars are processed before the rules are processed, as you've seen from the debug output.

Changing the ruleset to be parsed top-to-bottom, line-by-line, is actually pretty easy, but it does mean that some existing rulesets may stop working as they are relying on the existing behaviour.

I would like to see this changed, so I'll put this on the 4.0 roadmap.

[gsherwood]

To do this, change Ruleset::processRuleset() to look over $ruleset->children() and switch on the name. E.g.,

foreach ($ruleset->children() as $child) {
    switch ($child->getName()) {
        case 'autoload':
        break;        
    }
}

[jrfnl]

To think over as a solution direction as suggested in #2395 (comment)

1. Read in the custom ruleset used.
2. Process the tags in a set order recursively.

Something along the lines of (pseudo-code):

function processRuleset($name) {
    $ruleset = // read custom ruleset XML file or standard XML file.

    foreach ($ruleset->rule as $rule) {
        processRuleset($rule);
    }

    foreach ($ruleset->config as $config) {
    }

    foreach ($ruleset->arg as $arg) {
    } 
}

[gsherwood]

Something along the lines of (pseudo-code):

That's basically what it does now, just in a different order.

I like the idea of telling developers that their rulesets are processed line by line as it gives greater control and understanding than is the case right now. But this suggested change would have less impact overall and may be a better option.

I'm still not sure if it could come in before 4.0 - I'm not entirely sure what the implications of the change are as I haven't gone digging - but it is possible that a smaller change like this might be able to make it into 3.5.0.

[dingo-d]

Any possibility of this landing in 3.5.0? 🤞

[gsherwood]

Any possibility of this landing in 3.5.0?

This potentially breaks existing rulesets, so it wont be introduced before 4.0.

[alexanderpas]

To summarize the current status:

• An included ruleset can set a config value.
• An included ruleset can override a config value that has been set by a previously included ruleset.
• A config value can be set in a ruleset, if it has not been set in an included ruleset.
• Command line arguments override any value that has been set in a ruleset.

Overriding a config value in a ruleset where the config value has previously been set in an included config value is not supported, and is currently handled as if the directive doesn't exist at all (even if it does process them, there is no difference between the directive being present or not in the end result).

---

While line-by line processing does constitute a BC break, permitting overriding config variables in a ruleset does not constitute a BC break, since this is essentially the equivalent to the implementation of a new feature. (overriding an included config value in a ruleset)

This gives us the following:

• An included ruleset can set a config value.
• An included ruleset can override a config value that has been set by a previously included ruleset.
• A config value can be set in a ruleset, if it has not been set in an included ruleset.
• (NEW) A config value can be set in a ruleset, which overrides the value set in the included ruleset.
• Command line arguments override any value that has been set in a ruleset.

This also matches the expected and intended behavior, where the primary rulesets have the final say, not the rulesets they are including.

[gsherwood]

I've modified the ruleset processing code to process the ruleset top to bottom instead of in the old defined group order. The original test case now works - you can override the value of the testVersion config value after including the PHPCompatibility ruleset - you don't need to use a second included ruleset to achieve this.

[jrfnl]

@gsherwood Looking good! I'm going to have to have a play with this to see if I can find some more test cases to add.

The only thing I'm currently a little concerned about is the <autoload> directive. With the ruleset now processing top to bottom, this would always have to be at the top of a ruleset as otherwise sniffs/reports etc may not be found.

Should that directive be an exception and be processed before anything else to prevent such issues ?
It's not as if one <autoload> directive would overrule another. Every <autoload> should be read and preferably before any sniffs are loaded.

[gsherwood]

Should that directive be an exception and be processed before anything else to prevent such issues ?

I intentionally did not make it an exception. I think it's fine to require developers to include it at the top of their ruleset, as they would with code.