Updates from Drupal (for real this time)

[agh1]

Merged from Drupal's 8.x-2.x branch. Did a test run on CiviCRM core--edited ruleset.xml to remove a few new upstream checks that are a bit much for CiviCRM purposes.

It will generate a number of new errors as compared to before, but they're for legit issues (notably array indentation when you're including a new array as an argument for a function).

[totten]

I think @eileenmcnaughton will be glad if we're more closely aligned with upstream. :)

@agh1, quantitatively, do you know how much code would need a cleanup? I mean, is it something that someone at AGH can do in a half-hour? Or do we need recruit+coordinate several people? (I'd be happy to join in and cleanup a batch of files, but if it's big, then we'll need a coordinator to reach out to a bigger crew.)

Aside: At this point, we have two branches which follow this standard (civicrm-core master and 4.6). I'm not sure if we should either (a) cleanup both master and 4.6 to align with updated standards or (b) cleanup master and suspend enforcement on 4.6 (since changes should be infrequent now).

[agh1]

So here's the issue--not all of the current code appears to pass without errors, at least according to the way I run it. Running phpcs with the current standard gives 100 errors and warnings; with the new standard it gives 1713 errors and warnings. Do you know how things pass when you commit currently?

It's certainly more than we could do in-house in a half hour, but I think we could fairly quickly parcel the whole thing out to a bunch of volunteers. Happy to lead the effort and contribute our folks' time.

[totten]

Ooh, 1713 is high. If it really is that high, then agree we'll need to split the work.

A few thoughts about files already in the codebase with errors:

1. When submitting a PR, it only checks the styling on a modified file. Unmodified files are ignored.
2. The most likely ways to get some files with bad style are (a) the change was introduced by merge-forward (eg 4.6=>master), (b) a trigger-happy code-reviewer accepted a change prematurely, (c) the testbot had some kind of bug/outage, or (d) the standards changed. (None of these things are common, but all have happened at some point.)
3. Certain files are exempted -- i.e. auto-generated files (DAO's and api/v3/examples) as well as external dependencies (packages/ and vendor/). The civilint command tries to exclude these. In the past, I usually focused on specific folders:

## Find files
find CRM/ Civi api/ bin/ extern/ install/ tests/ -name '*.php'

## Find files and check with phpcs (and includ various args for loading coder module from buildkit)
find CRM/ Civi api/ bin/ extern/ install/ tests/ -name '*.php' | xargs phpcs-civi

A few tips that might help you with coordination:

• Problems are not evenly distributed across directories, so a simple division of directories can be very imbalanced. It might help to get some stats (phpcs-civi --report-csv=<reportFile>) and use the those to break out sublists.
• If you have a multi-core CPU, then you can speed up by splitting the work across multiple threads.

find CRM/[A-M]* -name '*.php' | xargs phpcs-civi --report-csv=report-1.csv &
find CRM/[N-Z]* -name '*.php' | xargs phpcs-civi --report-csv=report-2.csv &
find tests -name '*.php' | xargs phpcs-civi --report-csv=report-3.csv &
find Civi api bin extern install -name '*.php' | xargs phpcs-civi --report-csv=report-4.csv
cat report-{1,2,3,4}.csv > report-final.csv

• When a person does cleanup, the command phpcs -a is handy. It's a semi-interactive way to step-through/test a list of files while editing. (Ex: phpcs-civi -a CRM/Core/BAO/Mapping.php CRM/Utils/PseudoConstant.php)

[agh1]

Okay, checking again, it was 1669 rows when I actually opened the CSV in LibreOffice (for whatever reason there are more rows in the actual text file). I only ran it on CRM/ at first, thinking it would harbor the most problems and be representative of the others, so this number is based upon that.

I tried working through a few of these, and I'd say that 75% of it is multiline arrays as arguments within a function or foreach. They're tabbed over to somewhere strange, when the standard expects them to be farther left. (And really, a lot of them should be their own variable for legibility's sake.)

Besides those, there were a few inline comments, some extra spaces around a concatenating dot, and stuff like that.

There was one unused use statement (and a few more to come). I assume that shouldn't be "fixed", since it's new stuff from the Civi namespace, but I also wasn't sure whether that check should be disabled in the standard. FWIW, it's only a warning (I'm not sure what warnings do on new PRs).

I worked on this for a half hour and got it down to 1500 rows, so that's a rate of 338 rows per hour. You can see what got done at https://github.com/civicrm/civicrm-core/pull/7848

Why so fast? As I said, a lot of these are multiline arrays being tabbed too far over, and in many of those cases, there's one error per line of the array. Click the PHPCS error line number in Atom, shift-tab-tab-tab, and that's a bunch of errors all at once.

[colemanw]

This is great @agh1 +1 from me.

[agh1]

Okay, good news and bad news. I ran phpcs on Civi/, api/, bin/, extern/, install/, and tests/ and returned a whopping 6,606 lines of errors (on top of the ones in CRM/). On closer inspection, about 5,360 of them are from the api/v3/examples directory, and each file in there has 20 lines of errors. It appears that they're all from the following boilerplate:

/*
* This example has been generated from the API test suite.
* The test that created it is called "testActivityCreateCustomContactRefField"
* and can be found at:
* https://github.com/civicrm/civicrm-core/blob/master/tests/phpunit/api/v3/ActivityTest.php
*
* You can see the outcome of the API tests at
* https://test.civicrm.org/job/CiviCRM-master-git/
*
* To Learn about the API read
* http://wiki.civicrm.org/confluence/display/CRMDOC/Using+the+API
*
* Browse the api on your own site with the api explorer
* http://MYSITE.ORG/path/to/civicrm/api
*
* Read more about testing here
* http://wiki.civicrm.org/confluence/display/CRM/Testing
*
* API Standards documentation:
* http://wiki.civicrm.org/confluence/display/CRM/API+Architecture+Standards
*/

The asterisks need to be spaced one space to the right. I assume this is either generated somewhere or could be swapped in a giant regex bonanza. With that assumption, the whole project's estimate doubles, but it's still in a reasonable-ish 9- or 10-hour scale.

[kurund]

@agh1 +1. great work

[totten]

Any chance we could simply skip the autogenerated files? In the case of civilint, it's already skipping anything with /DAO/ or /examples/. When running via find/xargs, that would be something like:

find Civi api bin extern install -name '*.php' | grep -v /examples/ | grep -v /DAO/ | xargs phpcs-civi --report-csv=report-4.csv

[agh1]

Yeah it's even easier to run than that--I was already ignoring .js files:

phpcs --report=csv --report-file=report.txt --standard=Drupal --ignore=*.js,examples CRM/ Civi/ api/ bin/ extern/ install/ tests/

CSV of the results (called ".txt" because GitHub refuses CSVs for some reason)

[totten]

@agh1 OK, cool. We can merge this so that everyone has access to it, and I've granted you rights on civicrm/coder.git to push follow-ups as-needed.

Eventually, we'll also need to update buildkit (e.g. edit composer.json and run composer update drupal/coder). But the next step is probably in your court -- ie preparing instructions/task-list and sending out CTA.

Aside: I'm not sure how --standard=Drupal is working in your configuration. In the past, I found it necessary to specify the full path to use the customized ruleset (which is why phpcs-civi exists). If there's an update to phpcs that made this more automatic, that would be pretty cool.

[agh1]

I learned how to set up named standards from the WordPress standards because I like to keep the standards easy-to-change within Atom so I can use the WordPress standards for the CiviEvent Widget and the Drupal standards for everything else.

[agh1]

Sorry, had to rush out of CiviDay DC before it became Civinado DC. I put the standards somewhere handy like /usr/local/etc/coder/ and /usr/local/etc/WordPress-Coding-Standards/ and then do

phpcs --config-set installed_paths /usr/local/etc/coder/coder_sniffer,/usr/local/etc/WordPress-Coding-Standards/

That makes all the standards in those directory available by name if you do phpcs -i.