Magento-lts: Add phpCBF to worklow?

[sreichel]

Is it possible to run phpCBF (PSR2) before files are merged?

The doc PRs have automated fixes, but they dont cover all files ... with pre-merge hook we could improve code with every PR.

[tmotyl]

There are magento official rules for php code sniffer we can use. For just cgl changes (psr-2 and psr-12) we can also use the php-cs-fixer.

We could also add the check as pre-commit hooks using captain hooks, so peole will not push wrong code

[Flyingmana]

Do you mean as a check, or as a fixer?
Check is easy, as something which changes the Files is a lot harder.

Adding https://github.com/CaptainHookPhp/captainhook could be usefull, as long as we do not require it for all contributors to use.
Also Iam not a fan of mixing code and codestyle changes together, its often enough work to check one of it :/

[tmotyl]

My idea was that php-cs-fixer should be run as check (--dry-run) in gitlab actions/travis and with "fix mod" locally on git commit hook.
Then if commit fails, you wil see whitespace changes made by php-cs-fixer as unstaged changes.
So you add them to the commit (stage) and commit again.

Alternatively we can run dry-run in both cases, but provide a convenience script like "build/fixMyCommit.sh" which you can run to fix files from the last commit, and then stage the and commit --amend.

See how such a script works in TYPO3 world https://github.com/TYPO3/TYPO3.CMS/blob/master/Build/Scripts/cglFixMyCommit.sh
https://docs.typo3.org/m/typo3/guide-contributionworkflow/master/en-us/Appendix/CglFixMyCommit.html

[sreichel]

Do you mean as a check, or as a fixer?
Check is easy, as something which changes the Files is a lot harder.

Question was for fixer. Automated fixes (psr2?) for spaces, linebreaks, ....

E.g. I dont want to ask for fix spaces here https://github.com/OpenMage/magento-lts/pull/980/commits/dca888af02ebc4c4857237be27e51e963bf6af9f ...

Maybe we can add a CONTRIBUTING file and suggest using pre-commit hook like https://gist.github.com/fdemiramon/0423b4308218d417fbf3?

[tmotyl]

Travis/github actions should check for code style automatically and go red if commit doesnt comply. So the author should run fixer and push the patch again. We should not merge any new patches with broken code style

[Flyingmana]

could we formulate this a bit more positive?

"While a failing code style check is not a blocker, it requires a bit of extra work on our side, which is why for merging PullRequests in general, this are favored which do not show any errors during the codestyle check"

I speak of my own experience when I say, its very discouraging if I provide something, and then get told, that it cant be merged because of smaller codestyle issues.
Usually when I also do not have the time to fix them in the next few weeks

[sreichel]

I speak of my own experience when I say, its very discouraging if I provide something, and then get told, that it cant be merged because of smaller codestyle issues.
Usually when I also do not have the time to fix them in the next few weeks

Agreed.

I don't want to see PR blocked because of codestyle issues! Automated fixes would be nice to have .... otherwise i'll fix it manually :)

[colinmollenhour]

To do this my proposal would be to:

• Relax the rules considerably
• Run it on all of the app/code code at once in one huge commit
• Not run it on app/design/, lib/ or other code, just generally leave these alone
• Provide a script or one-liner (or pre-commit hook) so that developers can easily run it locally before pushing commits (easy to do with docker run --rm ...)
• Add a pipeline that fails if the code is not free of new errors. This will prevent some PRs from being merged but the above point should make it easy to fix

Otherwise we will just slowly drift away from clean code and end up changing the same lines multiple times thereby clobbering a clean git blame, wasting time of maintainers, etc.

[sreichel]

* Run it on all of the app/code code at once in one huge commit

Agree, but DOC block updates should be merged before. Dont want to fix conflicts ;)

[damien-biasotto]

If I may add :

Once the big commit is done, why not add a pre-commit hook to automatically fix any CS violations. I set up a similar hook at work (for LESS/JS and PHP).

You only focus on the code, and le the commit hook fix any CS issue.

[sreichel]

This is what i do, slightly modified ... https://github.com/smgladkovskiy/phpcs-git-pre-commit)

I guess most LTS users have composer based setup .... why not add something like this to require-dev?