PhpCsAutoFixerV2 alter code erroneously, breaking validation

elgandoz commented 5 years ago

Steps required to reproduce the problem

cd ./docroot/modules/custom/conventions_test/
phpcs --standard=Drupal --extensions='php,module,inc,install,test,profile,theme,css,info,txt,md' . => No errors
phpcs --standard=DrupalPractice --extensions='php,module,inc,install,test,profile,theme,css,info,txt,md' . => No errors
phpcbf --standard=Drupal --extensions='php,module,inc,install,test,profile,theme,css,info,txt,md' => No fixable errors were found
git commit . -m "Testing drupal-conventions"

Expected Result

Current code in the folder successfully committed.

Actual Result

Commit not successful, and code formatted against Drupal Conventions (with unexpected additions)

Further explanation

For this purpose I will use a example module:

<?php

/**
 * @file
 * Fake barebone module to test drupol/drupal-conventions.
 */

/**
 * Implements hook_help().
 */
function conventions_test_help($route_name) {
  switch ($route_name) {
    case 'help.page.conventions_test':
      $output = 'Hello world!';

      return $output;

    default:
  }
}

The check I perform before the commit is taken from this Drupal documentation page:

https://www.drupal.org/docs/8/modules/code-review-module/php-codesniffer-command-line-usage

I'm basically running drupalcs, drupalcsp and drupalcbf and all indicates the code is formatted according the standards.

When I commit, I get this log:

➜  conventions_test git:(feature/coding-standards) ✗ git commit . -m "Testing pre-commit checks"
GrumPHP detected a pre-commit command.
GrumPHP is sniffing your code!
Running task 1/8: Composer... ✔
Running task 2/8: ComposerNormalize... ✔
Running task 3/8: PhpLint... ✔
Running task 4/8: PhpCsAutoFixerV2... ✘
Running task 5/8: PhpCsFixerV2... ✔
Running task 6/8: Phpcs... ✘
Running task 7/8: YamlLint... ✔
Running task 8/8: JsonLint... ✔

You can fix all errors by running following commands:
'/Users/awmwebmaster/Sites/awmregister-dev/vendor/bin/php-cs-fixer' '--allow-risky=yes' '--config=./vendor/drupol/drupal-conventions/config/drupal8/php_cs_fixer.config.php' '--using-cache=no' '--path-mode=intersection' '--verbose' 'fix' 'docroot/modules/custom/conventions_test/conventions_test.module'
FILE: .../docroot/modules/custom/conventions_test/conventions_test.module
----------------------------------------------------------------------
FOUND 4 ERRORS AFFECTING 3 LINES
----------------------------------------------------------------------
  1 | ERROR | [x] Missing file doc comment
  3 | ERROR | [x] Expected 1 space before "="; 0 found
  3 | ERROR | [x] Expected 1 space after "="; 0 found
 14 | ERROR | [x] Opening brace should be on the same line as the
    |       |     declaration
----------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Time: 473ms; Memory: 8MB

You can fix some errors by running following command:
'/Users/awmwebmaster/Sites/awmregister-dev/vendor/bin/phpcbf' '--standard=./vendor/drupal/coder/coder_sniffer/Drupal' '--report=full' '--ignore=vendor/,node_modules/,tests/fixtures/,spec' '/Users/awmwebmaster/Sites/awmregister-dev/docroot/modules/custom/conventions_test/conventions_test.module'
To skip commit checks, add -n or --no-verify flag to commit command

The modules is also erroneously altered (note brackets and new declaration):

<?php

declare(strict_types=1);

/**
 * @file
 * Fake barebone module to test drupol/drupal-conventions.
 */

/**
 * Implements hook_help().
 */
function conventions_test_help($route_name)
{
  switch ($route_name) {
    case 'help.page.conventions_test':
      $output = 'Hello world!';

      return $output;

    default:
  }
}

During the validation process, keeping the editor open on the file and keeping an eye on a git client, I can see that at Running task 4/8: PhpCsAutoFixerV2... the file get changed, formatting the curly brackets and adding the unexpected declare(strict_types=1);. This set the validation in Running task 6/8: Phpcs... to fail.

The debug message is also somehow confusing, suggesting 2 different solutions on how to fix errors, the first using php_cs_fixer the second using phpcbf (both do no work in my case.)

drupol commented 5 years ago

Hi,

Thanks for the bug report.

This is an issue that is also bugging me and I don't know how to get around it for now. A couple of days ago, I've also opened an issue in order to have some feedback from the maintainer regarding that same issue: https://github.com/wearejust/grumphp-extra-tasks/issues/14

Despite the fact that I found this very handy and time saving, if I don't find a valid solution to this, I will most probably remove it from drupol/drupal-conventions :(

drupol commented 5 years ago

I also updated drupol/phpcsfixer-configs-php and drupol/phpcsfixer-configs-drupal, can you check again ?

elgandoz commented 5 years ago

Thanks for pointing that out.

For the time being I don't see how the package could work because of this and probably it would be better removing phpcsfixer until we have a fix for it.

Temporary I solved it removing the grumphp settings from the extra section of my composer.json and coping the content of /vendor/drupol/drupal-conventions/config/drupal8/grumphp.yml in grumphp.yml and removing all the references to phpcsfixer2.

In this way I can still run all the other checks and I'm able to successfully commit.

Below for reference my current configuration:

parameters:
  git_dir: .
  bin_dir: vendor/bin
  process_timeout: 3600
  ascii:
    failed: ~
    succeeded: ~

  extensions:
    - Wearejust\GrumPHPExtra\Extension\Loader
    - drupol\PhpConventions\GrumphpTasksExtension

  # PHP Code Sniffer parameters.
  tasks.phpcs.exclude: []
  tasks.phpcs.ignore_patterns:
    - vendor/
    - node_modules/
    - tests/fixtures/
    - spec
  tasks.phpcs.triggered_by:
    - inc
    - install
    - module
    - php
    - profile
    - theme
  tasks.phpcs.whitelist_patterns: []
  tasks.phpcs.standard: ./vendor/drupal/coder/coder_sniffer/Drupal
  tasks.phpcs.warning_severity: ~

  # Tasks.
  tasks:
    # Composer checks
    composer:
      metadata:
        priority: 6300

    # Composer Normalize
    composer_normalize:
      metadata:
        priority: 6200

    # PHP Lint
    phplint:
      triggered_by: ['php', 'module', 'install', 'profile', 'inc', 'theme']
      metadata:
        priority: 5000

    # PHP Code Sniffer.
    phpcs:
      exclude: '%tasks.phpcs.exclude%'
      standard: '%tasks.phpcs.standard%'
      ignore_patterns: '%tasks.phpcs.ignore_patterns%'
      triggered_by: '%tasks.phpcs.triggered_by%'
      whitelist_patterns: '%tasks.phpcs.whitelist_patterns%'
      warning_severity: '%tasks.phpcs.warning_severity%'
      metadata:
        priority: 3000

    # YAML Lint
    yamllint:
      whitelist_patterns: []
      ignore_patterns: []
      object_support: true
      exception_on_invalid_type: true
      parse_constant: true
      parse_custom_tags: true
      metadata:
        priority: 2000

    # JSON Lint
    jsonlint:
      ignore_patterns: []
      detect_key_conflicts: true
      metadata:
        priority: 1000

services:
  task.composer_normalize:
    class: drupol\PhpConventions\Grumphp\Task\ComposerNormalize
    arguments:
      - '@config'
      - '@process_builder'
      - '@formatter.phpcsfixer'
    tags:
      - {name: grumphp.task, config: composer_normalize}

Is there any better method to alter it?

On a side note, will I be able to run the check only on on specific folders? I can see there is some configuration for each individual task, but is there anything that I can apply globally? Hypothetically I would like to run the check only on modules/custom/* and themes/custom/*. In this way I can avoid to run the check on core, libraries, contrib modules and themes and force code standards only on custom written code.

drupol commented 5 years ago

I've just updated the packages containing the PHPCSFixer configuration, have you tried with the latest versions of them ? Here I tested and it seems to be fine.

elgandoz commented 5 years ago

I also updated drupol/phpcsfixer-configs-php and drupol/phpcsfixer-configs-drupal, can you check again ?

Sorry I only saw this after reloading.

drupol commented 5 years ago

There was indeed multiple issues.

Related to the recent updates of drupol/phpcsfixer-configs-php and drupal/phpcsfixer-configs-drupal and PSR12.
Related to the issue with php_cs_auto_fixer grumphp task that doesn't fail when it automatically fixes something.

I'm pretty sure that issue 1 is fixed, I'm waiting for your confirmation.

For the issue 2, I don't have feedback yet.

elgandoz commented 5 years ago

After some trial, it is looking great, although I'm still running in one issue.

Using my sample module as reference, phpcsfixer still adds declare(strict_types = 1); right after the <?php tag.

This possibly would be okay in a simple *.php file but given a *.module or *.inc the drupal coding standards require the file to start with a @file docblock.

Thus, even if the @file doc comment exist, the validation will not pass in the Phpcs task because the file no longer start with it. Additionally running phpcbf the top of the file will end up looking like this:

<?php

/**
 * @file
 */

declare(strict_types = 1);

/**
 * @file
 * Fake barebone module to test drupol/drupal-conventions.
 */

/**
 * Implements hook_help().
 *
 * One more test on this.
 */
function conventions_test_help($route_name) {
  [...]
}

This could be still fixed manually, resulting in a successful commit.

Although I understand the reason of the type declaration, I don't think enforcing it's a great idea, mainly because it is not required by the standards (missing in the majority of core and contrib modules) and declaring it may result in unexpected behaviour.

elgandoz commented 5 years ago

As additional note, the task PhpCsAutoFixerV2 takes a consistent amount of time compared to other tasks. The CPU load also gets quite high during that check, triggering the fan of my MacBookPro 2.2 Intel i7.

drupol commented 5 years ago

Although I understand the reason of the type declaration, I don't think enforcing it's a great idea, mainly because it is not required by the standards (missing in the majority of core and contrib modules) and declaring it may result in unexpected behaviour.

You're right, I always though it was by default in D8, but it's not, so, there is no reason to enforce it here. I removed it and tagged a new version: https://github.com/drupol/phpcsfixer-configs-drupal/commit/7cfe6ca0ebcf320f0b0c08a5aad5d87a7cbfc41b

drupol commented 5 years ago

Please do a composer update drupol/phpcsfixer-configs-drupal in a couple of minutes to get the update and let me know.

elgandoz commented 5 years ago

Now reproducing the steps of the issue results in a successful commit. Thanks for your work, it's very appreciated.

I have just 2 more questions:

What's the best way to override some of the configuration? Just adding a line in /grumphp.yml?
Is there a way to exclude folders from the pre-commit hook?

drupol commented 5 years ago

Haa I'm glad it works... It's so hard to make sure that PHPCS and PHP CS Fixer are working fine together :-)

drupol/drupal-conventions provides a very advanced Grumphp configuration, and you can override some parts.

Here are some examples:

Adding extra tasks: https://github.com/drupol/collection/blob/master/grumphp.yml.dist
Customizing one specific part of the configuration: https://github.com/drupol/memoize/blob/master/grumphp.yml.dist

If you want to know which part is configurable, check the Grumphp configuration which is imported, and look for variables. See how it's done in this Grumphp configuration file: https://github.com/drupol/php-conventions/blob/master/config/php71/grumphp.yml

I don't think that Grumphp is able to exclude folders from the pre-commit hook for now, maybe you should browse their issue queue if you want to do so. However, it's possible that a task provides this option, like the phpstan task and the ignore_patterns parameter.

I hope it helps :-)

Thanks for using drupol/drupal-conventions, don't hesitate to spread the word !

drupol / drupal-conventions