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.