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

orlangur · 2017-04-23T15:12:32Z

This PR fixes mistake introduced in 004c9bb which resulted in hundreds of files with coding style violations.

Description

As pointed out in #9251 (comment) manual fixing would take too much time and developer/reviewer efforts. Thus I did exactly what was described in #7166 (comment) to get maximum profit out of existing automated tools.

Review Notes

Manual commits: orlangur@a1f9e1a, 40ba673, c87c521
Automated commits (just repeat steps from commit message instead of manual review): orlangur@b51681c, 4dc56e0, c619404, d813974
PHPMD on Travis CI was not able to complete within 10 minutes limit thus I checked static tests in two chunks (modules only and all but modules in whitelist): https://travis-ci.org/orlangur/magento2/builds/224861865, https://travis-ci.org/orlangur/magento2/builds/224861991

Contribution checklist

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

- adjust php-cs-fixer configuration: add is_null and method_separation rules already enforced by phpcs sniffers, update excluded paths

- remove @codingStandardsIgnoreFile annotation all over the codebase - Step 0. Run command: find . -name "*.php" -type f | xargs sed -i -e "/@codingStandardsIgnoreFile/d"

- apply automatic php-cs-fixer fixes to make PHPCS happy - Step 0. Execute tool: ./vendor/bin/php-cs-fixer fix . -v

- 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/* .

- revert files with nontrivial issues to mainline state for further investigation - Step 0. Run command: git checkout bbe3ee0 -- lib/internal/Magento/Framework/Code/Test/Unit/_files/app/code/Magento/SomeModule/Model/SevenInterface.php lib/internal/Magento/Framework/ObjectManager/Test/Unit/Config/Reader/_files/ConfigDomMock.php lib/internal/Magento/Framework/Interception lib/internal/Magento/Framework/Model/Test/Unit/ResourceModel/Db/Collection/AbstractCollectionTest.php lib/internal/Magento/Framework/Code/ dev/tests/integration/testsuite/Magento/ImportExport/Model/Export/EntityAbstractTest.php dev/tests/integration/testsuite/Magento/Cms/Controller/RouterTest.php dev/tests/integration/testsuite/Magento/Framework/Interception/ dev/tests/static/get_github_changes.php setup/src/Magento/Setup/Test/Unit/Module/I18n/Parser/ParserTest.php app/code/Magento/Vault/Api/Data/PaymentTokenInterfaceFactory.php app/code/Magento/Customer/Test/Unit/Model/ResourceModel/AddressTest.php dev/tests/integration/testsuite/Magento/Setup/Console/Command/_files/phrases/TestPhrases.php

- apply manual fixes to make PHPCS happy, mainly "Line exceeds maximum limit of 120 characters" violations

- apply manual fixes to make PHPMD happy

…gStandardsIgnoreFile-massacre Conflicts: app/code/Magento/Backend/Block/Page/Notices.php app/code/Magento/Backend/Block/Widget/Container.php app/code/Magento/Catalog/Model/ResourceModel/Product/Indexer/Eav/Source.php app/code/Magento/CatalogRule/Setup/UpgradeData.php app/code/Magento/CatalogSearch/Model/Adapter/Mysql/Filter/Preprocessor.php app/code/Magento/CatalogSearch/Model/Search/IndexBuilder.php app/code/Magento/CatalogSearch/Test/Unit/Model/Adapter/Aggregation/AggregationResolverTest.php app/code/Magento/CatalogSearch/Test/Unit/Model/Adapter/Mysql/Dynamic/DataProviderTest.php app/code/Magento/Cms/Setup/ContentConverter.php app/code/Magento/GoogleOptimizer/Block/Code/Category.php app/code/Magento/GoogleOptimizer/Block/Code/Product.php app/code/Magento/LayeredNavigation/Block/Navigation/State.php app/code/Magento/Paypal/Model/Payflow/Service/Response/Transaction.php app/code/Magento/Paypal/Model/Payflow/Transparent.php app/code/Magento/Paypal/Test/Unit/Controller/Payflow/ReturnUrlTest.php app/code/Magento/Paypal/Test/Unit/Model/Payflow/Service/Response/TransactionTest.php app/code/Magento/Paypal/Test/Unit/Model/Payflow/TransparentTest.php app/code/Magento/Paypal/Test/Unit/Model/PayflowlinkTest.php app/code/Magento/Paypal/Test/Unit/Model/PayflowproTest.php app/code/Magento/Quote/Setup/ConvertSerializedDataToJson.php app/code/Magento/Sales/Setup/UpgradeData.php app/code/Magento/SalesRule/Setup/UpgradeData.php app/code/Magento/Tax/Block/Adminhtml/Rate/Toolbar/Add.php app/code/Magento/Widget/Block/Adminhtml/Widget.php app/code/Magento/Widget/Helper/Conditions.php app/code/Magento/Widget/Setup/LayoutUpdateConverter.php app/code/Magento/Widget/Setup/UpgradeData.php dev/tests/functional/tests/app/Magento/Store/Test/TestCase/UpdateStoreEntityTest.php dev/tests/integration/framework/tests/unit/testsuite/Magento/Test/ObjectManagerTest.php lib/internal/Magento/Framework/Data/Wysiwyg/Normalizer.php lib/internal/Magento/Framework/Indexer/BatchSizeManagement.php lib/internal/Magento/Framework/Indexer/Test/Unit/BatchSizeManagementTest.php

- apply manual fixes to make PHPCS happy

orlangur · 2017-05-16T17:39:41Z

Now synced and green: https://travis-ci.org/orlangur/magento2/builds/232866106 (integration test failure seems unrelated as it passed in previous run and main branch run), https://travis-ci.org/orlangur/magento2/builds/232587786

…acre

orlangur · 2017-06-06T20:10:42Z

@ishakhsuvarov @okorshenko please share your thoughts on current changes.

I'll do re-apply over latest mainline, so that there will be no merge commits, but will preserve old manual commits and any additional required changes will come in separate commits on top of that.

okorshenko · 2017-06-07T16:11:32Z

Hi @orlangur
Unfortunately it is very hard to verify such big changes. I was trying to stabilize this branch few times, but it is still failing.

Let's do the next. Please, separate each module and each framework component into individual PR. They will be smaller and easier for review and stabilization.

I will close this PR and will wait for series of PRs based on these changes. Thank you for collaboration.

orlangur · 2017-06-07T18:09:44Z

@okorshenko it was stable at the moment of publishing and I believe it would be pretty easy to re-apply and stabilize again at any moment.

Quote from description:

Review Notes

Manual commits: orlangur@a1f9e1a, 40ba673, c87c521
Automated commits (just repeat steps from commit message instead of manual review): orlangur@b51681c, 4dc56e0, c619404, d813974

Do you have any concerns regarding manual changes mentioned?

okorshenko · 2017-06-07T18:20:17Z

@orlangur it is almost impossible to review 4k changed files, despite these changes were done automatically. We must to make sure that all changes are secure. I understand your concern, but it will be better to deliver such big changes partially to 2.2 release. Otherwise they will go to 2.3 :(

orlangur · 2017-06-07T18:48:47Z

Hmmm, why need to review automated changes with bare eyes and not just repeat the same commands and then do git diff? Used tools are open source and unlikely to contain backdoors or something.

That was the whole point of my optimization - maximum automatization which minimize code review efforts :) If there is some requirement to review changes manually, such approach simply does not work.

Please correct me if I'm wrong - #8685 had to be fully reviewed manually?

Thanks for the quick feedback, will do my best to get this merged into mainline to the earliest possible release, just want to understand the circumstances.

…e Massacre #9367 - fixed code style

okorshenko · 2017-06-09T21:44:15Z

@orlangur we created the tool to verify #8685. Some issues were detected by our tool and were fixed manually. Unfortunately, we can't validate this PR in the same way, so it is too risky to accept it without verification. Unfortunately, open source tool are also buggy

okorshenko · 2017-07-12T17:48:48Z

Hi @orlangur
Would like to proceed with these changes? Please let me know if you are interested in these changes so that we can define the strategy how we can do this together. Thank you

okorshenko · 2017-07-12T17:51:42Z

Here is a good example how we can do this:
#2204 (comment) . As an option that can be a shell file with all required commands inside. So we can run the script per module and deliver changes by small batches
Also we can have you as an author of commits for the future references and history

orlangur · 2017-07-13T09:00:31Z

@okorshenko thanks, at first sight it looks like some commands applied all over the code base (not module batching), please tell me if I'm wrong.

I still believe it can be done pretty easily without small batching (what I would like to avoid: apply phpcbf and then php-cs-fixer for only some specifically mentioned modules; UPD: sorry, stupid me, we can apply changes all over the codebase but then commit only a part of changes). Also, to avoid massive code changes ever I was thinking of test enforcing php-cs-fixer rules similar to phpcs one.

I'm going to repeat massacre this weekend, please share if possible whether it fits into 2.2.0 release plan so that efforts are not wasted.

okorshenko · 2017-07-13T15:23:53Z

Hi @orlangur
I'm not sure that it will go to 2.2 at this time, but that can be a part of 2.3 for sure
We will be able to do this in 2.2.1 as well

orlangur added 7 commits April 22, 2017 15:51

[PSR-2 Compliance] The Great @codingStandardsIgnoreFile Massacre

a1f9e1a

- adjust php-cs-fixer configuration: add is_null and method_separation rules already enforced by phpcs sniffers, update excluded paths

[PSR-2 Compliance] The Great @codingStandardsIgnoreFile Massacre

b51681c

- remove @codingStandardsIgnoreFile annotation all over the codebase - Step 0. Run command: find . -name "*.php" -type f | xargs sed -i -e "/@codingStandardsIgnoreFile/d"

[PSR-2 Compliance] The Great @codingStandardsIgnoreFile Massacre

4dc56e0

- apply automatic php-cs-fixer fixes to make PHPCS happy - Step 0. Execute tool: ./vendor/bin/php-cs-fixer fix . -v

[PSR-2 Compliance] The Great @codingStandardsIgnoreFile Massacre

40ba673

- apply manual fixes to make PHPCS happy, mainly "Line exceeds maximum limit of 120 characters" violations

[PSR-2 Compliance] The Great @codingStandardsIgnoreFile Massacre

c87c521

- apply manual fixes to make PHPMD happy

orlangur force-pushed the the-great-@codingStandardsIgnoreFile-massacre branch from a4eea58 to c87c521 Compare April 27, 2017 06:30

orlangur mentioned this pull request May 3, 2017

Fixed coding standard violations in the Framework\ObjectManager namespace #9362

Merged

orlangur mentioned this pull request May 14, 2017

Fix coding standard in Magento AdminNotification module #9627

Merged

orlangur requested review from ishakhsuvarov and okorshenko May 15, 2017 16:38

okorshenko self-assigned this May 15, 2017

okorshenko added develop improvement labels May 15, 2017

orlangur added 2 commits May 15, 2017 16:06

[PSR-2 Compliance] The Great @codingStandardsIgnoreFile Massacre

52fd79a

- apply manual fixes to make PHPCS happy

Merge branch 'develop' into the-great-@codingStandardsIgnoreFile-mass…

af888b8

…acre

okorshenko added this to the May 2017 milestone May 16, 2017

okorshenko modified the milestones: May 2017, June 2017 Jun 1, 2017

okorshenko closed this Jun 7, 2017

magento-team pushed a commit that referenced this pull request Jun 7, 2017

MAGETWO-69226: [PSR-2 Compliance] The Great @codingStandardsIgnoreFil…

ba0e2bf

…e Massacre #9367 - fixed code style

korostii mentioned this pull request Jun 20, 2017

Transactional Email logo not updating #9991

Closed

orlangur mentioned this pull request Nov 21, 2017

CS: reapply fixers #12192

Closed

orlangur mentioned this pull request Jan 20, 2018

Fix code style issues with php-cs-fixer #13270

Closed

4 tasks

ihor-sviziev mentioned this pull request Jun 15, 2018

Avoid calling function repetitively in loop conditionals for better performance #16142

Closed

orlangur mentioned this pull request Jun 18, 2018

PHPDoc #16215

Merged

orlangur mentioned this pull request May 11, 2019

Proposed removal of some static checks for phtml template files magento/architecture#160

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

orlangur commented Apr 23, 2017 •

edited by okorshenko

Loading

orlangur commented May 16, 2017

orlangur commented Jun 6, 2017

okorshenko commented Jun 7, 2017

orlangur commented Jun 7, 2017

okorshenko commented Jun 7, 2017

orlangur commented Jun 7, 2017

okorshenko commented Jun 9, 2017

okorshenko commented Jul 12, 2017 •

edited

Loading

okorshenko commented Jul 12, 2017 •

edited

Loading

orlangur commented Jul 13, 2017

okorshenko commented Jul 13, 2017

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

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

Conversation

orlangur commented Apr 23, 2017 • edited by okorshenko Loading

Description

Review Notes

Contribution checklist

orlangur commented May 16, 2017

orlangur commented Jun 6, 2017

okorshenko commented Jun 7, 2017

orlangur commented Jun 7, 2017

Review Notes

okorshenko commented Jun 7, 2017

orlangur commented Jun 7, 2017

okorshenko commented Jun 9, 2017

okorshenko commented Jul 12, 2017 • edited Loading

okorshenko commented Jul 12, 2017 • edited Loading

orlangur commented Jul 13, 2017

okorshenko commented Jul 13, 2017

orlangur commented Apr 23, 2017 •

edited by okorshenko

Loading

okorshenko commented Jul 12, 2017 •

edited

Loading

okorshenko commented Jul 12, 2017 •

edited

Loading