acquia / blt

Acquia's toolset for automating Drupal 10 development, testing, and deployment.
https://docs.acquia.com/blt/
GNU General Public License v2.0
443 stars 395 forks source link

Allow PHPCS to use DrupalPractice standard #1786

Closed christopher-hopper closed 7 years ago

christopher-hopper commented 7 years ago

My system information:

When I run this command:

vendor/bin/blt validate:phpcs

I get the following output:

Iterating over fileset files.php.custom.modules...
Iterating over fileset files.php.custom.themes...
Iterating over fileset files.php.tests...

And I expected this to happen:

Iterating over fileset files.php.custom.modules...
FILE: ..._webapps/src/Plugin/Block/SingleItemBlock.php
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 30 | WARNING | #description values usually have to run through t()
    |         | for translation
----------------------------------------------------------------------
Iterating over fileset files.php.custom.themes...
Iterating over fileset files.php.tests...

I would like to suggest we make use (or allow use of) both the Drupal and DrupalPractice standards that are included in drupal/coder. Currently you hard-code a path the Drupal standard, but this is limiting.

Since drupal/coder version 8.2.11 [8.x-2.11], the package has supported use of Composer installer plugins, such as dealerdirect/phpcodesniffer-composer-installer, to auto-detect and configure available PHPCS standards found in package dependencies. This means that on any call to composer require, composer install or composer update, the path to new standards is automatically configured and the standards are available by name.

composer require --dev drupal/coder:^8.2.11
composer require --dev dealerdirect/phpcodesniffer-composer-installer
vendor/bin/phpcs -i
The installed coding standards are MySource, PEAR, PHPCS, PSR1, PSR2, Squiz, Zend, Drupal and DrupalPractice

From this point you can use the standards by name, and even specify multiple standards when running PHPCS commands.

vendor/bin/phpcs --standard=Drupal,DrupalPractice docroot/modules/custom/**/*

When I do this on my project, additional violation warnings, not detected by BLT, are given. Many are very helpful, like this from a custom Block class:

----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 30 | WARNING | #description values usually have to run through t()
    |         | for translation
----------------------------------------------------------------------
thom8 commented 7 years ago

This could be handled by templating a phpcs.xml file which would live in the root of the project.

For example -- https://github.com/thom8/drupalvm-live/blob/a6dcf39921c729f5fc0ecba8c0dff56a02754328/phpcs.xml

This has the added benefit of being able to reuse this configuration in an IDE like PHPStorm, so you get immediate feedback on coding violations.

https://www.jetbrains.com/help/phpstorm/php-code-sniffer.html#appointCodingStyle

christopher-hopper commented 7 years ago

Agree @thom8

That was going to be next after getting the hardcoded standards path fixed. I would like to see BLT generate a phpcs.xml.dist. It could simply set the paths included/excluded as per project settings and ensure the base standards were used.

For this issue though, baby steps. Get the standards registered properly, so hardcoded paths can be removed from the calls to PHPCS.

christopher-hopper commented 7 years ago

As I'm not familiar with the BLT code, if someone can point me in the right direction, I'd be happy to create an initial PR for comment.

thom8 commented 7 years ago

@christopher-hopper this is the Class for the validate:phpcs namespace.

https://github.com/acquia/blt/blob/8.x/src/Robo/Commands/Validate/PhpcsCommand.php

Perhaps it could skip any configuration if a phpcs.xml is found in the project root as phpcs automatically uses this for it's default config.