Add our own phpcs.xml file

This includes generic and Squiz sniffs, are these part of the sniffs used by Coder Sniffer?

Regarding #4: yes these are part of the official Drupal coding standard ruleset, I verified with @klausi.

1. diff --git a/phpcs.xml b/phpcs.xml
   new file mode 100644
   

   I would rename the file to phpcs.xml.dist. This is a convention indicating to developers that it is OK to copy and rename this file to phpcs.xml if they need to make any modifications for their project's needs.

   This pattern is also used for the phpunit.xml.dist file.

2. +++ b/phpcs.xml
   @@ -0,0 +1,187 @@
   +<?xml version="1.0"?>
   +<ruleset name="Drupal core coding standards">
   +  <description>Coding standard for Drupal core.</description>
   +  <file>./core</file>
   

   Add some documentation explaining the purpose of this file. It would be great if it mentions that this file is intended for automatic coding standards checks on the DrupalCI testbot. Also mention that this is a blacklist to temporarily disable checks while the work is still in progress.

3. +++ b/phpcs.xml
   @@ -0,0 +1,187 @@
   +  <exclude-pattern>./core/vendor/*</exclude-pattern>
   +  <exclude-pattern>./core/assets/vendor/*</exclude-pattern>
   

   Maybe we can put the file in the core/ folder? This is also where the phpunit.xml.dist file lives. We also don't need to scan anything outside the core folder, so it makes sense to put it there.

4. +++ b/phpcs.xml
   @@ -0,0 +1,187 @@
   +  <rule ref="~/.composer/vendor/drupal/coder/coder_sniffer/Drupal">
   +    <exclude name="Drupal.Array.Array"/>
   

   Instead of referring to the home folder, just assume that the ruleset is installed globally (ie. <rule ref="Drupal">).

   When this will be implemented on DrupalCI we can either symlink the Drupal ruleset inside the PHP CodeSniffer installation, or copy the XML file and replace the path.

   The same for the "Squiz" rule, this can also be considered to be installed globally.

The list of sniffs is not complete, but I guess this list only includes what is being actively violated? I will run the full test now and report back

I did a full scan with the supplied ruleset, but had many violations. I didn't count them but there were tens of thousands :)

It seems we are missing a whole bunch of sniffs in the blacklist. For example Squiz.Scope.MethodScope:

FILE: ...e/tests/Drupal/Tests/Core/Asset/CssCollectionGrouperUnitTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 37 | ERROR | Visibility must be declared on method "testGrouper"
    |       | (Squiz.Scope.MethodScope.Missing)
----------------------------------------------------------------------

Going to work on a script to generate this file automatically.

Here's a script. Here's how to use it:

First, install the latest dev version of the coder module, and install its dependencies (this includes PHP CodeSniffer):

# Go to the drupal root folder.
$ cd /path/to/drupal

# Download the latest development version of Coder.
$ drush dl coder --dev --package-handler=git_drupalorg

# Install coder dependencies. This includes PHP CodeSniffer.
$ cd modules/coder
$ composer install

# Symlink the Drupal ruleset into the PHP Codesniffer installation.
$ cd vendor/squizlabs/php_codesniffer/CodeSniffer/Standards/
$ ln -s ../../../../../coder_sniffer/Drupal .

Secondly, do a full scan of the core folder with the Drupal standard and save the result in a JSON file. Note that this can take around 15 minutes.

# Go to the drupal root folder.
$ cd /path/to/drupal

# Do a full scan of the core folder and save the result in a JSON file.
$ ./modules/coder/vendor/bin/phpcs --standard=Drupal --extensions=css,inc,info,install,js,module,php,profile,test,theme --ignore=core/vendor,core/assets/vendor --report=json > report.json

Now, finally, run the script and feed it the JSON file:

./phpcs-generate.php report.json

I had a problem generating the JSON report, there is a bug in PHP CodeSniffer that causes the JSON to be invalid if any of the errors or warnings contain a newline or tab character. I've attached a patch that works around this in PHPCS, and created a pull request: Issue #716: Prevent generating invalid json reports when messages contain newlines or tabs.

Hmm I'm still getting an invalid JSON file. The file is 6MB large which makes it a bit difficult to find where it goes wrong :-/

Reworked the script to use XML instead. This works a lot better. I do have to edit the XML file to remove some invalid characters such as EOL characters which end up in the XML file.

If you install PHP CodeSniffer and the ruleset using the instructions from #10, you can then generate the coding standards report with:

$ ./modules/coder/vendor/bin/phpcs --standard=phpcs.xml --extensions=css,inc,info,install,js,module,php,profile,test,theme --ignore=core/vendor,core/assets/vendor --report=xml core/ > report.xml

You'll need to edit the XML to remove a couple of illegal characters, then finally generate the phpcs.xml file:

$ ./phpcs-generate.php report.xml > phpcs.xml

Here's the phpcs.xml file as generated by the script against current HEAD. Will now run a full coding standards check using this ruleset. If it returns green I'll roll up a proper patch.

Running the full scan with the generated phpcs.xml file I get the following result:

<?xml version="1.0" encoding="UTF-8"?>
<phpcs version="2.3.4">
<file name="/home/pieter/v/drupal/drupal/core/modules/system/tests/fixtures/HtaccessTest/access_test.module" errors="0" warnings="1" fixable="0">
 <warning line="1" column="1" source="Internal.NoCodeFound" severity="5" fixable="0">No PHP code was found in this file and short open tags are not allowed by this install of PHP. This file may be using short open tags but PHP does not allow them.</warning>
</file>
<file name="/home/pieter/v/drupal/drupal/core/tests/Drupal/Tests/Core/Asset/css_test_files/comment_hacks.css" errors="0" warnings="1" fixable="0">
 <warning line="1" column="1" source="Internal.Tokenizer.Exception" severity="5" fixable="0">File appears to be minified and cannot be processed</warning>
</file>
</phpcs>

For some reason the "Internal" sniffs are still being checked, even though they are excluded in the phpcs.xml file. This is probably a bug in PHP CodeSniffer.

All of these failures are very specific, they deal with minified files, or PHP files that actually do not contain PHP. We can work around them by excluding these files.

Good catch with the comment_hacks.css file. Updated script to include some helpful comments, so it's clear why we are excluding these files.

This is how the generated file looks right now. The newlines I added manually, I didn't figure out how to get DOMDocument to generate newlines.

I will now run a full coding standards test against the latest HEAD, then regenerate the file, then do a full test with the generated file to see if we now catch all violations.

I'm very +1 on this change and the approach taken to begin introducing coding standards checking. It will fit well with the current plan for coding standards fixes Drupal 8.

I think we probably should have a framework manager review too, so tagging for that.

Also bumping to major, because coding standards compliance is a huge pain point for core developers.

+++ b/core/phpcs.xml.dist
@@ -0,0 +1,98 @@
+  <arg name="extensions" value="css,inc,info,install,js,module,php,profile,test,theme"/>

I think we should not do js - that is covered by eslint. And i ponder about css too

1. +++ b/core/phpcs.xml.dist
   @@ -0,0 +1,98 @@
   +  <arg name="extensions" value="css,inc,info,install,js,module,php,profile,test,theme"/>
   

   is there a reason to not mention yml files? Theoretically they could have code styles as well at some point?

2. +++ b/core/phpcs.xml.dist
   @@ -0,0 +1,98 @@
   +  <!--Exclude third party code.-->
   +  <exclude-pattern>./core/assets/vendor/*</exclude-pattern>
   +  <exclude-pattern>./core/vendor/*</exclude-pattern>
   

   MHH, so this file is relative to /core/... even the file is already in /core, this sounds wrongish

3. +++ b/core/phpcs.xml.dist
   @@ -0,0 +1,98 @@
   +  <!--Blacklist of coding standard rules that are not yet fixed. Ref. https://www.drupal.org/node/2571965-->
   +  <rule ref="Drupal">
   

   It feels like at this point we might want to opt in first and then at some point swap it around?

Re: #28 point 3, no, we definitely want to blacklist first and then opt in one at a time as core is compliant with each rule so that we can add bot support to ensure it doesn't regress.

Re #28.2 I'll have a look if we can treat the core/ folder as the root.

I managed to overwrite my report.xml file, regenerating it and running a new test will take about 30 minutes.

Updated script. Addressed all remarks, except adding support for YML files, this is not currently covered by PHP CodeSniffer. I also like the suggestion of @alexpott to limit it only to what it does best: PHP files. We can use other tools such as JSLint to scan the other files. I bet there are good CSS scanners too.

Changes:

1. No longer scan CSS and JS.
2. Updated paths to use the core/ folder as the root folder. This was in fact already how it was implemented in the DrupalCI issue #1266444: DrupalCI should return failed for poor code style - php.

I did a full scan using this ruleset on the DrupalCI test bot and it returned 0 failures!

I will rerun this test and capture the output. I'll also run a test with one of the rules removed to check if the bot correctly fails the test if any violations are detected.

Here are the test results from the DrupalCI test bot with the patch from #1266444: DrupalCI should return failed for poor code style - php applied. The relevant part can be seen right at the end of the output.

• result-good.txt: test run with the phpcs.xml.dist file from #32. The command does not return with an error status. No coding standards violations found.
• result-bad.txt: test run with the phpcs.xml.dist file from #32 with the Drupal.Files.EndFileNewline line removed from the blacklist. The command lists the found violations and correctly returns an error status.

For some reason I thought we already fail with this issue, but yeah the idea is to track the overall status. In this case blacklisting makes sense.

This is a fantastic idea. Let's do it...

Committed 65c2f39 and pushed to 8.0.x. Thanks!