Overruling arbitrary config directives

[jrfnl]

An arbitrary config directive testVersion is used for the PHPCompatibility standard to pass the PHP versions code should be compatible with and is subsequently used by all sniffs in the standard.

Config directives can be set:

1. Using phpcs --config-set name value upon which they are written to the CodeSniffer.conf file and used as a default value.
2. Using <config name="name" value="value"/> in a custom ruleset (since PHPCS 2.6.0 ?).
3. On the fly, using phpcs --runtime-set name value when running PHPCS.

As far as I can tell, the current precedence for which value is used, is:

1. Custom ruleset
2. If not found in custom ruleset -> command line
3. If not found in custom ruleset or command line -> CodeSniffer.conf

I've seen numerous reports and support requests around this and would like to report the following issues with this:

1. Command line does not overrule ruleset

As per the above precedence order, an arbitrary config directive in a custom ruleset takes precedence. This does not allow for on-the-fly / one-time-only overruling of this directive from the command-line, which is the expected behaviour.

To reproduce the issue - taking PHPCompatibility as the example - with this custom ruleset:

<?xml version="1.0"?>
<ruleset name="TestRuleset">
    <config name="testVersion" value="5.3-"/>
    <rule ref="PHPCompatibility"/>
</ruleset>

Running the below command over PHPCS itself, will still report on issues for PHP 5.3: phpcs --standard=./test-ruleset.xml --runtime-set testVersion 5.4-

2. Custom ruleset cannot overrule value in included ruleset

Given a company-wide ruleset in vendor/company/qa/phpcs.xml.dist:

<?xml version="1.0"?>
<ruleset name="CompanyRuleset">
    <config name="testVersion" value="5.3-"/>
    <rule ref="PHPCompatibility"/>
</ruleset>

which would be included in a project ruleset:

<?xml version="1.0"?>
<ruleset name="ProjectRuleset">
    <config name="testVersion" value="7.0-"/>
    <rule ref="./vendor/company/qa/phpcs.xml.dist"/>
</ruleset>

The config directive in the top-level ruleset - the project ruleset - does not overrule the value as set in the included Company ruleset.

The order of the directives does not make a difference.

I.e. if the project ruleset has the config directive after the ruleset inclusion, it still doesn't work:

<?xml version="1.0"?>
<ruleset name="ProjectRuleset">
    <rule ref="./vendor/company/qa/phpcs.xml.dist"/>
    <config name="testVersion" value="7.0-"/>
</ruleset>

---

As a secondary issue, though I suppose this is quite specific to the PHPCompatibility standard, it would be great to have some sort of way to overrule the value inline.

Example situation: A project which is supposed to be compatible with PHP 5.4 to latest.

• In this project there are some files which are only compatible with PHP 5.6+ and will only be loaded if PHP 5.6 is detected, with alternative files being loaded for PHP 5.4 and 5.5.
• Using testVersion 5.4-, the PHP 5.6+ file would trigger all sorts of errors.
• However, excluding the file through the ruleset would also silence errors regarding issues with, for instance, PHP 7.2, which should be reported.

It would be awesome if it were possible to use something like the new phpcs:set comments to change the value of the arbitrary config variable on the fly.

I realize that phpcs:set is intended for sniff properties, but as all the sniffs in PHPCompatibility use this "property", it would be completely impractical to set it for each and every sniff, which is why PHPCompatibility uses the arbitrary config variable way of passing this information.

Any ideas about how to find a way to allow for this usecase would be very much appreciated!

[jrfnl]

Related: #451, #167

[photodude]

upvote for fixing Command line does not overrule ruleset upvote for fixing Custom ruleset cannot overrule value in included ruleset

The command line should rule all. rulesets should be based on the highest level included standards < custom standards < intermediate standard < project standard < final standard This hierarchical ruleset ordering follows the general logic of selectively applying rulesets.

[gsherwood]

1. Command line does not overrule ruleset

This is a fairly simple change that I can commit. What it means is that any config value set using --runtime-set will mean that all attempts to set the value by any rulesets will be ignored. That's the same way all the built-in options work, so that makes sense.

2. Custom ruleset cannot overrule value in included ruleset

The way rulesets are processed means that PHPCS calls setConfigData every time a config tag is processed. Changing the order of your tags should mean that the config data set via the included ruleset will get stored first, then overwritten by the config data you set in your own ruleset. So it's a last-wins way of setting values. I don't know why that doesn't work for you - it's working for me.

But that's not how the built-in options work. They work using a first-wins style of setting values, so you need to precede your includes with the arg tags you want.

It would be good to change the the config values to use the same method, but it's such a big BC break that I'm reluctant to do so in a 3.x version.

As a secondary issue, though I suppose this is quite specific to the PHPCompatibility standard, it would be great to have some sort of way to overrule the value inline.

If the last-wins method of setting config values remains, this should be fairly easy. But these values would need to override any runtime-set values, so they would need to be forced into the config array, which is fine. But then, how do you restore the previous value after you've processed that file?

Sniff settings are reset after each file, but config values remain for the entire run. So I think we'd actually need a temp config for the current file that can be looked at first, before looking into the main config.

To me, this is a feature I think is still too specific to put into PHPCS 3.x. But I would really like to move to a sniff properties model where they can share config settings (like indent) and that would require solving these sorts of issues. That feels like a good change for 4.x to me as it means sniff devs would need to review how they are using config options.

[gsherwood]

I've pushed a change to make the runtime set values take precedence over everything else.

[jrfnl]

@gsherwood Excellent! I'm going to have a play it this week.

[gsherwood]

I'm going to close this now, but please let me know if you find any problems.

[timoschinkel]

@gsherwood This fix messes up my solution for testing sniffs for different php versions. Currently I use an implementation of \PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest::setCliValues() in my testcase and in that method I set the php version based on the test file I receive using: $config->setCommandLineValues(['--runtime-set', 'php_version', '70100']);. This fix sets \PHP_CodeSniffer\Config::$overriddenDefaults to an array and thus my runtime setting of php is ignore when the second file is passed through setCliValues().

Am I misusing \PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest::setCliValues() and should I test php version dependent Sniffs in another way?

[jrfnl]

@timoschinkel Just wondering while trying to understand your issue: why are you not testing on the actual PHP versions using a matrix on Travis ? AFAICS, that way you wouldn't need to overrule the php_version from within your tests.

[timoschinkel]

@jrfnl The sniff - checking scope modifiers for constants - is only applicable for a subset of php versions. So the sniff itself has different behavior based on the php version it is run on and thus I have different test cases with different errors/warnings. Since I want to control the pre- and post conditions of my unit tests I opted to specify the php version in the test itself. A version matrix will help to see if the code runs on multiple php versions, but this is imho not the solution to the issue at hand.

I went through the code of the testcase and saw the method to specify cli values for a testcase per file, which is exactly what is needed for this situation. Hence I assumed it worked. And it did until the PR for this issue was merged :)

As far as I can see this issue will apply to any configuration set via \PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest::setCliValues(). So not only for the php version.

[gsherwood]

I set the php version based on the test file I receive using: $config->setCommandLineValues(['--runtime-set', 'php_version', '70100']);.

@timoschinkel That's not how I anticipated that method being used, but that doesn't mean it's wrong - just not tested. I use it to set config values directly, like $config->tabWidth = 4 and not to setup custom config (or runtime) values.

A version matrix will help to see if the code runs on multiple php versions, but this is imho not the solution to the issue at hand.

That's actually how I test the included sniffs where there are variations between PHP versions.

One example is: https://github.com/squizlabs/PHP_CodeSniffer/blob/master/src/Standards/Generic/Tests/PHP/DisallowAlternativePHPTagsUnitTest.php#L25

Here, the PHP version is being checked to offer up different test files to the runner. It's basically the opposite of what you're doing I think, but it obviously requires running the tests on different versions of PHP.

Do you think that's an option?

[timoschinkel]

I understand your approach, but from a testing perspective I don't agree with it :)

If I see time I might dive into this a bit more. But for now I will shelf the PR that used this functionality.

[jrfnl]

@timoschinkel Is the PR branch you mentioned public ? If so, would you be willing to share a link ? I'd like to have a look and see if I can find a solution for you.

[timoschinkel]

@jrfnl Yes it is: https://github.com/timoschinkel/PHP_CodeSniffer/tree/psr12/constant-declaration-sniff

It's the first sniff from our own PSR-12 work that I'm trying to convert to a viable PR for Codesniffer.

[jrfnl]

@timoschinkel I've had a look at the sniff and while I think I understand what you're trying to do, the unit tests can be much more simplified to let the "normal" PHP version used in the Travis matrix test the functioning of the sniff.

What you are/were trying to do now was basically testing whether the Config class works as expected from within a sniff and while I agree, it may be a good idea to add unit tests for the Config class, it is not the task of a sniff to do.

[gsherwood]

I've finally taken a look as well and agree with @jrfnl - if I was to accept this into PHPCS I'd want it to run the tests like the rest of the sniffs and run with the native PHP version each runner is using. It makes it much easier for me to maintain tests in bulk.

I understand the appeal of being able to test as if you were running on different PHP versions, but that's just not how I test things. I would not feel confident saying that a sniff supports PHP 7.0 (for example) if I'm only simulating the test runs for that version and not running directly on it.

Just sounds like a difference of opinion, which is a shame if it means not contributing a PSR-12 sniff. Another one I'll need to find time to write myself 😄

[timoschinkel]

@gsherwood

Just sounds like a difference of opinion

Exactly this :) I had not yet responded to the previous comment because I could not find any time to examine the change made to fix this issue a bit more thoroughly. I currently have another PR in progress, after that I will take another look at this one.

[jrfnl]

Just realized that I posted the link to the suggested fix for @timoschinkel only on Twitter, so for prosperity/helping others, let's put the link here too:

For some reason your fork does not come up when I try to create a PR in GH, so I can't send it to you with the suggested changes. You can find the commit (+reasoning in the commit message) here: https://github.com/jrfnl/PHP_CodeSniffer/commit/1f469d517e82ab657c455a0b1f040502ac06c7c6

Source: https://twitter.com/jrf_nl/status/999266937522806784