#26: Add Sniff for Getters not change state

Magento/Sniffs/Functions/GetterStateSniff.php

@@ -0,0 +1,103 @@
+<?php
+/**
+ * Copyright © Magento. All rights reserved.
+ * See COPYING.txt for license details.
+ */
+
+namespace Magento\Sniffs\Functions;
+
+use PHP_CodeSniffer\Sniffs\Sniff;
+use PHP_CodeSniffer\Files\File;
+use PHP_CodeSniffer\Util\Tokens;
+
+/**
+ * Detects getter that change the state of an object.
+ */
+class GetterStateSniff implements Sniff
+{
+
+    /**
+     * String representation of warning.
+     *
+     * @var string
+     */
+    protected $warningMessage = 'Getters SHOULD NOT change the state of an object.';
+
+    /**
+     * Warning violation code.
+     *
+     * @var string
+     */
+    protected $warningCode = 'GetterState';
+
+    /**
+     * @inheritDoc
+     */
+    public function register()
+    {
+        return [T_FUNCTION];
+    }
+
+    /**
+     * @inheritDoc
+     */
+    public function process(File $phpcsFile, $stackPtr)
+    {
+        $methodName = $phpcsFile->getDeclarationName($stackPtr);
+
+        if ($methodName === null || strpos($methodName, 'get') !== 0) {
+            // Ignore closures and no getters
+            return;
+        }
+
+        $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 (array_key_exists($code, $thisScopeCloser)) {
+                // Detect line end scope change to function scope.
+                $isObjectScope = false;
+            }
+
+            if (array_key_exists($code, $isObjectScopeToken)) {
+                $isObjectScope = true;
+            }
+
+            if ($token['content'] === '$this') {
+                $isObjectScope = true;
+            }
+
+            $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.
+                $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);
+                }
+            }
+        }
+    }
+}

Magento/Tests/Functions/GetterStateUnitTest.inc

@@ -0,0 +1,138 @@
+<?php
+
+namespace Foo\Bar;
+
+
+abstract class Bar
+{
+    public static $foobar = 100;
+}
+
+class Foo extends Bar
+{
+    /**
+     * @var int
+     */
+    private static $staticProperty;
+
+    /**
+     * @var int
+     */
+    private $property;
+
+    /**
+     * @return int
+     */
+    public function getStaticProperty()
+    {
+        self::$staticProperty = 12;
+        static::$staticProperty -= 12;
+        self::$staticProperty .= 12;
+        return self::$staticProperty;
+    }
+
+    /**
+     * @return int
+     */
+    public function getProperty()
+    {
+        if (true) {
+
+        }
+
+        $this->property = 1223;
+        return $this->property;
+    }
+
+    /**
+     * @return int
+     */
+    public function getPropertyCached()
+    {
+        if ($this->property === null) {
+            $this->property = 1223;
+        }
+
+        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;
+        return $this->property;
+    }
+
+    /**
+     * @return int
+     */
+    public function normalMethod()
+    {
+        $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) {
+    $test = 123;
+};
+

Magento/Tests/Functions/GetterStateUnitTest.php

@@ -0,0 +1,36 @@
+<?php
+/**
+ * Copyright © Magento. All rights reserved.
+ * See COPYING.txt for license details.
+ */
+
+namespace Magento\Tests\Functions;
+
+use PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest;
+
+/**
+ * Class GetterStateUnitTest
+ */
+class GetterStateUnitTest extends AbstractSniffUnitTest
+{
+    /**
+     * @inheritdoc
+     */
+    protected function getErrorList()
+    {
+        return [];
+    }
+
+    /**
+     * @inheritdoc
+     */
+    protected function getWarningList()
+    {
+        return [
+            28 => 1,
+            29 => 1,
+            30 => 1,
+            43 => 1,
+        ];
+    }
+}

Magento/ruleset.xml

@@ -121,6 +121,10 @@
         <severity>8</severity>
         <type>warning</type>
     </rule>
+    <rule ref="Magento.Functions.GetterState">
+        <severity>8</severity>
+        <type>warning</type>
+    </rule>
     <rule ref="Magento.Functions.StaticFunction">
         <severity>8</severity>
         <type>warning</type>

phpunit.xml.dist

@@ -8,4 +8,9 @@
     <php>
         <env name="PHPCS_IGNORE_TESTS" value="Generic,MySource,PEAR,PSR1,PSR2,Squiz,Zend"/>
     </php>
+    <filter>
+        <whitelist addUncoveredFilesFromWhitelist="false">
+            <directory suffix=".php">Magento/Sniffs</directory>
+        </whitelist>
+    </filter>
 </phpunit>