Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sniffs not loaded when one-standard directories are being registered in installed_paths #1581

Conversation

jrfnl
Copy link
Contributor

@jrfnl jrfnl commented Jul 29, 2017

If any external standard or a custom ruleset refers to a complete standard and this standard is registered in the installed_paths config as a one-standard directory, a Reference sniff ... does not exist error will be thrown and no sniffs will be run.

Bug introduced in #1450 (by me, sorry!).

The reason for this is that the getInstalledStandardPath() method expects a string, but will in actual fact receive a SimpleXMLElement object when a complete standard is included by another standard. This was previously not documented in the function doc block as a possibility.

While in the original code, the $standards name was only used as a concatenated string - which triggered the SimpleXML.toString() method -, the fix in #1450 introduced a strict comparison without the toString() conversion, causing the bug.

Installed_paths string $standards SimpleXMLElement $standards
"Standards" directory
one-standard directory ❌ <= This is what this fixes.

While debugging this, I also noticed that this method is called for every single ref in the ruleset and would go through the whole loop before returning null, while for refs which refer to a custom xml ruleset or to a sniff or sniff category, null can be returned a lot earlier and for paths which don't resolve to a ruleset, part of the loop can be skipped.


Testing the fix:

Using the WordPress Coding Standards as an example to demonstrate what I mean, set installed_paths like so:
phpcs --config-set installed_paths /path/to/WordPressCS/WordPress,/path/to/WordPressCS/WordPress-Core,/path/to/WordPressCS/WordPress-Docs,/path/to/WordPressCS/WordPress-Extra,/path/to/WordPressCS/WordPress-VIP

Custom ruleset test.xml:

<?xml version="1.0"?>
<ruleset name="Test">
	<rule ref="WordPress-Core"/>
</ruleset>

Then run PHPCS against any file:
phpcs ./test-file.php --standard=test.xml

…`installed_paths`.

If any external standard or a custom ruleset refers to a complete standard and this standard is  registered in the `installed_paths` config as a one-standard directory, a `Reference sniff ...  does not exist` error will be thrown and no sniffs will be run.

Bug introduced in squizlabs#1450 (by me, sorry!).

The reason for this is that the `getInstalledStandardPath()` method expects a string, but will  in actual fact receive a `SimpleXMLElement` object when a complete standard is included by  another standard. This was previously not documented in the function doc block as a possibility.

While in the original code, the `$standards` name was only used as a concatenated string - which  triggered the `SimpleXML.toString()` method -, the fix in squizlabs#1450 introduced a strict comparison  without the `toString()` conversion, causing the bug.

Installed_paths | string `$standards` | SimpleXMLElement `$standards`
--------------- | ------------------- | ----------------------
"Standards" directory | ✅ | ✅
one-standard directory | ✅ | ❌ <= This is what this fixes.

While debugging this, I also noticed that this method is called for every single ref in the  ruleset and would go through the whole loop before returning `null`, while for `refs` which  refer to a custom xml ruleset or to a sniff or sniff category, `null` can be returned a lot  earlier and for paths which don't resolve to a ruleset, part of the loop can be skipped.
@jrfnl jrfnl force-pushed the feature/fix-get-installated-path-vs-one-dir-standards branch from 10566ea to 7cfadc6 Compare July 29, 2017 05:44
jrfnl added a commit to WordPress/WordPress-Coding-Standards that referenced this pull request Aug 1, 2017
Version 0.4.x of the DealerDirect Composer plugin registers each standard individually as if they were root-directory standards, not a collection of standards which exposes a bug in PHPCS 3.x.

Refs:
* squizlabs/PHP_CodeSniffer/pull/1581
* PHPCSStandards/composer-installer/issues/33
jrfnl added a commit to WordPress/WordPress-Coding-Standards that referenced this pull request Aug 1, 2017
Version 0.4.0 of the DealerDirect Composer plugin registers each standard individually as if they were root-directory standards, not a collection of standards which exposes a bug in PHPCS 3.x.

Refs:
* squizlabs/PHP_CodeSniffer/pull/1581
* PHPCSStandards/composer-installer/issues/33
@jrfnl
Copy link
Contributor Author

jrfnl commented Aug 6, 2017

@gsherwood Is there anything I can do or clarify about this PR to move this forward ?

@gsherwood
Copy link
Member

Is there anything I can do or clarify about this PR to move this forward ?

No, it's just my time. I'm trying to find a little time to work on PHPCS each day, but it's not working out so well. But I'll get through the backlog.

@gsherwood gsherwood added this to the 3.1.0 milestone Aug 6, 2017
@jrfnl
Copy link
Contributor Author

jrfnl commented Aug 6, 2017

@gsherwood ❤️

@gsherwood gsherwood changed the title Hotfix for when one-standard directories are being registered in installed_paths Sniffs not loaded when one-standard directories are being registered in installed_paths Aug 16, 2017
gsherwood added a commit that referenced this pull request Aug 16, 2017
@gsherwood
Copy link
Member

I looked into where this was coming from and I don't think there is any problem with your previous fix. I think the source of this problem was actually from a call to expandRulesetReference where I thought I was passing in a string but I was really passing into a SimpleXML object. It was being implicitly cast to a string in many places, so I just made it explicit before I made the call.

I'll take a look at the performance bit of the PR now.

@gsherwood gsherwood merged commit 7cfadc6 into squizlabs:master Aug 16, 2017
gsherwood added a commit that referenced this pull request Aug 16, 2017
@jrfnl jrfnl deleted the feature/fix-get-installated-path-vs-one-dir-standards branch August 16, 2017 01:41
@gsherwood
Copy link
Member

Those performance changes looked good, thanks. I removed the string casts and param comment change now that the method should only accept strings.

@jrfnl
Copy link
Contributor Author

jrfnl commented Aug 16, 2017

Excellent. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants