Overruling arbitrary config directives #1821
Comments
|
upvote for fixing Command line does not overrule ruleset The command line should rule all. |
This is a fairly simple change that I can commit. What it means is that any config value set using
The way rulesets are processed means that PHPCS calls setConfigData every time a 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 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.
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 |
…set in rulesets or the CodeSniffer.conf file (ref #1821)
|
I've pushed a change to make the runtime set values take precedence over everything else. |
|
@gsherwood Excellent! I'm going to have a play it this week. |
|
I'm going to close this now, but please let me know if you find any problems. |
|
@gsherwood This fix messes up my solution for testing sniffs for different php versions. Currently I use an implementation of Am I misusing |
|
@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 |
|
@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 |
@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
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? |
|
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. |
|
@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. |
|
@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. |
|
@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 |
|
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 😄 |
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. |
|
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:
Source: https://twitter.com/jrf_nl/status/999266937522806784 |
Properties are supposed to be set per sniff, but (bug or feature?) it turns out that setting them for a standard will set them for all sniffs included in that standard. Sniffs which don't use the property will just ignore it. Sniffs which do use it, now have it available as if it were set for the individual sniff. As this was unknown (and possibly/probably not supported) in the past with older PHPCS versions, WPCS added the ability to set the `minimum_supported_wp_version` for all sniffs in one go via a `config`directive. The problem with a `config` directive is that it is currently impossible to overrule it from a repo ruleset. It can only be overruled on the command-line, and even then only since PHPCS 3.3.0. For that reason, the `config` directive was previously set in each individual repo ruleset. Properties, however, can easily be overruled from within repo rulesets, so this change will allow us to manage the "_minimum supported WP version_" centrally, while still allowing for individual repos to overrule the default value. Ref: * https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/wiki/Customizable-sniff-properties#minimum-wp-version-to-check-for-usage-of-deprecated-functions-classes-and-function-parameters - WPCS documentation about the property/config directive. * squizlabs/PHP_CodeSniffer#2501 - PR through which this "feature" was discovered, included unit tests demonstrating it. * squizlabs/PHP_CodeSniffer#1821 - Issue which made the config directive possible to be overruled from the command-line. * squizlabs/PHP_CodeSniffer#2197 Feature request to allow for config directives to be overruled by other rulesets.
This adds a new external PHPCS standard called `PHPCSDev` for use by sniff developers to check the code style of their sniff repo code.
Often, sniff repos will use the code style of the standard they are adding. However, not all sniff repos are actually about code style.
So for those repos which need a basic standard which will still keep their code-base consistent, this standard should be useful.
The standard checks the following:
* Compliance with PSR-2.
* Use of camelCaps variable and function names.
* Use of normalized arrays.
* All files, classes, functions and properties are documented with a docblock and contain the minimally needed information.
* A small number of arbitrary additional code style checks.
* PHP cross-version compatibility, while allowing for the tokens back-filled by PHPCS itself.
**Note**: for optimal results, the project custom ruleset should set the `testVersion` config variable.
This is not done by default as config variables are currently difficult to overrule.
Refs:
- squizlabs/PHP_CodeSniffer#2197
- squizlabs/PHP_CodeSniffer#1821
The ruleset can be used like any other ruleset and specific sniffs and settings can be added to or overruled from a custom project based ruleset.
Includes adding QA checks for this ruleset to the Travis script.
This adds a new external PHPCS standard called `PHPCSDev` for use by sniff developers to check the code style of their sniff repo code.
Often, sniff repos will use the code style of the standard they are adding. However, not all sniff repos are actually about code style.
So for those repos which need a basic standard which will still keep their code-base consistent, this standard should be useful.
The standard checks the following:
* Compliance with PSR-2.
* Use of camelCaps variable and function names.
* Use of normalized arrays.
* All files, classes, functions and properties are documented with a docblock and contain the minimally needed information.
* A small number of arbitrary additional code style checks.
* PHP cross-version compatibility, while allowing for the tokens back-filled by PHPCS itself.
**Note**: for optimal results, the project custom ruleset should set the `testVersion` config variable.
This is not done by default as config variables are currently difficult to overrule.
Refs:
- squizlabs/PHP_CodeSniffer#2197
- squizlabs/PHP_CodeSniffer#1821
The ruleset can be used like any other ruleset and specific sniffs and settings can be added to or overruled from a custom project based ruleset.
Includes adding QA checks for this ruleset to the Travis script.
This adds a new external PHPCS standard called `PHPCSDev` for use by sniff developers to check the code style of their sniff repo code.
Often, sniff repos will use the code style of the standard they are adding. However, not all sniff repos are actually about code style.
So for those repos which need a basic standard which will still keep their code-base consistent, this standard should be useful.
The standard checks the following:
* Compliance with PSR-2.
* Use of camelCaps variable and function names.
* Use of normalized arrays.
* All files, classes, functions and properties are documented with a docblock and contain the minimally needed information.
* A small number of arbitrary additional code style checks.
* PHP cross-version compatibility, while allowing for the tokens back-filled by PHPCS itself.
**Note**: for optimal results, the project custom ruleset should set the `testVersion` config variable.
This is not done by default as config variables are currently difficult to overrule.
Refs:
- squizlabs/PHP_CodeSniffer#2197
- squizlabs/PHP_CodeSniffer#1821
The ruleset can be used like any other ruleset and specific sniffs and settings can be added to or overruled from a custom project based ruleset.
Includes adding QA checks for this ruleset to the Travis script.
This adds a new external PHPCS standard called `PHPCSDev` for use by sniff developers to check the code style of their sniff repo code.
Often, sniff repos will use the code style of the standard they are adding. However, not all sniff repos are actually about code style.
So for those repos which need a basic standard which will still keep their code-base consistent, this standard should be useful.
The standard checks the following:
* Compliance with PSR-12 with a few, selective exceptions:
- Constant visibility will not be demanded as the minimum PHP requirement of PHPCS is PHP 5.4 and constant visibility did not become available until PHP 7.1.
- The first condition of a multi-line control structure is allowed to be on the same line as the control structure keyword (PSR2 style).
**Note:** PSR12 enforces a blank line between each of the file header blocks.
This rule can currently not (yet) be turned off in a modular way, i.e. for just one block. It is either on or off.
This rule conflicts with the common practice of having no blank line between the PHP open tag and the file docblock.
To turn this rule off, add `<exclude name="PSR12.Files.FileHeader.SpacingAfterBlock"/>` to the project specific ruleset.
* Use of camelCaps variable and function names.
* Use of normalized arrays.
* All files, classes, functions and properties are documented with a docblock and contain the minimally needed information.
Includes allowing for more than one `@since` tag to document a class's changelog.
* Don't use Yoda conditions.
* A small number of arbitrary additional code style checks.
* PHP cross-version compatibility, while allowing for the tokens back-filled by PHPCS itself.
**Note**: for optimal results, the project custom ruleset should set the `testVersion` config variable.
This is not done by default as config variables are currently difficult to overrule.
Refs:
- squizlabs/PHP_CodeSniffer#2197
- squizlabs/PHP_CodeSniffer#1821
The ruleset can be used like any other ruleset and specific sniffs and settings can be added to or overruled from a custom project based ruleset.
Includes:
* Setting up the `composer.json`.
* Adding QA checks for this ruleset via the Travis.
An arbitrary config directive
testVersionis 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:
phpcs --config-set name valueupon which they are written to theCodeSniffer.conffile and used as a default value.<config name="name" value="value"/>in a custom ruleset (since PHPCS 2.6.0 ?).phpcs --runtime-set name valuewhen running PHPCS.As far as I can tell, the current precedence for which value is used, is:
CodeSniffer.confI'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:
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:which would be included in a project 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:
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.
testVersion 5.4-, the PHP 5.6+ file would trigger all sorts of errors.It would be awesome if it were possible to use something like the new
phpcs:setcomments to change the value of the arbitrary config variable on the fly.I realize that
phpcs:setis 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!
The text was updated successfully, but these errors were encountered: