Skip to content

Commit a2c6b29

Browse files
faizan-shkfaizan-shk
committed
AC-6841:Fix ArgumentsReader::getParentCall() to support named constructor arguments
1 parent 4b43b2b commit a2c6b29

File tree

7 files changed

+201
-32
lines changed

7 files changed

+201
-32
lines changed

‎lib/internal/Magento/Framework/Code/Reader/ArgumentsReader.php‎

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -137,16 +137,22 @@ private function processType(\ReflectionClass $class, \Laminas\Code\Reflection\P
137137
* @param \ReflectionClass $class
138138
* @param array $classArguments
139139
* @return array|null
140+
* @throws \ReflectionException
140141
*/
141-
public function getParentCall(\ReflectionClass $class, array $classArguments)
142+
public function getParentCall(\ReflectionClass $class, array $classArguments): ?array
142143
{
143144
/** Skip native PHP types */
144145
if (!$class->getFileName()) {
145146
return null;
146147
}
147148

148149
$trimFunction = function (&$value) {
149-
$value = trim($value, PHP_EOL . ' $');
150+
$position = strpos($value, ':');
151+
if ($position !== false) {
152+
$value = trim(substr($value, 0, $position), PHP_EOL . ' ');
153+
} else {
154+
$value = trim($value, PHP_EOL . ' $');
155+
}
150156
};
151157

152158
$method = $class->getMethod('__construct');
@@ -158,10 +164,11 @@ public function getParentCall(\ReflectionClass $class, array $classArguments)
158164
$content = implode('', array_slice($source, $start, $length));
159165
$pattern = '/parent::__construct\(([ ' .
160166
PHP_EOL .
161-
']*[$]{1}[a-zA-Z0-9_]*,)*[ ' .
167+
']*' .
168+
'([a-zA-Z0-9_]+([ ' . PHP_EOL . '])*:([ ' . PHP_EOL . '])*)*[$][a-zA-Z0-9_]*,)*[ ' .
162169
PHP_EOL .
163170
']*' .
164-
'([$]{1}[a-zA-Z0-9_]*){1}[' .
171+
'([a-zA-Z0-9_]+([ ' . PHP_EOL . '])*:([ ' . PHP_EOL . '])*)*([$][a-zA-Z0-9_]*)[' .
165172
PHP_EOL .
166173
' ]*\);/';
167174

@@ -174,8 +181,13 @@ public function getParentCall(\ReflectionClass $class, array $classArguments)
174181
return null;
175182
}
176183

184+
$isNamedArgument = false;
177185
$arguments = substr(trim($arguments), 20, -2);
178186
$arguments = explode(',', $arguments);
187+
$position = strpos(current($arguments), ':');
188+
if ($position !== false) {
189+
$isNamedArgument = true;
190+
}
179191
array_walk($arguments, $trimFunction);
180192

181193
$output = [];
@@ -186,7 +198,12 @@ public function getParentCall(\ReflectionClass $class, array $classArguments)
186198
'position' => $argumentPosition,
187199
'type' => $type,
188200
];
201+
202+
if ($isNamedArgument) {
203+
$output[$argumentPosition]['isNamedArgument'] = true;
204+
}
189205
}
206+
190207
return $output;
191208
}
192209

‎lib/internal/Magento/Framework/Code/Test/Unit/Reader/ArgumentsReaderTest.php‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -310,6 +310,33 @@ public function testGetParentCallWithSeparateLineFormat()
310310
$this->assertEquals($expectedResult, $actualResult);
311311
}
312312

313+
public function testGetParentCallWithNamedArguments()
314+
{
315+
$class = new \ReflectionClass('ClassWithNamedArgumentsForParentCall');
316+
$actualResult = $this->_model->getParentCall(
317+
$class,
318+
[
319+
'stdClassObject' => ['type' => '\stdClass'],
320+
'runeTimeException' => ['type' => '\ClassExtendsDefaultPhpType']
321+
]
322+
);
323+
$expectedResult = [
324+
[
325+
'name' => 'stdClassObject',
326+
'position' => 0,
327+
'type' => '\stdClass',
328+
'isNamedArgument' => true
329+
],
330+
[
331+
'name' => 'runeTimeException',
332+
'position' => 1,
333+
'type' => '\ClassExtendsDefaultPhpType',
334+
'isNamedArgument' => true
335+
],
336+
];
337+
$this->assertEquals($expectedResult, $actualResult);
338+
}
339+
313340
/**
314341
* @param string $requiredType
315342
* @param string $actualType

‎lib/internal/Magento/Framework/Code/Test/Unit/Reader/_files/ClassesForArgumentsReader.php‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -257,3 +257,26 @@ public function __construct(\stdClass $stdClassObject, \ClassExtendsDefaultPhpTy
257257
$this->argumentTwo = $secondClass;
258258
}
259259
}
260+
class ClassWithNamedArgumentsForParentCall extends FirstClassForParentCall
261+
{
262+
/**
263+
* @var stdClass
264+
*/
265+
protected $_stdClassObject;
266+
267+
/**
268+
* @var ClassExtendsDefaultPhpType
269+
*/
270+
protected $_runeTimeException;
271+
272+
/**
273+
* @param stdClass $stdClassObject
274+
* @param ClassExtendsDefaultPhpType $runeTimeException
275+
*/
276+
public function __construct(\stdClass $stdClassObject, \ClassExtendsDefaultPhpType $runeTimeException)
277+
{
278+
parent::__construct(stdClassObject: $stdClassObject, runeTimeException: $runeTimeException);
279+
$this->_stdClassObject = $stdClassObject;
280+
$this->_runeTimeException = $runeTimeException;
281+
}
282+
}

‎lib/internal/Magento/Framework/Code/Test/Unit/Validator/ConstructorIntegrityTest.php‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
require_once __DIR__ . '/../_files/app/code/Magento/SomeModule/Model/Four/Test.php';
1717
require_once __DIR__ . '/../_files/app/code/Magento/SomeModule/Model/Five/Test.php';
1818
require_once __DIR__ . '/../_files/app/code/Magento/SomeModule/Model/Six/Test.php';
19+
require_once __DIR__ . '/../_files/app/code/Magento/SomeModule/Model/NamedArguments/ChildClassTest.php';
1920
require_once __DIR__ . '/_files/ClassesForConstructorIntegrity.php';
2021
class ConstructorIntegrityTest extends TestCase
2122
{
@@ -39,6 +40,11 @@ public function testValidateIfClassHasParentConstructCall()
3940
$this->assertTrue($this->_model->validate(\Magento\SomeModule\Model\Two\Test::class));
4041
}
4142

43+
public function testValidateIfClassHasParentConstructCallWithNamedArguments()
44+
{
45+
$this->assertTrue($this->_model->validate(\Magento\SomeModule\Model\NamedArguments\ChildClassTest::class));
46+
}
47+
4248
public function testValidateIfClassHasArgumentsQtyEqualToParentClass()
4349
{
4450
$this->assertTrue($this->_model->validate(\Magento\SomeModule\Model\Three\Test::class));

‎lib/internal/Magento/Framework/Code/Test/Unit/_files/app/code/Magento/SomeModule/Model/NamedArguments/ChildClassTest.php‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
<?php
2+
declare(strict_types=1);
3+
namespace Magento\SomeModule\Model\NamedArguments;
4+
5+
require_once __DIR__ . '/ParentClassTest.php';
6+
7+
class ChildClassTest extends ParentClassTest
8+
{
9+
/**
10+
* @var stdClass
11+
*/
12+
protected $stdClassObject;
13+
14+
/**
15+
* @var array
16+
*/
17+
protected $arrayVariable;
18+
19+
/**
20+
* @param stdClass $stdClassObject
21+
* @param array $arrayVariable
22+
*/
23+
public function __construct(\stdClass $stdClassObject, array $arrayVariable)
24+
{
25+
parent::__construct(arrayVariable: $arrayVariable, stdClassObject: $stdClassObject);
26+
}
27+
}

‎lib/internal/Magento/Framework/Code/Test/Unit/_files/app/code/Magento/SomeModule/Model/NamedArguments/ParentClassTest.php‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
<?php
2+
3+
namespace Magento\SomeModule\Model\NamedArguments;
4+
5+
class ParentClassTest
6+
{
7+
/**
8+
* @var stdClass
9+
*/
10+
protected $stdClassObject;
11+
12+
/**
13+
* @var array
14+
*/
15+
protected $arrayVariable;
16+
17+
/**
18+
* @param stdClass $stdClassObject
19+
* @param array $arrayVariable
20+
*/
21+
public function __construct(\stdClass $stdClassObject, array $arrayVariable)
22+
{
23+
$this->stdClassObject = $stdClassObject;
24+
$this->arrayVariable = $arrayVariable;
25+
}
26+
}

‎lib/internal/Magento/Framework/Code/Validator/ConstructorIntegrity.php‎

Lines changed: 71 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
namespace Magento\Framework\Code\Validator;
99

1010
use Magento\Framework\Code\ValidatorInterface;
11+
use Magento\Framework\Exception\ValidatorException;
1112
use Magento\Framework\Phrase;
1213

1314
class ConstructorIntegrity implements ValidatorInterface
@@ -30,7 +31,7 @@ public function __construct(\Magento\Framework\Code\Reader\ArgumentsReader $argu
3031
*
3132
* @param string $className
3233
* @return bool
33-
* @throws \Magento\Framework\Exception\ValidatorException
34+
* @throws ValidatorException
3435
* @SuppressWarnings(PHPMD.CyclomaticComplexity)
3536
* @SuppressWarnings(PHPMD.NPathComplexity)
3637
*/
@@ -65,35 +66,26 @@ public function validate($className)
6566
/** Get parent class __construct arguments */
6667
$parentArguments = $this->_argumentsReader->getConstructorArguments($parent, true, true);
6768

68-
foreach ($parentArguments as $index => $requiredArgument) {
69-
if (isset($callArguments[$index])) {
70-
$actualArgument = $callArguments[$index];
71-
} else {
72-
if ($requiredArgument['isOptional']) {
73-
continue;
74-
}
69+
if (isset(current($callArguments)['isNamedArgument'])) {
70+
$callArguments = array_column($callArguments, null, 'name');
7571

76-
$classPath = str_replace('\\', '/', $class->getFileName());
77-
throw new \Magento\Framework\Exception\ValidatorException(
78-
new Phrase(
79-
'Missed required argument %1 in parent::__construct call. File: %2',
80-
[$requiredArgument['name'], $classPath]
81-
)
82-
);
72+
foreach ($parentArguments as $requiredArgument) {
73+
if (isset($callArguments[$requiredArgument['name']])) {
74+
$actualArgument = $callArguments[$requiredArgument['name']];
75+
$this->checkCompatibleTypes($requiredArgument['type'], $actualArgument['type'], $class);
76+
} else {
77+
$this->checkIfRequiredArgumentIsOptional($requiredArgument, $class);
78+
}
8379
}
84-
85-
$isCompatibleTypes = $this->_argumentsReader->isCompatibleType(
86-
$requiredArgument['type'],
87-
$actualArgument['type']
88-
);
89-
if (false == $isCompatibleTypes) {
90-
$classPath = str_replace('\\', '/', $class->getFileName());
91-
throw new \Magento\Framework\Exception\ValidatorException(
92-
new Phrase(
93-
'Incompatible argument type: Required type: %1. Actual type: %2; File: %3%4%5',
94-
[$requiredArgument['type'], $actualArgument['type'], PHP_EOL, $classPath, PHP_EOL]
95-
)
96-
);
80+
} else {
81+
// Need to separate logic for unnamed arguments as we cannot consider `argument name` for unnamed arguments
82+
foreach ($parentArguments as $index => $requiredArgument) {
83+
if (isset($callArguments[$index])) {
84+
$actualArgument = $callArguments[$index];
85+
$this->checkCompatibleTypes($requiredArgument['type'], $actualArgument['type'], $class);
86+
} else {
87+
$this->checkIfRequiredArgumentIsOptional($requiredArgument, $class);
88+
}
9789
}
9890
}
9991

@@ -118,4 +110,55 @@ public function validate($className)
118110
}
119111
return true;
120112
}
113+
114+
/**
115+
* Check argument type compatibility
116+
*
117+
* @param string $requiredArgumentType
118+
* @param string $actualArgumentType
119+
* @param \ReflectionClass $class
120+
* @return void
121+
* @throws ValidatorException
122+
*/
123+
private function checkCompatibleTypes(
124+
$requiredArgumentType,
125+
$actualArgumentType,
126+
\ReflectionClass $class
127+
): void {
128+
$isCompatibleTypes = $this->_argumentsReader->isCompatibleType(
129+
$requiredArgumentType,
130+
$actualArgumentType
131+
);
132+
133+
if (!$isCompatibleTypes) {
134+
$classPath = str_replace('\\', '/', $class->getFileName());
135+
throw new ValidatorException(
136+
new Phrase(
137+
'Incompatible argument type: Required type: %1. Actual type: %2; File: %3%4%5',
138+
[$requiredArgumentType, $actualArgumentType, PHP_EOL, $classPath, PHP_EOL]
139+
)
140+
);
141+
}
142+
}
143+
144+
/**
145+
* Check if argument argument is optional
146+
*
147+
* @param array $requiredArgument
148+
* @param \ReflectionClass $class
149+
* @return void
150+
* @throws ValidatorException
151+
*/
152+
private function checkIfRequiredArgumentIsOptional(array $requiredArgument, \ReflectionClass $class): void
153+
{
154+
if (!$requiredArgument['isOptional']) {
155+
$classPath = str_replace('\\', '/', $class->getFileName());
156+
throw new ValidatorException(
157+
new Phrase(
158+
'Missed required argument %1 in parent::__construct call. File: %2',
159+
[$requiredArgument['name'], $classPath]
160+
)
161+
);
162+
}
163+
}
121164
}

0 commit comments

Comments
 (0)