PhpCsAutoFixerV2 alter code erroneously, breaking validation
Closed this issue · 12 comments
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 errorsphpcs --standard=DrupalPractice --extensions='php,module,inc,install,test,profile,theme,css,info,txt,md' .
=> No errorsphpcbf --standard=Drupal --extensions='php,module,inc,install,test,profile,theme,css,info,txt,md'
=> No fixable errors were foundgit 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:
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.)
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: wearejust/grumphp-extra-tasks#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 :(
I also updated drupol/phpcsfixer-configs-php and drupol/phpcsfixer-configs-drupal, can you check again ?
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.
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.
I also updated drupol/phpcsfixer-configs-php and drupol/phpcsfixer-configs-drupal, can you check again ?
Sorry I only saw this after reloading.
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.
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.
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.
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: drupol/phpcsfixer-configs-drupal@7cfe6ca
Please do a composer update drupol/phpcsfixer-configs-drupal
in a couple of minutes to get the update and let me know.
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?
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 !