From 3d0eab5ac560a4d2233242a091a36f9620d08260 Mon Sep 17 00:00:00 2001 From: Nathaniel Rogers Date: Wed, 29 Aug 2018 11:23:21 +1000 Subject: [PATCH 1/5] Updated the error reporting for the Config Element Iterator. Attempt to make the debugging a little easier when the system configuration is incorrect. Needed to add the App state class and the Logger to determine how much logging is used and how to get the error to the end developer or user. Updated the PHPUnit test to have logger and state mocks. Included a Unit test for confirming the exception message in developer mode. --- .../Config/Structure/Element/Iterator.php | 31 ++++++++++++++++++- .../Config/Structure/Element/IteratorTest.php | 27 +++++++++++++++- 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/app/code/Magento/Config/Model/Config/Structure/Element/Iterator.php b/app/code/Magento/Config/Model/Config/Structure/Element/Iterator.php index f53ecd710b28c..f489c1528ff1b 100644 --- a/app/code/Magento/Config/Model/Config/Structure/Element/Iterator.php +++ b/app/code/Magento/Config/Model/Config/Structure/Element/Iterator.php @@ -39,12 +39,30 @@ class Iterator implements \Iterator */ protected $_lastId; + /** + * @var \Psr\Log\LoggerInterface|null + */ + protected $_logger; + + /** + * @var \Magento\Framework\App\State|null + */ + protected $_state; + /** * @param \Magento\Config\Model\Config\Structure\AbstractElement $element + * @param \Psr\Log\LoggerInterface|null $logger + * @param \Magento\Framework\App\State|null $state */ - public function __construct(\Magento\Config\Model\Config\Structure\AbstractElement $element) + public function __construct( + \Magento\Config\Model\Config\Structure\AbstractElement $element, + \Psr\Log\LoggerInterface $logger = null, + \Magento\Framework\App\State $state = null + ) { $this->_flyweight = $element; + $this->_logger = $logger; + $this->_state = $state; } /** @@ -53,6 +71,7 @@ public function __construct(\Magento\Config\Model\Config\Structure\AbstractEleme * @param array $elements * @param string $scope * @return void + * @throws \Symfony\Component\Config\Definition\Exception\InvalidConfigurationException */ public function setElements(array $elements, $scope) { @@ -60,6 +79,16 @@ public function setElements(array $elements, $scope) $this->_scope = $scope; if (count($elements)) { $lastElement = end($elements); + $keys = array_keys($elements); + $elementKey = end($keys); + if (!isset($lastElement['id'])) { + if ($this->_logger) { + $this->_logger->error("Invalid module adminhtml/system.xml config for element with key '$elementKey'."); + } + if ($this->_state->getMode() == \Magento\Framework\App\State::MODE_DEVELOPER) { + throw new \Symfony\Component\Config\Definition\Exception\InvalidConfigurationException("Invalid configuration element defined for element with key '$elementKey'"); + } + } $this->_lastId = $lastElement['id']; } } diff --git a/app/code/Magento/Config/Test/Unit/Model/Config/Structure/Element/IteratorTest.php b/app/code/Magento/Config/Test/Unit/Model/Config/Structure/Element/IteratorTest.php index 1a0f3d03b060c..8b92e3009eda4 100644 --- a/app/code/Magento/Config/Test/Unit/Model/Config/Structure/Element/IteratorTest.php +++ b/app/code/Magento/Config/Test/Unit/Model/Config/Structure/Element/IteratorTest.php @@ -17,12 +17,24 @@ class IteratorTest extends \PHPUnit\Framework\TestCase */ protected $_flyweightMock; + /** + * @var \PHPUnit_Framework_MockObject_MockObject + */ + protected $_loggerMock; + + /** + * @var \PHPUnit_Framework_MockObject_MockObject + */ + protected $_stateMock; + protected function setUp() { $elementData = ['group1' => ['id' => 1], 'group2' => ['id' => 2], 'group3' => ['id' => 3]]; $this->_flyweightMock = $this->createMock(\Magento\Config\Model\Config\Structure\Element\Group::class); + $this->_loggerMock = $this->createMock(\Psr\Log\LoggerInterface::class); + $this->_stateMock = $this->createMock(\Magento\Framework\App\State::class); - $this->_model = new \Magento\Config\Model\Config\Structure\Element\Iterator($this->_flyweightMock); + $this->_model = new \Magento\Config\Model\Config\Structure\Element\Iterator($this->_flyweightMock, $this->_loggerMock, $this->_stateMock); $this->_model->setElements($elementData, 'scope'); } @@ -68,6 +80,19 @@ public function testIsLast($elementId, $result) $this->assertEquals($result, $this->_model->isLast($elementMock)); } + /** + * Test whether an element without an id will throw the expected exception and message when in developer mode + * + * @expectedException \Symfony\Component\Config\Definition\Exception\InvalidConfigurationException + */ + public function testInvalidConfigThrowsExceptionInDeveloperMode() { + $this->_stateMock->expects($this->at(0))->method('getMode')->willReturn(State::MODE_DEVELOPER); + $elementData = ['group1' => ['id' => 1], 'group2' => ['id' => 2], 'group3' => ['id' => 3], 'group4' => ['invalid' => "broken"]]; + $expectedMessage = "Invalid configuration element defined for element with key 'group4'"; + $this->_model->setElements($elementData, 'scope'); + $this->expectExceptionMessage($expectedMessage); + } + public function isLastDataProvider() { return [[1, false], [2, false], [3, true]]; From 8da9c4f1068e7c8a8d7e4461c62df8d787fe44d8 Mon Sep 17 00:00:00 2001 From: Nathaniel Rogers Date: Wed, 29 Aug 2018 16:44:40 +1000 Subject: [PATCH 2/5] Need to fully qualify the class name for \Magento\Framework\App\State or import the class at the top, but it isn't common to see in core code. --- .../Test/Unit/Model/Config/Structure/Element/IteratorTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/code/Magento/Config/Test/Unit/Model/Config/Structure/Element/IteratorTest.php b/app/code/Magento/Config/Test/Unit/Model/Config/Structure/Element/IteratorTest.php index 8b92e3009eda4..98e1f542d7955 100644 --- a/app/code/Magento/Config/Test/Unit/Model/Config/Structure/Element/IteratorTest.php +++ b/app/code/Magento/Config/Test/Unit/Model/Config/Structure/Element/IteratorTest.php @@ -86,7 +86,7 @@ public function testIsLast($elementId, $result) * @expectedException \Symfony\Component\Config\Definition\Exception\InvalidConfigurationException */ public function testInvalidConfigThrowsExceptionInDeveloperMode() { - $this->_stateMock->expects($this->at(0))->method('getMode')->willReturn(State::MODE_DEVELOPER); + $this->_stateMock->expects($this->at(0))->method('getMode')->willReturn(\Magento\Framework\App\State::MODE_DEVELOPER); $elementData = ['group1' => ['id' => 1], 'group2' => ['id' => 2], 'group3' => ['id' => 3], 'group4' => ['invalid' => "broken"]]; $expectedMessage = "Invalid configuration element defined for element with key 'group4'"; $this->_model->setElements($elementData, 'scope'); From 91439075d072c87c9b839b2a46a8457d5e1d5d1a Mon Sep 17 00:00:00 2001 From: Nathaniel Rogers Date: Tue, 22 Jan 2019 18:26:15 +1000 Subject: [PATCH 3/5] https://community.magento.com/t5/Magento-2-x-Feature-Requests-and/M2-1-No-CLI-option-to-exclude-patterns-on-setup-di-compile-and/idi-p/48447 Added event dispatch with excluded patterns so observers can exclude specific custom paths when running setup:di:compile. Ideally, because this forces the vendors to acknowledge that the compilation isn't working, they will be prompted to fix the actual issue instead of simply excluding the problem. --- .../Console/Command/DiCompileCommand.php | 29 ++++++++++++++++++- .../Console/Command/DiCompileCommandTest.php | 15 +++++++++- 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/setup/src/Magento/Setup/Console/Command/DiCompileCommand.php b/setup/src/Magento/Setup/Console/Command/DiCompileCommand.php index 68e26ec83c3f7..35059e626d1e4 100644 --- a/setup/src/Magento/Setup/Console/Command/DiCompileCommand.php +++ b/setup/src/Magento/Setup/Console/Command/DiCompileCommand.php @@ -72,6 +72,11 @@ class DiCompileCommand extends Command */ private $componentRegistrar; + /** + * @var \Magento\Framework\Event\ManagerInterface + */ + private $eventManager; + /** * Constructor * @@ -82,6 +87,7 @@ class DiCompileCommand extends Command * @param Filesystem $filesystem * @param DriverInterface $fileDriver * @param \Magento\Framework\Component\ComponentRegistrar $componentRegistrar + * @param \Magento\Framework\Event\ManagerInterface $eventManager */ public function __construct( DeploymentConfig $deploymentConfig, @@ -90,7 +96,8 @@ public function __construct( ObjectManagerProvider $objectManagerProvider, Filesystem $filesystem, DriverInterface $fileDriver, - ComponentRegistrar $componentRegistrar + ComponentRegistrar $componentRegistrar, + \Magento\Framework\Event\ManagerInterface $eventManager = null ) { $this->deploymentConfig = $deploymentConfig; $this->directoryList = $directoryList; @@ -99,6 +106,7 @@ public function __construct( $this->filesystem = $filesystem; $this->fileDriver = $fileDriver; $this->componentRegistrar = $componentRegistrar; + $this->eventManager = $eventManager; parent::__construct(); } @@ -162,6 +170,7 @@ protected function execute(InputInterface $input, OutputInterface $output) 'application' => $this->getExcludedModulePaths($modulePaths), 'framework' => $this->getExcludedLibraryPaths($libraryPaths), 'setup' => $this->getExcludedSetupPaths($setupPath), + 'custom-paths' => $this->getExcludedCustomPaths($modulePaths), ]; $this->configureObjectManager($output); @@ -400,4 +409,22 @@ private function getOperationsConfiguration( return $operations; } + + /** + * Exclude custom directories + * + * @param array $modulePaths + * + * @return array + */ + protected function getExcludedCustomPaths(array $modulePaths) { + $excludedCustomPaths = []; + + /** @var \Magento\Framework\Event\ManagerInterface $eventManager */ + if (!$this->eventManager) { + return $excludedCustomPaths; + } + $this->eventManager->dispatch('setup_di_compile_excluded_patterns', array('modulePaths' => $modulePaths, 'excludedPaths' => &$excludedCustomPaths)); + return $excludedCustomPaths; + } } diff --git a/setup/src/Magento/Setup/Test/Unit/Console/Command/DiCompileCommandTest.php b/setup/src/Magento/Setup/Test/Unit/Console/Command/DiCompileCommandTest.php index ca94a1b6fd559..4fe28c2eb9954 100644 --- a/setup/src/Magento/Setup/Test/Unit/Console/Command/DiCompileCommandTest.php +++ b/setup/src/Magento/Setup/Test/Unit/Console/Command/DiCompileCommandTest.php @@ -42,6 +42,9 @@ class DiCompileCommandTest extends \PHPUnit\Framework\TestCase /** @var \Magento\Framework\Component\ComponentRegistrar|\PHPUnit_Framework_MockObject_MockObject */ private $componentRegistrarMock; + /** @var \Magento\Framework\Event\ManagerInterface|\PHPUnit_Framework_MockObject_MockObject */ + private $eventManageMock; + /** @var \Symfony\Component\Console\Output\OutputInterface|\PHPUnit_Framework_MockObject_MockObject */ private $outputMock; @@ -85,6 +88,15 @@ public function setUp() [ComponentRegistrar::LIBRARY, ['/path/to/library/one', '/path (1)/to/library/two']], ]); + $this->eventManageMock = $this->createMock(\Magento\Framework\Event\ManagerInterface::class); + $this->eventManageMock->expects($this->any())->method('dispatch')->with( + 'setup_di_compile_excluded_patterns', array( + 'modulePaths' => ['/path/to/module/one', '/path (1)/to/module/two'], + 'excludedPaths' => [] + ))->willReturnMap([ + ['#^(?:/custom/path/to)/exclude#'] + ]); + $this->outputFormatterMock = $this->createMock( \Symfony\Component\Console\Formatter\OutputFormatterInterface::class ); @@ -99,7 +111,8 @@ public function setUp() $objectManagerProviderMock, $this->filesystemMock, $this->fileDriverMock, - $this->componentRegistrarMock + $this->componentRegistrarMock, + $this->eventManageMock ); } From 6b8a950369e78e0ff4a7fa7840e2c84aeb1ba1ba Mon Sep 17 00:00:00 2001 From: Nathaniel Rogers Date: Wed, 23 Jan 2019 10:45:52 +1000 Subject: [PATCH 4/5] Reverting Iterator Debug changes as the Pull Request was rejected. See Commit 8da9c4f1068e7c8a8d7e4461c62df8d787fe44d8 --- .../Config/Structure/Element/Iterator.php | 33 ++----------------- .../Config/Structure/Element/IteratorTest.php | 29 ++-------------- 2 files changed, 4 insertions(+), 58 deletions(-) diff --git a/app/code/Magento/Config/Model/Config/Structure/Element/Iterator.php b/app/code/Magento/Config/Model/Config/Structure/Element/Iterator.php index f489c1528ff1b..c15a82d709960 100644 --- a/app/code/Magento/Config/Model/Config/Structure/Element/Iterator.php +++ b/app/code/Magento/Config/Model/Config/Structure/Element/Iterator.php @@ -39,30 +39,12 @@ class Iterator implements \Iterator */ protected $_lastId; - /** - * @var \Psr\Log\LoggerInterface|null - */ - protected $_logger; - - /** - * @var \Magento\Framework\App\State|null - */ - protected $_state; - /** * @param \Magento\Config\Model\Config\Structure\AbstractElement $element - * @param \Psr\Log\LoggerInterface|null $logger - * @param \Magento\Framework\App\State|null $state */ - public function __construct( - \Magento\Config\Model\Config\Structure\AbstractElement $element, - \Psr\Log\LoggerInterface $logger = null, - \Magento\Framework\App\State $state = null - ) + public function __construct(\Magento\Config\Model\Config\Structure\AbstractElement $element) { $this->_flyweight = $element; - $this->_logger = $logger; - $this->_state = $state; } /** @@ -71,7 +53,6 @@ public function __construct( * @param array $elements * @param string $scope * @return void - * @throws \Symfony\Component\Config\Definition\Exception\InvalidConfigurationException */ public function setElements(array $elements, $scope) { @@ -79,16 +60,6 @@ public function setElements(array $elements, $scope) $this->_scope = $scope; if (count($elements)) { $lastElement = end($elements); - $keys = array_keys($elements); - $elementKey = end($keys); - if (!isset($lastElement['id'])) { - if ($this->_logger) { - $this->_logger->error("Invalid module adminhtml/system.xml config for element with key '$elementKey'."); - } - if ($this->_state->getMode() == \Magento\Framework\App\State::MODE_DEVELOPER) { - throw new \Symfony\Component\Config\Definition\Exception\InvalidConfigurationException("Invalid configuration element defined for element with key '$elementKey'"); - } - } $this->_lastId = $lastElement['id']; } } @@ -177,4 +148,4 @@ public function isLast(\Magento\Config\Model\Config\Structure\ElementInterface $ { return $element->getId() == $this->_lastId; } -} +} \ No newline at end of file diff --git a/app/code/Magento/Config/Test/Unit/Model/Config/Structure/Element/IteratorTest.php b/app/code/Magento/Config/Test/Unit/Model/Config/Structure/Element/IteratorTest.php index 982620d106f29..712541bf5de4b 100644 --- a/app/code/Magento/Config/Test/Unit/Model/Config/Structure/Element/IteratorTest.php +++ b/app/code/Magento/Config/Test/Unit/Model/Config/Structure/Element/IteratorTest.php @@ -17,24 +17,12 @@ class IteratorTest extends \PHPUnit\Framework\TestCase */ protected $_flyweightMock; - /** - * @var \PHPUnit_Framework_MockObject_MockObject - */ - protected $_loggerMock; - - /** - * @var \PHPUnit_Framework_MockObject_MockObject - */ - protected $_stateMock; - protected function setUp() { $elementData = ['group1' => ['id' => 1], 'group2' => ['id' => 2], 'group3' => ['id' => 3]]; $this->_flyweightMock = $this->createMock(\Magento\Config\Model\Config\Structure\Element\Group::class); - $this->_loggerMock = $this->createMock(\Psr\Log\LoggerInterface::class); - $this->_stateMock = $this->createMock(\Magento\Framework\App\State::class); - $this->_model = new \Magento\Config\Model\Config\Structure\Element\Iterator($this->_flyweightMock, $this->_loggerMock, $this->_stateMock); + $this->_model = new \Magento\Config\Model\Config\Structure\Element\Iterator($this->_flyweightMock); $this->_model->setElements($elementData, 'scope'); } @@ -80,19 +68,6 @@ public function testIsLast($elementId, $result) $this->assertEquals($result, $this->_model->isLast($elementMock)); } - /** - * Test whether an element without an id will throw the expected exception and message when in developer mode - * - * @expectedException \Symfony\Component\Config\Definition\Exception\InvalidConfigurationException - */ - public function testInvalidConfigThrowsExceptionInDeveloperMode() { - $this->_stateMock->expects($this->at(0))->method('getMode')->willReturn(\Magento\Framework\App\State::MODE_DEVELOPER); - $elementData = ['group1' => ['id' => 1], 'group2' => ['id' => 2], 'group3' => ['id' => 3], 'group4' => ['invalid' => "broken"]]; - $expectedMessage = "Invalid configuration element defined for element with key 'group4'"; - $this->_model->setElements($elementData, 'scope'); - $this->expectExceptionMessage($expectedMessage); - } - /** * @return array */ @@ -100,4 +75,4 @@ public function isLastDataProvider() { return [[1, false], [2, false], [3, true]]; } -} +} \ No newline at end of file From 42c0af41500559a74b678e8eb92188b0918ba9bf Mon Sep 17 00:00:00 2001 From: Nathaniel Rogers Date: Wed, 23 Jan 2019 10:47:10 +1000 Subject: [PATCH 5/5] Added Missing new line at the end of the Iterator files, completely reverted. --- .../Magento/Config/Model/Config/Structure/Element/Iterator.php | 2 +- .../Test/Unit/Model/Config/Structure/Element/IteratorTest.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/code/Magento/Config/Model/Config/Structure/Element/Iterator.php b/app/code/Magento/Config/Model/Config/Structure/Element/Iterator.php index c15a82d709960..f53ecd710b28c 100644 --- a/app/code/Magento/Config/Model/Config/Structure/Element/Iterator.php +++ b/app/code/Magento/Config/Model/Config/Structure/Element/Iterator.php @@ -148,4 +148,4 @@ public function isLast(\Magento\Config\Model\Config\Structure\ElementInterface $ { return $element->getId() == $this->_lastId; } -} \ No newline at end of file +} diff --git a/app/code/Magento/Config/Test/Unit/Model/Config/Structure/Element/IteratorTest.php b/app/code/Magento/Config/Test/Unit/Model/Config/Structure/Element/IteratorTest.php index 712541bf5de4b..dcb7a90e55290 100644 --- a/app/code/Magento/Config/Test/Unit/Model/Config/Structure/Element/IteratorTest.php +++ b/app/code/Magento/Config/Test/Unit/Model/Config/Structure/Element/IteratorTest.php @@ -75,4 +75,4 @@ public function isLastDataProvider() { return [[1, false], [2, false], [3, true]]; } -} \ No newline at end of file +}