Code style enforcement

[totten]

Following up on the recent code cleanup ( http://forum.civicrm.org/index.php/topic,35312.0.html ), the master branch has reached a base level of code-style quality. To ensure that it stays at that level, I've enabled style checks as part of the PR testing process. There are a few questions that I think are likely to pop-up as we start to do this:

Q: Where are standards enforced?

Standards are enforced on the https://github.com/civicrm/civicrm-core/ repository for the "master" branch (and will be enforced for future branches, such as 4.6 or 5.0). However, the existing branches (4.3, 4.4, and 4.5) have too many pre-existing violations, so standards are not enforced there.

Q: Will I have to wait 90min to see if my PR passes the code-style tests?

A: No. The PR testing process will short-circuit if there are code-style issues -- so it should fail within a few minutes. If the style is good, then it will proceed to run the test-suite.

Q: Should I check the code-style locally?

A: Yes. That's even faster!

Q: How do I check the code-style locally?

A (Short): Download the latest civicrm-buildkit (https://github.com/civicrm/civicrm-buildkit). As you develop (but before committing), use the shell to run "civilint" in your git repository. This is exactly the same script used by the test servers. (Note: Buildkit includes a lot of other tools, such as amp/civibuild. You can use civilint without those.)

A (Long): The code-style checks are based on the following open-source tools:

• https://github.com/squizlabs/PHP_CodeSniffer - PHP_CodeSniffer (aka "phpcs") is a flexible tool for enforcing code style.
• http://drupal.org/project/coder - Drupal Coder 8.x-2.x is a set of configurations and addons for phpcs for enforcing Drupal's code style.
• https://github.com/civicrm/coder - Coder 8.x-2.x-civi is a relaxed fork of Drupal Coder. A number of rules have been disabled to work with Civi's codebase.
• http://jshint.com - JSHint detects common errors and misleading code in Javascript.

All of these tools are bundled with buildkit and used on the test/demo infrastructure. Additionally, buildkit includes a wrapper script ("civilint") which calls all the above with suitable arguments. By default, civilint will scan uncommitted changes.

Many IDE's and text-editors have support for integrating with these tools directly, e.g.

• phpcs with PHPStorm -- https://www.jetbrains.com/phpstorm/help/using-php-code-sniffer-tool.html
• phpcs with Netbeans -- http://plugins.netbeans.org/plugin/40282/phpmd-php-codesniffer-plugin
• jshint with many different tools -- http://jshint.com/install/

When configuring an IDE to work with phpcs, you must configure a custom style. Use the configuration from https://github.com/civicrm/coder. (In buildkit, the files are located at $BUILDKIT/vendor/drupal/coder/coder_sniffer/Drupal.)

Q: What is Civi code-style? How does it relate to (Drupal/Joomla/WordPress/PSR-2) code-style?

A: Several years ago, Civi adopted the Drupal code style because it had the broadest familiarity within the community. A few notable variations are required for compatibility -- most notably, the naming conventions (for classes, functions, variables, etc.) are different. Personally, I think the quickest way to recognize a code-style is by looking at an example (e.g. https://gist.github.com/totten/7c7ddd229962d49f5138), but you can also take a deep dive into specs:

• Drupal Code Standards: https://www.drupal.org/coding-standards
• Civi Code Standards: http://wiki.civicrm.org/confluence/display/CRMDOC/PHP+Code+and+Inline+Documentation

Q: What if I think the standard is wrong?

You're probably right -- but coding standards are highly subjective and generally painful to negotiate. That's why we re-used a popular one rather than negotiating a new one.

Q: What about extensions?

You can use the same tools (civilint, phpcs, coder, etc) to check style in your extensions.

[Dave Greenberg]

Kudos to Tim for leading the charge on this AND to everyone who helped get the master branch cleaned up !!!

[TwoMice]

That's great, Tim! I know this has been an idea for a long time -- it's great that you've actually got it in place.

[Erik Hommel]

First of all, great stuff that we are doing this!!!

I have as silly question though.....I used to follow the 'implicit' CiviCRM coding standard for naming functions and variables (camel case so $contactId, retrieveMyFirstName). A couple of months ago we decided at CiviCooP to start enforcing some coding standards and we checked the wiki page. At the time it said:

CamelCase is used throughout the existing codebase for function and variable names. Changing this is going to be a big logistical challenge. At the same time, some functions (e.g., Drupal hooks) need underscores to work propertly.

This is a complex problem – in terms of both automated enforcement and refactoring. It should be addressed, as the current inconsistent usage is troublesome. However, in the interest of getting some standards in place now, we're opting to avoid this issue for now.

We interpreted this as: in the future the Drupal standards (so $contact_id and retrieve_my_first_name) should be used and at some point in time we might get to cleaning this up. So I have hit myself over the fingers many times and now successfully trained myself to use lower case.
Do I now understand correctly that we are accepting camel case as the desired coding style? The new wiki page now says

This item is not part of the CiviCRM Coding Standards.

so I really have no idea what to use now so the code style enforcement says I am a good boy :-)

[totten]

Yes, I agree that it's ambiguous/non-committal -- AFAIK, we've never had a discussion about what the naming-convention should actually be. We've only had the discussion that Drupal's naming-convention is really problematic for our codebase.

I ran some quick stats on our codebase ( https://gist.github.com/totten/98745bfaff7e4e5ebd1f ). Expressions like "camelCase" or "_camelCase" outnumber "under_score" or "_under_score" by almost 4:1 (167k vs 43k).

Personally, I'd vote for this policy on new-or-rewritten code (where it's doesn't break existing interfaces):
 * For DAO properties, use underscores. (These correspond to the DB schema - which uses underscores.)
 * For everything else, use camelCase.

[Erik Hommel]

Thanks Tim, we will use that :-) And I will update the Wiki page accordingly. Thanks for the work!

[Coleman Watts]

+1 camelCase - it's easily the worst part of the drupal coding standards (and there is some very stiff competition for that distinction).

[totten]

A couple related things that came up in other media:

1. Some auto-generated files are ignored by the code-style check (i.e. any file in a "DAO" or "examples" folder). If the corresponding code-generator is brought into compliance, then we can tighten up the style-check.

2. If jshint is overzealous, one can overrule it by either (a) making putting a special comment next to the offending line or (b) adding a ".jshintrc" file. There's more discussion of this at http://jshint.com/docs/

3. The idea to short-circuit testing was influenced by my experience as a new contributor to joomla-cms ( https://github.com/joomla/joomla-cms/pull/2764 ). I haven't internalized their code-style yet, so I made a lot of mistakes -- e.g. someone on the thread would comment about an issue/feature/change, then I'd commit+push; it would immediately fail (because I started breaking style), so I'd commit+push again. If I had to wait 90min for feedback, I probably would have moved on to something else and forgotten to cleanup the PR.

[Erik Hommel]

I am really happy you have taken this on Tim, cool!