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. 😉

[ghost]

As I was suggesting in #3213, I highly recommend pulling the sniffs out of coder_review and into their own repo. This means that Backdrop core can use them in GitHub Actions (as per this issue), @serundeputy can use them for Drush (instead of his own forked repo), I can use them for Bee, and then coder_review can use them too (without needing to duplicate them in its own repo). And core, Drush, Bee, etc. who want to use these sniffs in their various checks don't need to pull in all the other coder_review code unnecessarily.

So I guess that means I prefer @indigoxela's 2nd option above. Now, what version of phpcs these sniffs support is another question altogether, but I'm leaning towards using modern standards of 3.x, rather than old, legacy code.

Hope that all makes sense.

[quicksketch]

I agree with @BWPanda. And I like option #2 best. We start a new repository with minimal coding standards and work to expand it; starting with 3.x version of PHPCS.

[indigoxela]

What do we need to get progress here:

1. A repository for the Backdrop sniffs, with sniffs compatible with phpcs 3.x
2. Update the existing PR to get sniffs from that new repo

Seems feasible. 😉

I'd not recommend to use the full Coder Review module, as it provides loads of things we don't need here - we just need the sniffs.

And they should be compatible with a more recent phpcs, 3.x - which is not the case with the sniffs in coder_review (still requires 1.x).

@hosef you did a lot of work re updating the sniffs, and so did @quicksketch https://github.com/backdrop-contrib/coder_review/pulls But both try to fix the sniffs inside the module.

@BWPanda suggests to put the sniffs in an extra repo and I fully agree with that.

We "just" need to figure out, how to proceed with the sniffs repo.

@hosef do you think, your PR for coder_review (the sniffs) is a possible starting point for the new repo?
@quicksketch or would you prefer to start completely fresh and update the rulesets from D to meet the B standards? (yes, there are some subtle differences)

[klonos]

@BWPanda suggests to put the sniffs in an extra repo and I fully agree with that.

Same ✋🏼 ...I think I'd prefer the new repo under https://github.com/backdrop instead of https://github.com/backdrop-contrib (less people with access to it, but still not necessarily limited to the core committers).

[ghost]

I think /backdrop-ops is a better group for the new repo @klonos.

[klonos]

Yes, or that @BWPanda 👍🏼 ...my point was:

• not in contrib, cause too many people have too much access there
• access should not be limited only to core committers, so that more people can contribute to managing/maintaining it

[hosef]

@indigoxela I think that my PR would be a good starting point if you want to update the sniffs to work with a newer version of PHP_CodeSniffer, but there are still a lot of things that don't work in it. If I remember correctly, they changed the string parsing interfaces, so most of the checks related to comments are completely broken.

[indigoxela]

so most of the checks related to comments are completely broken.

Then it might make sense to start fresh with a Drupal sniffs fork...

[indigoxela]

Here we go, rebased and switched to a different approach. 🎉 https://github.com/backdrop/backdrop/pull/3749/files

Instead of figuring out where to put and maintain our custom ruleset repo, I decided to only cherry-pick from the built-in rulesets, which reduces the new Backdrop ruleset to a single xml file.

Sure, sooner or later we should create a repo and also create custom rules - but that seems to block us currently. So, why not simply postpone the whole repo topic?

Good thing: we can now use the latest and greatest phpcs (3.7 ATM), which ships with php 8.1 support. And that also means: no more composer, as shivammathur/setup-php provides it as a tool.

Bad thing: most of the comment rules are gone. (The ones from Generic, Squiz and PEAR ruleset are a bit too opinionated, but I'll take a closer look.)

@quicksketch do you think, this is a feasible approach?

[indigoxela]

Long term goal: combine this and the (otherwise unrelated) PHPStan PR to one workflow (use the same GHA runner).

[indigoxela]

Here's a repo to discuss:

• name
• location
• maintenance
• functionality

[jenlampton]

@indigoxela I forked your repo into the backdrop-ops group, and made you an admin:
https://github.com/backdrop-ops/phpcs_backdrop

edit: I have been instructed not to do this... cleaning up the mess now.

• name: @quicksketch likes phpcs (https://github.com/backdrop-ops/phpcs)
• location: backdrop-ops
• maintenance: I added @indigoxela -- @BWPanda and/or @serundeputy let me know if you also want access.
• functionality: @quicksketch is happy as-is

[jenlampton]

Okay, I have cloned and pushed the repo up to https://github.com/backdrop-ops/phpcs. @indigoxela you are still an admin :)

[indigoxela]

I'm not sure if using the same name as the commandline tool itself is ideal...

I'm still admin, and already fixed another bug. 😉

[indigoxela]

PR updated:

• Removed confusing (temporary 😀 ) xml file (we now have a repo, so we can checkout instead of providing it here)
• Tagged repo (1.0.0-beta1) and use that tag for actions/checkout

Last steps: if we're happy with the outcome, the test.php file should get removed before merge.

[laryn]

I like it and I think it will be very helpful on PRs.

[hosef]

@indigoxela Do you know if the current rule set in https://github.com/backdrop-ops/phpcs matches our coding standards outlined on https://docs.backdropcms.org/doc-standards?

[indigoxela]

@hosef they do, because I built it around that documentation. 😉 If you find something that does not, please open an issue in backdrop-ops/phpcs.

[hosef]

Ok, I just wanted to check.

If that is the case, then I think that this looks good to go. I have tried out the sniffs locally and I didn't get any errors using the latest phpcs and PHP 8.1.

[indigoxela]

Great! So it's time to remove the test.php file (done).

[hosef]

Merged backdrop/backdrop#3749 into 1.x. Thanks @indigoxela, @jenlampton, @quicksketch, @laryn, @BWPanda, and @klonos!

Special thanks to @indigoxela for doing a lot of the heavy lifting of setting up our GitHub Actions.

[klonos]

@indigoxela @hosef is there a way to point the GHA to latest release instead of a hard-coded tag of the backdrop-ops/phpcs repo? (unfortunately, GitHub doesn't have an API or easy way to retrieve the latest tag of a repo as it allows retrieving the lates release) ...separate issue? ...follow up PR?

Asking because otherwise we'll need to be creating and merging PRs against core to update the version of the linting each time we update https://github.com/backdrop-ops/phpcs.

[hosef]

@klonos I am not sure about the technical details there.

As far as if we should, it might be better to include changes to the coding standards check with a patch that fixes the new coding standards problems. That way we don't make PRs that were previously passing suddenly not pass with no changes in the core code that they could see.

[indigoxela]

is there a way to point the GHA to latest release instead of a hard-coded tag

The tag is there to have a reliably consistent version, while development can still go on (if needed). There has to be a way for fixes.
IMO it's too early for a stable release (zero feedback so far).

Keep in mind that phpcs (GHA) only runs on changed code in a PR. So if we decide that some standard needs adaption, this doesn't mean that all of a sudden everything breaks. 😉

[bugfolder]

Keep in mind that phpcs (GHA) only runs on changed code in a PR.

Well, that's not quite correct (at least the way it's working out). If you change one line in a file, PHPCS runs on the entire file, not just the changed code, and so will flag preexisting issues in the file unrelated to the code changes. For example, in https://github.com/backdrop/backdrop/pull/4246/files, the second change at line 670 was the original code change; the first change at line 378 was flagged as being needed by PHPCS.

[indigoxela]

Well, that's not quite correct ..

At least, that's what the documentation of that action says.

https://github.com/thenabeel/action-phpcs

The default scope of that action is "blame", which means only changed code.

I can see the nagging on this commit - to me it seems, that the line number is wrong. The nagging's for line 667, but the hint points to line 378. Probably a bug in the action. 🤷