GHA: action thenabeel/action-phpcs seems unmaintained

[indigoxela]

Description of the bug

Not actually sure, what's going on, but one PR just doesn't pass the phpcs check - for no obvious reason. And the phpcs output is odd. No annotations, but still a failure.

All the information we get is:

Error: Error: Command failed: phpcs --report=json -q --encoding=undefined --standard=../phpcs/Backdrop ...

That reads like: "something went wrong, lol". Not really helpful.

What's different in this PR compared to the other (passing) ones: A lot more files changed (35). But that doesn't explain anything.

Now the problem: the action thenabeel/action-phpcs, which we use to only run checks on actually changed or added code seems unmaintained (by @thenabeel). It doesn't even have an issue queue enabled. And it's a fork of an also unmaintained action.

The action still runs based on nodejs 12, which is deprecated in GHA as of September this year.

https://github.blog/changelog/2022-09-22-github-actions-all-actions-will-begin-running-on-node16-instead-of-node12/ Not sure, if this has impact here.

The action provides very little options to debug, so it's tricky to figure out what fails, anyway. What are our options:

• Ignore failures like that, which means, we're back to manual review.
• Drop the whole phpcs check - same problem as above: exhausting manual review.
• Search for an alternative action (there seems to be none, that's maintained).
• Write and maintain the whole check ourselves (not that we really have resources for that)
• ... did I miss anything?

[klonos]

Not an expert at all in this @indigoxela, but what about https://github.com/marketplace/actions/action-php-codesniffer ?

[indigoxela]

Not an expert at all in this @indigoxela, but what about...

That's the (also) unmaintained action, our current one has been forked from. :wink:

[avpaderno]

Isn't PHP_CodeSniffer a different Github action?

[indigoxela]

Isn't PHP_CodeSniffer a different Github action?

That's a completely different and unrelated one, and we couldn't use it, as it runs on all code, which would produce masses of failures because of all our legacy code.

[avpaderno]

That action checks all the files when the scan_all option in the workflow file is set to true; otherwise, it checks only the changed files. At least, that is what I understand from the code used in the main.ts file.

    if (scan_all) {
      files = await getAllFiles()
    } else {
      files = await getChangedFiles()
    }

[indigoxela]

it checks only the changed files

Thanks, didn't see that. But that won't help, either. Think of a file like core/includes/common.inc with 8938 lines and 325 errors.

[ghost]

I also experienced this issue in https://github.com/backdrop/backdrop/pull/4349 I assume it's either the number of files that it can't handle, or it's the length of the generated command that breaks it.

I looked into fixing it, wondering if instead of providing a list of filenames, we saved the list of filenames to a file and then told phpcs to check the filenames listed in that file instead (so the command would be much shorter). The GH action we're using didn't seem to support that, but plain ol' phpcs does, and I found we're calling it manually in bee, so why not do the same here?

I tested this in my own repo and it worked. phpcs was able to check all the files without that error. Then I wondered if the fix was using a file of filenames, or just replacing the action with our own manual phpcs command. So I tried passing it the list of filenames like normal and that worked too. So it seems it's just the action we're using that doesn't handle too many files.

I then realised it was checking all files, not just the ones that were changed in the PR, so I found an action to get the list of changed files and passed that to phpcs instead. After a few more tweaks I think I got it working nicely.

Here's a PR: https://github.com/backdrop/backdrop/pull/4355

[indigoxela]

@BWPanda many thanks for giving that a try.

One difference between our current action and the one you chose (tj-actions/changed-files): the current one works with git blame, so only lines that the current author changed (now or previously) are considered.

"changed-files" seems to work with ... yeah, well :wink: - which can lead to a lot of nagging when changing something in bigger files. That seems a bit hard for first-time-contributors.

[avpaderno]

Even the currently used action reports phpcs errors in places that are not touched from a PR. Probably, it reports less irrelevant errors, but it still does. (I noticed that in the PR I created to change php.net links.)

[klonos]

...the current one works with git blame, so only lines that the current author changed (now or previously) are considered.

@indigoxela I share your concern, but once we get our entire codebase through PHPCS and fix every pre-existing coding standard violation, then this will not matter as much. Right? So it may be throwing too many errors now, but these will slowly be reduced to practically only those changed by each PR.

As it is currently, the PHPCS check fails randomly most of the time. So what we end up doing is to ignore it. We could do the same with the new GHA, but with this key difference: we create a separate PR to fix all errors with the files currently touched by the PR. We can keep ignoring PRs that touch a lot of files, and only action those that change up to say 4-5 files. That should slowly get us to a point where there are no complains by PHPCS. I mean, at some point we simply have to bite the bullet and fix everything anyway. Right?

What do you think?

[indigoxela]

What do you think?

Sure, just do what's necessary and you think is appropriate. :+1:

[herbdool]

I'm with @indigoxela that it's better to have it focus just on git blame.

I'm not entirely clear on how it works. So long as it's just for the lines changed in the current PR.

One thing I have noticed that is odd: currently if the PR branch is behind 1.x then it'll check all the commits between them.

[klonos]

...currently if the PR branch is behind 1.x then it'll check all the commits between them.

Yeah, I have also noticed that. I don't get as many failures once I rebase my PR branch.

...sometimes, the errors/failures go away if I just close/wait/reopen the PR.

[herbdool]

If the only realistic option is to check the whole changed file then that's what we've got, so I'm not going to block a change here.

[klonos]

@indigoxela it's not what I think. One of our core committers started cleaning things up one module at a time in #3213, and the plan was for others to also help get things done. However, we didn't get very far (the https://github.com/backdrop/backdrop/tree/phpcs branch hasn't been touched in a long time).

[ghost]

I'll have to dig deeper into how the current action works re. apparently only reporting on changed lines (and not whole files). The generated phpcs command doesn't look much different to what I'm using here, so I assume it's the files themselves that must be special... I'll do some more research/testing and see what I can come up with.

[ghost]

After doing a bit of research into this, I discovered that PHPCS doesn't support checking only changed lines in files:

...there is no feature to format just the lines that have been modified. This often doesn't make sense for the sniffs as they may generate errors on lines that have not changed, but the errors are caused by changes that were made elsewhere in the file.

(https://github.com/squizlabs/PHP_CodeSniffer/issues/3111#issuecomment-690786285)

Just confirming that PHPCS can't check only changed lines because it requires the entire file contents to tokenize the code and find errors. So using an external script or tool is the way to go.

(https://github.com/squizlabs/PHP_CodeSniffer/issues/678#issuecomment-132437542)

Some tools exist for this, such as https://github.com/123inkt/php-codesniffer-baseline and https://github.com/DaveLiddament/sarb (among others), but IMO that's just a hack to get PHPCS to do something it's not designed to do.

This can be further seen in our own current implementation, as per the comments above:

Even the currently used action reports phpcs errors in places that are not touched from a PR.

As it is currently, the PHPCS check fails randomly most of the time.

currently if the PR branch is behind 1.x then it'll check all the commits between them.

The whole thing just seems hacky, broken, and hard to maintain. Not to mention, adding additional libraries/code to allow checking only changed lines adds more points of failure and increases the complexity of our tests.

I therefore recommend we go with @klonos' suggestion to use PHPCS as intended and check whole files (that have been changed by the PR).

Regarding the issue of new contributors being put-off by huge amounts of coding errors in code they didn't touch, we can either:

• Mention to them that this isn't their fault and they can either fix them all, fix some of them, or none of them, and then the core committer can make a judgement call and still merge their PR (a test failure doesn't prevent merging), or
• Mark this issue as blocked by https://github.com/backdrop/backdrop-issues/issues/3213 and get all the existing errors in core fixed first before making this change.

I'm actually leaning towards the latter...

[klonos]

@BWPanda ~I also prefer the second option~ I would prefer something like this:

• allow people to file a PR and do a full PHPCS check (only changed files, but all lines of code within them)
• if PHPCS fails, we mark the issue/PR as blocked on #3213.
• we fix the touched files in a separate PR (and since PHPCS will run, make sure all is green)
• we merge the PHPCS PR into 1.x
• we ask the author of the original PR to rebase (and help them out if they don't know how)
• now the original PR shouldn't fail

I volunteer myself to help with these PHPCS-only PRs 🤚🏼

Here are some additional/alternative ideas for consideration that might also help:

• we can add a message in the automated comment that Tugboat adds to the PR, letting people know that PHPCS will most likely fail, and that a) that is not necessarily their fault or that there's something wrong with their code, b) it is optional for them to fix the errors by this check, and c) they need to focus on the 9 php checks and fix these errors, since these are the only checks that must absolutely pass.
• I've seen some GHAs that do not fire automatically, rather than be triggered manually by a Slack-like "command" comment, like /rebase. Perhaps we can do the same with the PHPCS check, and then only core committers and those few of us that know about it can trigger it. Once we have covered the entirety of our codebase and know that PHPCS passes, then we can add the check to the automated ones.

The above will make it a bit easier for first-time contributors.

PS: I plan to bring this up during the weekly dev meeting and see how others feel.

[ghost]

@klonos I think you misunderstood... For the second option, I meant that we should mark this issue (#5893) as blocked and not merge my PR (to make these proposed changes) until the rest of the coding issues in core are fixed first. Then we can make this change and then it won't complain about existing code 'cause it'll all be fixed already.

[klonos]

@klonos I think you misunderstood...

Sorry @BWPanda. Yes, I did misunderstand, so I've amended my reply.

So OK, but fixing all of the issues in core all at once sounds like a big undertaking; and waiting for that to happen in smaller chunks over time will take a long time. In the meantime, we'll have to live with a dysfunctional/annoying PHPCS check. I certainly do NOT want to remove it though, since a check with false positives is better than no check at all.

[herbdool]

I'm leaning towards the 1st option: explain the situation to new contributors. Core committers can also edit the PRs to fix some of the errors before merging to help, so it's not just up to the contributor.

[ghost]

I tried playing around with the code to see if I could get PHPCS itself to just check changed files from the PR (without needing the additional action for that), but couldn't get it working. So the PR is ready for review/testing if we want to go ahead with this.