Add a Github Action to run phpcs (coding standards)

[indigoxela]

Now that we have GHA up and running for Simpletest, we should also add a linter action using phpcs.

There's a related meta issue, but that one doesn't seem appropriate, as it's mostly about Zen.CI and about fixing standards in existing code.

What we need:

• run phpcs in a GHA (automated job), showing annotations
• run phpcs with our custom standards
• run phpcs only against added/changed code in a PR, as we have a lot of legacy code not conforming yet

Turned out that this is feasible with an existing action

---

Related:

• Update core to Backdrop CSS Coding standards #2785
• Add CSS (JS?) code formatting automation #5568

[indigoxela]

My PR seems to work so far, see it in action with some intentional failures: https://github.com/backdrop/backdrop/pull/3749/files

The report output is a bit... It seems like the output style is hardcoded to "checkstyle" (xml), so we end up with the ". I'm not sure, if we can do anything about that.

The annotations seem pretty helpful, though. Just a little hard to read sometimes.

Note that there are other phpcs related actions available, but they come with disadvantages. The ones requiring a configured bot and a github token, for instance, aren't (to my understanding) available for non-members. And they require additional configuration via Github UI (by people with sufficient permissions).
I have to admit, though, that I didn't try them all out, I quickly decided for thenabeel/action-phpcs, after reading several descriptions (and issues).
Another note: thenabeel/action-phpcs is a fork from an abandoned action.

Our standards and the required older phpcs get installed via composer, see https://github.com/serundeputy/coder
And, yes, sooner or later this repo should also receive some loving care. 😉

[klonos]

It seems that the Coder version that we are installing via https://packagist.org/packages/backdrop/coder is being pulled from https://github.com/serundeputy/coder. Should we move that to another community-controlled repo? There currently are:

• https://github.com/backdrop-contrib/coder which points to:
  • https://github.com/backdrop-contrib/coder_review
  • https://github.com/backdrop-contrib/coder_upgrade

Should we change packagist to https://github.com/backdrop-contrib/coder_review?

[quicksketch]

I think could side-step questions about composer for now by downloading the latest coder release zip file, then caching the downloaded file per https://docs.github.com/en/actions/advanced-guides/caching-dependencies-to-speed-up-workflows

[indigoxela]

Regarding the coder repo: note the difference between coder (by @serundeputy) and the fully blown module coder_review, which contains lots of stuff we don't need, but does not take care of installing the phpcs dependency (correct version 1.5.x).

That's why composer has advantages here.

Regarding caching: I can copy/paste the setup from i18n but it isn't any faster than installing every time. The cached stuff also has to get pulled every time.

[indigoxela]

If I'm getting it right, we are stuck here because of concerns regarding the repo for our coding standards? Correct me if I'm wrong.

Regarding composer: I'd like to defend my approach. Having the composer.json in the main directory might be confusing for people - this file is really only needed for GHA, so the .github/misc directory is the better place for it.

Regarding caching: Note my previous comment - there's no performance gain at all, because with or without caching, the files have to get pulled into the runner box. It's just a matter of from where this pulling happens. (In case we don't use caching, the composer.json isn't needed and we'd simply do composer require ..., but if we do use caching, we need that file.)

Regarding using composer anyway: yes, that makes sense, because we need a special version of phpcs, that's compatible with our coding standards. Getting our current setup working with a more recent phpcs, requires a rewrite of all our sniff classes.

Regarding some concerns regarding the output (") - very likely we have to live with that. It's caused by the hardcoded usage of "checkstyle" output in the action we use (thenabeel/action-phpcs@v8).

[quicksketch]

If I'm getting it right, we are stuck here because of concerns regarding the repo for our coding standards? Correct me if I'm wrong.

That's one part. The other:

we need a special version of phpcs, that's compatible with our coding standards. Getting our current setup working with a more recent phpcs, requires a rewrite of all our sniff classes.

I had hoped that this work may have been done already in @serundeputy's fork. But alas, no it's still using that old version.

Perhaps we should make a 1.x branch of coder that uses PHPCS 1.5.x (like Drupal 7), but immediately branch it into a 3.x version that is build on PHPCS 3.x (3.6.0 being the latest version https://github.com/squizlabs/PHP_CodeSniffer/releases).

Or should we bite the bullet now and delay GitHub integration until this work has been done, so we don't have two sets of coding standards that need to be maintained.

[indigoxela]

Perhaps we should make a 1.x branch of coder that uses PHPCS 1.5.x

I'm not sure, but the contrib projects using phpcs (how many? I know at least one) use serundeputy/coder together with phpcs 1.5.x...

If there's consensus, that we need our sniffs repo somewhere in backdrop-contrib land, we could start with 3.x right away.
But... the syntax for 3.x sniffs is really different. It might even make sense to fork from Drupal coder (again) and start fresh with that?

Rewriting our existing 1.x sniffs might require more work than it's worth it.

Drupal repo: https://github.com/pfrenssen/coder

[laryn]

Is this helpful?
backdrop-contrib/coder_review#14

[indigoxela]

Is this helpful?

Yes, for sure. Wow, I wasn't aware that @hosef already did a lot of work regarding phpcs compatibility. 👍

But, wait, right, this would push the PHP version requirement to 5.4+, which means we have to revisit issues like #3992 and/or #4712. Are we stuck again?

UPDATE: No, of course we're not stuck. In the runner we can choose any PHP version we like, and I'm pretty sure, no developer still works on (or is relying on) 5.3.

[laryn]

See also backdrop-contrib/coder_review#12 (comment)

[indigoxela]

We have options:

1. live with the outdated phpcs for now (1.5)
2. create an independent repo for only the sniffs - compatible with 3.x, use that in our GHA, and trying to get the module UI working step by step
3. fix everything in backdrop-contrib/coder_review and take that as base for our checks (might take some time, though)

TBH, I wouldn't want the Backdrop Coder Review UI (or module altogether) being a show stopper for automatic (and annotated) checks in a GHA.
Personally, I think the benefit of having phpcs checks in our GH workflow is big enough to consider option 1. or 2. But that's just my 2c. 😉