From fd3f2a22b8e5b2cffaf17daccc29f461c26c5f51 Mon Sep 17 00:00:00 2001 From: Yevhen Sentiabov Date: Fri, 24 Jan 2020 14:26:19 -0600 Subject: [PATCH 1/3] Github #26532: di:setup:compile fails with anonymous classes - Fixed parsing for anonymous classes during di:compile run - Small code optimizations - Extended and refactored related unit tests --- .../Module/Di/Code/Scanner/PhpScanner.php | 86 ++++++++++--------- .../Di/Code/Reader/ClassesScannerTest.php | 4 +- .../Module/Di/Code/Scanner/PhpScannerTest.php | 84 +++++++++--------- .../Model/StubWithAnonymousClass.php | 36 ++++++++ 4 files changed, 128 insertions(+), 82 deletions(-) create mode 100644 setup/src/Magento/Setup/Test/Unit/Module/Di/_files/app/code/Magento/SomeModule/Model/StubWithAnonymousClass.php diff --git a/setup/src/Magento/Setup/Module/Di/Code/Scanner/PhpScanner.php b/setup/src/Magento/Setup/Module/Di/Code/Scanner/PhpScanner.php index 1cd242acbe50b..be25cf605ef80 100644 --- a/setup/src/Magento/Setup/Module/Di/Code/Scanner/PhpScanner.php +++ b/setup/src/Magento/Setup/Module/Di/Code/Scanner/PhpScanner.php @@ -3,6 +3,8 @@ * Copyright © Magento, Inc. All rights reserved. * See COPYING.txt for license details. */ +declare(strict_types=1); + namespace Magento\Setup\Module\Di\Code\Scanner; use Magento\Framework\Api\Code\Generator\ExtensionAttributesGenerator; @@ -12,9 +14,7 @@ use \Magento\Framework\Reflection\TypeProcessor; /** - * Class PhpScanner - * - * @package Magento\Setup\Module\Di\Code\Scanner + * Finds factory and extension attributes classes which require auto-generation. */ class PhpScanner implements ScannerInterface { @@ -53,15 +53,28 @@ public function __construct(Log $log, TypeProcessor $typeProcessor = null) protected function _findMissingClasses($file, $classReflection, $methodName, $entityType) { $missingClasses = []; - if ($classReflection->hasMethod($methodName)) { - $constructor = $classReflection->getMethod($methodName); - $parameters = $constructor->getParameters(); - /** @var $parameter \ReflectionParameter */ - foreach ($parameters as $parameter) { - preg_match('/\[\s\<\w+?>\s([\w\\\\]+)/s', $parameter->__toString(), $matches); - if (isset($matches[1]) && substr($matches[1], -strlen($entityType)) == $entityType) { - $missingClassName = $matches[1]; - if ($this->shouldGenerateClass($missingClassName, $entityType, $file)) { + if (!$classReflection->hasMethod($methodName)) { + return $missingClasses; + } + + $factorySuffix = '\\' . ucfirst(FactoryGenerator::ENTITY_TYPE); + $constructor = $classReflection->getMethod($methodName); + $parameters = $constructor->getParameters(); + /** @var $parameter \ReflectionParameter */ + foreach ($parameters as $parameter) { + preg_match('/\[\s\<\w+?>\s([\w\\\\]+)/s', $parameter->__toString(), $matches); + if (isset($matches[1]) && substr($matches[1], -strlen($entityType)) == $entityType) { + $missingClassName = $matches[1]; + if ($this->shouldGenerateClass($missingClassName, $entityType, $file)) { + + if (substr($missingClassName, -strlen($factorySuffix)) == $factorySuffix) { + $entityName = rtrim(substr($missingClassName, 0, -strlen($factorySuffix)), '\\'); + $this->_log->add( + Log::CONFIGURATION_ERROR, + $missingClassName, + 'Invalid Factory declaration for class ' . $entityName . ' in file ' . $file + ); + } else { $missingClasses[] = $missingClassName; } } @@ -110,24 +123,12 @@ protected function getSourceClassName($missingClassName, $entityType) */ protected function _fetchFactories($reflectionClass, $file) { - $factorySuffix = '\\' . ucfirst(FactoryGenerator::ENTITY_TYPE); $absentFactories = $this->_findMissingClasses( $file, $reflectionClass, '__construct', ucfirst(FactoryGenerator::ENTITY_TYPE) ); - foreach ($absentFactories as $key => $absentFactory) { - if (substr($absentFactory, -strlen($factorySuffix)) == $factorySuffix) { - $entityName = rtrim(substr($absentFactory, 0, -strlen($factorySuffix)), '\\'); - $this->_log->add( - Log::CONFIGURATION_ERROR, - $absentFactory, - 'Invalid Factory declaration for class ' . $entityName . ' in file ' . $file - ); - unset($absentFactories[$key]); - } - } return $absentFactories; } @@ -150,21 +151,19 @@ protected function _fetchMissingExtensionAttributesClasses($reflectionClass, $fi $missingClassName = $returnType['type']; if ($this->shouldGenerateClass($missingClassName, $entityType, $file)) { $missingExtensionInterfaces[] = $missingClassName; + + $extension = rtrim(substr($missingClassName, 0, -strlen('Interface')), '\\'); + if (!class_exists($extension)) { + $missingExtensionInterfaces[] = $extension; + } + $extensionFactory = $extension . 'Factory'; + if (!class_exists($extensionFactory)) { + $missingExtensionInterfaces[] = $extensionFactory; + } } } - $missingExtensionClasses = []; - $missingExtensionFactories = []; - foreach ($missingExtensionInterfaces as $missingExtensionInterface) { - $extension = rtrim(substr($missingExtensionInterface, 0, -strlen('Interface')), '\\'); - if (!class_exists($extension)) { - $missingExtensionClasses[] = $extension; - } - $extensionFactory = $extension . 'Factory'; - if (!class_exists($extensionFactory)) { - $missingExtensionFactories[] = $extensionFactory; - } - } - return array_merge($missingExtensionInterfaces, $missingExtensionClasses, $missingExtensionFactories); + + return $missingExtensionInterfaces; } /** @@ -223,8 +222,17 @@ protected function _fetchClasses($namespace, $tokenIterator, $count, $tokens) { $classes = []; for ($tokenOffset = $tokenIterator + 1; $tokenOffset < $count; ++$tokenOffset) { - if ($tokens[$tokenOffset] === '{') { - $classes[] = $namespace . "\\" . $tokens[$tokenIterator + 2][1]; + if ($tokens[$tokenOffset] !== '{') { + continue; + } + // anonymous classes should be omitted + if (is_array($tokens[$tokenIterator - 2]) && $tokens[$tokenIterator - 2][0] === T_NEW) { + continue; + } + + $class = $namespace . "\\" . $tokens[$tokenIterator + 2][1]; + if (!in_array($class, $classes)) { + $classes[] = $class; } } return $classes; diff --git a/setup/src/Magento/Setup/Test/Unit/Module/Di/Code/Reader/ClassesScannerTest.php b/setup/src/Magento/Setup/Test/Unit/Module/Di/Code/Reader/ClassesScannerTest.php index efb7e72b0ed08..4d9cd140122ad 100644 --- a/setup/src/Magento/Setup/Test/Unit/Module/Di/Code/Reader/ClassesScannerTest.php +++ b/setup/src/Magento/Setup/Test/Unit/Module/Di/Code/Reader/ClassesScannerTest.php @@ -3,6 +3,8 @@ * Copyright © Magento, Inc. All rights reserved. * See COPYING.txt for license details. */ +declare(strict_types=1); + namespace Magento\Setup\Test\Unit\Module\Di\Code\Reader; use Magento\Framework\App\Filesystem\DirectoryList; @@ -37,6 +39,6 @@ public function testGetList() $pathToScan = str_replace('\\', '/', realpath(__DIR__ . '/../../') . '/_files/app/code/Magento/SomeModule'); $actual = $this->model->getList($pathToScan); $this->assertTrue(is_array($actual)); - $this->assertCount(5, $actual); + $this->assertCount(6, $actual); } } diff --git a/setup/src/Magento/Setup/Test/Unit/Module/Di/Code/Scanner/PhpScannerTest.php b/setup/src/Magento/Setup/Test/Unit/Module/Di/Code/Scanner/PhpScannerTest.php index 5f8cd7643e87c..46d45770055b0 100644 --- a/setup/src/Magento/Setup/Test/Unit/Module/Di/Code/Scanner/PhpScannerTest.php +++ b/setup/src/Magento/Setup/Test/Unit/Module/Di/Code/Scanner/PhpScannerTest.php @@ -3,74 +3,74 @@ * Copyright © Magento, Inc. All rights reserved. * See COPYING.txt for license details. */ -namespace Magento\Setup\Test\Unit\Module\Di\Code\Scanner; +declare(strict_types=1); -require_once __DIR__ . '/../../_files/app/code/Magento/SomeModule/Helper/Test.php'; -require_once __DIR__ . '/../../_files/app/code/Magento/SomeModule/ElementFactory.php'; -require_once __DIR__ . '/../../_files/app/code/Magento/SomeModule/Model/DoubleColon.php'; -require_once __DIR__ . '/../../_files/app/code/Magento/SomeModule/Api/Data/SomeInterface.php'; +namespace Magento\Setup\Test\Unit\Module\Di\Code\Scanner; use Magento\Framework\Reflection\TypeProcessor; +use Magento\Setup\Module\Di\Code\Scanner\PhpScanner; +use Magento\Setup\Module\Di\Compiler\Log\Log; +use PHPUnit\Framework\MockObject\MockObject; class PhpScannerTest extends \PHPUnit\Framework\TestCase { /** - * @var \Magento\Setup\Module\Di\Code\Scanner\PhpScanner + * @var PhpScanner */ - protected $_model; + private $scanner; /** * @var string */ - protected $_testDir; - - /** - * @var array - */ - protected $_testFiles = []; + private $testDir; /** - * @var \PHPUnit_Framework_MockObject_MockObject + * @var Log|MockObject */ - protected $_logMock; + private $log; protected function setUp() { - $this->_logMock = $this->createMock(\Magento\Setup\Module\Di\Compiler\Log\Log::class); - $this->_model = new \Magento\Setup\Module\Di\Code\Scanner\PhpScanner($this->_logMock, new TypeProcessor()); - $this->_testDir = str_replace('\\', '/', realpath(__DIR__ . '/../../') . '/_files'); + $this->log = $this->createMock(Log::class); + $this->scanner = new PhpScanner($this->log, new TypeProcessor()); + $this->testDir = str_replace('\\', '/', realpath(__DIR__ . '/../../') . '/_files'); } public function testCollectEntities() { - $this->_testFiles = [ - $this->_testDir . '/app/code/Magento/SomeModule/Helper/Test.php', - $this->_testDir . '/app/code/Magento/SomeModule/Model/DoubleColon.php', - $this->_testDir . '/app/code/Magento/SomeModule/Api/Data/SomeInterface.php' + require_once __DIR__ . '/../../_files/app/code/Magento/SomeModule/Helper/Test.php'; + require_once __DIR__ . '/../../_files/app/code/Magento/SomeModule/ElementFactory.php'; + require_once __DIR__ . '/../../_files/app/code/Magento/SomeModule/Model/DoubleColon.php'; + require_once __DIR__ . '/../../_files/app/code/Magento/SomeModule/Api/Data/SomeInterface.php'; + require_once __DIR__ . '/../../_files/app/code/Magento/SomeModule/Model/StubWithAnonymousClass.php'; + + $testFiles = [ + $this->testDir . '/app/code/Magento/SomeModule/Helper/Test.php', + $this->testDir . '/app/code/Magento/SomeModule/Model/DoubleColon.php', + $this->testDir . '/app/code/Magento/SomeModule/Api/Data/SomeInterface.php', + $this->testDir . '/app/code/Magento/SomeModule/Model/StubWithAnonymousClass.php', ]; - $this->_logMock->expects( - $this->at(0) - )->method( - 'add' - )->with( - 4, - 'Magento\SomeModule\Module\Factory', - 'Invalid Factory for nonexistent class Magento\SomeModule\Module in file ' . $this->_testFiles[0] - ); - $this->_logMock->expects( - $this->at(1) - )->method( - 'add' - )->with( - 4, - 'Magento\SomeModule\Element\Factory', - 'Invalid Factory declaration for class Magento\SomeModule\Element in file ' . $this->_testFiles[0] - ); + $this->log->expects(self::at(0)) + ->method('add') + ->with( + 4, + 'Magento\SomeModule\Module\Factory', + 'Invalid Factory for nonexistent class Magento\SomeModule\Module in file ' . $testFiles[0] + ); + $this->log->expects(self::at(1)) + ->method('add') + ->with( + 4, + 'Magento\SomeModule\Element\Factory', + 'Invalid Factory declaration for class Magento\SomeModule\Element in file ' . $testFiles[0] + ); + + $result = $this->scanner->collectEntities($testFiles); - $this->assertEquals( + self::assertEquals( ['\\' . \Magento\Eav\Api\Data\AttributeExtensionInterface::class], - $this->_model->collectEntities($this->_testFiles) + $result ); } } diff --git a/setup/src/Magento/Setup/Test/Unit/Module/Di/_files/app/code/Magento/SomeModule/Model/StubWithAnonymousClass.php b/setup/src/Magento/Setup/Test/Unit/Module/Di/_files/app/code/Magento/SomeModule/Model/StubWithAnonymousClass.php new file mode 100644 index 0000000000000..793ee3bb16cdb --- /dev/null +++ b/setup/src/Magento/Setup/Test/Unit/Module/Di/_files/app/code/Magento/SomeModule/Model/StubWithAnonymousClass.php @@ -0,0 +1,36 @@ +factory = $factory; + } + + public function getSerializable(): \JsonSerializable + { + return new class() implements \JsonSerializable { + /** + * @inheritDoc + */ + public function jsonSerialize() + { + return [1, 2, 3]; + } + }; + } +} From 25fd9259e72bc484f5899aa4edf3de120b66db0c Mon Sep 17 00:00:00 2001 From: Alexander Steshuk Date: Wed, 29 Jan 2020 17:30:11 +0200 Subject: [PATCH 2/3] magento/magento2#26533: Unit test fix --- .../Unit/Module/Di/Code/Scanner/PhpScannerTest.php | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/setup/src/Magento/Setup/Test/Unit/Module/Di/Code/Scanner/PhpScannerTest.php b/setup/src/Magento/Setup/Test/Unit/Module/Di/Code/Scanner/PhpScannerTest.php index 46d45770055b0..3aea4166df838 100644 --- a/setup/src/Magento/Setup/Test/Unit/Module/Di/Code/Scanner/PhpScannerTest.php +++ b/setup/src/Magento/Setup/Test/Unit/Module/Di/Code/Scanner/PhpScannerTest.php @@ -7,6 +7,12 @@ namespace Magento\Setup\Test\Unit\Module\Di\Code\Scanner; +require_once __DIR__ . '/../../_files/app/code/Magento/SomeModule/Helper/Test.php'; +require_once __DIR__ . '/../../_files/app/code/Magento/SomeModule/ElementFactory.php'; +require_once __DIR__ . '/../../_files/app/code/Magento/SomeModule/Model/DoubleColon.php'; +require_once __DIR__ . '/../../_files/app/code/Magento/SomeModule/Api/Data/SomeInterface.php'; +require_once __DIR__ . '/../../_files/app/code/Magento/SomeModule/Model/StubWithAnonymousClass.php'; + use Magento\Framework\Reflection\TypeProcessor; use Magento\Setup\Module\Di\Code\Scanner\PhpScanner; use Magento\Setup\Module\Di\Compiler\Log\Log; @@ -38,12 +44,6 @@ protected function setUp() public function testCollectEntities() { - require_once __DIR__ . '/../../_files/app/code/Magento/SomeModule/Helper/Test.php'; - require_once __DIR__ . '/../../_files/app/code/Magento/SomeModule/ElementFactory.php'; - require_once __DIR__ . '/../../_files/app/code/Magento/SomeModule/Model/DoubleColon.php'; - require_once __DIR__ . '/../../_files/app/code/Magento/SomeModule/Api/Data/SomeInterface.php'; - require_once __DIR__ . '/../../_files/app/code/Magento/SomeModule/Model/StubWithAnonymousClass.php'; - $testFiles = [ $this->testDir . '/app/code/Magento/SomeModule/Helper/Test.php', $this->testDir . '/app/code/Magento/SomeModule/Model/DoubleColon.php', From e165d99f5994af39d49ed70155c003745158e5db Mon Sep 17 00:00:00 2001 From: Yevhen Sentiabov Date: Wed, 29 Jan 2020 10:53:49 -0600 Subject: [PATCH 3/3] Github #26532: di:setup:compile fails with anonymous classes - Small refactoring for code simplification --- .../Module/Di/Code/Scanner/PhpScanner.php | 47 ++++++++++--------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/setup/src/Magento/Setup/Module/Di/Code/Scanner/PhpScanner.php b/setup/src/Magento/Setup/Module/Di/Code/Scanner/PhpScanner.php index be25cf605ef80..c7aa6fc6eb0c3 100644 --- a/setup/src/Magento/Setup/Module/Di/Code/Scanner/PhpScanner.php +++ b/setup/src/Magento/Setup/Module/Di/Code/Scanner/PhpScanner.php @@ -10,8 +10,8 @@ use Magento\Framework\Api\Code\Generator\ExtensionAttributesGenerator; use Magento\Framework\Api\Code\Generator\ExtensionAttributesInterfaceGenerator; use Magento\Framework\ObjectManager\Code\Generator\Factory as FactoryGenerator; +use Magento\Framework\Reflection\TypeProcessor; use Magento\Setup\Module\Di\Compiler\Log\Log; -use \Magento\Framework\Reflection\TypeProcessor; /** * Finds factory and extension attributes classes which require auto-generation. @@ -50,7 +50,7 @@ public function __construct(Log $log, TypeProcessor $typeProcessor = null) * @param string $entityType * @return string[] */ - protected function _findMissingClasses($file, $classReflection, $methodName, $entityType) + private function findMissingFactories($file, $classReflection, $methodName, $entityType) { $missingClasses = []; if (!$classReflection->hasMethod($methodName)) { @@ -123,7 +123,7 @@ protected function getSourceClassName($missingClassName, $entityType) */ protected function _fetchFactories($reflectionClass, $file) { - $absentFactories = $this->_findMissingClasses( + $absentFactories = $this->findMissingFactories( $file, $reflectionClass, '__construct', @@ -177,11 +177,11 @@ public function collectEntities(array $files) { $output = [[]]; foreach ($files as $file) { - $classes = $this->_getDeclaredClasses($file); + $classes = $this->getDeclaredClasses($file); foreach ($classes as $className) { $reflectionClass = new \ReflectionClass($className); - $output [] = $this->_fetchFactories($reflectionClass, $file); - $output [] = $this->_fetchMissingExtensionAttributesClasses($reflectionClass, $file); + $output[] = $this->_fetchFactories($reflectionClass, $file); + $output[] = $this->_fetchMissingExtensionAttributesClasses($reflectionClass, $file); } } return array_unique(array_merge(...$output)); @@ -210,32 +210,30 @@ protected function _fetchNamespace($tokenIterator, $count, $tokens) } /** - * Fetch class names from tokenized PHP file + * Fetches class name from tokenized PHP file. * * @param string $namespace * @param int $tokenIterator * @param int $count * @param array $tokens - * @return array + * @return string|null */ - protected function _fetchClasses($namespace, $tokenIterator, $count, $tokens) + private function fetchClass($namespace, $tokenIterator, $count, $tokens):? string { - $classes = []; + // anonymous classes should be omitted + if (is_array($tokens[$tokenIterator - 2]) && $tokens[$tokenIterator - 2][0] === T_NEW) { + return null; + } + for ($tokenOffset = $tokenIterator + 1; $tokenOffset < $count; ++$tokenOffset) { if ($tokens[$tokenOffset] !== '{') { continue; } - // anonymous classes should be omitted - if (is_array($tokens[$tokenIterator - 2]) && $tokens[$tokenIterator - 2][0] === T_NEW) { - continue; - } - $class = $namespace . "\\" . $tokens[$tokenIterator + 2][1]; - if (!in_array($class, $classes)) { - $classes[] = $class; - } + return $namespace . "\\" . $tokens[$tokenIterator + 2][1]; } - return $classes; + + return null; } /** @@ -244,9 +242,9 @@ protected function _fetchClasses($namespace, $tokenIterator, $count, $tokens) * @param string $file * @return array */ - protected function _getDeclaredClasses($file) + private function getDeclaredClasses($file): array { - $classes = [[]]; + $classes = []; $namespaceParts = []; // phpcs:ignore $tokens = token_get_all(file_get_contents($file)); @@ -260,10 +258,13 @@ protected function _getDeclaredClasses($file) if (($tokens[$tokenIterator][0] == T_CLASS || $tokens[$tokenIterator][0] == T_INTERFACE) && $tokens[$tokenIterator - 1][0] != T_DOUBLE_COLON ) { - $classes[] = $this->_fetchClasses(join('', $namespaceParts), $tokenIterator, $count, $tokens); + $class = $this->fetchClass(join('', $namespaceParts), $tokenIterator, $count, $tokens); + if ($class !== null && !in_array($class, $classes)) { + $classes[] = $class; + } } } - return array_unique(array_merge(...$classes)); + return $classes; } /**