From 51fdf683c9172a05d15920db490fdcbed2714730 Mon Sep 17 00:00:00 2001 From: Lars Roettig Date: Fri, 8 Mar 2019 14:16:59 +0100 Subject: [PATCH 1/5] #26: Add Sniff for Gettersnot change state --- Magento/Sniffs/Functions/GetterStateSniff.php | 81 +++++++++++++++++++ .../Tests/Functions/GetterStateUnitTest.inc | 56 +++++++++++++ .../Tests/Functions/GetterStateUnitTest.php | 36 +++++++++ phpunit.xml.dist | 5 ++ 4 files changed, 178 insertions(+) create mode 100644 Magento/Sniffs/Functions/GetterStateSniff.php create mode 100644 Magento/Tests/Functions/GetterStateUnitTest.inc create mode 100644 Magento/Tests/Functions/GetterStateUnitTest.php diff --git a/Magento/Sniffs/Functions/GetterStateSniff.php b/Magento/Sniffs/Functions/GetterStateSniff.php new file mode 100644 index 00000000..d9cddf62 --- /dev/null +++ b/Magento/Sniffs/Functions/GetterStateSniff.php @@ -0,0 +1,81 @@ +getDeclarationName($stackPtr); + + if ($methodName === null || strpos($methodName, 'get') === false) { + // Ignore closures and no getters + return; + } + + $tokens = $phpcsFile->getTokens(); + $open = $tokens[$stackPtr]['scope_opener']; + $close = $tokens[$stackPtr]['scope_closer']; + + $isObjectScope = false; + $isObjectScopeToken = [T_SELF => T_SELF, T_PARENT => T_PARENT, T_STATIC => T_STATIC]; + + for ($i = ($open + 1); $i < $close; $i++) { + $token = $tokens[$i]; + + if (T_SEMICOLON === $token['code']) { + // Detect line end scope change to function scope. + $isObjectScope = false; + } + + if (array_key_exists($token['code'], $isObjectScopeToken)) { + $isObjectScope = true; + } + + if ($token['content'] === '$this') { + $isObjectScope = true; + } + + if ($isObjectScope === true && array_key_exists($token['code'], Tokens::$assignmentTokens)) { + $phpcsFile->addWarning($this->warningMessage, $i, $this->warningCode); + } + } + } +} diff --git a/Magento/Tests/Functions/GetterStateUnitTest.inc b/Magento/Tests/Functions/GetterStateUnitTest.inc new file mode 100644 index 00000000..2ff9c4da --- /dev/null +++ b/Magento/Tests/Functions/GetterStateUnitTest.inc @@ -0,0 +1,56 @@ +property = 1223; + return $this->property; + } + + /** + * @return int + */ + public function normalMethod() + { + $localVariable = 12; + return $localVariable; + } +} + +$d = function ($test) { + $test = 123; +}; + diff --git a/Magento/Tests/Functions/GetterStateUnitTest.php b/Magento/Tests/Functions/GetterStateUnitTest.php new file mode 100644 index 00000000..d4eb90f9 --- /dev/null +++ b/Magento/Tests/Functions/GetterStateUnitTest.php @@ -0,0 +1,36 @@ + 1, + 29 => 1, + 30 => 1, + 39 => 1, + ]; + } +} diff --git a/phpunit.xml.dist b/phpunit.xml.dist index 6d1afb07..b55ba133 100644 --- a/phpunit.xml.dist +++ b/phpunit.xml.dist @@ -8,4 +8,9 @@ + + + Magento/Sniffs + + From 9b34291c0f0ceb3e43c8257721159f1af49fe573 Mon Sep 17 00:00:00 2001 From: Lars Roettig Date: Fri, 8 Mar 2019 14:34:45 +0100 Subject: [PATCH 2/5] #26: Add Sniff for Gettersnot change state --- Magento/ruleset.xml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Magento/ruleset.xml b/Magento/ruleset.xml index 6c659ec2..c6f249ac 100644 --- a/Magento/ruleset.xml +++ b/Magento/ruleset.xml @@ -121,6 +121,10 @@ 8 warning + + 8 + warning + 8 warning From 230d2add9935fd8539c2e6edf34b3a43f6f0baa4 Mon Sep 17 00:00:00 2001 From: Lars Roettig Date: Fri, 8 Mar 2019 14:43:22 +0100 Subject: [PATCH 3/5] #26: Add Sniff for Gettersnot change state --- Magento/Sniffs/Functions/GetterStateSniff.php | 2 +- Magento/Tests/Functions/GetterStateUnitTest.inc | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/Magento/Sniffs/Functions/GetterStateSniff.php b/Magento/Sniffs/Functions/GetterStateSniff.php index d9cddf62..af352926 100644 --- a/Magento/Sniffs/Functions/GetterStateSniff.php +++ b/Magento/Sniffs/Functions/GetterStateSniff.php @@ -45,7 +45,7 @@ public function process(File $phpcsFile, $stackPtr) { $methodName = $phpcsFile->getDeclarationName($stackPtr); - if ($methodName === null || strpos($methodName, 'get') === false) { + if ($methodName === null || strpos($methodName, 'get') !== 0) { // Ignore closures and no getters return; } diff --git a/Magento/Tests/Functions/GetterStateUnitTest.inc b/Magento/Tests/Functions/GetterStateUnitTest.inc index 2ff9c4da..9c429b97 100644 --- a/Magento/Tests/Functions/GetterStateUnitTest.inc +++ b/Magento/Tests/Functions/GetterStateUnitTest.inc @@ -40,6 +40,12 @@ class Foo extends Bar return $this->property; } + public function TestigetFoo() + { + $this->property = 1223; + return $this->property; + } + /** * @return int */ From cc2b385eb45652e44b63d577edebf4ea66801139 Mon Sep 17 00:00:00 2001 From: Lars Roettig Date: Wed, 13 Mar 2019 19:37:45 +0100 Subject: [PATCH 4/5] #26: Add Sniff for Getters not change state --- Magento/Sniffs/Functions/GetterStateSniff.php | 14 ++++++++++- .../Tests/Functions/GetterStateUnitTest.inc | 24 +++++++++++++++---- .../Tests/Functions/GetterStateUnitTest.php | 2 +- 3 files changed, 34 insertions(+), 6 deletions(-) diff --git a/Magento/Sniffs/Functions/GetterStateSniff.php b/Magento/Sniffs/Functions/GetterStateSniff.php index af352926..1d35d7d4 100644 --- a/Magento/Sniffs/Functions/GetterStateSniff.php +++ b/Magento/Sniffs/Functions/GetterStateSniff.php @@ -74,7 +74,19 @@ public function process(File $phpcsFile, $stackPtr) } if ($isObjectScope === true && array_key_exists($token['code'], Tokens::$assignmentTokens)) { - $phpcsFile->addWarning($this->warningMessage, $i, $this->warningCode); + + $isWrappedByIf = false; + // Detect if the property warped by an if tag. + $ifTag = $phpcsFile->findPrevious(T_IF, $i); + if ($ifTag !== false) { + $open = $tokens[$ifTag]['scope_opener']; + $close = $tokens[$ifTag]['scope_closer']; + $isWrappedByIf = $open <= $i && $close >= $i; + } + + if ($isWrappedByIf === false) { + $phpcsFile->addWarning($this->warningMessage, $i, $this->warningCode); + } } } } diff --git a/Magento/Tests/Functions/GetterStateUnitTest.inc b/Magento/Tests/Functions/GetterStateUnitTest.inc index 9c429b97..d9028cd3 100644 --- a/Magento/Tests/Functions/GetterStateUnitTest.inc +++ b/Magento/Tests/Functions/GetterStateUnitTest.inc @@ -3,9 +3,9 @@ namespace Foo\Bar; -abstract class Bar{ - - public static $foobar = 100; +abstract class Bar +{ + public static $foobar = 100; } class Foo extends Bar @@ -36,11 +36,27 @@ class Foo extends Bar */ public function getProperty() { + if (true) { + + } + $this->property = 1223; return $this->property; } - public function TestigetFoo() + /** + * @return int + */ + public function getPropertyCached() + { + if ($this->property === null) { + $this->property = 1223; + } + + return $this->property; + } + + public function testigetFoo() { $this->property = 1223; return $this->property; diff --git a/Magento/Tests/Functions/GetterStateUnitTest.php b/Magento/Tests/Functions/GetterStateUnitTest.php index d4eb90f9..8f4b7843 100644 --- a/Magento/Tests/Functions/GetterStateUnitTest.php +++ b/Magento/Tests/Functions/GetterStateUnitTest.php @@ -30,7 +30,7 @@ protected function getWarningList() 28 => 1, 29 => 1, 30 => 1, - 39 => 1, + 43 => 1, ]; } } From e470c0f022901eb73a3dc9365696103f8f6cc2a5 Mon Sep 17 00:00:00 2001 From: Lars Roettig Date: Thu, 14 Mar 2019 19:32:48 +0100 Subject: [PATCH 5/5] Improve impelementation --- Magento/Sniffs/Functions/GetterStateSniff.php | 18 ++++-- .../Tests/Functions/GetterStateUnitTest.inc | 60 +++++++++++++++++++ 2 files changed, 74 insertions(+), 4 deletions(-) diff --git a/Magento/Sniffs/Functions/GetterStateSniff.php b/Magento/Sniffs/Functions/GetterStateSniff.php index 1d35d7d4..8f186daa 100644 --- a/Magento/Sniffs/Functions/GetterStateSniff.php +++ b/Magento/Sniffs/Functions/GetterStateSniff.php @@ -51,21 +51,29 @@ public function process(File $phpcsFile, $stackPtr) } $tokens = $phpcsFile->getTokens(); + if (!isset($tokens[$stackPtr]['scope_closer'])) { + // Probably an interface method no check + return; + } + $open = $tokens[$stackPtr]['scope_opener']; $close = $tokens[$stackPtr]['scope_closer']; $isObjectScope = false; $isObjectScopeToken = [T_SELF => T_SELF, T_PARENT => T_PARENT, T_STATIC => T_STATIC]; + $thisScopeCloser = array_merge(Tokens::$bracketTokens, + [T_SEMICOLON => T_SEMICOLON, T_COMMA => T_COMMA, T_COLON => T_COLON]); for ($i = ($open + 1); $i < $close; $i++) { $token = $tokens[$i]; + $code = $token['code']; - if (T_SEMICOLON === $token['code']) { + if (array_key_exists($code, $thisScopeCloser)) { // Detect line end scope change to function scope. $isObjectScope = false; } - if (array_key_exists($token['code'], $isObjectScopeToken)) { + if (array_key_exists($code, $isObjectScopeToken)) { $isObjectScope = true; } @@ -73,7 +81,9 @@ public function process(File $phpcsFile, $stackPtr) $isObjectScope = true; } - if ($isObjectScope === true && array_key_exists($token['code'], Tokens::$assignmentTokens)) { + $isRelevant = $isObjectScope === true && $code !== T_DOUBLE_ARROW; + + if ($isRelevant && array_key_exists($code, Tokens::$assignmentTokens)) { $isWrappedByIf = false; // Detect if the property warped by an if tag. @@ -90,4 +100,4 @@ public function process(File $phpcsFile, $stackPtr) } } } -} +} \ No newline at end of file diff --git a/Magento/Tests/Functions/GetterStateUnitTest.inc b/Magento/Tests/Functions/GetterStateUnitTest.inc index d9028cd3..b83b579b 100644 --- a/Magento/Tests/Functions/GetterStateUnitTest.inc +++ b/Magento/Tests/Functions/GetterStateUnitTest.inc @@ -56,6 +56,45 @@ class Foo extends Bar return $this->property; } + public function getPropertyLocal() + { + $local = $this->property; + $localArray = [ + 'payment' => [ + 'test' => [ + 'isActive' => $this->config->isActive(), + 'title' => $this->config->getTitle() + ] + ] + ]; + return $this->property; + } + + private function getSalesChannelForOrder($order) + { + $websiteId = (int)$order->getStore()->getWebsiteId(); + $websiteCode = $this->websiteRepository->getById($websiteId)->getCode(); + + return $this->salesChannelFactory->create([ + 'data' => [ + 'type' => '', + 'code' => $websiteCode + ] + ]); + } + + const MODE_AUTO = 0; + + const MODE_MANUAL = 1; + + public function getOptionsArray() + { + return [ + self::MODE_AUTO => __('Automatically'), + self::MODE_MANUAL => __('Manually') + ]; + } + public function testigetFoo() { $this->property = 1223; @@ -70,6 +109,27 @@ class Foo extends Bar $localVariable = 12; return $localVariable; } + + public function getStorageModel($storage = null, $params = []) + { + if ($storage === null) { + $storage = $this->_coreFileStorage->getCurrentStorageCode(); + } + + switch ($storage) { + case self::STORAGE_MEDIA_FILE_SYSTEM: + $model = $this->_fileFactory->create(); + break; + default: + return false; + } + + if (isset($params['init']) && $params['init']) { + $model->init(); + } + + return $model; + } } $d = function ($test) {