Just spent 1 hour trying to understand why my aliased ruleset.xml fails with exception.
It appeared that
alias drupalcs='phpcs --standard=/path/to/local/drupalcs/DrupalCodingStandard/ruleset.xml'
was not clear enough for me :) I did not specify the path to 'DrupalCodingStandard' properly - I included drupalcs directory in the path.
So can we have this changed to
alias drupalcs='phpcs --standard=/path/to/local/DrupalCodingStandard/ruleset.xml'
or at least
alias drupalcs='phpcs --standard=/path/to/local_drupalcs/DrupalCodingStandard/ruleset.xml'
Comments
Comment #1
klausiYes, the additional folder level has confused me as well when I joined this project. I would suggest to move everything in the DrupalCodingStandard directory to the root folder of the repository. And CodeSnifferTestFiles could be renamed to be just "Test".
Maybe there is a reason why this sub-directory exists, assigning to das-peter as he might know more about it.
Comment #2
das-peter commented@Alex: Thanks for debugging this! I hit this error too but I hadn't the time to debug it yet.
As far as I can remember this was is an artefact from early days. Idea was to ensure that the folder which contains the actual sniffs is always named "DrupalCodingStandard" - because the project was shipped as archive then.
But for now it should be fine to simple move that stuff around :)
Comment #3
ericduran commentedwe should just moved everything up a level.
Is this ok with everyone?
We probably should do it sooner rather than later.
Comment #4
ericduran commentedif we implement #1372998: Remove .info file to facilitate drush installation first then we can assume that by default the directory will be ~/.drush/drupalcs which we can probably document that better for most cases.
Comment #5
klausiYes, I'm fine with moving everything up a level. Not sure how many people use drush to retrieve drupalcs though.
Comment #6
das-peter commentedAlso fine with me.
Is there any advantage to use drush to get drupalcs? For me the tools is unrelated to specific drupal projects/installations. And if I hear
drushI'm always thinking of specific installations.Comment #7
ericduran commentedOk, I renamed the directory of the Test files to Test.
Changing the directory structure of the sniff is a little harder because the ruleset uses the directory name as the standard.
So if we moved all the sniff to the top level directory then from a download the sniff location is going to be drupalcs/Sniff this is going to require us to change the ruleset name to drupalcs which in turn will also require us to change all the classes from DrupalCodingStandard_ to drupalcs_ which is perfectly fine.
We just have to decide if we want to do all that.
This is also going to break any installation that other people might have from a git checkout.
I think is better for us to do this sooner rather than later since having the Sniff in a sub directory does seem weird.
We can chat on irc, or here :)
Comment #8
klausiYou don't have to rename the classes, you just need to make sure that your Git Checkout directory has the name "DrupalCodingStandard" instead of "drupalcs". Or, that the link that you create in PHP_CodeSniffer has the name "DrupalCodingStandard".
BTW: If you want to rename the classes, I think they should be prefixed with "Drupal". This would be consistent with Zend, Squiz, PEAR etc.
Comment #9
ericduran commented@klausi, but because the short-name is drupalcs by default a standard git checkout is going to be drupalcs. We could tell people to rename it or set up the alias with the DrupalCodingStandard name but that would required them to do it.
so, idk.
Comment #10
das-peter commentedWhat if we don't move the file but change the way how we exclude some file types in our own code?
To be able to handle those excludes in the ruleset.xml we've to make the move - but this brings other disadvantages.
Or we could provide a patch for phpCodeSniffer :) As far as I understand this bug-report is about a similar issue: http://pear.php.net/bugs/bug.php?id=19144
Comment #11
klausi@ericduran: people have to copy/symlink the Git checkout to the PHP_CodeSniffer directory anyway, so I don't think that causes any additional harm.
@das-peter: I think ruleset.xml is important and we should leverage it. Separating file types from the actual sniff code is a very powerful concept that we should follow. Polluting sniff code with file type checks is not a good idea (yes, I already did it for *.txt files, but I will look into it how we can avoid it).
So renaming our standard to "Drupal" in all classes and the install instructions seems to be the cleanest solution to me.
Comment #12
das-peter commentedUnfortunately that's not fully correct. All you've to do is to point the parameter
--standardto the folder the ruleset.xml is located.E.g. my eclipse integration runs it like this:
@klausi: I'm all for using ruleset.xml - but my main focus would be to keep the installation as easy as possible. If the user needs to know and has comply to naming conventions this could be a drawback.
Comment #13
klausiI just tested the --standard=/path/to/my/drupalcs/DrupalCodingStandards/ruleset.xml option and it failed horribly, because we reference our own sniffs in ruleset.xml and because required files with abstract classes are not included automatically. But this problem exists independently of our goal to move/rename here, so that should be handled in another issue.
So I still propose:
* Rename the standard to be just "Drupal" and rename all classes.
* Move everything in DrupalCodingStandard up one level.
The installation instructions would then be:
Once you have everything you will need to add this drupalcs/ directory into the Standards/ directory for CodeSniffer.
This can be accomplished by sym-linking the drupalcs directory into the standards folder for PHP_CodeSniffer. The code for that looks like this:
sudo ln -sv /path/to/drupalcs $(pear config-get php_dir)/PHP/CodeSniffer/Standards/DrupalUnfortunately you cannot run
phpcs --standard=/path/to/drupalcs/ruleset.xmlat the moment because then the customizations in our ruleset.xml do not work.Comment #14
das-peter commentedI'm absolutely fine with the approach in #13
Comment #15
ericduran commentedSame here, I'm all for it. :)
Comment #16
klausiGreat, I'll do it.
Comment #17
klausiFirst thing accomplished: renamed our standard to "Drupal" :-)
When moving everything up one level I encountered a problem: I created the symlink in the Standards folder of CodeSniffer as usual, pointing to my drupalcs root folder. But phpcs refuses to work with symlinks where the link name is not the same thing as the folder name it points to ("drupalcs" vs. "Drupal"). It uses the absolute paths to the sniff files to load the classes, so the class names do not match and PHP dies with a fatal error :-(
So i left the additional subfolder in place and just renamed it to "Drupal".
Here is the command I used to automatically replace DrupalCodingStandard everywhere:
Comment #18
das-peter commentedThanks klausi!
Maybe we can move the stuff once I figured out how to get the ruleset working when using
--standard=.... I think there's potential to solve both issues at the same time :)Comment #19
das-peter commentedI've just posted a pull request for PHP_CodeSniffer with a patch that solves two issues:
https://github.com/pear/PHP_CodeSniffer/pull/3
Comment #20
das-peter commentedYay, the follow-up pull request I made finally got in:
https://github.com/squizlabs/PHP_CodeSniffer/pull/16#issuecomment-4046944
https://github.com/squizlabs/PHP_CodeSniffer/commit/9ce8010f30b44ef84ccc...
This means the latest version of PHP_CodeSniffer will work with our ruleset!