Skip to content

Commit 8babc4f

Browse files
sivaschenkosivaschenko
authored
Merge pull request #107 from magento-commerce/autogenerated-class-not-in-constructor
Fixed undefined variable error in AutogeneratedClassNotInConstructorSniff
2 parents a30edcb + d903f9c commit 8babc4f

File tree

4 files changed

+153
-131
lines changed

4 files changed

+153
-131
lines changed

‎Magento2/Sniffs/PHP/AutogeneratedClassNotInConstructorSniff.php‎

Lines changed: 106 additions & 125 deletions
Original file line numberDiff line numberDiff line change
@@ -17,210 +17,191 @@ class AutogeneratedClassNotInConstructorSniff implements Sniff
1717
{
1818
private const ERROR_CODE = 'AutogeneratedClassNotInConstructor';
1919

20-
/**
21-
* @var array
22-
*/
23-
private $constructorParameters = [];
24-
25-
/**
26-
* @var array
27-
*/
28-
private $uses = [];
20+
private const AUTOGENERATED_CLASS_SUFFIXES = [
21+
'Factory'
22+
];
2923

3024
/**
3125
* @inheritdoc
3226
*/
3327
public function register()
3428
{
35-
return [T_FUNCTION, T_DOUBLE_COLON, T_USE];
29+
return [T_DOUBLE_COLON];
3630
}
3731

3832
/**
3933
* @inheritdoc
4034
*/
4135
public function process(File $phpcsFile, $stackPtr)
4236
{
43-
if ($phpcsFile->getTokens()[$stackPtr]['type'] === 'T_USE') {
44-
$this->registerUse($phpcsFile, $stackPtr);
37+
if (!$this->isClass($phpcsFile)) {
38+
return;
4539
}
46-
if ($phpcsFile->getTokens()[$stackPtr]['type'] === 'T_FUNCTION') {
47-
$this->registerConstructorParameters($phpcsFile, $stackPtr);
40+
41+
$tokens = $phpcsFile->getTokens();
42+
43+
if ($tokens[$stackPtr - 1]['content'] !== 'ObjectManager'
44+
&& $tokens[$stackPtr + 1]['content'] !== 'getInstance'
45+
) {
46+
return;
4847
}
49-
if ($phpcsFile->getTokens()[$stackPtr]['type'] === 'T_DOUBLE_COLON') {
50-
if (!$this->isObjectManagerGetInstance($phpcsFile, $stackPtr)) {
51-
return;
52-
}
5348

54-
$statementStart = $phpcsFile->findStartOfStatement($stackPtr);
55-
$statementEnd = $phpcsFile->findEndOfStatement($stackPtr);
56-
$equalsPtr = $phpcsFile->findNext(T_EQUAL, $statementStart, $statementEnd);
49+
if (!isset($tokens[$stackPtr + 4]) || $tokens[$stackPtr + 4]['code'] !== T_SEMICOLON) {
50+
$this->validateRequestedClass(
51+
$phpcsFile,
52+
$phpcsFile->findNext(T_OBJECT_OPERATOR, $stackPtr)
53+
);
54+
return;
55+
}
5756

58-
if (!$equalsPtr) {
59-
return;
60-
}
57+
$objectManagerVariableName = $this->getObjectManagerVariableName($phpcsFile, $stackPtr);
6158

62-
if (!$this->isVariableInConstructorParameters($phpcsFile, $equalsPtr, $statementEnd)) {
63-
$className = $this->obtainClassToGetOrCreate($phpcsFile, $stackPtr, $statementEnd);
59+
if (!$objectManagerVariableName) {
60+
return;
61+
}
6462

65-
$phpcsFile->addError(
66-
sprintf("Class %s needs to be requested in constructor, " .
67-
"otherwise compiler will not be able to find and generate these classes", $className),
68-
$stackPtr,
69-
self::ERROR_CODE
70-
);
71-
}
63+
$variablePosition = $phpcsFile->findNext(T_VARIABLE, $stackPtr, null, false, $objectManagerVariableName);
64+
if ($variablePosition) {
65+
$this->validateRequestedClass($phpcsFile, $phpcsFile->findNext(T_OBJECT_OPERATOR, $variablePosition));
7266
}
7367
}
7468

7569
/**
76-
* Check if it is a ObjectManager::getInstance
70+
* Check if the class is instantiated via get/create method, it is autogenerated and present in constructor
7771
*
7872
* @param File $phpcsFile
79-
* @param int $stackPtr
80-
* @return bool
73+
* @param int $arrowPosition
8174
*/
82-
private function isObjectManagerGetInstance(File $phpcsFile, int $stackPtr): bool
75+
private function validateRequestedClass(File $phpcsFile, int $arrowPosition): void
8376
{
84-
$prev = $phpcsFile->findPrevious(T_STRING, $stackPtr - 1);
85-
$next = $phpcsFile->findNext(T_STRING, $stackPtr + 1);
86-
return $prev &&
87-
$next &&
88-
$phpcsFile->getTokens()[$prev]['content'] === 'ObjectManager' &&
89-
$phpcsFile->getTokens()[$next]['content'] === 'getInstance';
77+
$requestedClass = $this->getRequestedClass($phpcsFile, $arrowPosition);
78+
79+
if (!$requestedClass
80+
|| !$this->isClassAutogenerated($requestedClass)
81+
|| $this->isConstructorParameter($phpcsFile, $requestedClass)
82+
) {
83+
return;
84+
}
85+
86+
$phpcsFile->addError(
87+
sprintf(
88+
'Class %s needs to be requested in constructor, ' .
89+
'otherwise compiler will not be able to find and generate this class',
90+
$requestedClass
91+
),
92+
$arrowPosition,
93+
self::ERROR_CODE
94+
);
9095
}
9196

9297
/**
93-
* Get the complete class namespace from the use's
98+
* Does the class have the suffix common for autogenerated classes e.g. Factory
9499
*
95100
* @param string $className
96-
* @return string
101+
* @return bool
97102
*/
98-
private function getClassNamespace(string $className): string
103+
private function isClassAutogenerated(string $className): bool
99104
{
100-
foreach ($this->uses as $key => $use) {
101-
if ($key === $className) {
102-
return $use;
105+
foreach (self::AUTOGENERATED_CLASS_SUFFIXES as $suffix) {
106+
if (substr($className, -strlen($suffix)) === $suffix) {
107+
return true;
103108
}
104109
}
105-
return $className;
110+
return false;
106111
}
107112

108113
/**
109-
* Register php uses
114+
* Get the variable name to which the ObjectManager::getInstance() result is assigned
110115
*
111116
* @param File $phpcsFile
112117
* @param int $stackPtr
118+
* @return string|null
113119
*/
114-
private function registerUse(File $phpcsFile, int $stackPtr): void
120+
private function getObjectManagerVariableName(File $phpcsFile, int $stackPtr): ?string
115121
{
116-
$useEnd = $phpcsFile->findEndOfStatement($stackPtr);
117-
$use = [];
118-
$usePosition = $stackPtr;
119-
while ($usePosition = $phpcsFile->findNext(T_STRING, $usePosition + 1, $useEnd)) {
120-
$use[] = $phpcsFile->getTokens()[$usePosition]['content'];
122+
$matches = [];
123+
$found = preg_match(
124+
'/(\$[A-Za-z]+) ?= ?ObjectManager::getInstance\(\);/',
125+
$phpcsFile->getTokensAsString($stackPtr - 5, 10),
126+
$matches
127+
);
128+
129+
if (!$found || !isset($matches[1])) {
130+
return null;
121131
}
122132

123-
$key = end($use);
124-
if ($phpcsFile->findNext(T_AS, $stackPtr, $useEnd)) {
125-
$this->uses[$key] = implode("\\", array_slice($use, 0, count($use) - 1));
126-
} else {
127-
$this->uses[$key] = implode("\\", $use);
128-
}
133+
return $matches[1];
129134
}
130135

131136
/**
132-
* Register php constructor parameters
137+
* Get class name requested from ObjectManager
133138
*
134139
* @param File $phpcsFile
135-
* @param int $stackPtr
140+
* @param int $callerPosition
141+
* @return string|null
136142
*/
137-
private function registerConstructorParameters(File $phpcsFile, int $stackPtr): void
143+
private function getRequestedClass(File $phpcsFile, int $callerPosition): ?string
138144
{
139-
$functionName = $phpcsFile->getDeclarationName($stackPtr);
140-
if ($functionName == '__construct') {
141-
$this->constructorParameters = $phpcsFile->getMethodParameters($stackPtr);
145+
$matches = [];
146+
$found = preg_match(
147+
'/->(get|create)\(([A-Za-z\\\]+)::class/',
148+
$phpcsFile->getTokensAsString($callerPosition, $phpcsFile->findNext(T_CLOSE_PARENTHESIS, $callerPosition)),
149+
$matches
150+
);
151+
152+
if (!$found || !isset($matches[2])) {
153+
return null;
142154
}
143-
}
144155

145-
/**
146-
* Get next token
147-
*
148-
* @param File $phpcsFile
149-
* @param int $from
150-
* @param int $to
151-
* @param int|string|array $types
152-
* @return mixed
153-
*/
154-
private function getNext(File $phpcsFile, int $from, int $to, $types)
155-
{
156-
return $phpcsFile->getTokens()[$phpcsFile->findNext($types, $from + 1, $to)];
156+
return $matches[2];
157157
}
158158

159159
/**
160-
* Get previous token
160+
* Does the file contain class declaration
161161
*
162162
* @param File $phpcsFile
163-
* @param int $from
164-
* @param int|string|array $types
165-
* @return mixed
166-
*/
167-
private function getPrevious(File $phpcsFile, int $from, $types)
168-
{
169-
return $phpcsFile->getTokens()[$phpcsFile->findPrevious($types, $from - 1)];
170-
}
171-
172-
/**
173-
* Get name of the variable without $
174-
*
175-
* @param string $parameterName
176-
* @return string
163+
* @return bool
177164
*/
178-
protected function variableName(string $parameterName): string
165+
private function isClass(File $phpcsFile): bool
179166
{
180-
return str_replace('$', '', $parameterName);
167+
foreach ($phpcsFile->getTokens() as $token) {
168+
if ($token['code'] === T_CLASS) {
169+
return true;
170+
}
171+
}
172+
return false;
181173
}
182174

183175
/**
184-
* Checks if a variable is present in the constructor parameters
176+
* Get an array of constructor parameters
185177
*
186178
* @param File $phpcsFile
187-
* @param int $equalsPtr
188-
* @param int $statementEnd
189-
* @return bool
179+
* @return array
190180
*/
191-
private function isVariableInConstructorParameters(File $phpcsFile, int $equalsPtr, int $statementEnd): bool
181+
private function getConstructorParameters(File $phpcsFile): array
192182
{
193-
if ($variable = $phpcsFile->findNext(T_VARIABLE, $equalsPtr, $statementEnd)) {
194-
$variableName = $phpcsFile->getTokens()[$variable]['content'];
195-
if ($variableName === '$this') {
196-
$variableName = $this->getNext($phpcsFile, $variable, $statementEnd, T_STRING)['content'];
197-
}
198-
foreach ($this->constructorParameters as $parameter) {
199-
$parameterName = $parameter['name'];
200-
if ($this->variableName($parameterName) === $this->variableName($variableName)) {
201-
return true;
202-
}
183+
foreach ($phpcsFile->getTokens() as $stackPtr => $token) {
184+
if ($token['code'] === T_FUNCTION && $phpcsFile->getDeclarationName($stackPtr) === '__construct') {
185+
return $phpcsFile->getMethodParameters($stackPtr);
203186
}
204187
}
205-
return false;
188+
return [];
206189
}
207190

208191
/**
209-
* Obtain the class inside ObjectManager::getInstance()->get|create()
192+
* Is the class name present between constructor parameters
210193
*
211194
* @param File $phpcsFile
212-
* @param int $next
213-
* @param int $statementEnd
214-
* @return string
195+
* @param string $className
196+
* @return bool
215197
*/
216-
private function obtainClassToGetOrCreate(File $phpcsFile, int $next, int $statementEnd): string
198+
private function isConstructorParameter(File $phpcsFile, string $className): bool
217199
{
218-
while ($next = $phpcsFile->findNext(T_DOUBLE_COLON, $next + 1, $statementEnd)) {
219-
if ($this->getNext($phpcsFile, $next, $statementEnd, T_STRING)['content'] === 'class') {
220-
$className = $this->getPrevious($phpcsFile, $next, T_STRING)['content'];
200+
foreach ($this->getConstructorParameters($phpcsFile) as $parameter) {
201+
if (strpos($parameter['content'], $className) !== false) {
202+
return true;
221203
}
222204
}
223-
224-
return $this->getClassNamespace($className);
205+
return false;
225206
}
226207
}

‎Magento2/Tests/PHP/AutogeneratedClassNotInConstructorUnitTest.1.php.inc‎

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,19 +12,19 @@ use Magento2\OneModel as Model;
1212
class Good
1313
{
1414
public function __construct(
15-
Model $model = null
15+
ModelFactory $model = null
1616
)
1717
{
18-
$this->model = $model ?? ObjectManager::getInstance()->get(Model::class);
18+
$this->model = $model ?? ObjectManager::getInstance()->get(ModelFactory::class);
1919
}
2020

2121
/**
2222
* @return Model
2323
*/
2424
public function otherMethodThatCallsGetInstanceBad(): void
2525
{
26-
$model = ObjectManager::getInstance()->get(Model::class);
27-
$model->something();
26+
$modelFactory = ObjectManager::getInstance()->get(OtherFactory::class);
27+
$modelFactory->create();
2828
}
2929

3030
/**
@@ -35,4 +35,18 @@ class Good
3535
$model = $this->model ?? ObjectManager::getInstance()->get(Model::class);
3636
$model->something();
3737
}
38+
39+
public function variablePositive(): void
40+
{
41+
$objectManager = ObjectManager::getInstance();
42+
$this->_entityFactory = $objectManager->get(EntityFactoryInterface::class);
43+
}
44+
45+
public function variableNegative(): void
46+
{
47+
$objectManager = ObjectManager::getInstance();
48+
$this->_entityFactory = $objectManager->get(EntityFactory::class);
49+
}
3850
}
51+
52+

‎Magento2/Tests/PHP/AutogeneratedClassNotInConstructorUnitTest.2.php.inc‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,34 @@ declare(strict_types = 1);
77

88
namespace Magento2\Tests\PHP;
99

10+
use Magento\Catalog\Model\Product;
11+
use Magento\Eav\Api\AttributeRepositoryInterface;
12+
use Magento\Eav\Setup\AddOptionToAttribute;
13+
use Magento\Eav\Setup\EavSetup;
14+
use Magento\Eav\Setup\EavSetupFactory;
15+
use Magento\Framework\App\ObjectManager;
16+
use Magento\Framework\Setup\ModuleDataSetupInterface;
17+
1018
class Bad
1119
{
1220
public function __construct()
1321
{
14-
$this->model = ObjectManager::getInstance()->get(Model::class);
22+
$this->model = ObjectManager::getInstance()->get(ModelFactory::class);
23+
}
24+
25+
protected function setUp(): void
26+
{
27+
ObjectManager::getInstance()
28+
->get(EavSetupFactory::class);
29+
$objectManager = ObjectManager::getInstance();
30+
31+
/** @var EavSetup $eavSetup */
32+
$this->eavSetup = $objectManager
33+
->get(EavSetupFactory::class)
34+
->create(['setup' => $this->setup]);
35+
36+
ObjectManager::getInstance()
37+
->get(\Full\Class\NameFactory::class);
1538
}
1639
}
1740

0 commit comments

Comments
 (0)