[PSR-2 Compliance] Fix #8612: Hundreds of PHPCS-based static tests violations in mainline #8685

orlangur · 2017-02-26T23:31:52Z

Scope of Work

Enabled PHPCS-based tests for the whole codebase
Combined three PHPCS-based tests into one
Polished up Magento Coding Standard into one ruleset.xml
- no new rules added
- only those rules were removed which are contained in PSR2
Made static tests run under PHP 7 which is ~3 times faster than PHP 5.6
Fixed all existing coding style violations, mostly using automation tools
- no @codingStandardsIgnoreFile annotations removed to reduce size of PR
- new @codingStandardsIgnoreFile only added when really necessary:
  - one wrongly named interface marked with @api (can be addressed separately, didn't want to mix with cosmetic changes)
  - few phtml files which show false-positive with currently used PHPCS 1.5.3 and no violation with latest stable PHPCS 2.8.0 (example: orlangur@ecf0948)

Code Review Notes

Changes which require manual review are not so big compared to the size of pull request: https://github.com/magento/magento2/compare/develop...orlangur:phpcs-tests-refactoring-no-auto-fixes?expand=1
Commits orlangur@74f3a5a and orlangur@bc79f6d are obtained using automatic tools, step-by-step instructions are present in commit messages; to review them simply repeat the same steps and assure that no changes shown by git diff
These instructions may be also applied to non-public code if the fix of PHPCS-based tests revealed a lot of violations in it

okorshenko · 2017-02-27T03:51:41Z

@orlangur thank you for this contribution. It will take some time to review all changes in this PR. And thank you for providing URL for the code change which requires manual review

okorshenko · 2017-02-27T03:53:18Z

Made static tests run under PHP 7 which is ~3 times faster than PHP 5.6

+100500 to your karma! 👍

PieterCappelle · 2017-02-28T18:27:50Z

Would clean up the code. Very nice PR.

orlangur · 2017-03-03T14:15:41Z

@okorshenko, please ping me back if I need to resolve merge conflicts in order to get this merged.

Currently there is only one pretty easy to fix conflict in app/code/Magento/Analytics/ReportXml/ReportProvider.php, as I did not change semantics of this piece of code, just formatting.

okorshenko · 2017-03-10T23:27:13Z

Hi @orlangur
Could you please provide step by step instruction how to reproduce your fix. So that we will review only manual changes and automatically verify auto changes.

Please, resolve the conflicts if you can. Thank you

orlangur · 2017-03-11T10:02:04Z

@okorshenko, in PR description I mentioned

Commits orlangur/magento2@74f3a5a and orlangur/magento2@bc79f6d are obtained using automatic tools, step-by-step instructions are present in commit messages; to review them simply repeat the same steps and assure that no changes shown by git diff

There you can see:

Step 0. Install php-cs-fixer globally: composer global require friendsofphp/php-cs-fixer:2.1.0

Step 1. Remove outdated php-cs-fixer config: rm .php_cs
...

for both of automatic commits :)

The last small commit 9db86d8 is manual too, it is not possible to squash with changes before automatic commits as those changes are in files with CRLF line breaks.

Ok, will resolve conflicts by hard reset before automatic commits + re-apply of automatic changes. So, don't be surprised there will be no merge commit (of course, if there are no conflicting changes with my manual changes in mainline).

…ests violations in mainline - enable PHPCS-based tests for the whole codebase - combine three PHPCS-based tests into one - polish up Magento Coding Standard into one ruleset.xml: no new rules added, only those rules removed which are already contained in PSR2 - run static tests under PHP 7 which is ~3 times faster than PHP 5.6

…ests violations in mainline - apply manual fixes to make PHPCS happy, changes for "Constants are not allowed as the first argument of translation function, use string literal instead" - third-party constants are simply ignored, not sure if this is correct or not

…ests violations in mainline - apply manual fixes to make PHPCS and PHPMD happy - some phtml templates marked as ignored for now as PHPCS 1.5.3 version produces false-positives on them (PHPCS 2.8.0 treats them fine)

…ests violations in mainline - apply automatic PHP-CS-FIXER fixes to make PHPCS happy - Step 0. Install php-cs-fixer globally: composer global require friendsofphp/php-cs-fixer:2.1.0 - Step 1. Remove outdated php-cs-fixer config: rm .php_cs - Step 2. Execute tool: ~/.composer/vendor/friendsofphp/php-cs-fixer/php-cs-fixer fix . --rules=no_extra_consecutive_blank_lines,method_separation -v - Step 3. Reset changed fixture file to not break related unit test: git checkout HEAD -- lib/internal/Magento/Framework/Code/Test/Unit/_files/app/code/Magento/SomeModule/Model/SevenInterface.php - Step 4. Restore outdated php-cs-fixer config: git checkout HEAD -- .php_cs

…ests violations in mainline - apply automatic PHPCBF fixes to make PHPCS happy - Step 0. Install phpcs globally: composer global require squizlabs/php_codesniffer:2.8.0 - Step 1. Execute tool to fix PSR-2 violations in PHP files: ~/.composer/vendor/squizlabs/php_codesniffer/scripts/phpcbf --standard=PSR2 --extensions=php --ignore=generated/*,vendor/*,var/* .

…ests violations in mainline - apply manual fixes to make PHPCS and PHPMD happy

orlangur · 2017-03-11T13:38:55Z

Rebased against latest mainline and reapplied fixes.

Manual commits: 0338636, c3e7df5, 8d379e6, f122079

Automated commits: 749518c, 002d771

Old branch (for the reference): https://github.com/magento/magento2/compare/develop...orlangur:phpcs-tests-refactoring-before-sync?expand=1

okorshenko · 2017-03-13T22:40:07Z

@orlangur thank you

okorshenko · 2017-03-14T00:20:54Z

@orlangur from the first look everything is ok.
There are 2 logical errors in /pub/errors/processor.php

!is_null($local) ==> $local === null

I will fix it on our side

Tomorrow I will code review one more time to make sure I didn't miss any other issues. After that, I will start merge procedure.

Thank you for this PR!

orlangur · 2017-03-14T08:51:37Z

!is_null($local) ==> $local === null

Oh, my bad, sorry, it would be better to fix it with php-cs-fixer too, just didn't bother to.

okorshenko · 2017-03-14T22:56:48Z

@orlangur could you please provide detailed explanation why some rules were removed from the ruleset:

Zend.Files.ClosingTag
Zend.NamingConventions.ValidVariableName (explude PrivateNoUnderscore)
PEAR.NamingConventions.ValidClassName
PEAR.Classes.ClassDeclaration
PEAR.Functions.FunctionDeclaration
PEAR.WhiteSpace.ScopeClosingBrace
PEAR.ControlStructures.ControlSignature

Please let me know if I missed some other rules

orlangur · 2017-03-15T06:46:00Z

@okorshenko explanation is already provided in 0338636 commit message:

polish up Magento Coding Standard into one ruleset.xml: no new rules added, only those rules removed which are already contained in PSR2

okorshenko · 2017-03-15T21:56:16Z

@orlangur thank you for your contribution. Your Pull Request has been successfully merged. Great job!

fooman · 2017-03-15T22:58:49Z

Awesome stuff @orlangur

orlangur · 2017-03-15T23:50:31Z

Cool, thx! :)

Didn't expect it to be merged that fast (or even merged at all :D). Magento 2 contribution starting to be a pleasure, keep up the decent pace reached 👍

PieterCappelle · 2017-03-16T19:01:20Z

Great to see that this commit has been merged so quickly. Keep up this fantastic progress!

Tier4 Kings PR Delivery 12.18.2023

okorshenko self-assigned this Feb 27, 2017

okorshenko added this to the February 2017 milestone Feb 27, 2017

okorshenko self-requested a review February 27, 2017 03:51

maksek modified the milestones: February 2017, March 2017 Mar 1, 2017

okorshenko added this to the March 2017 milestone Mar 1, 2017

orlangur mentioned this pull request Mar 8, 2017

Upgrade PHP CS Fixer to v2 + re-apply #8823

Closed

okorshenko added the improvement label Mar 10, 2017

orlangur added 6 commits March 11, 2017 13:19

[PSR-2 Compliance] Fix magento#8612: Hundreds of PHPCS-based static t…

f122079

…ests violations in mainline - apply manual fixes to make PHPCS and PHPMD happy

orlangur force-pushed the phpcs-tests-refactoring branch from 9db86d8 to f122079 Compare March 11, 2017 13:34

okorshenko approved these changes Mar 15, 2017

View reviewed changes

okorshenko added the Progress: accept label Mar 15, 2017

magento-team merged commit f122079 into magento:develop Mar 15, 2017

orlangur deleted the phpcs-tests-refactoring branch March 15, 2017 23:50

orlangur mentioned this pull request Jun 7, 2017

[PSR-2 Compliance] The Great @codingStandardsIgnoreFile Massacre #9367

Closed

4 tasks

orlangur mentioned this pull request Oct 11, 2017

PHP Livecodetest testCodeStyle() method does not use whitelist files #11362

Merged

4 tasks

orlangur mentioned this pull request Feb 18, 2019

[New Rule] Function/method call in for loop declaration MUST be avoided magento/magento-coding-standard#28

Closed

magento-devops-reposync-svc pushed a commit that referenced this pull request Dec 18, 2023

Merge pull request #8685 from magento-l3/Tier4-Kings-PR-12-18-2023

e53e613

Tier4 Kings PR Delivery 12.18.2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PSR-2 Compliance] Fix #8612: Hundreds of PHPCS-based static tests violations in mainline #8685

[PSR-2 Compliance] Fix #8612: Hundreds of PHPCS-based static tests violations in mainline #8685

orlangur commented Feb 26, 2017 •

edited by okorshenko

okorshenko commented Feb 27, 2017

okorshenko commented Feb 27, 2017 •

edited

PieterCappelle commented Feb 28, 2017

orlangur commented Mar 3, 2017

okorshenko commented Mar 10, 2017

orlangur commented Mar 11, 2017

orlangur commented Mar 11, 2017

okorshenko commented Mar 13, 2017

okorshenko commented Mar 14, 2017 •

edited

orlangur commented Mar 14, 2017

okorshenko commented Mar 14, 2017 •

edited

orlangur commented Mar 15, 2017

okorshenko commented Mar 15, 2017

fooman commented Mar 15, 2017

orlangur commented Mar 15, 2017

PieterCappelle commented Mar 16, 2017

[PSR-2 Compliance] Fix #8612: Hundreds of PHPCS-based static tests violations in mainline #8685

[PSR-2 Compliance] Fix #8612: Hundreds of PHPCS-based static tests violations in mainline #8685

Conversation

orlangur commented Feb 26, 2017 • edited by okorshenko

Scope of Work

Code Review Notes

okorshenko commented Feb 27, 2017

okorshenko commented Feb 27, 2017 • edited

PieterCappelle commented Feb 28, 2017

orlangur commented Mar 3, 2017

okorshenko commented Mar 10, 2017

orlangur commented Mar 11, 2017

orlangur commented Mar 11, 2017

okorshenko commented Mar 13, 2017

okorshenko commented Mar 14, 2017 • edited

orlangur commented Mar 14, 2017

okorshenko commented Mar 14, 2017 • edited

orlangur commented Mar 15, 2017

okorshenko commented Mar 15, 2017

fooman commented Mar 15, 2017

orlangur commented Mar 15, 2017

PieterCappelle commented Mar 16, 2017

orlangur commented Feb 26, 2017 •

edited by okorshenko

okorshenko commented Feb 27, 2017 •

edited

okorshenko commented Mar 14, 2017 •

edited

okorshenko commented Mar 14, 2017 •

edited