Another round of code formatting?

[Coleman Watts]

Tim can you enumerate the problems left. I just found one (and haven't solved it yet except for in 1 file): https://github.com/civicrm/civicrm-core/pull/4888

[Coleman Watts]

Ok I think this fixes all the misplaced types for variables: https://github.com/civicrm/civicrm-core/pull/4891

[Coleman Watts]

@eileen it looks like one of your cleanup commits had the side-effect of removing the newline between some comments
Here's the fix: https://github.com/colemanw/civicrm-core/commit/e0b82b44738ae827c23c946bd511f86c639fc6e4
As part of a larger PR (https://github.com/civicrm/civicrm-core/pull/4892) that includes yet more of my pet peeve fixes (we don't need to be told in the comments that a function is a function!)

[totten]

Thanks, Coleman.

Tim can you enumerate the problems left. I just found one (and haven't solved it yet except for in 1 file): https://github.com/civicrm/civicrm-core/pull/4888

Yeah, phpcs does a pretty thorough job of enumerating the problems. I've taken the latest output (circa bfc6355af), broken it down, and posted online. For anyone who's willing to help, please visit:

https://pad.riseup.net/p/QOOHQpetSXXo

[Coleman Watts]

I'm confused about this one:
http://think.hm/tmp/cleanup/c/cleanup-Drupal.CSS.ColourDefinition.NotLower.txt
Ok drupal likes lowercase color definitions. I couldn't care less, but what confuses me is why it only flagged one line. If you look at civicrm.css we mix upper and lower case like it was going out of style.

[Coleman Watts]

I've noticed that a lot of the messy code being flagged is in our test suite. Wondering how much effort we really want to put into that in the short term - maybe ignore that directory for now?

[Eileen]

I've taken a gandar at it out of interest and among the formatting stuff noticed a bunch of things that just couldn't work - a class that referenced a long-gone require_once, function calls that don't match the signature.

I added a PR to pull them out.

However, the thing that causes me consternation is not being able to flag when we have checked something & it's honestly truly OK - I usually try to get 'clean margins' before working on a file but things like vars that have to be declared but will never be used block that.

I found one fix on that....  Of course we'd need to be very copy & paste wary!

+++ b/api/v3/Location.php
@@ -5,7 +5,7 @@
  * @param $params
  * @return array
  */
-function civicrm_api3_location_create($params) {
+function civicrm_api3_location_create(/** @noinspection PhpUnusedParameterInspection */ $params) {
   return civicrm_api3_create_error("API (Location, Create) does not exist, use the Address/Phone/Email/Website API instead", array('obsoleted' => TRUE));
 }

@@ -14,7 +14,8 @@ function civicrm_api3_location_create($params) {
  *
  * @return array
  */
-function civicrm_api3_location_get($params) {
+function civicrm_api3_location_get(/** @noinspection PhpUnusedParameterInspection */
+  $params) {
   return civicrm_api3_create_error("API (Location, Get) does not exist, use the Address/Phone/Email/Website API instead", array('obsoleted' => TRUE));
 }

@@ -23,6 +24,6 @@ function civicrm_api3_location_get($params) {
  *
  * @return array
  */
-function civicrm_api3_location_delete($params) {
+function civicrm_api3_location_delete(/** @noinspection PhpUnusedParameterInspection */ $params) {
   return civicrm_api3_create_error("API (Location, Delete) does not exist, use the Address/Phone/Email/Website API instead", array('obsoleted' => TRUE));
 }
diff --git a/api/v3/OptionValue.php b/api/v3/OptionValue.php
index e78fe76..dc74b21 100644
--- a/api/v3/OptionValue.php

[Eileen]

By contrast the same error on this function

_civicrm_api3_mailing_contact_query

would appear to denote a serious problem - it returns exactly the same thing when called by

_civicrm_api3_mailing_contact_get_delivered

&&

_civicrm_api3_mailing_contact_get_bounced

This is mitigated by the fact it appears these functions are probably never called (I can't find any evidence of calls to them) & all 3 are probably obsolete

[Eileen]

update - the functions are called & the param is just misleading & obsolete
https://github.com/civicrm/civicrm-core/pull/4904

- but this highlights why it's good to be able to mark the difference between param not used - when it's a required part of the function signature & not used vs 'hey no-one noticed that'

[totten]

Big thanks to Chris B, Jon G, Eileen, Atif, Coleman, Dave G, Kurund, Monish, Pratisksha, Rohan, and Yashodha for helping with batches! . Also, shout out for klausi (from drupal.org/project/coder) who's been pushing patches / fixing bugs on drupal/coder 8.x-2.x. Since we've gotten organized about the manual cleanup, the issue count has gone down from 3990 to 592 (and that's with slightly harsher ruleset!).