Upstream 8.x-2.x

[agh1]

@seamuslee001 looks like we're overdue for a merge from upstream! When I did this in #2 I had to go through and adjust a handful of things that either modify sniffs we don't use or introduce new sniffs that are inappropriate or too strict.

For reference, these are our changes as compared to the latest commit prior to my last merge: https://github.com/ErichBSchulz/coder/compare/4eefe233859250bfade4e7734f26473bb9a96d91...civicrm:8.x-2.x-civi

[seamuslee001]

@agh1 i think they are still there but would appreciate thoughts on https://github.com/seamuslee001/coder/blob/8.x-2.x-civi/coder_sniffer/Drupal/ruleset.xml#L196 you can see the propertyDec;aration is still commented out but there are 2 new PSR2 declarations maybe they need to be commented out as well @totten

[seamuslee001]

btw this is the diff that is produced when i run a diff against the main branch @agh1 @totten https://gist.github.com/seamuslee001/6965863fc1f40aadea93760e2b8cff37

[totten]

@seamuslee001 I'd generally take an empirical/measured approach -- we should enable as many rules as we can without producing crazy backlog of tasks. So if, for example, PSR2.Namespaces.NamespaceDeclaration produces a 500 warnings that would have to be fixed manually -- then disable it. If it produces none, 👍 . If it produces a few warnings (or if the cleanup is amenable to automation), then I vote fix them.

Perhaps relevant: https://www.php-fig.org/psr/psr-2/#3-namespace-and-use-declarations -- on a substantive level, PSR-2's namespace/use policy seems pretty reasonable.

If we do go a cleanup route, and if it touches many files, then there's a stickier question -- because we currently apply the same ruleset to all tests on all versions. Finding a way to transition might be a longer discussion? Or, for low-tech approach, we could (1) merge the cleanups to code now and (2) activate new ruleset after a month or two.

[seamuslee001]

@totten i can take a look tomorrow i guess but also not 100% sure what the rule covers so wouldn't be sure exactly what might be checking for

[totten]

So you shouldn't have to know much about the rules to get a read on their impact. One can start by spotchecking a subset, e.g. this one takes (only) 40 sec on my laptop:

find Civi/Core/ CRM/Core/BAO/ api/ -name '*.php' | grep -v /examples/| time xargs phpcs-civi

That should give a sign (with clearer warning text) if there's anything extremely amiss. If it looks promising on the subset, then repeat with larger filesets. That's slower, so you'll either need to do it in the background, or get some coffee, or execute in parallel fashion. In the past, I improvised (running multiple threads in different console-tabs), but there might be a simpler way to parallel-task:

# Runs 3 concurrent tasks of up to 20 files each.
find Civi CRM api bin extern -name '*.php' | grep -v /examples/| time xargs -P3 -L20 phpcs-civi

[seamuslee001]

@totten so i ran that and this is the output it gave me https://gist.github.com/seamuslee001/380b7cd2e1bcab20d26f0ed0dc2389c7

[totten]

Cool.

To help visualize the distribution of issues, I did some string munging and got: https://gist.github.com/totten/26f3595e54f0d6428c7419eff71475dd

Based on this sample of 519 PHP file and 763 issues, we have a rate of ~1.47 issues per file. Extrapolating to 2761 PHP files in civicrm-core, the current draft ruleset is probably identifying circa 4000 issues. Which is a lot. :)

There are automated cleanups for some things (via phpcbf). But my trust in it depends on the specific issue. (Ex: The PHP open tag must be followed by exactly one blank line sounds safe for automatic cleanup.) Maybe there's a way to do partial automated cleanup (like temporarily hacking at the ruleset?)?

Poking through, the output highlights a bunch of good issues, but some things seem problematic:

• Expected "array|null" but found "array|NULL" for function return type
  • There's a bunch of errors about capitalization in | lists. In https://www.drupal.org/docs/develop/standards/coding-standards , it looks like they still advise using NULL instead of null, so this feels like a problem in the ruleset.
• Property name "$_gid" should not be prefixed with an underscore to indicate visibility
  • Renaming properties would be a contract-break. I'm a big :-1: on mass renaming of properties. Some individual/case-by-case might be OK, but I'd leave that out-of-scope for a broad push on cleanup. (In a broad cleanup, the mindset is too focused on quantity over quality; bad time for BC break.)
• Namespaced classes/interfaces/traits should be referenced with use statements and Namespaced classes, interfaces and traits should not begin with a file doc comment and Unused use statement
  • These sound like neat improvements to the linter. Maybe I'm being a bit of luddite, but I worry about the quality of these checks (given how dynamic PHP can be). Trust but verify?
• Expected "DAOObject" but found "DAO Object" for @var tag in member variable comment or Expected "arraycacheKeycacheValue" but found "array ($cacheKey => $cacheValue)"
  • These are clearly munging the content. It's fair to draw attention to these lines, but the inferred alternatives are so wrong. I wouldn't trust these with automatic/unsupervised cleanup.
• Array indentation error, expected 4 spaces but found 6
  • In the past, I remember some kind of instability when autocleaning things like this. (Ex: Running a cleanup would fix some indentation and break other indentation.) Maybe it's better today.
• Comments may not appear after statements
  • Silly rule IMHO, but OK, let's accept for sake of standardization. What happens when it autocleans? If it moves comments (but doesn't delete comments), then I guess that's OK...

TBH, the fastest thing is probably disabling suspicious rules so that we can focus on cleaning up the stuff that's more clear-cut. If there's time+patience left over, then dig deeper into the problematic ones.

[agh1]

If I recall correctly we left it last time such that we knew some rules would trip up some files but it was better to have those get flagged occasionally than to try doing an autoclean. Does that sound right?

For array indentation, a lot of the issue is arrays that are spelled out in the arguments of functions. I think I recall a lot of spots where it looked reasonable but didn't fit the sniff's idea of where it should be indented.

I wonder if of @totten's suspicious rules we should keep the namespaced classes (it won't affect as much of our code), array indents (we already have rules about this, so this is probably just a refinement), and comments after statements (same thing) and disable the others.

[seamuslee001]

@totten @agh1 i have updated the rules and it now outputs https://gist.github.com/7ef89a0f8752312a40b23a4d8878a2e8

[totten]

If I recall correctly we left it last time such that we knew some rules would trip up some files but it was better to have those get flagged occasionally than to try doing an autoclean. Does that sound right?

Hmm, I don't think so. There's currently a wide cap between the version of coder pegged in buildkit (a801890a82d52e23a83c5d820270b6e228843d79) and the head of origin/8.x-2.x-civi (503b3f4d91b4f37bf6004c5e15e3beb37fed6c6b; +161 commits). There is then an additional gap from origin/8.x-2.x-civi and this PR (0a53bbb18f0a199f932f5f239ef9e5bcce90e700; +197 commits).

Using current origin/8.x-2.x-civi (503b3f4d91b4f37bf6004c5e15e3beb37fed6c6b), I do see a high number of issues. I ran phpcbf-civi against the first 200 PHP files, and 59 of them had issues - or about 33%. That creates pretty high odds that future patch authors will regularly encounter such style issues.

For a benchmark, I did a couple batches of cleanup. It took me 20-30 min to handle one batch of 100 files (with assistance from phpcbf autoclean). With 2100 PHP files, that's ~7 hours of cleanup work to get the level of current origin/8.x-2.x-civi.

[seamuslee001]

@totten i have just updated to exclude some more rules which might be useful down the track but not right now

[seamuslee001]

@totten this is now the ouput after running phpcbf-civi and then running civilint https://gist.github.com/23a1a4da3a5560fb3dd04cd71e597411

[totten]

Alrighty, merging this. We're charging forth with upfront cleanups in other PRs, and it's clearer how to proceed there if this is merged (i.e. if there's one clear branch specifying the target ruleset). We can still tweak/disable rules as needed. It won't go live until we update buildkit.