ci: run magento coding standard for pull requests

[danslo]

Description

Runs the Magento Coding Standard on Pull Requests.
The check will only be performed for files that were added/modified within the pull request.

After installing dependencies from the lock file, we update the coding standard to the latest version.
The lock file might have an older version, in my case it was locked to v23, but we're already at v25 (which made me run into this issue.

Some questions:

• Would it be worth to use a filter pattern for the branch name. Should we use something like '2.[0-9]-develop'?
• This currently only runs for pull_request, not for push. Is there a scenario where we would need to run on push as well? We should assume that direct pushes to the main branch is prohibited to any contributor: any code that we do introduce is done by PRs, which we are checking here. Supporting push might be trickier, as we currently rely on github.event.pull_request.base.sha.
• Should we be using this specific standard at all? Are there better things out there? Should we stick with consistency?

Would love to hear your thoughts!

[pykettk]

Thanks for contributing this, @danslo ! 🧡

---

Is there a scenario where we would need to run on push as well?

I think you answered this question with your follow-up comment:

We should assume that direct pushes to the main branch is prohibited to any contributor: any code that we do introduce is done by PRs, which we are checking here.

I can't foresee an instance where we would want somebody to push changes directly so, personally, I think this current setup of running only for PRs is fine 🙂

---

Should we be using this specific standard at all? Are there better things out there? Should we stick with consistency?

I think using the Magento coding standard and being consistent with it is a good idea - it's not even an awful set of standards IMO 😅 If we disagree or have ideas for changes then adopting the Magento standards encourages us to push those changes there. Granted this is likely going to be slower than adopting a separate set of standards but I feel like it presents a more unified image 🤷‍♂️

[damienwebdev]

@danslo it is possible to compute the base sha of the last passing build, which may allow diffing against pushes.

Additionally, what's the argument against pinning with the lock file? I would much prefer not to have failing builds "because".

Additionally, is there a way to move this into: https://github.com/graycoreio/github-actions-magento2 instead? It would be nice if we had a central store (outside the repo) so that external users can use the exact same actions we do internally.

[danslo]

@pykettk

I think using the Magento coding standard and being consistent with it is a good idea - it's not even an awful set of standards IMO 😅 If we disagree or have ideas for changes then adopting the Magento standards encourages us to push those changes there. Granted this is likely going to be slower than adopting a separate set of standards but I feel like it presents a more unified image 🤷‍♂️

I think I agree!

@damienwebdev

@danslo it is possible to compute the base sha of the last passing build, which may allow diffing against pushes.

Yeah it should certainly be possible, I'm trying to figure out if we really need it.

Additionally, what's the argument against pinning with the lock file? I would much prefer not to have failing builds "because".

I guess two-fold:

1. composer.lock conflicts are all too common, especially with automatic merging from upstream. I would rather not handle that in the context of this PR - requires a wider conversation
2. The trade-off is between always having the latest coding standard and potentially something breaking upstream vs. always lagging behind on the coding standard until someone performs a composer update. I preferred the former, in part supported by this version constraint.

Additionally, is there a way to move this into: https://github.com/graycoreio/github-actions-magento2 instead? It would be nice if we had a central store (outside the repo) so that external users can use the exact same actions we do internally.

That's probably a good idea - and it also dodges the lock file problem! Anyone opposed to this?

[danslo]

Action moved here.

Current PR has been changed to use that action. Permissions are set to read-only.

Changing from draft to ready for review!

[ihor-sviziev]

@Vinai I think we can merge it

[Vinai]

Thank you @danslo for the MR, @damienwebdev, @pykettk and @ihor-sviziev for your thoughts and time!