From 9d1205cfac3a7aa60e53b0cbb53387b23d0018bf Mon Sep 17 00:00:00 2001 From: Lars Roettig Date: Thu, 21 Mar 2019 19:53:45 +0100 Subject: [PATCH 1/5] #20 Impelement ArrayMerge sniff in foreach --- .../Performance/ForeachArrayMergeSniff.php | 59 +++++++++++++++++++ .../Performance/ForeachArrayMergeUnitTest.inc | 17 ++++++ .../Performance/ForeachArrayMergeUnitTest.php | 33 +++++++++++ Magento2/ruleset.xml | 4 ++ 4 files changed, 113 insertions(+) create mode 100644 Magento2/Sniffs/Performance/ForeachArrayMergeSniff.php create mode 100644 Magento2/Tests/Performance/ForeachArrayMergeUnitTest.inc create mode 100644 Magento2/Tests/Performance/ForeachArrayMergeUnitTest.php diff --git a/Magento2/Sniffs/Performance/ForeachArrayMergeSniff.php b/Magento2/Sniffs/Performance/ForeachArrayMergeSniff.php new file mode 100644 index 00000000..d841f2a4 --- /dev/null +++ b/Magento2/Sniffs/Performance/ForeachArrayMergeSniff.php @@ -0,0 +1,59 @@ +getTokens(); + + $scopeOpener = $tokens[$stackPtr]['scope_opener']; + $scopeCloser = $tokens[$stackPtr]['scope_closer'] ; + + for ($i = $scopeOpener; $i < $scopeCloser; $i++) + { + $tag = $tokens[$i]; + if ($tag['code'] !== T_STRING) { + continue; + } + if ($tag['content'] !== 'array_merge') { + continue; + } + + $phpcsFile->addWarning($this->warningMessage, $i, $this->warningCode); + } + } +} diff --git a/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.inc b/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.inc new file mode 100644 index 00000000..89d0add2 --- /dev/null +++ b/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.inc @@ -0,0 +1,17 @@ + 'value', + 1 => 'value2', +]; + +$options = []; +foreach ($configurationSources as $source) { + $options = array_merge($options, $source); +} + +$options = []; +$itemCount = count($configurationSources); +for ($i = 0; $i <= $itemCount; $i++) { + $source = $options[$itemCount]; + $options = array_merge($options, $source)); +} diff --git a/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.php b/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.php new file mode 100644 index 00000000..f0a84921 --- /dev/null +++ b/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.php @@ -0,0 +1,33 @@ + 1, + 16 => 1, + ]; + } +} diff --git a/Magento2/ruleset.xml b/Magento2/ruleset.xml index 6cf09ab7..283b653a 100644 --- a/Magento2/ruleset.xml +++ b/Magento2/ruleset.xml @@ -230,6 +230,10 @@ 7 warning + + 7 + warning + 7 warning From eecaa67683c4937f18f8fabf9cba8d6f052d0f4d Mon Sep 17 00:00:00 2001 From: Lars Roettig Date: Tue, 26 Mar 2019 19:03:11 +0100 Subject: [PATCH 2/5] #20: add parrent --- .../Performance/ForeachArrayMergeSniff.php | 17 +++++++++++---- .../Performance/ForeachArrayMergeUnitTest.inc | 21 +++++++++++++++++-- .../Performance/ForeachArrayMergeUnitTest.php | 4 ++-- 3 files changed, 34 insertions(+), 8 deletions(-) diff --git a/Magento2/Sniffs/Performance/ForeachArrayMergeSniff.php b/Magento2/Sniffs/Performance/ForeachArrayMergeSniff.php index d841f2a4..52222ef8 100644 --- a/Magento2/Sniffs/Performance/ForeachArrayMergeSniff.php +++ b/Magento2/Sniffs/Performance/ForeachArrayMergeSniff.php @@ -16,7 +16,7 @@ class ForeachArrayMergeSniff implements Sniff * * @var string */ - protected $warningMessage = 'Array merge is slow in each functions. '; + protected $warningMessage = 'array_merge(...) is used in a loop and is a resources greedy construction.'; /** * Warning violation code. @@ -25,6 +25,11 @@ class ForeachArrayMergeSniff implements Sniff */ protected $warningCode = 'ForeachArrayMerge'; + /** + * @var array + */ + protected $foreachCache = []; + /** * @inheritdoc */ @@ -41,10 +46,9 @@ public function process(File $phpcsFile, $stackPtr) $tokens = $phpcsFile->getTokens(); $scopeOpener = $tokens[$stackPtr]['scope_opener']; - $scopeCloser = $tokens[$stackPtr]['scope_closer'] ; + $scopeCloser = $tokens[$stackPtr]['scope_closer']; - for ($i = $scopeOpener; $i < $scopeCloser; $i++) - { + for ($i = $scopeOpener; $i < $scopeCloser; $i++) { $tag = $tokens[$i]; if ($tag['code'] !== T_STRING) { continue; @@ -53,6 +57,11 @@ public function process(File $phpcsFile, $stackPtr) continue; } + $cacheKey = $phpcsFile->getFilename() . $i; + if (isset($this->foreachCache[$cacheKey])) { + continue; + } + $phpcsFile->addWarning($this->warningMessage, $i, $this->warningCode); } } diff --git a/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.inc b/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.inc index 89d0add2..d18eee90 100644 --- a/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.inc +++ b/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.inc @@ -5,8 +5,11 @@ $configurationSources = [ ]; $options = []; -foreach ($configurationSources as $source) { - $options = array_merge($options, $source); + +foreach ([] as $collection) { + foreach ($configurationSources as $source) { + $options = array_merge($options, $source); + } } $options = []; @@ -15,3 +18,17 @@ for ($i = 0; $i <= $itemCount; $i++) { $source = $options[$itemCount]; $options = array_merge($options, $source)); } + +class CSV +{ + public static function getColumns() + { + return []; + } +} + +foreach ([] as $collection) { + foreach ($configurationSources as $source) { + $options = array_merge(CSV::getColumns(), $source); + } +} \ No newline at end of file diff --git a/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.php b/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.php index f0a84921..ca10b295 100644 --- a/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.php +++ b/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.php @@ -26,8 +26,8 @@ public function getErrorList() public function getWarningList() { return [ - 9 => 1, - 16 => 1, + 11 => 1, + 19 => 1, ]; } } From e48193e1e8a01ec9444036dc8dcdcf749a262332 Mon Sep 17 00:00:00 2001 From: Lars Roettig Date: Wed, 27 Mar 2019 19:40:49 +0100 Subject: [PATCH 3/5] Update implementation --- .../Performance/ForeachArrayMergeSniff.php | 1 + .../Performance/ForeachArrayMergeUnitTest.inc | 17 +++++++++++++---- .../Performance/ForeachArrayMergeUnitTest.php | 2 +- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/Magento2/Sniffs/Performance/ForeachArrayMergeSniff.php b/Magento2/Sniffs/Performance/ForeachArrayMergeSniff.php index 52222ef8..0f6e2cd1 100644 --- a/Magento2/Sniffs/Performance/ForeachArrayMergeSniff.php +++ b/Magento2/Sniffs/Performance/ForeachArrayMergeSniff.php @@ -62,6 +62,7 @@ public function process(File $phpcsFile, $stackPtr) continue; } + $this->foreachCache[$cacheKey] = ''; $phpcsFile->addWarning($this->warningMessage, $i, $this->warningCode); } } diff --git a/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.inc b/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.inc index d18eee90..fa003a35 100644 --- a/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.inc +++ b/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.inc @@ -19,16 +19,25 @@ for ($i = 0; $i <= $itemCount; $i++) { $options = array_merge($options, $source)); } -class CSV +class SelectBuilder { - public static function getColumns() + private $columns = []; + + public function getColumns() { - return []; + return $this->columns; + } + + public function setColumns(array $columns) + { + $this->columns = $columns; } } +$selectBuilder = new SelectBuilder(); + foreach ([] as $collection) { foreach ($configurationSources as $source) { - $options = array_merge(CSV::getColumns(), $source); + $selectBuilder->setColumns(array_merge($selectBuilder->getColumns(), $source)); } } \ No newline at end of file diff --git a/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.php b/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.php index ca10b295..7efb0d0a 100644 --- a/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.php +++ b/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.php @@ -27,7 +27,7 @@ public function getWarningList() { return [ 11 => 1, - 19 => 1, + 19 => 1 ]; } } From 3d46f1206d4b72d4ec3bbc34ae3509a8f3e559ef Mon Sep 17 00:00:00 2001 From: Lars Roettig Date: Fri, 29 Mar 2019 16:45:22 +0100 Subject: [PATCH 4/5] #20: Fix phpcs --- Magento2/Sniffs/Performance/ForeachArrayMergeSniff.php | 3 +++ Magento2/Tests/Performance/ForeachArrayMergeUnitTest.inc | 2 +- Magento2/Tests/Performance/ForeachArrayMergeUnitTest.php | 6 ++++-- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/Magento2/Sniffs/Performance/ForeachArrayMergeSniff.php b/Magento2/Sniffs/Performance/ForeachArrayMergeSniff.php index 0f6e2cd1..ff32e846 100644 --- a/Magento2/Sniffs/Performance/ForeachArrayMergeSniff.php +++ b/Magento2/Sniffs/Performance/ForeachArrayMergeSniff.php @@ -9,6 +9,9 @@ use PHP_CodeSniffer\Files\File; use PHP_CodeSniffer\Sniffs\Sniff; +/** + * Detects array_merge(...) is used in a loop and is a resources greedy construction. + */ class ForeachArrayMergeSniff implements Sniff { /** diff --git a/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.inc b/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.inc index fa003a35..525ea6ba 100644 --- a/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.inc +++ b/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.inc @@ -16,7 +16,7 @@ $options = []; $itemCount = count($configurationSources); for ($i = 0; $i <= $itemCount; $i++) { $source = $options[$itemCount]; - $options = array_merge($options, $source)); + $options = array_merge($options, $source); } class SelectBuilder diff --git a/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.php b/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.php index 7efb0d0a..16273883 100644 --- a/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.php +++ b/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.php @@ -3,6 +3,7 @@ * Copyright © Magento. All rights reserved. * See COPYING.txt for license details. */ + namespace Magento2\Tests\Performance; use PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest; @@ -10,7 +11,7 @@ /** * Class EmptyCheckUnitTest */ -class ForeachArrayMergeUnitTest extends AbstractSniffUnitTest +class ForeachArrayMergeUnitTest extends AbstractSniffUnitTest { /** * @inheritdoc @@ -27,7 +28,8 @@ public function getWarningList() { return [ 11 => 1, - 19 => 1 + 19 => 1, + 41 => 1 ]; } } From 9fa11690bb02168f644a91dbeac2617d9fddc43a Mon Sep 17 00:00:00 2001 From: Lars Roettig Date: Sun, 31 Mar 2019 16:32:43 +0200 Subject: [PATCH 5/5] Review fixes --- Magento2/Sniffs/Performance/ForeachArrayMergeSniff.php | 1 - Magento2/Tests/Performance/ForeachArrayMergeUnitTest.inc | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/Magento2/Sniffs/Performance/ForeachArrayMergeSniff.php b/Magento2/Sniffs/Performance/ForeachArrayMergeSniff.php index ff32e846..eae75006 100644 --- a/Magento2/Sniffs/Performance/ForeachArrayMergeSniff.php +++ b/Magento2/Sniffs/Performance/ForeachArrayMergeSniff.php @@ -3,7 +3,6 @@ * Copyright © Magento. All rights reserved. * See COPYING.txt for license details. */ - namespace Magento2\Sniffs\Performance; use PHP_CodeSniffer\Files\File; diff --git a/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.inc b/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.inc index 525ea6ba..02aa5093 100644 --- a/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.inc +++ b/Magento2/Tests/Performance/ForeachArrayMergeUnitTest.inc @@ -40,4 +40,4 @@ foreach ([] as $collection) { foreach ($configurationSources as $source) { $selectBuilder->setColumns(array_merge($selectBuilder->getColumns(), $source)); } -} \ No newline at end of file +}