From 23d2178b702cc4f70487660ba19ad8201c4ef482 Mon Sep 17 00:00:00 2001
From: Jisse Reitsma <info@yireo.com>
Date: Sat, 14 Jul 2018 17:11:00 +0200
Subject: [PATCH 1/6] Add a new rule to check constructor for OM

---
 Extdn/Sniffs/Classes/ObjectManagerSniff.md  |  16 ++++
 Extdn/Sniffs/Classes/ObjectManagerSniff.php |  91 ++++++++++++++++++
 Extdn/Utils/Reflection.php                  | 101 ++++++++++++++++++++
 Extdn/Utils/TestPattern.php                 |  30 ++++++
 Extdn/ruleset.xml                           |   1 +
 5 files changed, 239 insertions(+)
 create mode 100644 Extdn/Sniffs/Classes/ObjectManagerSniff.md
 create mode 100644 Extdn/Sniffs/Classes/ObjectManagerSniff.php
 create mode 100644 Extdn/Utils/Reflection.php
 create mode 100644 Extdn/Utils/TestPattern.php

diff --git a/Extdn/Sniffs/Classes/ObjectManagerSniff.md b/Extdn/Sniffs/Classes/ObjectManagerSniff.md
new file mode 100644
index 0000000..8e65316
--- /dev/null
+++ b/Extdn/Sniffs/Classes/ObjectManagerSniff.md
@@ -0,0 +1,16 @@
+# Rule: Do not inject the Object Manager
+## Background
+The Object Manager should never be injected as a dependency into a class constructor. Injecting the Object Manager directly bypasses the DI mechanism that Magento makes flexible. Therefore, injecting the Object Manager in any kind of class is a code smell that leads to inflexibility.
+
+## Reasoning
+Whenever the Object Manager is directly injected into the constructor, it should be refactored into something else. 
+
+## How it works
+This rule uses PHP Reflection to determine the constructor arguments of a parsed class. If one of the arguments contains the word `ObjectManager`, the rule gives a warning.
+
+However, if the parsed class matches one of the following circumstances, the rule is bypassed:
+- When the class has a namespace that contains `/Test/`, indicating a test-class;
+- When the class has a name ending with `Factory`, `Proxy` or `Builder`;
+
+## How to fix
+If there is a match with this rule, the class constructor needs to be refactored so that dependencies are injected directly as a constructor argument, instead of via the Object Manager. Alternatively, this class needs to be created via a `Factory` class so that the `Factory` is able to use the Object Manager as needed.
\ No newline at end of file
diff --git a/Extdn/Sniffs/Classes/ObjectManagerSniff.php b/Extdn/Sniffs/Classes/ObjectManagerSniff.php
new file mode 100644
index 0000000..565b2a5
--- /dev/null
+++ b/Extdn/Sniffs/Classes/ObjectManagerSniff.php
@@ -0,0 +1,91 @@
+<?php
+declare(strict_types=1);
+
+namespace Extdn\Sniffs\Classes;
+
+use PHP_CodeSniffer\Files\File;
+use PHP_CodeSniffer\Sniffs\Sniff;
+use Extdn\Utils\TestPattern;
+use Extdn\Utils\Reflection;
+
+/**
+ * Class ObjectManagerSniff
+ *
+ * @package Extdn\Sniffs\Classes\Constructor
+ */
+class ObjectManagerSniff implements Sniff
+{
+    /**
+     * @var string
+     */
+    protected $message = 'The ObjectManager should not be injected into the constructor';
+
+    /**
+     * @inheritdoc
+     */
+    public function register()
+    {
+        return [T_CLASS];
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public function process(File $phpcsFile, $stackPtr)
+    {
+        $className = Reflection::findClassName($phpcsFile);
+        if (empty($className)) {
+            return false;
+        }
+
+        if (TestPattern::isTestClass($className)) {
+            return false;
+        }
+
+        $dependencyClasses = Reflection::getClassDependencies($className);
+        foreach ($dependencyClasses as $dependencyClass) {
+            if ($this->isObjectManagerAllowedWithClass($className)) {
+                continue;
+            }
+
+            if (!$this->isInstanceOfObjectManager($dependencyClass)) {
+                continue;
+            }
+
+            $warning = 'The dependency "\\' . $dependencyClass . '" is not allowed here.';
+            $phpcsFile->addWarning($warning, 0, get_class($this), [], 8);
+        }
+    }
+
+    /**
+     * @param string $className
+     *
+     * @return bool
+     */
+    private function isObjectManagerAllowedWithClass(string $className): bool
+    {
+        if (preg_match('/([a-zA-Z]+)(Factory|Builder)$/', $className)) {
+            return true;
+        }
+
+        if (preg_match('/Proxy$/', $className)) {
+            return true;
+        }
+
+        return false;
+    }
+
+    /**
+     * @param string $className
+     *
+     * @return bool
+     */
+    private function isInstanceOfObjectManager(string $className): bool
+    {
+        if (strstr($className, '\\ObjectManager')) {
+            return true;
+        }
+
+        return false;
+    }
+}
diff --git a/Extdn/Utils/Reflection.php b/Extdn/Utils/Reflection.php
new file mode 100644
index 0000000..292ffae
--- /dev/null
+++ b/Extdn/Utils/Reflection.php
@@ -0,0 +1,101 @@
+<?php
+declare(strict_types=1);
+
+namespace Extdn\Utils;
+
+use PHP_CodeSniffer\Files\File;
+use ReflectionClass;
+use ReflectionException;
+
+/**
+ * Class Reflection
+ *
+ * @package Extdn\Utils
+ */
+class Reflection
+{
+    /**
+     * @param string $className
+     *
+     * @return array
+     */
+    static public function getClassDependencies(string $className): array
+    {
+        try {
+            $class = new ReflectionClass($className);
+            $constructor = $class->getConstructor();
+            if (empty($constructor)) {
+                return [];
+            }
+
+            $parameters = $constructor->getParameters();
+            if (empty($parameters)) {
+                return [];
+            }
+
+            $dependencyClasses = [];
+            foreach ($parameters as $parameter) {
+                $dependencyClass = $parameter->getClass();
+                if (empty($dependencyClass)) {
+                    continue;
+                }
+
+                $dependencyClasses[] = $dependencyClass->getName();
+            }
+
+            return $dependencyClasses;
+
+        } catch (ReflectionException $reflectionException) {
+            return [];
+        }
+    }
+
+    /**
+     * @param File $phpcsFile
+     *
+     * @return string
+     */
+    static public function findClassName(File $phpcsFile): string
+    {
+        $tokens = $phpcsFile->getTokens();
+
+        $namespaceParts = [];
+        $namespaceFound = false;
+        $className = '';
+        $classFound = false;
+
+        foreach ($tokens as $token) {
+            if (!is_array($token)) {
+                continue;
+            }
+
+            if ($token['type'] == 'T_NAMESPACE') {
+                $namespaceFound = true;
+                continue;
+            } else if ($namespaceFound && $token['type'] == 'T_STRING') {
+                $namespaceParts[] = $token['content'];
+                continue;
+            } else if ($namespaceFound && $token['type'] == 'T_SEMICOLON') {
+                $namespaceFound = false;
+                continue;
+            }
+
+            if ($token['type'] == 'T_CLASS') {
+                $classFound = true;
+                continue;
+            } else if ($classFound && $token['type'] == 'T_STRING') {
+                $className = $token['content'];
+                $classFound = false;
+                continue;
+            }
+        }
+
+        if (!$className) {
+            return '';
+        }
+
+        $className = implode('\\', $namespaceParts) . "\\$className";
+
+        return $className;
+    }
+}
diff --git a/Extdn/Utils/TestPattern.php b/Extdn/Utils/TestPattern.php
new file mode 100644
index 0000000..9346cf2
--- /dev/null
+++ b/Extdn/Utils/TestPattern.php
@@ -0,0 +1,30 @@
+<?php
+declare(strict_types=1);
+
+namespace Extdn\Utils;
+
+/**
+ * Class TestPattern
+ *
+ * @package Extdn\Utils
+ */
+class TestPattern
+{
+    /**
+     * @param string $className
+     *
+     * @return bool
+     */
+    static public function isTestClass(string $className): bool
+    {
+        if (strstr($className, '\\Test\\')) {
+            return true;
+        }
+
+        if (preg_match('/Test$/', $className)) {
+            return true;
+        }
+
+        return false;
+    }
+}
diff --git a/Extdn/ruleset.xml b/Extdn/ruleset.xml
index 9490b4b..4ebc7ba 100644
--- a/Extdn/ruleset.xml
+++ b/Extdn/ruleset.xml
@@ -8,5 +8,6 @@
     </rule>
     <rule ref="Extdn.Blocks.SetTemplateInBlock"/>
     <rule ref="Extdn.Classes.StrictTypes"/>
+    <rule ref="Extdn.Classes.ObjectManager"/>
     <rule ref="Extdn.Templates.TemplateObjectManager"/>
 </ruleset>

From f9288f6ab5605f3797eacdf131ff26062383535b Mon Sep 17 00:00:00 2001
From: Jisse Reitsma <info@yireo.com>
Date: Sat, 14 Jul 2018 17:34:08 +0200
Subject: [PATCH 2/6] Add tests

---
 Extdn/Samples/Classes/ObjectManager.php       | 15 ++++++++
 .../Tests/Classes/ObjectManagerUnitTest.1.inc | 13 +++++++
 Extdn/Tests/Classes/ObjectManagerUnitTest.php | 38 +++++++++++++++++++
 3 files changed, 66 insertions(+)
 create mode 100644 Extdn/Samples/Classes/ObjectManager.php
 create mode 100644 Extdn/Tests/Classes/ObjectManagerUnitTest.1.inc
 create mode 100644 Extdn/Tests/Classes/ObjectManagerUnitTest.php

diff --git a/Extdn/Samples/Classes/ObjectManager.php b/Extdn/Samples/Classes/ObjectManager.php
new file mode 100644
index 0000000..49c26dd
--- /dev/null
+++ b/Extdn/Samples/Classes/ObjectManager.php
@@ -0,0 +1,15 @@
+<?php
+
+namespace Extdn\Samples\Classes;
+
+use Magento\Framework\App\ObjectManager as CoreObjectManager;
+use Magento\Framework\ObjectManagerInterface;
+
+class ObjectManager
+{
+    public function __construct(
+        ObjectManagerInterface $objectManager1,
+        CoreObjectManager $objectManager2
+    ) {
+    }
+}
diff --git a/Extdn/Tests/Classes/ObjectManagerUnitTest.1.inc b/Extdn/Tests/Classes/ObjectManagerUnitTest.1.inc
new file mode 100644
index 0000000..77fe63b
--- /dev/null
+++ b/Extdn/Tests/Classes/ObjectManagerUnitTest.1.inc
@@ -0,0 +1,13 @@
+<?php
+namespace Extdn\Samples\Classes;
+
+use Magento\Framework\ObjectManager\ObjectManager;
+
+class ObjectManagerUnitTest
+{
+    public function __construct(
+        ObjectManager $objectManager
+    ) {
+
+    }
+}
diff --git a/Extdn/Tests/Classes/ObjectManagerUnitTest.php b/Extdn/Tests/Classes/ObjectManagerUnitTest.php
new file mode 100644
index 0000000..60b2b60
--- /dev/null
+++ b/Extdn/Tests/Classes/ObjectManagerUnitTest.php
@@ -0,0 +1,38 @@
+<?php
+/**
+ * Copyright © ExtDN. All rights reserved.
+ */
+declare(strict_types=1);
+
+namespace Extdn\Tests\Classes;
+
+use PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest;
+
+/**
+ * Class ObjectManagerUnitTest
+ *
+ * @package Extdn\Tests\Classes
+ */
+class ObjectManagerUnitTest extends AbstractSniffUnitTest
+{
+
+    /**
+     * @inheritdoc
+     */
+    public function getErrorList()
+    {
+        return [];
+    }
+
+    /**
+     * @inheritdoc
+     */
+    public function getWarningList($testFile = '')
+    {
+        if ($testFile === 'ObjectManagerUnitTest.3.inc') {
+            return [];
+        }
+
+        return [1 => 1];
+    }
+}

From 8e161e9a335a31a414d7585078ba827b935152c9 Mon Sep 17 00:00:00 2001
From: Jisse Reitsma <info@yireo.com>
Date: Sat, 14 Jul 2018 21:40:08 +0200
Subject: [PATCH 3/6] Finalize tests

---
 Extdn/Sniffs/Classes/ObjectManagerSniff.php   | 13 ++++++++-----
 Extdn/Tests/Classes/ObjectManagerUnitTest.php |  6 +++++-
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/Extdn/Sniffs/Classes/ObjectManagerSniff.php b/Extdn/Sniffs/Classes/ObjectManagerSniff.php
index 565b2a5..66478ae 100644
--- a/Extdn/Sniffs/Classes/ObjectManagerSniff.php
+++ b/Extdn/Sniffs/Classes/ObjectManagerSniff.php
@@ -1,4 +1,8 @@
 <?php
+/**
+ * Copyright © ExtDN. All rights reserved.
+ */
+
 declare(strict_types=1);
 
 namespace Extdn\Sniffs\Classes;
@@ -38,9 +42,8 @@ public function process(File $phpcsFile, $stackPtr)
             return false;
         }
 
-        if (TestPattern::isTestClass($className)) {
-            return false;
-        }
+        // Make sure to load the file itself, so that autoloading can be skipped
+        include_once($phpcsFile->getFilename());
 
         $dependencyClasses = Reflection::getClassDependencies($className);
         foreach ($dependencyClasses as $dependencyClass) {
@@ -53,7 +56,7 @@ public function process(File $phpcsFile, $stackPtr)
             }
 
             $warning = 'The dependency "\\' . $dependencyClass . '" is not allowed here.';
-            $phpcsFile->addWarning($warning, 0, get_class($this), [], 8);
+            $phpcsFile->addWarning($warning, null, 'warning');
         }
     }
 
@@ -82,7 +85,7 @@ private function isObjectManagerAllowedWithClass(string $className): bool
      */
     private function isInstanceOfObjectManager(string $className): bool
     {
-        if (strstr($className, '\\ObjectManager')) {
+        if (strstr($className, 'ObjectManager')) {
             return true;
         }
 
diff --git a/Extdn/Tests/Classes/ObjectManagerUnitTest.php b/Extdn/Tests/Classes/ObjectManagerUnitTest.php
index 60b2b60..4974a6d 100644
--- a/Extdn/Tests/Classes/ObjectManagerUnitTest.php
+++ b/Extdn/Tests/Classes/ObjectManagerUnitTest.php
@@ -15,6 +15,10 @@
  */
 class ObjectManagerUnitTest extends AbstractSniffUnitTest
 {
+    protected function setUp()
+    {
+        parent::setUp();
+    }
 
     /**
      * @inheritdoc
@@ -29,7 +33,7 @@ public function getErrorList()
      */
     public function getWarningList($testFile = '')
     {
-        if ($testFile === 'ObjectManagerUnitTest.3.inc') {
+        if ($testFile === 'ObjectManagerUnitTest.2.inc') {
             return [];
         }
 

From 12f95b7aed94016bbc3874290c6d57fafb8787ce Mon Sep 17 00:00:00 2001
From: Jisse Reitsma <info@yireo.com>
Date: Sat, 14 Jul 2018 21:44:47 +0200
Subject: [PATCH 4/6] Return ReflectionClasses from Reflection util itself

---
 Extdn/Sniffs/Classes/ObjectManagerSniff.php | 4 ++--
 Extdn/Utils/Reflection.php                  | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/Extdn/Sniffs/Classes/ObjectManagerSniff.php b/Extdn/Sniffs/Classes/ObjectManagerSniff.php
index 66478ae..1dec6ee 100644
--- a/Extdn/Sniffs/Classes/ObjectManagerSniff.php
+++ b/Extdn/Sniffs/Classes/ObjectManagerSniff.php
@@ -51,11 +51,11 @@ public function process(File $phpcsFile, $stackPtr)
                 continue;
             }
 
-            if (!$this->isInstanceOfObjectManager($dependencyClass)) {
+            if (!$this->isInstanceOfObjectManager($dependencyClass->getName())) {
                 continue;
             }
 
-            $warning = 'The dependency "\\' . $dependencyClass . '" is not allowed here.';
+            $warning = 'The dependency "\\' . $dependencyClass->getName() . '" is not allowed here.';
             $phpcsFile->addWarning($warning, null, 'warning');
         }
     }
diff --git a/Extdn/Utils/Reflection.php b/Extdn/Utils/Reflection.php
index 292ffae..a3ee867 100644
--- a/Extdn/Utils/Reflection.php
+++ b/Extdn/Utils/Reflection.php
@@ -17,7 +17,7 @@ class Reflection
     /**
      * @param string $className
      *
-     * @return array
+     * @return ReflectionClass[]
      */
     static public function getClassDependencies(string $className): array
     {
@@ -40,7 +40,7 @@ static public function getClassDependencies(string $className): array
                     continue;
                 }
 
-                $dependencyClasses[] = $dependencyClass->getName();
+                $dependencyClasses[] = $dependencyClass;
             }
 
             return $dependencyClasses;

From 4c1ba675fc01096bb778ab0c2820820ad0807495 Mon Sep 17 00:00:00 2001
From: Jisse Reitsma <info@yireo.com>
Date: Sat, 14 Jul 2018 21:54:53 +0200
Subject: [PATCH 5/6] Code cleanup

---
 Extdn/Sniffs/Classes/ObjectManagerSniff.php | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Extdn/Sniffs/Classes/ObjectManagerSniff.php b/Extdn/Sniffs/Classes/ObjectManagerSniff.php
index 1dec6ee..6a3320a 100644
--- a/Extdn/Sniffs/Classes/ObjectManagerSniff.php
+++ b/Extdn/Sniffs/Classes/ObjectManagerSniff.php
@@ -9,13 +9,12 @@
 
 use PHP_CodeSniffer\Files\File;
 use PHP_CodeSniffer\Sniffs\Sniff;
-use Extdn\Utils\TestPattern;
 use Extdn\Utils\Reflection;
 
 /**
  * Class ObjectManagerSniff
  *
- * @package Extdn\Sniffs\Classes\Constructor
+ * @package Extdn\Sniffs\Classes
  */
 class ObjectManagerSniff implements Sniff
 {

From 6d965fbd1578ed55c32fff6d1d0d70f177b5023a Mon Sep 17 00:00:00 2001
From: Jisse Reitsma <info@yireo.com>
Date: Sat, 14 Jul 2018 22:16:38 +0200
Subject: [PATCH 6/6] Do not include utils in this PR

---
 Extdn/Utils/Reflection.php  | 101 ------------------------------------
 Extdn/Utils/TestPattern.php |  30 -----------
 2 files changed, 131 deletions(-)
 delete mode 100644 Extdn/Utils/Reflection.php
 delete mode 100644 Extdn/Utils/TestPattern.php

diff --git a/Extdn/Utils/Reflection.php b/Extdn/Utils/Reflection.php
deleted file mode 100644
index a3ee867..0000000
--- a/Extdn/Utils/Reflection.php
+++ /dev/null
@@ -1,101 +0,0 @@
-<?php
-declare(strict_types=1);
-
-namespace Extdn\Utils;
-
-use PHP_CodeSniffer\Files\File;
-use ReflectionClass;
-use ReflectionException;
-
-/**
- * Class Reflection
- *
- * @package Extdn\Utils
- */
-class Reflection
-{
-    /**
-     * @param string $className
-     *
-     * @return ReflectionClass[]
-     */
-    static public function getClassDependencies(string $className): array
-    {
-        try {
-            $class = new ReflectionClass($className);
-            $constructor = $class->getConstructor();
-            if (empty($constructor)) {
-                return [];
-            }
-
-            $parameters = $constructor->getParameters();
-            if (empty($parameters)) {
-                return [];
-            }
-
-            $dependencyClasses = [];
-            foreach ($parameters as $parameter) {
-                $dependencyClass = $parameter->getClass();
-                if (empty($dependencyClass)) {
-                    continue;
-                }
-
-                $dependencyClasses[] = $dependencyClass;
-            }
-
-            return $dependencyClasses;
-
-        } catch (ReflectionException $reflectionException) {
-            return [];
-        }
-    }
-
-    /**
-     * @param File $phpcsFile
-     *
-     * @return string
-     */
-    static public function findClassName(File $phpcsFile): string
-    {
-        $tokens = $phpcsFile->getTokens();
-
-        $namespaceParts = [];
-        $namespaceFound = false;
-        $className = '';
-        $classFound = false;
-
-        foreach ($tokens as $token) {
-            if (!is_array($token)) {
-                continue;
-            }
-
-            if ($token['type'] == 'T_NAMESPACE') {
-                $namespaceFound = true;
-                continue;
-            } else if ($namespaceFound && $token['type'] == 'T_STRING') {
-                $namespaceParts[] = $token['content'];
-                continue;
-            } else if ($namespaceFound && $token['type'] == 'T_SEMICOLON') {
-                $namespaceFound = false;
-                continue;
-            }
-
-            if ($token['type'] == 'T_CLASS') {
-                $classFound = true;
-                continue;
-            } else if ($classFound && $token['type'] == 'T_STRING') {
-                $className = $token['content'];
-                $classFound = false;
-                continue;
-            }
-        }
-
-        if (!$className) {
-            return '';
-        }
-
-        $className = implode('\\', $namespaceParts) . "\\$className";
-
-        return $className;
-    }
-}
diff --git a/Extdn/Utils/TestPattern.php b/Extdn/Utils/TestPattern.php
deleted file mode 100644
index 9346cf2..0000000
--- a/Extdn/Utils/TestPattern.php
+++ /dev/null
@@ -1,30 +0,0 @@
-<?php
-declare(strict_types=1);
-
-namespace Extdn\Utils;
-
-/**
- * Class TestPattern
- *
- * @package Extdn\Utils
- */
-class TestPattern
-{
-    /**
-     * @param string $className
-     *
-     * @return bool
-     */
-    static public function isTestClass(string $className): bool
-    {
-        if (strstr($className, '\\Test\\')) {
-            return true;
-        }
-
-        if (preg_match('/Test$/', $className)) {
-            return true;
-        }
-
-        return false;
-    }
-}