civicrm / coder

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

deactivate PHP.UpperCaseConstant rule #3

Closed ErichBSchulz closed 5 years ago

ErichBSchulz commented 7 years ago

I appreciate this maybe controversial, but seem worth discussing?

The change is justifiable on the basis that:

nganivet commented 7 years ago

1) that would clearly conflict with the Drupal coding standards (https://www.drupal.org/docs/develop/standards/coding-standards, requires all constants in uppercase) on which the CiviCRM coding standards are based. 2) this would have a huge impact on the CiviCRM core code base, not to mention all extensions. We're looking at hours of work refactoring all to new standards (doubt we can simply use a search/replace because of comments, javascript files, templates files, SQL files, etc.)

totten commented 7 years ago

-1

Civi coding standard is generally supposed to be "Use the Drupal standard unless it's broken". (Ex: renaming existing fields/methods/functions/classes would break things, so those rules are excluded.)

Don't get me wrong -- I have several nitpicks about the Drupal conventions and PSR-2 conventions but prefer to let the Drupal and PHP-FIG spend their time on bikeshedding.

Maybe I'm missing something in this discussion -- is there an actual use-case where the behavior of json_encode() is relevant? In my mind, these three things are all different spaces: PHP code, JS code, JSON data. The conventions of one shouldn't have any functional bearing on the other, right?

seamuslee001 commented 7 years ago

I'm -1 on this i find it simpler to read the code with TRUE and FALSE and NULL and also i think as evidenced by https://github.com/civicrm/civicrm-core/blob/master/CRM/Mailing/Info.php#L179 you can if needed use 1 and 0 to represent TRUE and FALSE in javascript if passing from PHP to JS

ErichBSchulz commented 7 years ago

Worth noting that this change would simply deactivate the existing rule to make the CiviCRM standards agnostic about case (just like php). No work is required to existing code.

I can see the rationale for the drupal standard back in the day before the ascendancy of JSON.

But in 2016 I would argue that this drupal standard is broken and arcane. JSON is now dominant and increasingly we are all doing more coding in both JS and PHP. Increasingly new coders will come to the project and not have previous experience with PHP but will know JS well.

Enforcing this arbitrary standard just adds a barrier to entry to new coders.

By way of background, this patch was triggered by Seamus submitting a patch to change all my PHP lowercase true/false/null constants to upper case. So he has some skin in this game ;-)

Agree this isn't a big deal... just putting it out there :-)

agh1 commented 7 years ago

I have definitely run up against this--my natural preference in writing code is setting those as lowercase.

However, when it comes to setting the standard for CiviCRM, my feeling is similar to Tim's. Any rule set will be annoying for at least some people. If nothing else, we're cross-CMS, and WordPress's rules seem to be completely contrary to Drupal's on anything that's vaguely controversial (lowercase constants, whitespace within parentheses, no line break before else, etc.) so the choices are really

Since, at the moment, most CiviCRM developers are also writing for a CMS (or a few), and the CMSes have larger developer communities than CiviCRM does, the only reasonable options in my mind are to use the Drupal or WordPress rules. This preference for consistency with the CMS is much stronger than my feeling that ALL CAPS NULL is overkill.

(Practically speaking, I find that using a PHPCS plugin in your editor makes it painless to follow the standard. I use Atom with the linter-phpcs package, and I direct it to the Drupal standard within the coder directory. Before I can type a semicolon, it yells at me if I try to write true in lowercase. Even better, I can point it to the WordPress coding standards when working on a WordPress plugin, and I can be reminded about all of those quirks.)