PHPCS is invoked inconsistently between blt validate and git pre-commit hooks

[malikkotob]

My system information:

• Operating system type: macOS
• Operating system version: Mojave 10.14
• BLT version: 9.2.3

Output of blt doctor:

vagrant@local:/var/www/project$ blt doctor
+---------------------------+----------------------------------------------------+
| Property                  | Value                                              |
+---------------------------+----------------------------------------------------+
| %paths.%root              | /var/www/project/docroot                               |
| %paths.%site              | sites/default                                      |
| %paths.%modules           | sites/all/modules                                  |
| %paths.%themes            | sites/all/themes                                   |
| %paths.%config-sync       | /var/www/project/config/default                        |
| %paths.%files             | sites/default/files                                |
| %paths.%temp              | /tmp                                               |
| %paths.%private           | /var/www/project/files-private                         |
| admin-theme               | seven                                              |
| alias-searchpaths.0       | /var/www/project/drush/sites                           |
| blt-version               | 9.2.3                                              |
| bootstrap                 | Successful                                         |
| composer-version          | Composer version 1.8.2 2019-01-29 15:00:53         |
| config-sync               | /var/www/project/config/default                        |
| db-driver                 | mysql                                              |
| db-hostname               | localhost                                          |
| db-name                   | drupal                                             |
| db-password               | drupal                                             |
| db-port                   | 3306                                               |
| db-status                 | Connected                                          |
| db-username               | drupal                                             |
| drupal-settings-file      | sites/default/settings.php                         |
| drupal-version            | 8.6.7                                              |
| drush-alias-files.0       | /var/www/project/drush/sites/project.site.yml              |
| drush-cache-directory     | /home/vagrant/.drush/cache                         |
| drush-conf.0              | /var/www/project/vendor/drush/drush/drush.yml          |
| drush-conf.1              | /var/www/project/drush/drush.yml                       |
| drush-conf.2              | /var/www/project/docroot/sites/default/local.drush.yml |
| drush-script              | /var/www/project/vendor/bin/drush                      |
| drush-temp                | /tmp                                               |
| drush-version             | 9.5.2                                              |
| files                     | sites/default/files                                |
| install-profile           | project_lightning                                      |
| modules                   | sites/all/modules                                  |
| php-bin                   | /usr/bin/php7.1                                    |
| php-conf.0                | /etc/php/7.1/cli/php.ini                           |
| php-os                    | Linux                                              |
| private                   | /var/www/project/files-private                         |
| root                      | /var/www/project/docroot                               |
| site                      | sites/default                                      |
| stacks.drupal-vm.inited   | true                                               |
| stacks.dev-desktop.inited | false                                              |
| temp                      | /tmp                                               |
| theme                     | stark                                              |
| themes                    | sites/all/themes                                   |
| uri                       | http://local.blted8.com                            |
+---------------------------+----------------------------------------------------+
+--------------------------------------+--------------------------------------------------------------+
| Check                                | Problem                                                      |
+--------------------------------------+--------------------------------------------------------------+
| BehatCheck:checkBaseUrl:uri          | base_url in tests/behat/local.yml does not match the site    |
|                                      | URI.                                                         |
|                                      |   Behat base_url is set to                                   |
|                                      | http://local.project.com.                                        |
|                                      |   Drush site URI is set to                                   |
|                                      | http://local.blted8.com.                                     |
| ConfigCheck:checkGitConfig           | Git repositories are not defined in blt.yml.                 |
|                                      |   Add values for git.remotes to blt.yml to enabled automated |
|                                      | deployment.                                                  |
| NodeCheck:checkNodeVersionFileExists | Neither .nvmrc nor .node-version file found in repo root.    |
| WebUriCheck:checkUriResponse         | Did not get a response from http://local.blted8.com          |
|                                      | Is your *AMP stack running?                                  |
|                                      | Is your /etc/hosts file correctly configured?                |
|                                      | Is your web server configured to serve this URI from         |
|                                      | /var/www/project/docroot?                                        |
|                                      | Is options.uri set correctly in                              |
|                                      | /var/www/project/docroot/sites/default/local.drush.yml?          |
+--------------------------------------+--------------------------------------------------------------+
[error]  BLT Doctor discovered one or more critical issues.

When I run this command:

blt validate

I get the following output:

vagrant@local:/var/www/project$ blt validate
> tests:composer:validate
Validating composer.json and composer.lock...
> tests:php:lint
Linting PHP files...
Iterating over fileset files.php.tests...
> tests:phpcs:sniff:all
[ExecStack] /var/www/project/vendor/bin/phpcs
[ExecStack] Running /var/www/project/vendor/bin/phpcs in /var/www/project
.... 4 / 4 (100%)


Time: 199ms; Memory: 8MB

[ExecStack] Done in 0.261s
> tests:yaml:lint:all
Validating yaml syntax for all custom modules and exported config...
Iterating over fileset files.yaml...
 62/62 [============================] 100%
> tests:twig:lint:all
Validating twig syntax for all custom modules and themes...
All Twig files contain valid syntax.

And I expected this to happen:

PHPCS to flag the same issues it is flagging as part of its execution during the git pre-commit hook:

project (master *+)$ git commit -m 'PRO-0: Run blt acsf init recipe.'
Executing .git/hooks/pre-commit...
> tests:phpcs:sniff:files
Sniffing directories containing changed files...
[File\Write] Writing to /Users/malik.kotob/Sites/project/tmp/phpcs-fileset.
[ExecStack] '/Users/malik.kotob/Sites/project/vendor/bin/phpcs' --file-list='/Users/malik.kotob/Sites/project/tmp/phpcs-fileset' -l
...W..EE.E.WE....W 18 / 18 (100%)



FILE: /Users/malik.kotob/Sites/project/docroot/sites/g/SimpleRest.php
---------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
---------------------------------------------------------------------------------------
 92 | WARNING | Potential security problem: SSL peer verification must not be disabled
---------------------------------------------------------------------------------------


FILE: /Users/malik.kotob/Sites/project/docroot/sites/g/settings.php
--------------------------------------------------------------------------------------------------
FOUND 11 ERRORS AND 5 WARNINGS AFFECTING 13 LINES
--------------------------------------------------------------------------------------------------
  16 | ERROR   | [x] Doc comment short description must end with a full stop
  33 | ERROR   | [ ] Doc comment short description must be on a single line, further text should
     |         |     be a separate paragraph
  35 | ERROR   | [x] Perl-style comments are not allowed; use "// Comment" instead
  36 | ERROR   | [x] Perl-style comments are not allowed; use "// Comment" instead
  39 | ERROR   | [x] Doc comment short description must end with a full stop
  70 | WARNING | [ ] Line exceeds 80 characters; contains 100 characters
  70 | ERROR   | [x] Perl-style comments are not allowed; use "// Comment" instead
  71 | WARNING | [ ] Line exceeds 80 characters; contains 127 characters
  71 | ERROR   | [x] Perl-style comments are not allowed; use "// Comment" instead
  72 | WARNING | [ ] Line exceeds 80 characters; contains 219 characters
  72 | ERROR   | [x] Perl-style comments are not allowed; use "// Comment" instead
  85 | ERROR   | [ ] If the line declaring an array spans longer than 80 characters, each element
     |         |     should be broken into its own line
 191 | WARNING | [ ] Variable $config_directories is undefined.
 198 | ERROR   | [x] Namespaced classes/interfaces/traits should be referenced with use
     |         |     statements
 200 | ERROR   | [x] Namespaced classes/interfaces/traits should be referenced with use
     |         |     statements
 207 | WARNING | [ ] Variable $config_directories is undefined.
--------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 9 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------------------------------


FILE: /Users/malik.kotob/Sites/project/docroot/sites/g/sites.inc
-------------------------------------------------------------------------------------------------
FOUND 6 ERRORS AFFECTING 6 LINES
-------------------------------------------------------------------------------------------------
  92 | ERROR | [x] Namespaced classes/interfaces/traits should be referenced with use statements
  93 | ERROR | [x] Namespaced classes/interfaces/traits should be referenced with use statements
 101 | ERROR | [x] Namespaced classes/interfaces/traits should be referenced with use statements
 664 | ERROR | [x] Namespaced classes/interfaces/traits should be referenced with use statements
 706 | ERROR | [x] Namespaced classes/interfaces/traits should be referenced with use statements
 712 | ERROR | [x] Namespaced classes/interfaces/traits should be referenced with use statements
-------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 6 MARKED SNIFF VIOLATIONS AUTOMATICALLY
-------------------------------------------------------------------------------------------------


FILE: /Users/malik.kotob/Sites/project/factory-hooks/post-install/post-install.php
----------------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------------------
 15 | ERROR | [x] Additional blank lines found at end of doc comment
----------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------------------


FILE: /Users/malik.kotob/Sites/project/factory-hooks/post-settings-php/memcache.php
-----------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
-----------------------------------------------------------------------------------
 12 | WARNING | Variable $site_settings is undefined.
 13 | WARNING | Variable $settings is undefined.
-----------------------------------------------------------------------------------


FILE: /Users/malik.kotob/Sites/project/factory-hooks/post-sites-php/includes.php
--------------------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
--------------------------------------------------------------------------------
 18 | ERROR   | Missing parameter type
 55 | WARNING | Variable $data is undefined.
--------------------------------------------------------------------------------


FILE: /Users/malik.kotob/Sites/project/hooks/common/pre-web-activate/000-acquia-deployment.php
----------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------------------------------
 330 | WARNING | Potential security problem: SSL peer verification must not be disabled
----------------------------------------------------------------------------------------------

Time: 400ms; Memory: 12MB

[ExecStack]  Exit code 2
[CompletionWrapper]  Command `tests:phpcs:sniff:files 'blt/blt.yml
composer.json
composer.lock
docroot/.htaccess
docroot/sites/default/.gitignore
docroot/sites/default/acsf.settings.php
docroot/sites/default/settings.php
docroot/sites/g/.gitignore
docroot/sites/g/SimpleRest.php
docroot/sites/g/apc_rebuild.php
docroot/sites/g/services.yml
docroot/sites/g/settings.php
docroot/sites/g/sites.inc
docroot/sites/sites.php
factory-hooks/db-update/db-update.sh
factory-hooks/post-install/post-install.php
factory-hooks/post-settings-php/includes.php
factory-hooks/post-settings-php/memcache.php
factory-hooks/post-sites-php/includes.php
factory-hooks/post-theme-deploy/clear-twig-cache.sh
factory-hooks/pre-settings-php/includes.php
hooks/README.md
hooks/acquia/db_connect.php
hooks/acquia/uri.php
hooks/common/post-db-copy/000-acquia_required_scrub.php
hooks/common/pre-web-activate/000-acquia-deployment.php
hooks/samples/acquia-cloud-site-factory-post-db.sh
hooks/samples/hello-world.sh'` exited with code 2.
[CompletionWrapper]  Exit code 1

[mikemadison13]

@grasmash is there history on why this is this way that we should know about here?

[wu-edward]

Don't know for sure if this is the cause, but the commit to update to PHPCS 3.0 (from #3132) removed the bootstrap file that BLT was previously using. Bootstrap file's purpose:

Its purpose is to take a list of files specified for sniffing, and filter it
 * according to the file patterns established in phpcs.xml. By default, phpcs
 * will ignore any file patterns defined in phpcs.xml when a list of files is
 * specified.
 *
 * "If you have asked PHP_CodeSniffer to check a specific file rather than an
 * entire directory, the extension of the specified file will be ignored."

Bootstrap file was removed because it's incompatible with 3.0, and in my testing, I did not see a functionality regression.

[mikemadison13]

@wu-edward well that could certainly explain why / how we have so much yucky code in BLT 10...

[wu-edward]

I did look into writing a new bootstrap file, but implementation would be completely different, and any examples I could find were for the the 2.x version of PHPCS.

[danepowell]

All of the failures in the OP come from the following directories:

• sites/g
• hooks
• factory-hooks

The contents of those directories mostly come from upstream products (e.g. ACSF) that have code style problems that BLT can't control. So in fact I think it's desirable (at least for now) that we don't sniff those.

Assuming we agree on that, then blt validate is behaving correctly here, and it's the pre-commit phpcs call that's not respecting the fileset defined in phpcs.xml.dist, due to the loss of the bootstrap file Edward referenced. So the solution would be to replace that with a new bootstrap file (or equivalent function in BLT) that properly filters the files passed to sniffFileList() by the filesets defined in phpcs.xml.dist.

[danepowell]

Ugh... so I've looked through how the bootstrap file is used in PHPCS 3 and it's radically less friendly than in PHPCS 2. We could probably make it work but it would be inelegant.

I think a better solution for this issue in the short term is to just exclude ACSF-provided files from sniffing by adding the following to phpcs.xml.dist:

  <exclude-pattern>factory-hooks</exclude-pattern>
  <exclude-pattern>hooks</exclude-pattern>
  <exclude-pattern>docroot/sites/g</exclude-pattern>

It doesn't fix the root issue here but at least fixes the symptom so fewer people run into this as a regression. The sooner we get that in the sooner we can release the next 9.x / 9.2.x. @malikkotob and @mikemadison13 thoughts?

[mikemadison13]

I added pcphs ignored for specific analysis in the factory hooks. The thing that is still unclear to me why validate ignored the issue and the commit hook doesn't. We may have to do as you suggest though to be consistent...