PHPCS: set the minimum supported WP version via a property #131
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
|
CR 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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_versionfor all sniffs in one go via aconfigdirective.The problem with a
configdirective 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
configdirective 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.
I've tested this change extensively with repo specific configs adjusted for this change and have found no problems.
Ref: