Add PHP Code Sniffer

[ataylorme]

Multidev created successfully! http://pr-50-pantheon-wp-best-practices.pantheonsite.io/

[ataylorme]

@schrapel mind reviewing this? Basically CircleCI will fail if PHP CodeSniffer fails WordPress coding standards.

I ignored web/private/scripts as there were errors related to not using WordPress functions, like escaping and wp_remote_get instead of curl. However, these scripts run independently outside of WordPress and the site so I think it's safe to ignore them. Hopefully your editor will pick up on the config file and ignore them as well.

[tobeycodes]

Will do later today or tomorrow AM

[ataylorme]

@stevector and @schrapel - now I remember why I didn't install this in require-dev and installed it globally instead. The --dev flag of composer install is deprecated and no longer works.

Since there isn't a way to install dev dependencies only Composer installs all the third-party WordPress dependencies as well. This makes code sniffing difficult as we would either need to 1) exclude all the third-party dependencies or 2) only include our custom code dependencies.

I'd prefer not to do either of those. Having the code sniffer installed globally allows us to run the code sniffer before third party dependencies are downloaded.

[tobeycodes]

I can not find anything on Composers website where it has been deprecated. My understanding is --dev is default behaviour and --no-dev should be use for builds https://getcomposer.org/doc/03-cli.md#install

We currently do the former and exclude third-party dependencies in the form of wordpress themes/plugins.

I am very much a fan of the 12factor approach https://12factor.net/dependencies. I want to be able to run phpcs on my local machine, circleci, some random other server and know that I am always using the same version of phpcs and the results will always be the same. I don't want to be tightly coupled to any one service.

Happy to agree to disagree on this though as I do see that updating a phpcs for dependencies frequently can be a pain

[ataylorme]

@schrapel,

I want to be able to run phpcs on my local machine, circleci, some random other server and know that I am always using the same version of phpcs and the results will always be the same. I don't want to be tightly coupled to any one service.

I completely agree with this philosophy.

I can not find anything on Composers website where it has been deprecated. My understanding is --dev is default behaviour and --no-dev should be use for builds

The deprecated warning comes from Composer itself when running composer install --dev.

You are using the deprecated option "dev". Dev packages are installed by default now.

While dev packages are installed by default so are packages listed under require. In this case, the third-party themes, plugins and other items.

In order to code sniff only our custom code we need to run phpcs on the contents of this repository without any additional WordPress dependencies installed via Composer. After code sniffing is done then download the standard dependencies listed in the require section of composer.json.

As it stands now composer install downloads everything, including the require and require-dev sections of composer.json. This means code sniffing is running on the fully built site, not just the custom code in this repository, this failing as the dependencies have violations.

The --no-dev option for composer install will exclude require-dev items but we need the opposite - a way to only download require-dev items without require items, which doesn't seem to exist.

To see what I mean look at the logs for failing build 384. There are multiple code violations for third-party themes and plugins.

The only way I can think to get around this is to install phpcs globally via Composer, run the sniff, then run a standard composer install to download the dependencies.

Paths now between code in this repository and Composer directories are mixed - e.g. web/wp-content/themes. So we would need to explicitly ignore dependency paths when running phpcs, which doesn't seem like a better solution as those would need to be updated whenever dependencies are updated.

One other possibility is to create a second copy of this repository before installing Composer dependencies and code sniff that directory. bin/test-wordpress-coding-standards.sh could maybe do this in a temp directory but I don't like this workaround either as it wouldn't be easy to recreate locally.

If you have a better solution I'm open to options.

[ataylorme]

The only way I can think to get around this is to install phpcs globally via Composer, run the sniff, then run a standard composer install to download the dependencies.

I created the php-codesniffer-tmp-dir branch to show this.

It's working and I'm able to run the bin/test-wordpress-coding-standards.sh locally but is still fairly slow. Installing phpcs with Composer globally on CircleCI was faster but not easily reproducible locally.

[stevector]

In order to code sniff only our custom code we need to run phpcs on the contents of this repository without any additional WordPress dependencies installed via Composer.

If the problem is that too-complex logic is needed to tell the difference between custom code and outside dependencies, that seems like a standalone problem that will have ramifications beyond code sniffing.

I suggest any custom code go in a directory like plugins/custom and themes/custom rather than just plugins or themes.

Without that, .gitignore will also have to be updated just as often as code sniffing includes/excludes. Also for the human developers, how are they supposed to look a plugins directory with multiple subdirectories and know which are custom and which are external? If the answer is a simple naming convention, then that naming convention can be used by the code sniffer to include/exclude.

[tobeycodes]

If you have code in themes/custom/ you are going to then bundle code to recognise this folder as another themes/plugin directory. I think this is more complicated that just documenting the process for including a new folder in the .gitignore, phpcs.xml.dist.

[ataylorme]

I suggest any custom code go in a directory like plugins/custom and themes/custom rather than just plugins or themes.

I agree this would be ideal but WordPress does not support this and creating a bundler, as @schrapel said, is a pain.

.gitignore will also have to be updated just as often as code sniffing includes/excludes

.gitignore uses negation then selective inclusion by ignoring everything in wp-content/plugins and wp-content/themes then selectively including custom code. So, yes, .gitignore will need to be edited when new custom code is added but not when dependencies are added.

I'll try editing code sniffing to perform in a similar manner - including custom code only rather than trying to ignore dependencies.

[tobeycodes]

That's what we do

<?xml version="1.0"?>
<ruleset name="Toby">
  <description>Toby Coding Standards</description>

  <!-- Scan all files in directory -->
  <file>./config</file>
  <file>./web/private</file>
  <file>./web/index.php</file>
  <file>./web/wp-config.php</file>
  <file>./web/wp-content/plugins/toby-core</file>
  <file>./web/wp-content/plugins/toby-events</file>

  <!-- Scan only PHP files -->
  <arg name="extensions" value="php"/>

  <!-- Show colors in console -->
  <arg value="-colors"/>

  <!-- Show sniff codes in all reports -->
  <arg value="ns"/>

  <!-- Use WordPress-VIP as a base -->
  <rule ref="WordPress-VIP"/>
</ruleset>

[ataylorme]

@schrapel - thanks for the input. I found having phpcs parse all files, especially after composer install has run and dependencies are downloaded, to be slow.

Rather than include items in the code sniffing rules I'm running phpcs explicitly on the custom code files/directories in bin/test-wordpress-coding-standards.sh. By grabbing just the directories we need and catching PHP only files with find this runs very quickly.

See CircleCI build 393.

I've also split up the old bin/build.sh into multiple part (Composer and gulp) so gulp can only be ran when code sniffing passes and we want to deploy to Pantheon.

I created bin/local-build.sh and updated the README to help with downloading dependencies and code sniffing locally. bin/test-wordpress-coding-standards.sh can also be run locally (assuming compoer install has run).

I think this is a good middle ground. Yes, the custom code paths will need to be updates in .gitignore and bin/test-wordpress-coding-standards.sh but the addition of new custom plugins/themes should be rare - at least more so than changing dependencies.

@stevector would love to get your thoughts as well.

[ataylorme]

That's what we do

@schrapel your example is working well. I was mistakenly passing the current directory to the code sniffer like so: ./vendor/bin/phpcs --standard=phpcs.ruleset.xml .

Removing that speeds things up considerably.

[tobeycodes]

@ataylorme is there a reason the file is named phpcs.ruleset.xml. My understanding would be ruleset would apply to something like WPCS or PSR and then we would bundle a file called phpcs.xml.dist . You could then drop the --standard as this would be looked at by default. What do you think?

[ataylorme]

is there a reason the file is named phpcs.ruleset.xml My understanding would be ruleset would apply to something like WPCS or PSR and then we would bundle a file called phpcs.xml.dist . You could then drop the --standard as this would be looked at by default.

Just my lack of understanding. I'll give this a go