Upgrade PHP CS Fixer to v2 + re-apply

[keradus]

vide #8822

[magento-cicd2]

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

[PieterCappelle]

+1.

[orlangur]

LOOOL :) You made my day if not month. I was the one who introduced .php_cs configuration into Magento 2 code together with lame git hook. It's really nice to see one of php-cs-fixer authors paid attention to Magento 2 code base now 👍

So, let me share some insight on Magento 2 codebase current state of things and what I was planning to do after #8685 (where php-cs-fixer together with phpcbf definitely did an excellent job 👍 ). php-cs-fixer upgrade obviously was in TODO and I'm happy to be not alone in this war :)

First of all, I noticed that without my attention php-cs-fixer usage in Magento 2 became abandoned, most likely the only time it was ever applied to codebase was switching to short array syntax. When I introduced .php_cs configuration the plan was to introduce static test enforcing this set of rules soon after, but it happened to be not possible not being a Magento employee due to stuck community PR processing process :D The things seem to be moving in right direction now in 2k17 but there is still a lot of work left from troublous times (here is a quick example for illustration purposes: #4844).

Here is a realistic plan of how the things could be done within Magento 2 circumstances. I'm open for discussion and can provide rationale for every item by request as I could share not enough insight and/or overlooked something.

0. Make codebase PSR-2 compliant, enforce it with PHPCS-based test properly
   • [PSR-2 Compliance] Fix #8612: Hundreds of PHPCS-based static tests violations in mainline #8685
1. Make codebase really PSR-2 compliant by removing @codingStandardsIgnoreFile from *.php files and fixing all violations
   • @codingStandardsIgnoreFile usage in *.php needs to be prohibited with some sanity check
2. Upgrade PHPCS to v2 (or directly v3 if already stable)
   • trickiest part is custom sniffs adoption, especially the huge all-in-one PHPDoc checker (most of them needs to be replaced with builtin PHPCS sniffs)
3. Make templates PSR-2 compliant by removing @codingStandardsIgnoreFile from *.phtml files and fixing all violations
   • it is not possible before PHPCS upgrade due to false-positives in version 1.5.3 currently used
4. Implement testNoViolationsDetectedByPhpCodingStandardsFixer static test
   • similar to testNoViolationsDetectedByPhpCodeSniffer and other 60d8cf0#diff-d6597432edc35ce95ce77c222caaf7bbL222
   • to be enabled right after we re-apply php-cs-fixer all over the codebase whenever it is occurred
   • @keradus, question to you - how fast php-cs-fixer can be in dry run mode? can we always scan full codebase or analyze only changed compared to mainline files, like we do for phpmd?
5. Define Magento 2 php-cs-fixer ruleset
   • this is just "which rules will be present in .php_cs configuration file of Magento 2 repo"
   • maybe make it extend Symfony 2 ruleset and not just PSR-2? why not if the rule "interface must end with Interface" which I didn't like was adopted :)
   • this step is needed according to my understanding to do re-apply only once and not just migrate v1 php-cs-fixer configuration which contains list of rules of my choice existed at that time
6. Upgrade php-cs-fixer, re-apply all over the codebase
7. [OPTIONAL] Proper git hook involving php-cs-fixer and phpcbf
   • main problem with current hook is that it is slow as hell due to StaticReview design limitation allowing to check only one file at a time
8. [OPTIONAL] Make PHPCS, PHP-CS-FIXER share same config of folders/paths
9. [OPTIONAL] Strict PSR-2 compliance
   • fulfill all SHOULDs preserving BC of code
10. [OPTIONAL] More php-cs-fixer, phpcs rules making code even more clean and consistent
    • I really like the way Symfony ruleset is constantly improving in php-cs-fixer and applied to Symfony codebase at the right moment

@keradus, I do not expect you willing to participate in anything but 6. which you already implemented, just want to get maximum from your efforts :) Full list is in a greater degree for Magento guys to comprehend the whole picture and assess how it aligns with Magento plans.

[keradus]

Hi @orlangur , thank you for your response !

First of all, I noticed that without my attention php-cs-fixer usage in Magento 2 became abandoned

My main purpose was to upgrade PHP CS Fixer from v1 to v2, as v1 is no longer maintened.
If current state is that repo is not running the tool properly yet, could we update PHP CS Fixer anyway (vide #8822) ?

I'm happy to be not alone in this war :)

It's not a war. Variety is good. Using any SCA tool is good. not sure if there is a need for using PHP Coding Standards Fixer and PHP Code Sniffer at the same time makes sense, if there are issues on applying any of them in the end...
#4844 case make me sad and indeed, it may not be willing to pursue this (re-apply) PR.
From what you wrote I understand that doing SCA is a will of community, but not a prio of maintainers.

Short suggestions - if possible, try to adopt existing rules (of any SCA tools), adjusting them if needed, but avoid writing custom rules. A lot of time and issue with maintainig it after some time. Also, well established rules are easier to follow by ppl.

---

ad 0. I was not aware of #8685, I think it makes this, mine PR obsolete and ready to close (still, I would leave #8822 as example how to update). Would you agree?

ad 4. speed is relative.
PHP CS Fixer v2 is at least twice faster than v1. Better speed up could be gained by running the tool on PHP 7 (up to 10 times faster comparing to v1).
Then, you could cache the outcome to not fix file twice if it was not modified.
And if you know the list of modified files, you could ask to only fix those (single Tool run with multiple files, not multiple Tool run per single file) - it's always faster to ask only about modified files, if I did modified 1 of 1000 files it's faster for any tool to check that one file instead of 1k files...
Fainaly, with next minor release, --stop-on-violation flag to stop fixing on first violation. Flag especially for CI integration.
(In the and, everything described here could be achieved running the code in test like you presented, even if we have example for CI integration)

ad 5. I thought it's already in .php_cs file. If not, then #8822 could be closed. Would you agree?
Anyway, definietly not my responsibility to choose a ruleset for this project ;) It's up to your community.

ad 7. I thought that StaticReview is dead. Try https://github.com/phpro/grumphp

ad 10. Funny part here - we don't make new rules and apply them on repo. We take existing rules (that are written down or already present in 95% of code) and then write rules for them ;)
Same was with PSR.

---

At any steps related to PHP CS Fixer feel free to ping me for any help, assistance or just a review.

[orlangur]

could we update PHP CS Fixer anyway (vide #8822) ?

Yeah, absolutely. I'm going to review it.

It's not a war. Variety is good.

The war against bad looking or bad smelling code I mean ;)

not sure if there is a need for using PHP Coding Standards Fixer and PHP Code Sniffer at the same time makes sense, if there are issues on applying any of them in the end...

The main motivation for php-cs-fixer usage was to reduce amount of manual CS fixing :) At that time phpcbf didn't even exist and phpcs could only report issues and not fix them. As both of php-cs-fixer/phpcbf are not interchangeable and are actively improving I think it would be good to stick to both of them, just because there is no any conceptual issue with getting benefits from both of them.

ad 0. I was not aware of #8685, I think it makes this ... Would you agree?

Actually I intentionally applied just a couple of php-cs-fixer rules there 74f3a5a to pass custom Magento sniffs checking these things and increase probability of merging, there could be too many conflicts for some internally ongoing development if we re-apply all rules. So, yeah, please close this PR and let's accomplish a half of "6. Upgrade php-cs-fixer, re-apply all over the codebase", the other one may wait.

Finally, with next minor release, --stop-on-violation flag to stop fixing on first violation. Flag especially for CI integration.

In worst case it will be the same time anyway, I don't trust the delta mechanism after #8612 but concrete decision could be made in scope of testNoViolationsDetectedByPhpCodingStandardsFixer static test implementation. What I know is that phpcs takes some 2-3 minutes to scan this full codebase, for the php-cs-fixer seeing concrete numbers I'll try some tweaks, like switching to delta check or exclude already covered PSR-2 rules, for instance.

ad 5. I thought it's already in .php_cs file

What I was trying to say is to revise existing ruleset before re-apply (to avoid double re-apply) and enable all possible useful fixers :) #8822 without changing list of used fixers is totally fine.

ad 7. I thought that StaticReview is dead. Try https://github.com/phpro/grumphp

Thanks, didn't hear of it before, if it allows to apply php-cs-fixer for full changeset at once I'll definitely give it a try.

ad 10. Funny part here - we don't make new rules and apply them on repo

Aha, I see :) For me it's usually an opposite process: I see something I don't like or something I really like in another project and then apply it to my project with automated tests enforcement to eliminate any discussions.

[keradus]

Thanks for answer.
Closing as discussed.