diff --git a/Magento2/Sniffs/PHP/AutogeneratedClassNotInConstructorSniff.php b/Magento2/Sniffs/PHP/AutogeneratedClassNotInConstructorSniff.php index 6c56d07e..0b397523 100644 --- a/Magento2/Sniffs/PHP/AutogeneratedClassNotInConstructorSniff.php +++ b/Magento2/Sniffs/PHP/AutogeneratedClassNotInConstructorSniff.php @@ -17,22 +17,16 @@ class AutogeneratedClassNotInConstructorSniff implements Sniff { private const ERROR_CODE = 'AutogeneratedClassNotInConstructor'; - /** - * @var array - */ - private $constructorParameters = []; - - /** - * @var array - */ - private $uses = []; + private const AUTOGENERATED_CLASS_SUFFIXES = [ + 'Factory' + ]; /** * @inheritdoc */ public function register() { - return [T_FUNCTION, T_DOUBLE_COLON, T_USE]; + return [T_DOUBLE_COLON]; } /** @@ -40,187 +34,174 @@ public function register() */ public function process(File $phpcsFile, $stackPtr) { - if ($phpcsFile->getTokens()[$stackPtr]['type'] === 'T_USE') { - $this->registerUse($phpcsFile, $stackPtr); + if (!$this->isClass($phpcsFile)) { + return; } - if ($phpcsFile->getTokens()[$stackPtr]['type'] === 'T_FUNCTION') { - $this->registerConstructorParameters($phpcsFile, $stackPtr); + + $tokens = $phpcsFile->getTokens(); + + if ($tokens[$stackPtr - 1]['content'] !== 'ObjectManager' + && $tokens[$stackPtr + 1]['content'] !== 'getInstance' + ) { + return; } - if ($phpcsFile->getTokens()[$stackPtr]['type'] === 'T_DOUBLE_COLON') { - if (!$this->isObjectManagerGetInstance($phpcsFile, $stackPtr)) { - return; - } - $statementStart = $phpcsFile->findStartOfStatement($stackPtr); - $statementEnd = $phpcsFile->findEndOfStatement($stackPtr); - $equalsPtr = $phpcsFile->findNext(T_EQUAL, $statementStart, $statementEnd); + if (!isset($tokens[$stackPtr + 4]) || $tokens[$stackPtr + 4]['code'] !== T_SEMICOLON) { + $this->validateRequestedClass( + $phpcsFile, + $phpcsFile->findNext(T_OBJECT_OPERATOR, $stackPtr) + ); + return; + } - if (!$equalsPtr) { - return; - } + $objectManagerVariableName = $this->getObjectManagerVariableName($phpcsFile, $stackPtr); - if (!$this->isVariableInConstructorParameters($phpcsFile, $equalsPtr, $statementEnd)) { - $className = $this->obtainClassToGetOrCreate($phpcsFile, $stackPtr, $statementEnd); + if (!$objectManagerVariableName) { + return; + } - $phpcsFile->addError( - sprintf("Class %s needs to be requested in constructor, " . - "otherwise compiler will not be able to find and generate these classes", $className), - $stackPtr, - self::ERROR_CODE - ); - } + $variablePosition = $phpcsFile->findNext(T_VARIABLE, $stackPtr, null, false, $objectManagerVariableName); + if ($variablePosition) { + $this->validateRequestedClass($phpcsFile, $phpcsFile->findNext(T_OBJECT_OPERATOR, $variablePosition)); } } /** - * Check if it is a ObjectManager::getInstance + * Check if the class is instantiated via get/create method, it is autogenerated and present in constructor * * @param File $phpcsFile - * @param int $stackPtr - * @return bool + * @param int $arrowPosition */ - private function isObjectManagerGetInstance(File $phpcsFile, int $stackPtr): bool + private function validateRequestedClass(File $phpcsFile, int $arrowPosition): void { - $prev = $phpcsFile->findPrevious(T_STRING, $stackPtr - 1); - $next = $phpcsFile->findNext(T_STRING, $stackPtr + 1); - return $prev && - $next && - $phpcsFile->getTokens()[$prev]['content'] === 'ObjectManager' && - $phpcsFile->getTokens()[$next]['content'] === 'getInstance'; + $requestedClass = $this->getRequestedClass($phpcsFile, $arrowPosition); + + if (!$requestedClass + || !$this->isClassAutogenerated($requestedClass) + || $this->isConstructorParameter($phpcsFile, $requestedClass) + ) { + return; + } + + $phpcsFile->addError( + sprintf( + 'Class %s needs to be requested in constructor, ' . + 'otherwise compiler will not be able to find and generate this class', + $requestedClass + ), + $arrowPosition, + self::ERROR_CODE + ); } /** - * Get the complete class namespace from the use's + * Does the class have the suffix common for autogenerated classes e.g. Factory * * @param string $className - * @return string + * @return bool */ - private function getClassNamespace(string $className): string + private function isClassAutogenerated(string $className): bool { - foreach ($this->uses as $key => $use) { - if ($key === $className) { - return $use; + foreach (self::AUTOGENERATED_CLASS_SUFFIXES as $suffix) { + if (substr($className, -strlen($suffix)) === $suffix) { + return true; } } - return $className; + return false; } /** - * Register php uses + * Get the variable name to which the ObjectManager::getInstance() result is assigned * * @param File $phpcsFile * @param int $stackPtr + * @return string|null */ - private function registerUse(File $phpcsFile, int $stackPtr): void + private function getObjectManagerVariableName(File $phpcsFile, int $stackPtr): ?string { - $useEnd = $phpcsFile->findEndOfStatement($stackPtr); - $use = []; - $usePosition = $stackPtr; - while ($usePosition = $phpcsFile->findNext(T_STRING, $usePosition + 1, $useEnd)) { - $use[] = $phpcsFile->getTokens()[$usePosition]['content']; + $matches = []; + $found = preg_match( + '/(\$[A-Za-z]+) ?= ?ObjectManager::getInstance\(\);/', + $phpcsFile->getTokensAsString($stackPtr - 5, 10), + $matches + ); + + if (!$found || !isset($matches[1])) { + return null; } - $key = end($use); - if ($phpcsFile->findNext(T_AS, $stackPtr, $useEnd)) { - $this->uses[$key] = implode("\\", array_slice($use, 0, count($use) - 1)); - } else { - $this->uses[$key] = implode("\\", $use); - } + return $matches[1]; } /** - * Register php constructor parameters + * Get class name requested from ObjectManager * * @param File $phpcsFile - * @param int $stackPtr + * @param int $callerPosition + * @return string|null */ - private function registerConstructorParameters(File $phpcsFile, int $stackPtr): void + private function getRequestedClass(File $phpcsFile, int $callerPosition): ?string { - $functionName = $phpcsFile->getDeclarationName($stackPtr); - if ($functionName == '__construct') { - $this->constructorParameters = $phpcsFile->getMethodParameters($stackPtr); + $matches = []; + $found = preg_match( + '/->(get|create)\(([A-Za-z\\\]+)::class/', + $phpcsFile->getTokensAsString($callerPosition, $phpcsFile->findNext(T_CLOSE_PARENTHESIS, $callerPosition)), + $matches + ); + + if (!$found || !isset($matches[2])) { + return null; } - } - /** - * Get next token - * - * @param File $phpcsFile - * @param int $from - * @param int $to - * @param int|string|array $types - * @return mixed - */ - private function getNext(File $phpcsFile, int $from, int $to, $types) - { - return $phpcsFile->getTokens()[$phpcsFile->findNext($types, $from + 1, $to)]; + return $matches[2]; } /** - * Get previous token + * Does the file contain class declaration * * @param File $phpcsFile - * @param int $from - * @param int|string|array $types - * @return mixed - */ - private function getPrevious(File $phpcsFile, int $from, $types) - { - return $phpcsFile->getTokens()[$phpcsFile->findPrevious($types, $from - 1)]; - } - - /** - * Get name of the variable without $ - * - * @param string $parameterName - * @return string + * @return bool */ - protected function variableName(string $parameterName): string + private function isClass(File $phpcsFile): bool { - return str_replace('$', '', $parameterName); + foreach ($phpcsFile->getTokens() as $token) { + if ($token['code'] === T_CLASS) { + return true; + } + } + return false; } /** - * Checks if a variable is present in the constructor parameters + * Get an array of constructor parameters * * @param File $phpcsFile - * @param int $equalsPtr - * @param int $statementEnd - * @return bool + * @return array */ - private function isVariableInConstructorParameters(File $phpcsFile, int $equalsPtr, int $statementEnd): bool + private function getConstructorParameters(File $phpcsFile): array { - if ($variable = $phpcsFile->findNext(T_VARIABLE, $equalsPtr, $statementEnd)) { - $variableName = $phpcsFile->getTokens()[$variable]['content']; - if ($variableName === '$this') { - $variableName = $this->getNext($phpcsFile, $variable, $statementEnd, T_STRING)['content']; - } - foreach ($this->constructorParameters as $parameter) { - $parameterName = $parameter['name']; - if ($this->variableName($parameterName) === $this->variableName($variableName)) { - return true; - } + foreach ($phpcsFile->getTokens() as $stackPtr => $token) { + if ($token['code'] === T_FUNCTION && $phpcsFile->getDeclarationName($stackPtr) === '__construct') { + return $phpcsFile->getMethodParameters($stackPtr); } } - return false; + return []; } /** - * Obtain the class inside ObjectManager::getInstance()->get|create() + * Is the class name present between constructor parameters * * @param File $phpcsFile - * @param int $next - * @param int $statementEnd - * @return string + * @param string $className + * @return bool */ - private function obtainClassToGetOrCreate(File $phpcsFile, int $next, int $statementEnd): string + private function isConstructorParameter(File $phpcsFile, string $className): bool { - while ($next = $phpcsFile->findNext(T_DOUBLE_COLON, $next + 1, $statementEnd)) { - if ($this->getNext($phpcsFile, $next, $statementEnd, T_STRING)['content'] === 'class') { - $className = $this->getPrevious($phpcsFile, $next, T_STRING)['content']; + foreach ($this->getConstructorParameters($phpcsFile) as $parameter) { + if (strpos($parameter['content'], $className) !== false) { + return true; } } - - return $this->getClassNamespace($className); + return false; } } diff --git a/Magento2/Tests/PHP/AutogeneratedClassNotInConstructorUnitTest.1.php.inc b/Magento2/Tests/PHP/AutogeneratedClassNotInConstructorUnitTest.1.php.inc index f7a461ed..c5833baf 100644 --- a/Magento2/Tests/PHP/AutogeneratedClassNotInConstructorUnitTest.1.php.inc +++ b/Magento2/Tests/PHP/AutogeneratedClassNotInConstructorUnitTest.1.php.inc @@ -12,10 +12,10 @@ use Magento2\OneModel as Model; class Good { public function __construct( - Model $model = null + ModelFactory $model = null ) { - $this->model = $model ?? ObjectManager::getInstance()->get(Model::class); + $this->model = $model ?? ObjectManager::getInstance()->get(ModelFactory::class); } /** @@ -23,8 +23,8 @@ class Good */ public function otherMethodThatCallsGetInstanceBad(): void { - $model = ObjectManager::getInstance()->get(Model::class); - $model->something(); + $modelFactory = ObjectManager::getInstance()->get(OtherFactory::class); + $modelFactory->create(); } /** @@ -35,4 +35,18 @@ class Good $model = $this->model ?? ObjectManager::getInstance()->get(Model::class); $model->something(); } + + public function variablePositive(): void + { + $objectManager = ObjectManager::getInstance(); + $this->_entityFactory = $objectManager->get(EntityFactoryInterface::class); + } + + public function variableNegative(): void + { + $objectManager = ObjectManager::getInstance(); + $this->_entityFactory = $objectManager->get(EntityFactory::class); + } } + + diff --git a/Magento2/Tests/PHP/AutogeneratedClassNotInConstructorUnitTest.2.php.inc b/Magento2/Tests/PHP/AutogeneratedClassNotInConstructorUnitTest.2.php.inc index a72f5bb0..0e474247 100644 --- a/Magento2/Tests/PHP/AutogeneratedClassNotInConstructorUnitTest.2.php.inc +++ b/Magento2/Tests/PHP/AutogeneratedClassNotInConstructorUnitTest.2.php.inc @@ -7,11 +7,34 @@ declare(strict_types = 1); namespace Magento2\Tests\PHP; +use Magento\Catalog\Model\Product; +use Magento\Eav\Api\AttributeRepositoryInterface; +use Magento\Eav\Setup\AddOptionToAttribute; +use Magento\Eav\Setup\EavSetup; +use Magento\Eav\Setup\EavSetupFactory; +use Magento\Framework\App\ObjectManager; +use Magento\Framework\Setup\ModuleDataSetupInterface; + class Bad { public function __construct() { - $this->model = ObjectManager::getInstance()->get(Model::class); + $this->model = ObjectManager::getInstance()->get(ModelFactory::class); + } + + protected function setUp(): void + { + ObjectManager::getInstance() + ->get(EavSetupFactory::class); + $objectManager = ObjectManager::getInstance(); + + /** @var EavSetup $eavSetup */ + $this->eavSetup = $objectManager + ->get(EavSetupFactory::class) + ->create(['setup' => $this->setup]); + + ObjectManager::getInstance() + ->get(\Full\Class\NameFactory::class); } } diff --git a/Magento2/Tests/PHP/AutogeneratedClassNotInConstructorUnitTest.php b/Magento2/Tests/PHP/AutogeneratedClassNotInConstructorUnitTest.php index 9122213f..0424ac06 100644 --- a/Magento2/Tests/PHP/AutogeneratedClassNotInConstructorUnitTest.php +++ b/Magento2/Tests/PHP/AutogeneratedClassNotInConstructorUnitTest.php @@ -19,11 +19,15 @@ public function getErrorList($filename = '') if ($filename === 'AutogeneratedClassNotInConstructorUnitTest.1.php.inc') { return [ 26 => 1, + 48 => 1 ]; } if ($filename === 'AutogeneratedClassNotInConstructorUnitTest.2.php.inc') { return [ - 14 => 1, + 22 => 1, + 28 => 1, + 33 => 1, + 37 => 1 ]; } return [];