Ruleset::processRule(): fix handling of rules included via path #2501
Conversation
jrfnl
force-pushed
the
feature/2497-map-settings-to-sniff-included-via-path
branch
2 times, most recently
from
6a3e3ad
to
287dbdc
Compare
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.
jrfnl
force-pushed
the
feature/2497-map-settings-to-sniff-included-via-path
branch
from
287dbdc
to
ae271d0
Compare
|
Rebased for merge conflicts. |
jrfnl
force-pushed
the
feature/2497-map-settings-to-sniff-included-via-path
branch
from
ae271d0
to
9d79087
Compare
|
And rebased again for merge conflicts. |
jrfnl
force-pushed
the
feature/2497-map-settings-to-sniff-included-via-path
branch
2 times, most recently
from
269541f
to
1764798
Compare
jrfnl
force-pushed
the
feature/2497-map-settings-to-sniff-included-via-path
branch
from
1764798
to
e8f2c75
Compare
|
Rebased yet again |
This fixes a bug where properties set for rules included via a path to the sniff file or to an included standards file, would be disregarded if the slashes used in the path did not match the slashes expected for the OS on which the ruleset is being run. Fixes 2497
|
Sorry, have finally got to this. I'm a bit worried about modifying committed test files during a test run (much more so for me running them locally). Would it instead be possible to have different XML files that are included conditionally based on the OS the test is run on? |
No, it wouldn't, or rather: there are already different test files for different OSes. The problem is that I cannot hard-code an absolute file-path in the XML file as the path will be different depending on where the tests are being run from, while an absolute file path is exactly the situation which needs testing. As I documented in the test file:
I understand the question, but I actually anticipated that. |
Yep, this is the bit I missed - I thought it was just slashes.
I saw that, but I'm still nervous about modifying committed files. Ideally, I'd refactor the class to allow the contents of the XML file to be passed instead of only read from disk, but I'm not sure I have the time for that at the moment, so may need to leave as is. Thanks for clarifying for me. |
I understand, but there just isn't any other way to test this and not having tests for a change like this, which is fundamental in the ruleset processing, seemed irresponsible. While working on this, I've ran the tests a significant number of times and didn't run into any issues with the logic now in place. The only other thing I can suggest for now, would be to have a |
|
Thanks a lot for this fix.
They run fine, so I'll just make sure I don't commit something if they ever fail in the future. Thanks a lot for writing these. |
|
@gsherwood Thanks for the merge, one more issue down 🎉 |
This fixes a bug where properties set for rules included via a path to the sniff file or to an included standards file, would be disregarded if the slashes used in the path did not match the slashes expected for the OS on which the ruleset is being run.
Includes adding initial integration tests for the Ruleset class.
Fixes #2497