Closed josephdpurcell closed 8 years ago
I tested this similar to how I did here: https://github.com/codeclimate/codeclimate-phpcodesniffer/pull/31#issuecomment-224438255
The container does build and the code standards for both WordPress and Drupal appear to be supported. However, there seems to be a discrepancy between what PHPCS discovers running locally vs in the container.
I compared the results of running codeclimate analyze with running: phpcs --standard="Drupal" src/ | grep "| \(WARNING\|ERROR\)" | wc -l
Here are the results:
Any thoughts?
Hi @josephdpurcell it's hard to say without knowing what the missing 60/403 issues are. Some things I know of off the top of my head:
The engine also ignores warnings by default unless you've updated your .codeclimate.yml
to include them:
phpcodesniffer:
enabled: true
config:
standard: "Drupal"
ignore_warnings: false
Hey @josephdpurcell,
I did some digging and found the discrepancy to be caused by slightly different versions of PHPCS and the underlying standard packages.
You can see differences in known rules by comparing the output of ./vendor/bin/phpcs --standard="Drupal" -e
locally and within the docker container.
A solution would be to better lock down the versions of packages installed. I think the following composer.json
would do:
{
"require": {
"squizlabs/php_codesniffer": "2.6.1",
"barracudanetworks/forkdaemon-php": "1.0.7",
"danielstjules/stringy": "2.3.2",
"drupal/coder": "8.2.7",
"wp-coding-standards/wpcs": "0.9.0"
}
}
I have redone the test given the above input, here is the .codeclimate.yml
and code I'm now working with: https://github.com/josephdpurcell/drupal-replication/blob/8.x-1.x/.codeclimate.yml
(Due to pastebin character limit, I'm using the drupal-replication repo instead.)
Here is the output from phpcs: http://pastebin.com/nNiBGJZE And the output from codeclimate: http://pastebin.com/sAHxdjZY
Number of errors:
Here is how I calculated for PHPCS:
wget -O output.txt http://pastebin.com/raw/nNiBGJZE
cat output.txt | grep "| \(WARNING\|ERROR\)" | wc -l
Comparing the output, it looks like perhaps Code Climate isn't accounting for warnings even with the ignore_warnings
flag off?
Code Climate:
== src/AllDocs/AllDocs.php (22 issues) ==
1: Missing file doc comment [phpcodesniffer]
...
PHPCS:
FILE: ...ub.com/josephdpurcell/drupal-replication/src/AllDocs/AllDocs.php
----------------------------------------------------------------------
FOUND 21 ERRORS AND 2 WARNINGS AFFECTING 21 LINES
----------------------------------------------------------------------
5 | WARNING | [x] Unused use statement
Assuming that is the case, I re-ran PHPCS with phpcs --standard="Drupal" -n src/
which ignores warnings and got: http://pastebin.com/vA86prmd, which is 321 errors.
End of my analysis I am left with:
I'll investigate more later if I have time.
Ah! That would explain it. I'll compare versions and retry the checks to verify.
That was it!
Here is how I checked:
$ /home/joep/src/github.com/josephdpurcell/codeclimate-phpcodesniffer/vendor/bin/phpcs --config-set installed_paths /home/joep/src/github.com/josephdpurcell/codeclimate-phpcodesniffer/vendor/drupal/coder/coder_sniffer
$ /home/joep/src/github.com/josephdpurcell/codeclimate-phpcodesniffer/vendor/bin/phpcs --standard=Drupal -n src/ > output.txt
$ cat output.txt | grep "| \(WARNING\|ERROR\)" | wc -l
327
Assuming 327 == 327 and I'm not missing something different that should be obvious, I think this PR is good to go!
Oh! There's the question of the composer.json and what it should be set to.
I'm not sure what is best: on the one hand, leaving the versions loose gives the opportunity for a composer update to pull in latest updates, on the other hand doing so will result in variations in GPAs on projects.
I think down the road it would be nice to be able to specify which version of code standards the project is compliant against, but for now my thought is that ^8.2 and ^0.9 is reasonable--assuming semantic versioning, perhaps those should hold until the next major versions of those projects, Drupal 9 and WordPress 5?
That sounds good to me. Something like this perhaps?
{
"require": {
"squizlabs/php_codesniffer": "^2.5",
"barracudanetworks/forkdaemon-php": "^1.0",
"danielstjules/stringy": "^2.3",
"drupal/coder": "^8.2",
"wp-coding-standards/wpcs": "^0.9"
}
}
Sure. I updated the composer.json, but I did not update the specified version in the lock file. I figure it would be best to isolate updates to dependent libraries from this PR.
LGTM
This PR should address https://github.com/codeclimate/codeclimate-phpcodesniffer/issues/30 and https://github.com/codeclimate/codeclimate-phpcodesniffer/issues/15.
This PR is an alternative to https://github.com/codeclimate/codeclimate-phpcodesniffer/pull/31 and https://github.com/codeclimate/codeclimate-phpcodesniffer/pull/29.