squizlabs / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
10.67k stars 1.48k forks source link

relative ruleset and sniff rules don't work together #641

Closed ofbeaton closed 9 years ago

ofbeaton commented 9 years ago

My original working setup: Install phpcs in the PATH using pear, drop a symlink to my custom ruleset into the PEAR CodeSniffer dir with other rulesets like PSR2. Run phpcs from my project. Everything works great.

My new setup: Install phpcs through composer, and specify my custom style through the command line. My rule declarations in my ruleset no longer work.

My ruleset file looks something like this:

<ruleset name="MyRules">
  <rule ref="MyRules.Commenting.MyCommentChecker">
    <severity>3</severity>
  </rule>
</ruleset>

And my ruleset dir looks like:

/MyRules/Sniffs
/MyRules/Sniffs/Commenting
/MyRules/Sniffs/Commenting/MyCommentCheckerSniff.php
/MyRules/ruleset.xml

and I am running phpcs with:

bin/phpcs --standard=MyRules --extensions=php src/

I get an error, saying PHP Fatal Error: Uncaught exception 'PHP_CodeSniffer_Exception' with message 'Referenced sniff "MyRules.Commenting.MycommentChecker" does not exist.

I'm oversimplifying, as what I actually do is run a relative ruleset.xml in my project, which then includes a relative ruleset that is downloaded in my vendor dir (one for my all projects) that looks like the above. Maybe the problem lies in that extra complexity but I have a feeling that it's in the <rule ref="MyRuleset. part instead not working for relative rulesets.

Or maybe you've changed how we are supposed to write these rules to change the severity of sniffs. Please let me know!

gsherwood commented 9 years ago

I haven't looked into this issue entirely, but this bit:

<rule ref="MyRules.Commenting.MyCommentChecker">
  <severity>3</severity>
</rule>

Is probably not doing what you want. You can only set the severity of a message; not an entire sniff. So if should have 4 parts in the ref like the example on here:https://github.com/squizlabs/PHP_CodeSniffer/wiki/Annotated-ruleset.xml

The specific example on there is this:

 <rule ref="Generic.Commenting.Todo.CommentFound">
  <message>Please review this TODO comment: %s</message>
  <severity>3</severity>
 </rule>

This probably has nothing to do with your problem though.

When you include a reference like this, PHPCS looks at all rulesets that have been included. Even if you including a ruleset using a relative reference, it should be found as long as you use the standard name in your refs after you included the standard.

It's hard to know if everything is working for you without actually seeing both rulesets, but I'll try and replicate using test rulesets when I get a chance.

Your other option, given it was working when you had it installed, is to just tell PHPCS that it is an installed standard. This allows any ruleset to use MyRules without having to include the standard itself. PHPCS will find and include it automatically. Some info is here: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Configuration-Options#setting-the-installed-standard-paths

So you'd use it by running: phpcs --config-set installed_paths /path/to/MyRules

ofbeaton commented 9 years ago

Thanks for the quick reply Greg, and you were right, I went and looked at my actual ruleset and I did not transfer it into the ticket properly. My apologies.

I am in fact doing:

<rule ref="MyRules.Commenting.Fixme.CommentFound">
  <severity>3</severity>
</rule>

With my file being:

/MyRules
/MyRules/ruleset.xml
/MyRules/Sniffs/
/MyRules/Sniffs/Commenting/
/MyRules/Sniffs/Commenting/FixmeSniff.php

So this was indeed not actual my problem.

Setting installed_paths helped me figure out what my actual problem was, and it is nice to hear that this is an option!

Relative paths work fine, I am sorry to have troubled you with a problem on my end.

My problem was that my Ruleset name did not match the name of the directory it was in. The ruleset name must match the name of the directory the ruleset file is found in.

I had something like this:

/mycompany/acomposerpackagename/ruleset.xml

But the name in ruleset.xml did not match acomposerpackagename it was more generic like mycompany. This led to the confusing error message that mycompany was not found when specifying the rule ref=. I did not get this message originally because I used a soft link into the rulesets directory to expose my ruleset, with the proper mycompany directory name.

It seems to me like the ruleset name="" should be authoritive and phpcs should simply keep track of where each ruleset maps to disk. The containing directory should not matter.

This is not a big hurdle however, in my composer package I moved my ruleset.xml into a subdirectory to /mycompany/acomposerpackagename/mycompany/ruleset.xml and my relative path and severity instructions started working properly with no errors.

I was also really happy you decided to add an automated fixer, this is a great feature. The name of phpcbf is confusing however, since phpcb is the Php Code Browser utility most of us use. I am not sure why you did not use phpcsf. Nevertheless, thank you for including the fixer it has proven invaluable!

gsherwood commented 9 years ago

It seems to me like the ruleset name="" should be authoritive and phpcs should simply keep track of where each ruleset maps to disk. The containing directory should not matter.

I'd have to be very careful with something like that because of potential BC breaks. That name is used as a "friendly name" of the standard to be printed in the installed standards list and inside docs and output, so there is no guarantee that all custom standards out there would have it matching the dir name and thus keep working after an upgrade.

I know the Wordpress ones would break for sure because I have them checked out and can see that use spaces in the name attribute and not the directories. Forcing them to change one of both of those values would need a strong benefit behind it.

A possible alternative would be to add a new attribute (code maybe) that could be used to set the codename of the standard no matter where it is installed. The name attribute could continue to be used for the friendly name and the dirname could continue to be used for the codename if the standard doesn't have a specific code set.

Think that is workable?

ofbeaton commented 9 years ago

That's a good point about name. code could work, but I myself question the usefulness. I'm not sure if my use case is common, and was easily solved using subdirs. I think to answer this you have to think what would a standard in a packagist repo look like? We usually use src so a subdir for the standard isn't horrible nesting or anything.

I wonder if there is some elegant way to use the autoloader and namespaces, but I imagine not because there isn't an easy way to "find all classes in this namespace" before they are loaded, so that's a dead end.

I solved my issue so if you think it will help others great, if not then I'm not itching for a feature request.

Great community support though! Thank you for keeping it up.

gsherwood commented 9 years ago

code could work, but I myself question the usefulness

Fair enough. If it comes up again, at least there is an option to go with.

Great community support though! Thank you for keeping it up.

Thanks. I appreciate that.

AleksandarSavic95 commented 5 years ago

For anyone else having problems with importing custom rules to PhpStorm, make sure that:

the ruleset name must match the name of the directory the ruleset file is found in.

This is already mentioned by @ofbeaton, but I stress it again, since that is the only way PhpStorm will read Your configuration properly.

Make sure You did the following: image