search

civicrm / coder

Code sniffer configuration for CiviCRM. (Relaxed variant of Drupal CS)
5 stars 11 forks source link

Enable `IncorrectParamVarName` - Slight tightening of rules #17

Open eileenmcnaughton opened 2 years ago

eileenmcnaughton commented 2 years ago

Per https://github.com/civicrm/civicrm-core/pull/22515 there are a handful of these- we can fix the bulk of them pretty easily

totten commented 2 years ago

On surface level, this makes sense and seems appropriate.

On the substance... I'm a bit confused. I tried checking this out and running it against the files from https://github.com/civicrm/civicrm-core/pull/22515 (ie the commit before aec7c57d6b2abb90af586ca9d493add1f390a603; aka aec7c57d6b2abb90af586ca9d493add1f390a603~1; aka 83634f6732c05385a46ac4b5b21445865ad842c1). It doesn't raise any warnings (with or without this patch).

[bknix-min:~/bknix/build/wpmaster/web/wp-content/plugins/civicrm/civicrm] phpcs -v --standard="$HOME/bknix/vendor/drupal/coder/coder_sniffer/Drupal"  CRM/Core/Smarty/plugins/modifier.print_array.php CRM/Core/Resources/*php CRM/Activity/BAO/Activity.php  CRM/Contact/BAO/Query.php CRM/Contact/Form/Task/Label.php  CRM/Contact/Page/AJAX.php CRM/Core/BAO/CustomField.php CRM/Core/BAO/Domain.php  CRM/Core/BAO/SchemaHandler.php CRM/Core/BAO/Translation.php CRM/Core/BAO/UFGroup.php CRM/Core/Payment/BaseIPN.php CRM/Core/Resources.php
Registering sniffs in the Drupal standard... DONE (111 sniffs registered)
Creating file list... DONE (19 files in queue)
Changing into directory /Users/totten/bknix/build/wpmaster/web/wp-content/plugins/civicrm/civicrm/CRM/Core/Smarty/plugins
Processing modifier.print_array.php [PHP => 761 tokens in 106 lines]... DONE in 46ms (0 errors, 0 warnings)
Changing into directory /Users/totten/bknix/build/wpmaster/web/wp-content/plugins/civicrm/civicrm/CRM/Core/Resources
Processing Bundle.php [PHP => 520 tokens in 77 lines]... DONE in 29ms (0 errors, 0 warnings)
Processing CollectionAdderInterface.php [PHP => 1199 tokens in 232 lines]... DONE in 64ms (0 errors, 0 warnings)
Processing CollectionAdderTrait.php [PHP => 2694 tokens in 429 lines]... DONE in 154ms (0 errors, 0 warnings)
Processing CollectionInterface.php [PHP => 774 tokens in 168 lines]... DONE in 57ms (0 errors, 0 warnings)
Processing CollectionTrait.php [PHP => 3070 tokens in 414 lines]... DONE in 193ms (0 errors, 0 warnings)
Processing Common.php [PHP => 2182 tokens in 304 lines]... DONE in 106ms (0 errors, 0 warnings)
Processing Strings.php [PHP => 546 tokens in 101 lines]... DONE in 25ms (0 errors, 0 warnings)
Changing into directory /Users/totten/bknix/build/wpmaster/web/wp-content/plugins/civicrm/civicrm/CRM/Activity/BAO
Processing Activity.php [PHP => 21652 tokens in 2798 lines]... DONE in 1.13 secs (0 errors, 0 warnings)
Changing into directory /Users/totten/bknix/build/wpmaster/web/wp-content/plugins/civicrm/civicrm/CRM/Contact/BAO
Processing Query.php [PHP => 60196 tokens in 7282 lines]... DONE in 2.94 secs (0 errors, 0 warnings)
Changing into directory /Users/totten/bknix/build/wpmaster/web/wp-content/plugins/civicrm/civicrm/CRM/Contact/Form/Task
Processing Label.php [PHP => 2885 tokens in 377 lines]... DONE in 140ms (0 errors, 0 warnings)
Changing into directory /Users/totten/bknix/build/wpmaster/web/wp-content/plugins/civicrm/civicrm/CRM/Contact/Page
Processing AJAX.php [PHP => 8226 tokens in 1021 lines]... DONE in 392ms (0 errors, 0 warnings)
Changing into directory /Users/totten/bknix/build/wpmaster/web/wp-content/plugins/civicrm/civicrm/CRM/Core/BAO
Processing CustomField.php [PHP => 21581 tokens in 2761 lines]... DONE in 1.1 secs (0 errors, 0 warnings)
Processing Domain.php [PHP => 2434 tokens in 369 lines]... DONE in 133ms (0 errors, 0 warnings)
Processing SchemaHandler.php [PHP => 7053 tokens in 935 lines]... DONE in 326ms (0 errors, 0 warnings)
Processing Translation.php [PHP => 1373 tokens in 166 lines]... DONE in 68ms (0 errors, 0 warnings)
Processing UFGroup.php [PHP => 29104 tokens in 3598 lines]... DONE in 1.51 secs (0 errors, 0 warnings)
Changing into directory /Users/totten/bknix/build/wpmaster/web/wp-content/plugins/civicrm/civicrm/CRM/Core/Payment
Processing BaseIPN.php [PHP => 3043 tokens in 469 lines]... DONE in 164ms (0 errors, 0 warnings)
Changing into directory /Users/totten/bknix/build/wpmaster/web/wp-content/plugins/civicrm/civicrm/CRM/Core
Processing Resources.php [PHP => 4069 tokens in 582 lines]... DONE in 222ms (0 errors, 0 warnings)

Additionally, I tried editing a *.php file to introduce 6-8 different forms of misnaming/mistyping and couldn't find anything to provoke IncorrectParamVarName (although I could get other warnings; but they're not affected by the change here).

I can confirm that I am really loading this ruleset - eg if I hack ruleset.xml to insert a syntax-error, and if I run phpcs -- then it correctly complains about the syntax error.

Possibly relevant: --standard=Drupal does not work for me. I have to give the full path to the ruleset. Compare:

$ phpcs -v --standard="Drupal"  CRM/Core/Smarty/plugins/modifier.print_array.php

ERROR: the "Drupal" coding standard is not installed. The installed coding standards are PEAR, Zend, PSR2, MySource, Squiz, PSR1 and PSR12
$ phpcs -v --standard="$HOME/bknix/vendor/drupal/coder/coder_sniffer/Drupal"  CRM/Core/Smarty/plugins/modifier.print_array.php

Registering sniffs in the Drupal standard... DONE (111 sniffs registered)
Creating file list... DONE (1 files in queue)
Changing into directory /Users/totten/bknix/build/wpmaster/web/wp-content/plugins/civicrm/civicrm/CRM/Core/Smarty/plugins
Processing modifier.print_array.php [PHP => 761 tokens in 106 lines]... DONE in 47ms (0 errors, 0 warnings)

If --standard=Drupal works for you... then there's something different in how we have phpcs setup. Is it possible that you've actually got it loading some other/unseen flavor of the standard? Or can you see anything wrong in how I'm applying it above?

eileenmcnaughton commented 2 years ago

@totten so I did add it to my path per the install instructions

phpcs --config-set installed_paths ~/.composer/vendor/drupal/coder/coder_sniffer

Ideally we would remove ALL the rules in the section that says <!-- CIVICRM: The following are decent checks, but they're currently impractical. -->

I did try that & got a few too many changes to push up in one go - perhaps just removing 1 is a bit conservative but I did fix a few bits from it - but there might not be any / many left that cause this rule to get upset.

If I let it run on the iats folder it fatals & there is something else somewhere causing problems (possibly bower_components) since I'm just running it in the civi-root

totten commented 2 years ago 
phpcs --config-set installed_paths ~/.composer/vendor/drupal/coder/coder_sniffer

Since I've installed into a buildkit folder rather than the global-composer, that translates for me to:

phpcs --config-set installed_paths ~/bknix/vendor/drupal/coder/coder_sniffer

I can confirm that this causes it to pick up on --standard=Drupal. (Although... using this mode writes changes into ~/bknix/vendor/squizlabs/php_codesniffer/, and I find it easier to juggle alternate standards by using full-paths... so I'll probably continue doing that for the moment...)

I can confirm that upstream drupal/coder@master returns issues on the #22515 files (approx 3000-4000 issues of varying quality, based on skimming the checkstyle report).

$ phpcs --report=checkstyle --standard="/tmp/coder/coder_sniffer/Drupal"  CRM/Core/Smarty/plugins/modifier.print_array.php CRM/Core/Resources/*php CRM/Activity/BAO/Activity.php  CRM/Contact/BAO/Query.php CRM/Contact/Form/Task/Label.php  CRM/Contact/Page/AJAX.php CRM/Core/BAO/CustomField.php CRM/Core/BAO/Domain.php  CRM/Core/BAO/SchemaHandler.php CRM/Core/BAO/Translation.php CRM/Core/BAO/UFGroup.php CRM/Core/Payment/BaseIPN.php CRM/Core/Resources.php \
   | wc
   3757   52986  674742

--

I still don't see how this tightening is relevant to #22515...

But don't get me wrong - if civicrm-core currently passes the sniff IncorrectParamVarNam, then that's all we need to demonstrate. We want ramp-up these rules as soon as civicrm-core permits. So AFAICS, this can be merged.

eileenmcnaughton commented 2 years ago

@totten yeah - lets merge it if it isnt failing anything for you. I already fixed all the things I found that failed with this - it's a pretty conservate hardening - but we can make go a bit further once it's locked in