Skip to content

#20 Impelement ArrayMerge sniff in foreach #72

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Apr 8, 2019

Conversation

larsroettig
Copy link
Member

@larsroettig larsroettig commented Mar 21, 2019

Implementation for #20

Currently we have many findings in 2.3 develop


bin/phpcs --standard=Magento2 --sniffs=Magento2.Performance.ForeachArrayMerge  app/code app/lib

Magento 2 findings with current Implementation

------------------------------------------------------------------------------------------------------
A TOTAL OF 0 ERRORS AND 280 WARNINGS WERE FOUND IN 212 FILES
------------------------------------------------------------------------------------------------------

@lenaorobei
Copy link
Contributor

I checked some findings and they look false-positive to me. Probably, they require serious code refactoring, not just moving array_merge outside the loop.
Need more thoughts here to understand if we really want to warn developers about any case of usage array_merge inside the loop.

@larsroettig larsroettig requested a review from lenaorobei March 29, 2019 16:17
@buskamuza
Copy link

We should accept it, but include explanation of how to fix the problematic code. Think about a catalog of recommendations (either here in wiki or in devdocs).

@lenaorobei lenaorobei merged commit a5bf741 into develop Apr 8, 2019
@lenaorobei lenaorobei deleted the 20-ForeachArrayMerge branch April 8, 2019 16:04
@orlangur
Copy link

@larsroettig @buskamuza

A TOTAL OF 0 ERRORS AND 280 WARNINGS WERE FOUND IN 212 FILES

So how this rule can be useful?

magento-devops-reposync-svc pushed a commit that referenced this pull request Sep 27, 2021
…oding-standard-281

[Imported] AC-669 install upgrade test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants