From 34f7eafd372db51f9b49ad6e720cfb5bafce6734 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Mart=C3=ADnez?= Date: Wed, 11 Sep 2019 15:27:07 +0200 Subject: [PATCH 01/10] Allow use factories and OM for creating objects with variadic arguments in constructor --- .../Framework/Code/Reader/ClassReader.php | 16 +++++++++++++++- .../ObjectManager/Factory/AbstractFactory.php | 10 +++++++--- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/lib/internal/Magento/Framework/Code/Reader/ClassReader.php b/lib/internal/Magento/Framework/Code/Reader/ClassReader.php index fe96e9cc742aa..25a7e8e2174cf 100644 --- a/lib/internal/Magento/Framework/Code/Reader/ClassReader.php +++ b/lib/internal/Magento/Framework/Code/Reader/ClassReader.php @@ -28,7 +28,8 @@ public function getConstructor($className) $parameter->getName(), $parameter->getClass() !== null ? $parameter->getClass()->getName() : null, !$parameter->isOptional() && !$parameter->isDefaultValueAvailable(), - $parameter->isDefaultValueAvailable() ? $parameter->getDefaultValue() : null, + $this->getReflectionParameterDefaultValue($parameter), + $parameter->isVariadic(), ]; } catch (\ReflectionException $e) { $message = $e->getMessage(); @@ -40,6 +41,19 @@ public function getConstructor($className) return $result; } + /** + * @param \ReflectionParameter $parameter + * @return array|mixed|null + */ + private function getReflectionParameterDefaultValue(\ReflectionParameter $parameter) + { + if ($parameter->isVariadic()) { + return []; + } + + return $parameter->isDefaultValueAvailable() ? $parameter->getDefaultValue() : null; + } + /** * Retrieve parent relation information for type in a following format * array( diff --git a/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php b/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php index 15c4cb098b84d..261d9bc7834ab 100644 --- a/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php +++ b/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php @@ -226,7 +226,7 @@ protected function resolveArgumentsInRuntime($requestedType, array $parameters, { $resolvedArguments = []; foreach ($parameters as $parameter) { - list($paramName, $paramType, $paramRequired, $paramDefault) = $parameter; + list($paramName, $paramType, $paramRequired, $paramDefault, $isVariadic) = $parameter; $argument = null; if (!empty($arguments) && (isset($arguments[$paramName]) || array_key_exists($paramName, $arguments))) { $argument = $arguments[$paramName]; @@ -243,9 +243,13 @@ protected function resolveArgumentsInRuntime($requestedType, array $parameters, $argument = $paramDefault; } - $this->resolveArgument($argument, $paramType, $paramDefault, $paramName, $requestedType); + if ($isVariadic && is_array($argument)) { + $resolvedArguments = array_merge($resolvedArguments, $argument); + } else { + $this->resolveArgument($argument, $paramType, $paramDefault, $paramName, $requestedType); + $resolvedArguments[] = $argument; + } - $resolvedArguments[] = $argument; } return $resolvedArguments; } From 47a32285ec3c5fb2032f56bdc1df8e11a8b5dece Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Mart=C3=ADnez?= Date: Wed, 11 Sep 2019 16:52:45 +0200 Subject: [PATCH 02/10] Fix Static Tests build --- .../Framework/Code/Reader/ClassReader.php | 11 +++++++--- .../ObjectManager/Factory/AbstractFactory.php | 21 ++++++++++++------- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/lib/internal/Magento/Framework/Code/Reader/ClassReader.php b/lib/internal/Magento/Framework/Code/Reader/ClassReader.php index 25a7e8e2174cf..27e633c660689 100644 --- a/lib/internal/Magento/Framework/Code/Reader/ClassReader.php +++ b/lib/internal/Magento/Framework/Code/Reader/ClassReader.php @@ -5,12 +5,15 @@ */ namespace Magento\Framework\Code\Reader; +/** + * Class ClassReader + */ class ClassReader implements ClassReaderInterface { /** * Read class constructor signature * - * @param string $className + * @param string $className * @return array|null * @throws \ReflectionException */ @@ -42,7 +45,9 @@ public function getConstructor($className) } /** - * @param \ReflectionParameter $parameter + * Get reflection parameter default value + * + * @param \ReflectionParameter $parameter * @return array|mixed|null */ private function getReflectionParameterDefaultValue(\ReflectionParameter $parameter) @@ -63,7 +68,7 @@ private function getReflectionParameterDefaultValue(\ReflectionParameter $parame * ... * ) * - * @param string $className + * @param string $className * @return string[] */ public function getParents($className) diff --git a/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php b/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php index 261d9bc7834ab..ed530b1455930 100644 --- a/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php +++ b/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php @@ -11,6 +11,9 @@ use Psr\Log\LoggerInterface; use Magento\Framework\App\ObjectManager; +/** + * Class AbstractFactory + */ abstract class AbstractFactory implements \Magento\Framework\ObjectManager\FactoryInterface { /** @@ -49,10 +52,10 @@ abstract class AbstractFactory implements \Magento\Framework\ObjectManager\Facto protected $creationStack = []; /** - * @param \Magento\Framework\ObjectManager\ConfigInterface $config - * @param ObjectManagerInterface $objectManager + * @param \Magento\Framework\ObjectManager\ConfigInterface $config + * @param ObjectManagerInterface $objectManager * @param \Magento\Framework\ObjectManager\DefinitionInterface $definitions - * @param array $globalArguments + * @param array $globalArguments */ public function __construct( \Magento\Framework\ObjectManager\ConfigInterface $config, @@ -91,6 +94,8 @@ public function setArguments($arguments) } /** + * Get definitions + * * @return \Magento\Framework\ObjectManager\DefinitionInterface */ public function getDefinitions() @@ -105,7 +110,7 @@ public function getDefinitions() * Create object * * @param string $type - * @param array $args + * @param array $args * * @return object * @throws RuntimeException @@ -130,9 +135,9 @@ protected function createObject($type, $args) /** * Resolve an argument * - * @param array &$argument + * @param array &$argument * @param string $paramType - * @param mixed $paramDefault + * @param mixed $paramDefault * @param string $paramName * @param string $requestedType * @@ -214,8 +219,8 @@ protected function parseArray(&$array) * Resolve constructor arguments * * @param string $requestedType - * @param array $parameters - * @param array $arguments + * @param array $parameters + * @param array $arguments * * @return array * From 67b3f6ea2325e495c99e0d70ce4cebc0e0080f10 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Mart=C3=ADnez?= Date: Wed, 11 Sep 2019 17:53:28 +0200 Subject: [PATCH 03/10] Unit tests for variadic arguments in constructor --- .../Test/Unit/Factory/CompiledTest.php | 18 +++ .../Test/Unit/Factory/FactoryTest.php | 135 ++++++++++++++++-- .../Test/Unit/Factory/Fixture/Variadic.php | 35 +++++ 3 files changed, 177 insertions(+), 11 deletions(-) create mode 100644 lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/Fixture/Variadic.php diff --git a/lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/CompiledTest.php b/lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/CompiledTest.php index 779a0d04ebc52..648a74b91495d 100644 --- a/lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/CompiledTest.php +++ b/lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/CompiledTest.php @@ -38,6 +38,9 @@ class CompiledTest extends \PHPUnit\Framework\TestCase /** @var ObjectManager */ private $objectManager; + /** + * Setup tests + */ protected function setUp() { $this->objectManager = new ObjectManager($this); @@ -57,6 +60,9 @@ protected function setUp() $this->objectManager->setBackwardCompatibleProperty($this->factory, 'definitions', $this->definitionsMock); } + /** + * Test create simple + */ public function testCreateSimple() { $expectedConfig = $this->getSimpleConfig(); @@ -106,6 +112,9 @@ public function testCreateSimple() $this->assertNull($result->getNullValue()); } + /** + * Test create simple configured arguments + */ public function testCreateSimpleConfiguredArguments() { $expectedConfig = $this->getSimpleNestedConfig(); @@ -170,6 +179,9 @@ public function testCreateSimpleConfiguredArguments() $this->assertNull($result->getNullValue()); } + /** + * Test create get arguments in runtime + */ public function testCreateGetArgumentsInRuntime() { // Stub OM to create test assets @@ -308,18 +320,21 @@ private function getRuntimeParameters() 1 => DependencyTesting::class, 2 => true, 3 => null, + 4 => false, ], 1 => [ 0 => 'sharedDependency', 1 => DependencySharedTesting::class, 2 => true, 3 => null, + 4 => false, ], 2 => [ 0 => 'value', 1 => null, 2 => false, 3 => 'value', + 4 => false, ], 3 => [ 0 => 'valueArray', @@ -329,18 +344,21 @@ private function getRuntimeParameters() 0 => 'default_value1', 1 => 'default_value2', ], + 4 => false, ], 4 => [ 0 => 'globalValue', 1 => null, 2 => false, 3 => '', + 4 => false, ], 5 => [ 0 => 'nullValue', 1 => null, 2 => false, 3 => null, + 4 => false, ], ]; } diff --git a/lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/FactoryTest.php b/lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/FactoryTest.php index 309bf48548ec7..c062fcee2cde3 100644 --- a/lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/FactoryTest.php +++ b/lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/FactoryTest.php @@ -10,6 +10,9 @@ use Magento\Framework\ObjectManager\Factory\Dynamic\Developer; use Magento\Framework\ObjectManager\ObjectManager; +/** + * Class FactoryTest + */ class FactoryTest extends \PHPUnit\Framework\TestCase { /** @@ -27,6 +30,9 @@ class FactoryTest extends \PHPUnit\Framework\TestCase */ private $objectManager; + /** + * Setup tests + */ protected function setUp() { $this->config = new Config(); @@ -35,6 +41,9 @@ protected function setUp() $this->factory->setObjectManager($this->objectManager); } + /** + * Test create without args + */ public function testCreateNoArgs() { $this->assertInstanceOf('StdClass', $this->factory->create(\StdClass::class)); @@ -55,7 +64,7 @@ public function testResolveArgumentsException() $definitionsMock = $this->createMock(\Magento\Framework\ObjectManager\DefinitionInterface::class); $definitionsMock->expects($this->once())->method('getParameters') ->will($this->returnValue([[ - 'firstParam', 'string', true, 'default_val', + 'firstParam', 'string', true, 'default_val', false ]])); $this->factory = new Developer( @@ -136,16 +145,16 @@ public function testCreateUsingReflection() $definitions = $this->createMock(\Magento\Framework\ObjectManager\DefinitionInterface::class); // should be more than defined in "switch" of create() method $definitions->expects($this->once())->method('getParameters')->with($type)->will($this->returnValue([ - ['one', null, false, null], - ['two', null, false, null], - ['three', null, false, null], - ['four', null, false, null], - ['five', null, false, null], - ['six', null, false, null], - ['seven', null, false, null], - ['eight', null, false, null], - ['nine', null, false, null], - ['ten', null, false, null], + ['one', null, false, null, false], + ['two', null, false, null, false], + ['three', null, false, null, false], + ['four', null, false, null, false], + ['five', null, false, null, false], + ['six', null, false, null, false], + ['seven', null, false, null, false], + ['eight', null, false, null, false], + ['nine', null, false, null, false], + ['ten', null, false, null, false], ])); $factory = new Developer($this->config, null, $definitions); $result = $factory->create( @@ -165,4 +174,108 @@ public function testCreateUsingReflection() ); $this->assertSame(10, $result->getArg(9)); } + + /** + * Test create objects with variadic argument in constructor + * + * @param $createArgs + * @param $expectedArg0 + * @param $expectedArg1 + * @dataProvider testCreateUsingVariadicDataProvider + */ + public function testCreateUsingVariadic( + $createArgs, + $expectedArg0, + $expectedArg1 + ) { + $type = \Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\Variadic::class; + $definitions = $this->createMock(\Magento\Framework\ObjectManager\DefinitionInterface::class); + + $definitions->expects($this->once())->method('getParameters')->with($type)->will($this->returnValue([ + [ + 'oneScalars', + \Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\OneScalar::class, + false, + [], + true + ], + ])); + $factory = new Developer($this->config, null, $definitions); + + + + /** @var \Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\Variadic $variadic */ + $variadic = is_null($createArgs) + ? $factory->create($type) + : $factory->create($type, $createArgs); + + $this->assertSame($expectedArg0, $variadic->getOneScalarByKey(0)); + $this->assertSame($expectedArg1, $variadic->getOneScalarByKey(1)); + } + + /** + * @return array + */ + public function testCreateUsingVariadicDataProvider() { + $oneScalar1 = $this->createMock(\Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\OneScalar::class); + $oneScalar2 = $this->createMock(\Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\OneScalar::class); + + return [ + 'without_args' => [ + null, + null, + null, + ], + 'with_empty_args' => [ + [], + null, + null, + ], + 'with_empty_args_value' => [ + [ + 'oneScalars' => [] + ], + null, + null, + ], + 'with_args' => [ + [ + 'oneScalars' => [ + $oneScalar1, + $oneScalar2, + ] + ], + $oneScalar1, + $oneScalar2, + ], + ]; + } + + /** + * Test data can be injected into variadic arguments from di config + */ + public function testCreateVariadicFromDiConfig() + { + $oneScalar1 = $this->createMock(\Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\OneScalar::class); + $oneScalar2 = $this->createMock(\Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\OneScalar::class); + + // let's imitate that Variadic is configured by providing DI configuration for it + $this->config->extend( + [ + \Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\Variadic::class => [ + 'arguments' => [ + 'oneScalars' => [ + $oneScalar1, + $oneScalar2, + ] + ] + ], + ] + ); + /** @var \Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\Variadic $variadic */ + $variadic = $this->factory->create(\Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\Variadic::class); + + $this->assertSame($oneScalar1, $variadic->getOneScalarByKey(0)); + $this->assertSame($oneScalar2, $variadic->getOneScalarByKey(1)); + } } diff --git a/lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/Fixture/Variadic.php b/lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/Fixture/Variadic.php new file mode 100644 index 0000000000000..af26f7456fdde --- /dev/null +++ b/lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/Fixture/Variadic.php @@ -0,0 +1,35 @@ +oneScalars = $oneScalars; + } + + /** + * @param string $key + * @return mixed + */ + public function getOneScalarByKey($key) + { + return $this->oneScalars[$key] ?? null; + } +} From 487edadf0fee63f4c1550e1f470920a0d6f64ce9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Mart=C3=ADnez?= Date: Wed, 11 Sep 2019 18:56:28 +0200 Subject: [PATCH 04/10] Fix Static Tests build --- .../ObjectManager/Factory/AbstractFactory.php | 6 +- .../Test/Unit/Factory/FactoryTest.php | 110 ++++++++++++------ 2 files changed, 77 insertions(+), 39 deletions(-) diff --git a/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php b/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php index ed530b1455930..f3dc14a02c12b 100644 --- a/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php +++ b/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php @@ -120,7 +120,9 @@ protected function createObject($type, $args) try { return new $type(...array_values($args)); } catch (\TypeError $exception) { - /** @var LoggerInterface $logger */ + /** + * @var LoggerInterface $logger + */ $logger = ObjectManager::getInstance()->get(LoggerInterface::class); $logger->critical( sprintf('Type Error occurred when creating object: %s, %s', $type, $exception->getMessage()) @@ -249,7 +251,7 @@ protected function resolveArgumentsInRuntime($requestedType, array $parameters, } if ($isVariadic && is_array($argument)) { - $resolvedArguments = array_merge($resolvedArguments, $argument); + $resolvedArguments += $argument; } else { $this->resolveArgument($argument, $paramType, $paramDefault, $paramName, $requestedType); $resolvedArguments[] = $argument; diff --git a/lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/FactoryTest.php b/lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/FactoryTest.php index c062fcee2cde3..dc49cc84e23d8 100644 --- a/lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/FactoryTest.php +++ b/lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/FactoryTest.php @@ -50,22 +50,34 @@ public function testCreateNoArgs() } /** - * @expectedException \UnexpectedValueException + * @expectedException \UnexpectedValueException * @expectedExceptionMessage Invalid parameter configuration provided for $firstParam argument */ public function testResolveArgumentsException() { $configMock = $this->createMock(\Magento\Framework\ObjectManager\Config\Config::class); - $configMock->expects($this->once())->method('getArguments') - ->will($this->returnValue([ - 'firstParam' => 1, - ])); + $configMock->expects($this->once())->method('getArguments')->will( + $this->returnValue( + [ + 'firstParam' => 1, + ] + ) + ); $definitionsMock = $this->createMock(\Magento\Framework\ObjectManager\DefinitionInterface::class); - $definitionsMock->expects($this->once())->method('getParameters') - ->will($this->returnValue([[ - 'firstParam', 'string', true, 'default_val', false - ]])); + $definitionsMock->expects($this->once())->method('getParameters')->will( + $this->returnValue( + [ + [ + 'firstParam', + 'string', + true, + 'default_val', + false + ] + ] + ) + ); $this->factory = new Developer( $configMock, @@ -80,9 +92,14 @@ public function testResolveArgumentsException() ); } + /** + * Test create with one arg + */ public function testCreateOneArg() { - /** @var \Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\OneScalar $result */ + /** + * @var \Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\OneScalar $result + */ $result = $this->factory->create( \Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\OneScalar::class, ['foo' => 'bar'] @@ -91,6 +108,9 @@ public function testCreateOneArg() $this->assertEquals('bar', $result->getFoo()); } + /** + * Test create with injectable + */ public function testCreateWithInjectable() { // let's imitate that One is injectable by providing DI configuration for it @@ -101,7 +121,9 @@ public function testCreateWithInjectable() ], ] ); - /** @var \Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\Two $result */ + /** + * @var \Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\Two $result + */ $result = $this->factory->create(\Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\Two::class); $this->assertInstanceOf(\Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\Two::class, $result); $this->assertInstanceOf( @@ -113,8 +135,8 @@ public function testCreateWithInjectable() } /** - * @param string $startingClass - * @param string $terminationClass + * @param string $startingClass + * @param string $terminationClass * @dataProvider circularDataProvider */ public function testCircular($startingClass, $terminationClass) @@ -139,23 +161,30 @@ public function circularDataProvider() ]; } + /** + * Test create using reflection + */ public function testCreateUsingReflection() { $type = \Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\Polymorphous::class; $definitions = $this->createMock(\Magento\Framework\ObjectManager\DefinitionInterface::class); // should be more than defined in "switch" of create() method - $definitions->expects($this->once())->method('getParameters')->with($type)->will($this->returnValue([ - ['one', null, false, null, false], - ['two', null, false, null, false], - ['three', null, false, null, false], - ['four', null, false, null, false], - ['five', null, false, null, false], - ['six', null, false, null, false], - ['seven', null, false, null, false], - ['eight', null, false, null, false], - ['nine', null, false, null, false], - ['ten', null, false, null, false], - ])); + $definitions->expects($this->once())->method('getParameters')->with($type)->will( + $this->returnValue( + [ + ['one', null, false, null, false], + ['two', null, false, null, false], + ['three', null, false, null, false], + ['four', null, false, null, false], + ['five', null, false, null, false], + ['six', null, false, null, false], + ['seven', null, false, null, false], + ['eight', null, false, null, false], + ['nine', null, false, null, false], + ['ten', null, false, null, false], + ] + ) + ); $factory = new Developer($this->config, null, $definitions); $result = $factory->create( $type, @@ -178,9 +207,9 @@ public function testCreateUsingReflection() /** * Test create objects with variadic argument in constructor * - * @param $createArgs - * @param $expectedArg0 - * @param $expectedArg1 + * @param $createArgs + * @param $expectedArg0 + * @param $expectedArg1 * @dataProvider testCreateUsingVariadicDataProvider */ public function testCreateUsingVariadic( @@ -191,20 +220,24 @@ public function testCreateUsingVariadic( $type = \Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\Variadic::class; $definitions = $this->createMock(\Magento\Framework\ObjectManager\DefinitionInterface::class); - $definitions->expects($this->once())->method('getParameters')->with($type)->will($this->returnValue([ - [ + $definitions->expects($this->once())->method('getParameters')->with($type)->will( + $this->returnValue( + [ + [ 'oneScalars', \Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\OneScalar::class, false, [], true - ], - ])); + ], + ] + ) + ); $factory = new Developer($this->config, null, $definitions); - - - /** @var \Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\Variadic $variadic */ + /** + * @var \Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\Variadic $variadic + */ $variadic = is_null($createArgs) ? $factory->create($type) : $factory->create($type, $createArgs); @@ -216,7 +249,8 @@ public function testCreateUsingVariadic( /** * @return array */ - public function testCreateUsingVariadicDataProvider() { + public function testCreateUsingVariadicDataProvider() + { $oneScalar1 = $this->createMock(\Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\OneScalar::class); $oneScalar2 = $this->createMock(\Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\OneScalar::class); @@ -272,7 +306,9 @@ public function testCreateVariadicFromDiConfig() ], ] ); - /** @var \Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\Variadic $variadic */ + /** + * @var \Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\Variadic $variadic + */ $variadic = $this->factory->create(\Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\Variadic::class); $this->assertSame($oneScalar1, $variadic->getOneScalarByKey(0)); From 5d3d6aa541c7200a76709420bab76c0f84433252 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Mart=C3=ADnez?= Date: Wed, 11 Sep 2019 19:22:59 +0200 Subject: [PATCH 05/10] Fix Static Tests build --- .../Magento/Framework/ObjectManager/Factory/AbstractFactory.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php b/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php index f3dc14a02c12b..11fa0e5d8c7b0 100644 --- a/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php +++ b/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php @@ -137,7 +137,7 @@ protected function createObject($type, $args) /** * Resolve an argument * - * @param array &$argument + * @param array $argument * @param string $paramType * @param mixed $paramDefault * @param string $paramName From 087023562a8f3b625e35059c8705747885090732 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Mart=C3=ADnez?= Date: Wed, 11 Sep 2019 19:47:44 +0200 Subject: [PATCH 06/10] Add tests for classes with non variadic and variadic arguments in constructor --- .../ObjectManager/Factory/AbstractFactory.php | 2 +- .../Test/Unit/Factory/FactoryTest.php | 127 ++++++++++++++++++ .../Unit/Factory/Fixture/SemiVariadic.php | 53 ++++++++ 3 files changed, 181 insertions(+), 1 deletion(-) create mode 100644 lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/Fixture/SemiVariadic.php diff --git a/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php b/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php index 11fa0e5d8c7b0..bb77fed4c4476 100644 --- a/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php +++ b/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php @@ -251,7 +251,7 @@ protected function resolveArgumentsInRuntime($requestedType, array $parameters, } if ($isVariadic && is_array($argument)) { - $resolvedArguments += $argument; + $resolvedArguments = array_merge($resolvedArguments, $argument); } else { $this->resolveArgument($argument, $paramType, $paramDefault, $paramName, $requestedType); $resolvedArguments[] = $argument; diff --git a/lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/FactoryTest.php b/lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/FactoryTest.php index dc49cc84e23d8..d192bee2fbc6b 100644 --- a/lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/FactoryTest.php +++ b/lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/FactoryTest.php @@ -314,4 +314,131 @@ public function testCreateVariadicFromDiConfig() $this->assertSame($oneScalar1, $variadic->getOneScalarByKey(0)); $this->assertSame($oneScalar2, $variadic->getOneScalarByKey(1)); } + + /** + * Test create objects with non variadic and variadic argument in constructor + * + * @param $createArgs + * @param $expectedFooValue + * @param $expectedArg0 + * @param $expectedArg1 + * @dataProvider testCreateUsingSemiVariadicDataProvider + */ + public function testCreateUsingSemiVariadic( + $createArgs, + $expectedFooValue, + $expectedArg0, + $expectedArg1 + ) { + $type = \Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\SemiVariadic::class; + $definitions = $this->createMock(\Magento\Framework\ObjectManager\DefinitionInterface::class); + + $definitions->expects($this->once())->method('getParameters')->with($type)->will( + $this->returnValue( + [ + [ + 'foo', + null, + false, + \Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\SemiVariadic::DEFAULT_FOO_VALUE, + false + ], + [ + 'oneScalars', + \Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\OneScalar::class, + false, + [], + true + ], + ] + ) + ); + $factory = new Developer($this->config, null, $definitions); + + /** + * @var \Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\SemiVariadic $semiVariadic + */ + $semiVariadic = is_null($createArgs) + ? $factory->create($type) + : $factory->create($type, $createArgs); + + $this->assertSame($expectedFooValue, $semiVariadic->getFoo()); + $this->assertSame($expectedArg0, $semiVariadic->getOneScalarByKey(0)); + $this->assertSame($expectedArg1, $semiVariadic->getOneScalarByKey(1)); + } + + /** + * @return array + */ + public function testCreateUsingSemiVariadicDataProvider() + { + $oneScalar1 = $this->createMock(\Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\OneScalar::class); + $oneScalar2 = $this->createMock(\Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\OneScalar::class); + + return [ + 'without_args' => [ + null, + \Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\SemiVariadic::DEFAULT_FOO_VALUE, + null, + null, + ], + 'with_empty_args' => [ + [], + \Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\SemiVariadic::DEFAULT_FOO_VALUE, + null, + null, + ], + 'only_with_foo_value' => [ + [ + 'foo' => 'baz' + ], + 'baz', + null, + null, + ], + 'only_with_oneScalars_empty_value' => [ + [ + 'oneScalars' => [] + ], + \Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\SemiVariadic::DEFAULT_FOO_VALUE, + null, + null, + ], + 'only_with_oneScalars_full_value' => [ + [ + 'oneScalars' => [ + $oneScalar1, + $oneScalar2, + ] + ], + \Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\SemiVariadic::DEFAULT_FOO_VALUE, + $oneScalar1, + $oneScalar2, + ], + 'with_all_values_defined_in_right_order' => [ + [ + 'foo' => 'baz', + 'oneScalars' => [ + $oneScalar1, + $oneScalar2, + ] + ], + 'baz', + $oneScalar1, + $oneScalar2, + ], + 'with_all_values_defined_in_reverse_order' => [ + [ + 'oneScalars' => [ + $oneScalar1, + $oneScalar2, + ], + 'foo' => 'baz', + ], + 'baz', + $oneScalar1, + $oneScalar2, + ], + ]; + } } diff --git a/lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/Fixture/SemiVariadic.php b/lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/Fixture/SemiVariadic.php new file mode 100644 index 0000000000000..1074a96d31a0b --- /dev/null +++ b/lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/Fixture/SemiVariadic.php @@ -0,0 +1,53 @@ +foo = $foo; + $this->oneScalars = $oneScalars; + } + + /** + * @param string $key + * @return mixed + */ + public function getOneScalarByKey($key) + { + return $this->oneScalars[$key] ?? null; + } + + /** + * @return string + */ + public function getFoo(): string + { + return $this->foo; + } +} From 8faaeb177fb0639f8696a4872c1fea2142dd281f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Mart=C3=ADnez?= Date: Wed, 11 Sep 2019 19:49:10 +0200 Subject: [PATCH 07/10] Fix Static Tests build --- .../Test/Unit/Factory/Fixture/SemiVariadic.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/Fixture/SemiVariadic.php b/lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/Fixture/SemiVariadic.php index 1074a96d31a0b..8773dec1c48d6 100644 --- a/lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/Fixture/SemiVariadic.php +++ b/lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/Fixture/SemiVariadic.php @@ -11,6 +11,7 @@ class SemiVariadic { const DEFAULT_FOO_VALUE = 'bar'; + /** * @var OneScalar[] */ @@ -23,7 +24,8 @@ class SemiVariadic /** * SemiVariadic constructor. - * @param string $foo + * + * @param string $foo * @param OneScalar[] ...$oneScalars */ public function __construct( @@ -35,7 +37,7 @@ public function __construct( } /** - * @param string $key + * @param mixed $key * @return mixed */ public function getOneScalarByKey($key) From 60ea1d7b4e1dcb4ad28b82980ef54fc5676634bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Mart=C3=ADnez?= Date: Wed, 11 Sep 2019 21:32:42 +0200 Subject: [PATCH 08/10] Avoid array_merge() in loop --- .../Framework/ObjectManager/Factory/AbstractFactory.php | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php b/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php index bb77fed4c4476..65a1b345c353b 100644 --- a/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php +++ b/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php @@ -251,13 +251,14 @@ protected function resolveArgumentsInRuntime($requestedType, array $parameters, } if ($isVariadic && is_array($argument)) { - $resolvedArguments = array_merge($resolvedArguments, $argument); + $resolvedArguments[] = $argument; } else { $this->resolveArgument($argument, $paramType, $paramDefault, $paramName, $requestedType); - $resolvedArguments[] = $argument; + $resolvedArguments[] = [$argument]; } } - return $resolvedArguments; + + return empty($resolvedArguments) ? [] : array_merge(...$resolvedArguments); } } From d7f312a1dcc74d377e952c1ab86f5550bfbf503a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Mart=C3=ADnez?= Date: Thu, 12 Sep 2019 00:00:07 +0200 Subject: [PATCH 09/10] Resolve cyclomatic complexity, add support for simple parameter declaration --- .../ObjectManager/Factory/AbstractFactory.php | 54 +++++++++++-------- .../Test/Unit/Factory/FactoryTest.php | 17 +++++- 2 files changed, 49 insertions(+), 22 deletions(-) diff --git a/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php b/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php index 65a1b345c353b..9279e0b3edf43 100644 --- a/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php +++ b/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php @@ -233,32 +233,44 @@ protected function resolveArgumentsInRuntime($requestedType, array $parameters, { $resolvedArguments = []; foreach ($parameters as $parameter) { - list($paramName, $paramType, $paramRequired, $paramDefault, $isVariadic) = $parameter; - $argument = null; - if (!empty($arguments) && (isset($arguments[$paramName]) || array_key_exists($paramName, $arguments))) { - $argument = $arguments[$paramName]; - } elseif ($paramRequired) { - if ($paramType) { - $argument = ['instance' => $paramType]; - } else { - $this->creationStack = []; - throw new \BadMethodCallException( - 'Missing required argument $' . $paramName . ' of ' . $requestedType . '.' - ); - } - } else { - $argument = $paramDefault; - } + $resolvedArguments[] = $this->getResolvedArgument((string)$requestedType, $parameter, $arguments); + } - if ($isVariadic && is_array($argument)) { - $resolvedArguments[] = $argument; + return empty($resolvedArguments) ? [] : array_merge(...$resolvedArguments); + } + + /** + * Get resolved argument from parameter + * + * @param string $requestedType + * @param array $parameter + * @param array $arguments + * @return array + */ + private function getResolvedArgument(string $requestedType, array $parameter, array $arguments): array + { + list($paramName, $paramType, $paramRequired, $paramDefault, $isVariadic) = $parameter; + $argument = null; + if (!empty($arguments) && (isset($arguments[$paramName]) || array_key_exists($paramName, $arguments))) { + $argument = $arguments[$paramName]; + } elseif ($paramRequired) { + if ($paramType) { + $argument = ['instance' => $paramType]; } else { - $this->resolveArgument($argument, $paramType, $paramDefault, $paramName, $requestedType); - $resolvedArguments[] = [$argument]; + $this->creationStack = []; + throw new \BadMethodCallException( + 'Missing required argument $' . $paramName . ' of ' . $requestedType . '.' + ); } + } else { + $argument = $paramDefault; + } + if ($isVariadic) { + return is_array($argument) ? $argument : [$argument]; } - return empty($resolvedArguments) ? [] : array_merge(...$resolvedArguments); + $this->resolveArgument($argument, $paramType, $paramDefault, $paramName, $requestedType); + return [$argument]; } } diff --git a/lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/FactoryTest.php b/lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/FactoryTest.php index d192bee2fbc6b..cc86d794bd80a 100644 --- a/lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/FactoryTest.php +++ b/lib/internal/Magento/Framework/ObjectManager/Test/Unit/Factory/FactoryTest.php @@ -272,7 +272,14 @@ public function testCreateUsingVariadicDataProvider() null, null, ], - 'with_args' => [ + 'with_single_arg' => [ + [ + 'oneScalars' => $oneScalar1 + ], + $oneScalar1, + null, + ], + 'with_full_args' => [ [ 'oneScalars' => [ $oneScalar1, @@ -404,6 +411,14 @@ public function testCreateUsingSemiVariadicDataProvider() null, null, ], + 'only_with_oneScalars_single_value' => [ + [ + 'oneScalars' => $oneScalar1 + ], + \Magento\Framework\ObjectManager\Test\Unit\Factory\Fixture\SemiVariadic::DEFAULT_FOO_VALUE, + $oneScalar1, + null, + ], 'only_with_oneScalars_full_value' => [ [ 'oneScalars' => [ From a2cbfa6a2fcb456c087ec7d96636eddec5e5945b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adri=C3=A1n=20Mart=C3=ADnez?= Date: Thu, 12 Sep 2019 00:05:46 +0200 Subject: [PATCH 10/10] Fix Static Tests build --- .../Framework/ObjectManager/Factory/AbstractFactory.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php b/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php index 9279e0b3edf43..7094b116ead3f 100644 --- a/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php +++ b/lib/internal/Magento/Framework/ObjectManager/Factory/AbstractFactory.php @@ -242,9 +242,9 @@ protected function resolveArgumentsInRuntime($requestedType, array $parameters, /** * Get resolved argument from parameter * - * @param string $requestedType - * @param array $parameter - * @param array $arguments + * @param string $requestedType + * @param array $parameter + * @param array $arguments * @return array */ private function getResolvedArgument(string $requestedType, array $parameter, array $arguments): array