Coding standard located under an installed_path with the same directory name throws an error #1450
Conversation
This file contains hidden or 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
If a "Standards" directory is called the same as one of the standards it contains and that standard is requested an _"ERROR: the ... coding standard is not installed."_ will be thrown and the sniffs will not run. While I suppose this change in behaviour between PHPCS 3.x and PHPCS 2.x is intended to add support for "one standard" type of libraries, it breaks support for multi-standard libraries where one of the standards has the same name as the repository. The change I'm proposing in this PR will maintain support for the PHPCS 2.x behaviour - thus not breaking existing registered standards - while at the same time supporting the new feature.
This was referenced May 7, 2017
| Just curious - what does the 🔥 emoji represent in these recent PRs? |
gsherwood changed the title installed_paths in PHPCS 3.x | Thanks a lot for finding and fixing this. Yes, it was to allow installed_paths to point directly to coding standards instead of having to point to the parent dir. Unfortunately my WordPress CS install dir is not called |
| Regarding the 🔥 I've been playing a bit with commit messages lately to see if it helps in quick grasping of what a commit is all about, inspired by: |
| Thanks for merging this!
I was running into it for both WP as well as PHPCompatibility, duh me I suppose, but it was an easy fix to make ;-) |
Interesting concept. I'd personally have no hope of remembering all that :) |
| Nor me, I use what seems to make sense at the time, though I generally find that emojis can be a powerful way to underline what a commit is about. |
jrfnl added a commit to jrfnl/PHP_CodeSniffer that referenced this pull request Jul 29, 2017
…`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 | :white_checkmark: | :white_checkmark: one-standard directory | :white_checkmark: | ❌ <= 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 added a commit to jrfnl/PHP_CodeSniffer that referenced this pull request Jul 29, 2017
…`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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
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.
If a "Standards" directory is called the same as one of the standards it contains and that standard is requested, an "ERROR: the ... coding standard is not installed." error will be thrown and the sniffs will not run.
While I suppose this change in behaviour between PHPCS 3.x and PHPCS 2.x is intended to add support for "one standard" type of libraries, it breaks support for multi-standard libraries where one of the standards has the same name as the repository.
The change I'm proposing in this PR will maintain support for the PHPCS 2.x behaviour - thus not breaking existing registered standards - while at the same time supporting the new feature.