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

klausi’s picture

Assigned: Unassigned » das-peter

Yes, 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.

das-peter’s picture

@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 :)

ericduran’s picture

we should just moved everything up a level.

Is this ok with everyone?

We probably should do it sooner rather than later.

ericduran’s picture

if 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.

klausi’s picture

Yes, I'm fine with moving everything up a level. Not sure how many people use drush to retrieve drupalcs though.

das-peter’s picture

Also 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 drush I'm always thinking of specific installations.

ericduran’s picture

Ok, 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 :)

klausi’s picture

You 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.

ericduran’s picture

@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.

das-peter’s picture

What 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

klausi’s picture

@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.

das-peter’s picture

people have to copy/symlink the Git checkout to the PHP_CodeSniffer directory anyway,

Unfortunately that's not fully correct. All you've to do is to point the parameter --standard to the folder the ruleset.xml is located.
E.g. my eclipse integration runs it like this:

"C:\Program Files (x86)\PHP\php.exe" -c C:\Users\DAS-PE~1\AppData\Local\Temp\zend_debug\session1379400227174309905.tmp -d asp_tags=off "C:\Program Files\eclipse\plugins\org.phpsrc.eclipse.pti.tools.codesniffer_1.3.0.R20111119000000\php\tools\phpcs.php" --report=xml --standard=W:\htdocs\drupalcs\DrupalCodingStandard W:\htdocs\drupalcs_TestFiles\good.php

@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.

klausi’s picture

I 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/Drupal

Unfortunately you cannot run phpcs --standard=/path/to/drupalcs/ruleset.xml at the moment because then the customizations in our ruleset.xml do not work.

das-peter’s picture

I'm absolutely fine with the approach in #13

ericduran’s picture

Same here, I'm all for it. :)

klausi’s picture

Assigned: das-peter » klausi

Great, I'll do it.

klausi’s picture

First 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:

find . -path './.git' -prune -o -type f -exec sed -i "s/DrupalCodingStandard/Drupal/g" {} \;
das-peter’s picture

Thanks 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 :)

das-peter’s picture

I've just posted a pull request for PHP_CodeSniffer with a patch that solves two issues:
https://github.com/pear/PHP_CodeSniffer/pull/3

das-peter’s picture

Status: Active » Fixed

Yay, 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!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.