diff --git a/dev/tests/integration/testsuite/Magento/Framework/View/Layout/MergeTest.php b/dev/tests/integration/testsuite/Magento/Framework/View/Layout/MergeTest.php index ca7dbdb2ef273..52d72665ae934 100644 --- a/dev/tests/integration/testsuite/Magento/Framework/View/Layout/MergeTest.php +++ b/dev/tests/integration/testsuite/Magento/Framework/View/Layout/MergeTest.php @@ -5,6 +5,7 @@ */ namespace Magento\Framework\View\Layout; +use Magento\Framework\App\State; use Magento\Framework\Phrase; /** @@ -396,17 +397,23 @@ public function testLoadWithInvalidArgumentThrowsException() /** * Test loading invalid layout + * + * @expectedException \Exception + * @expectedExceptionMessage Layout is invalid. */ public function testLoadWithInvalidLayout() { $this->_model->addPageHandles(['default']); - $this->_appState->expects($this->any())->method('getMode')->will($this->returnValue('developer')); + $this->_appState->expects($this->once())->method('getMode')->willReturn(State::MODE_DEVELOPER); - $this->_layoutValidator->expects($this->any())->method('getMessages') - ->will($this->returnValue(['testMessage1', 'testMessage2'])); + $this->_layoutValidator->expects($this->any()) + ->method('getMessages') + ->willReturn(['testMessage1', 'testMessage2']); - $this->_layoutValidator->expects($this->any())->method('isValid')->will($this->returnValue(false)); + $this->_layoutValidator->expects($this->any()) + ->method('isValid') + ->willThrowException(new \Exception('Layout is invalid.')); $suffix = md5(implode('|', $this->_model->getHandles())); $cacheId = "LAYOUT_{$this->_theme->getArea()}_STORE{$this->scope->getId()}_{$this->_theme->getId()}{$suffix}"; @@ -420,4 +427,15 @@ public function testLoadWithInvalidLayout() $this->_model->load(); } + + /** + * @expectedException \Magento\Framework\Config\Dom\ValidationException + * @expectedExceptionMessageRegExp /_mergeFiles\/layout\/file_wrong\.xml\' is not valid/ + */ + public function testLayoutUpdateFileIsNotValid() + { + $this->_appState->expects($this->once())->method('getMode')->willReturn(State::MODE_DEVELOPER); + + $this->_model->addPageHandles(['default']); + } } diff --git a/lib/internal/Magento/Framework/Config/Dom.php b/lib/internal/Magento/Framework/Config/Dom.php index eacd5ba80525a..a5b92f9d57091 100644 --- a/lib/internal/Magento/Framework/Config/Dom.php +++ b/lib/internal/Magento/Framework/Config/Dom.php @@ -12,6 +12,8 @@ namespace Magento\Framework\Config; use Magento\Framework\Config\Dom\UrnResolver; +use Magento\Framework\Config\Dom\ValidationSchemaException; +use Magento\Framework\Phrase; /** * Class Dom @@ -313,8 +315,10 @@ public static function validateDomDocument( $errors = self::getXmlErrors($errorFormat); } } catch (\Exception $exception) { + $errors = self::getXmlErrors($errorFormat); libxml_use_internal_errors(false); - throw $exception; + array_unshift($errors, new Phrase('Processed schema file: %1', [$schema])); + throw new ValidationSchemaException(new Phrase(implode("\n", $errors))); } libxml_set_external_entity_loader(null); libxml_use_internal_errors(false); diff --git a/lib/internal/Magento/Framework/Config/Dom/ValidationSchemaException.php b/lib/internal/Magento/Framework/Config/Dom/ValidationSchemaException.php new file mode 100644 index 0000000000000..555c57202afaf --- /dev/null +++ b/lib/internal/Magento/Framework/Config/Dom/ValidationSchemaException.php @@ -0,0 +1,16 @@ +validationStateMock = $this->getMock( - \Magento\Framework\Config\ValidationStateInterface::class, - [], - [], - '', - false + $this->validationStateMock = $this->getMockForAbstractClass( + \Magento\Framework\Config\ValidationStateInterface::class ); $this->validationStateMock->method('isValidationRequired') ->willReturn(true); @@ -176,7 +172,29 @@ public function testValidateUnknownError() $domMock->expects($this->once()) ->method('schemaValidate') ->with($schemaFile) - ->will($this->returnValue(false)); - $this->assertEquals(['Unknown validation error'], $dom->validateDomDocument($domMock, $schemaFile)); + ->willReturn(false); + $this->assertEquals( + ["Element 'unknown_node': This element is not expected. Expected is ( node ).\nLine: 1\n"], + $dom->validateDomDocument($domMock, $schemaFile) + ); + } + + /** + * @expectedException \Magento\Framework\Config\Dom\ValidationSchemaException + */ + public function testValidateDomDocumentThrowsException() + { + if (!function_exists('libxml_set_external_entity_loader')) { + $this->markTestSkipped('Skipped on HHVM. Will be fixed in MAGETWO-45033'); + } + $xml = ''; + $schemaFile = __DIR__ . '/_files/sample.xsd'; + $dom = new \Magento\Framework\Config\Dom($xml, $this->validationStateMock); + $domMock = $this->getMock(\DOMDocument::class, ['schemaValidate'], []); + $domMock->expects($this->once()) + ->method('schemaValidate') + ->with($schemaFile) + ->willThrowException(new \Exception()); + $dom->validateDomDocument($domMock, $schemaFile); } } diff --git a/lib/internal/Magento/Framework/View/Layout/etc/layout_merged.xsd b/lib/internal/Magento/Framework/View/Layout/etc/layout_merged.xsd index 88db4682622ff..d6faf66fdc094 100644 --- a/lib/internal/Magento/Framework/View/Layout/etc/layout_merged.xsd +++ b/lib/internal/Magento/Framework/View/Layout/etc/layout_merged.xsd @@ -7,6 +7,8 @@ --> + + @@ -20,29 +22,31 @@ - - + + - - - - - - - - - - - - - - - - - - + + + + + + Layout Type definition + + + + + + + + + + + + + + diff --git a/lib/internal/Magento/Framework/View/Model/Layout/Merge.php b/lib/internal/Magento/Framework/View/Model/Layout/Merge.php index 90220320952af..ce7f4bbb1a1f8 100644 --- a/lib/internal/Magento/Framework/View/Model/Layout/Merge.php +++ b/lib/internal/Magento/Framework/View/Model/Layout/Merge.php @@ -5,9 +5,11 @@ */ namespace Magento\Framework\View\Model\Layout; +use Magento\Framework\App\State; use Magento\Framework\Filesystem\DriverPool; use Magento\Framework\Filesystem\File\ReadFactory; use Magento\Framework\View\Model\Layout\Update\Validator; +use Magento\Framework\Config\Dom\ValidationException; /** * Layout merge model @@ -201,7 +203,9 @@ public function __construct( */ public function addUpdate($update) { - $this->updates[] = $update; + if (!in_array($update, $this->updates)) { + $this->updates[] = $update; + } return $this; } @@ -447,20 +451,24 @@ public function load($handles = []) * @param string $cacheId * @param string $layout * @return $this + * @throws \Exception */ protected function _validateMergedLayout($cacheId, $layout) { $layoutStr = '' . $layout . ''; - if ($this->appState->getMode() === \Magento\Framework\App\State::MODE_DEVELOPER) { - if (!$this->layoutValidator->isValid($layoutStr, Validator::LAYOUT_SCHEMA_MERGED, false)) { - $messages = $this->layoutValidator->getMessages(); - //Add first message to exception - $message = reset($messages); - $this->logger->info( - 'Cache file with merged layout: ' . $cacheId - . ' and handles ' . implode(', ', (array)$this->getHandles()) . ': ' . $message - ); + try { + $this->layoutValidator->isValid($layoutStr, Validator::LAYOUT_SCHEMA_MERGED, false); + } catch (\Exception $e) { + $messages = $this->layoutValidator->getMessages(); + //Add first message to exception + $message = reset($messages); + $this->logger->info( + 'Cache file with merged layout: ' . $cacheId + . ' and handles ' . implode(', ', (array)$this->getHandles()) . ': ' . $message + ); + if ($this->appState->getMode() === \Magento\Framework\App\State::MODE_DEVELOPER) { + throw $e; } } @@ -693,7 +701,19 @@ protected function _loadFileLayoutUpdatesXml() /** @var $fileXml \Magento\Framework\View\Layout\Element */ $fileXml = $this->_loadXmlString($fileStr); if (!$fileXml instanceof \Magento\Framework\View\Layout\Element) { - $this->_logXmlErrors($file->getFilename(), libxml_get_errors()); + $xmlErrors = $this->getXmlErrors(libxml_get_errors()); + $this->_logXmlErrors($file->getFilename(), $xmlErrors); + if ($this->appState->getMode() === State::MODE_DEVELOPER) { + throw new ValidationException( + new \Magento\Framework\Phrase( + "Theme layout update file '%1' is not valid.\n%2", + [ + $file->getFilename(), + implode("\n", $xmlErrors) + ] + ) + ); + } libxml_clear_errors(); continue; } @@ -721,21 +741,31 @@ protected function _loadFileLayoutUpdatesXml() * Log xml errors to system log * * @param string $fileName - * @param array $libXmlErrors + * @param array $xmlErrors * @return void */ - protected function _logXmlErrors($fileName, $libXmlErrors) + protected function _logXmlErrors($fileName, $xmlErrors) + { + $this->logger->info( + sprintf("Theme layout update file '%s' is not valid.\n%s", $fileName, implode("\n", $xmlErrors)) + ); + } + + /** + * Get formatted xml errors + * + * @param array $libXmlErrors + * @return array + */ + private function getXmlErrors($libXmlErrors) { $errors = []; if (count($libXmlErrors)) { foreach ($libXmlErrors as $error) { $errors[] = "{$error->message} Line: {$error->line}"; } - - $this->logger->info( - sprintf("Theme layout update file '%s' is not valid.\n%s", $fileName, implode("\n", $errors)) - ); } + return $errors; } /** diff --git a/lib/internal/Magento/Framework/View/Model/Layout/Update/Validator.php b/lib/internal/Magento/Framework/View/Model/Layout/Update/Validator.php index b3bf3de34079d..97149cbb827a0 100644 --- a/lib/internal/Magento/Framework/View/Model/Layout/Update/Validator.php +++ b/lib/internal/Magento/Framework/View/Model/Layout/Update/Validator.php @@ -6,6 +6,7 @@ namespace Magento\Framework\View\Model\Layout\Update; use Magento\Framework\Config\Dom\UrnResolver; +use Magento\Framework\Config\Dom\ValidationSchemaException; /** * Validator for custom layout update @@ -16,6 +17,8 @@ class Validator extends \Zend_Validate_Abstract { const XML_INVALID = 'invalidXml'; + const XSD_INVALID = 'invalidXsd'; + const HELPER_ARGUMENT_TYPE = 'helperArgumentType'; const UPDATER_MODEL = 'updaterModel'; @@ -93,6 +96,9 @@ protected function _initMessageTemplates() self::XML_INVALID => (string)new \Magento\Framework\Phrase( 'Please correct the XML data and try again. %value%' ), + self::XSD_INVALID => (string)new \Magento\Framework\Phrase( + 'Please correct the XSD data and try again. %value%' + ), ]; } return $this; @@ -101,7 +107,7 @@ protected function _initMessageTemplates() /** * Returns true if and only if $value meets the validation requirements * - * If $value fails validation, then this method returns false, and + * If $value fails validation, then this method throws exception, and * getMessages() will return an array of messages that explain why the * validation failed. * @@ -109,6 +115,7 @@ protected function _initMessageTemplates() * @param string $schema * @param bool $isSecurityCheck * @return bool + * @throws \Exception */ public function isValid($value, $schema = self::LAYOUT_SCHEMA_PAGE_HANDLE, $isSecurityCheck = true) { @@ -132,10 +139,13 @@ public function isValid($value, $schema = self::LAYOUT_SCHEMA_PAGE_HANDLE, $isSe } } catch (\Magento\Framework\Config\Dom\ValidationException $e) { $this->_error(self::XML_INVALID, $e->getMessage()); - return false; + throw $e; + } catch (ValidationSchemaException $e) { + $this->_error(self::XSD_INVALID, $e->getMessage()); + throw $e; } catch (\Exception $e) { $this->_error(self::XML_INVALID); - return false; + throw $e; } return true; } diff --git a/lib/internal/Magento/Framework/View/Test/Unit/Model/Layout/MergeTest.php b/lib/internal/Magento/Framework/View/Test/Unit/Model/Layout/MergeTest.php new file mode 100644 index 0000000000000..7767fcccf1f2a --- /dev/null +++ b/lib/internal/Magento/Framework/View/Test/Unit/Model/Layout/MergeTest.php @@ -0,0 +1,95 @@ +objectManagerHelper = new ObjectManager($this); + + $this->scope = $this->getMockForAbstractClass(\Magento\Framework\Url\ScopeInterface::class); + $this->layoutValidator = $this->getMockBuilder(\Magento\Framework\View\Model\Layout\Update\Validator::class) + ->disableOriginalConstructor() + ->getMock(); + $this->logger = $this->getMockForAbstractClass(\Psr\Log\LoggerInterface::class); + $this->appState = $this->getMockBuilder(\Magento\Framework\App\State::class) + ->disableOriginalConstructor() + ->getMock(); + + $this->model = $this->objectManagerHelper->getObject( + \Magento\Framework\View\Model\Layout\Merge::class, + [ + 'scope' => $this->scope, + 'layoutValidator' => $this->layoutValidator, + 'logger' => $this->logger, + 'appState' => $this->appState + ] + ); + } + + /** + * @expectedException \Magento\Framework\Config\Dom\ValidationSchemaException + * @expectedExceptionMessage Processed schema file is not valid. + */ + public function testValidateMergedLayoutThrowsException() + { + $messages = [ + 'Please correct the XSD data and try again.' + ]; + $this->scope->expects($this->once())->method('getId')->willReturn(1); + $this->layoutValidator->expects($this->once()) + ->method('isValid') + ->willThrowException( + new ValidationSchemaException( + new Phrase('Processed schema file is not valid.') + ) + ); + $this->layoutValidator->expects($this->once()) + ->method('getMessages') + ->willReturn($messages); + $this->appState->expects($this->once()) + ->method('getMode') + ->willReturn(State::MODE_DEVELOPER); + + $this->model->load(); + } +} diff --git a/lib/internal/Magento/Framework/View/Test/Unit/Model/Layout/Update/ValidatorTest.php b/lib/internal/Magento/Framework/View/Test/Unit/Model/Layout/Update/ValidatorTest.php index 5fed45d8be458..d63ab14f29ee6 100644 --- a/lib/internal/Magento/Framework/View/Test/Unit/Model/Layout/Update/ValidatorTest.php +++ b/lib/internal/Magento/Framework/View/Test/Unit/Model/Layout/Update/ValidatorTest.php @@ -5,6 +5,7 @@ */ namespace Magento\Framework\View\Test\Unit\Model\Layout\Update; +use Magento\Framework\Phrase; use \Magento\Framework\View\Model\Layout\Update\Validator; class ValidatorTest extends \PHPUnit_Framework_TestCase @@ -12,62 +13,71 @@ class ValidatorTest extends \PHPUnit_Framework_TestCase /** * @var \Magento\Framework\TestFramework\Unit\Helper\ObjectManager */ - protected $_objectHelper; + private $_objectHelper; + + /** + * @var \Magento\Framework\Config\DomFactory|\PHPUnit_Framework_MockObject_MockObject + */ + private $domConfigFactory; + + /** + * @var \Magento\Framework\View\Model\Layout\Update\Validator|\PHPUnit_Framework_MockObject_MockObject + */ + private $model; + + /** + * @var \Magento\Framework\Config\Dom\UrnResolver|\PHPUnit_Framework_MockObject_MockObject + */ + private $urnResolver; protected function setUp() { $this->_objectHelper = new \Magento\Framework\TestFramework\Unit\Helper\ObjectManager($this); + $this->domConfigFactory = $this->getMockBuilder( + \Magento\Framework\Config\DomFactory::class + )->disableOriginalConstructor()->getMock(); + $this->urnResolver = $this->getMockBuilder( + \Magento\Framework\Config\Dom\UrnResolver::class + )->disableOriginalConstructor()->getMock(); + + $this->model = $this->_objectHelper->getObject( + \Magento\Framework\View\Model\Layout\Update\Validator::class, + ['domConfigFactory' => $this->domConfigFactory, 'urnResolver' => $this->urnResolver] + ); } /** * @param string $layoutUpdate - * @param boolean $isSchemaValid * @return Validator */ - protected function _createValidator($layoutUpdate, $isSchemaValid = true) + protected function _createValidator($layoutUpdate) { - $domConfigFactory = $this->getMockBuilder( - \Magento\Framework\Config\DomFactory::class - )->disableOriginalConstructor()->getMock(); - - $urnResolver = new \Magento\Framework\Config\Dom\UrnResolver(); $params = [ 'xml' => '' . trim($layoutUpdate) . '', - 'schemaFile' => $urnResolver->getRealPath('urn:magento:framework:View/Layout/etc/page_layout.xsd'), + 'schemaFile' => $this->urnResolver->getRealPath('urn:magento:framework:View/Layout/etc/page_layout.xsd'), ]; - $exceptionMessage = 'validation exception'; - $domConfigFactory->expects( + $this->domConfigFactory->expects( $this->once() )->method( 'createDom' )->with( $this->equalTo($params) - )->will( - $isSchemaValid ? $this->returnSelf() : $this->throwException( - new \Magento\Framework\Config\Dom\ValidationException($exceptionMessage) - ) - ); - $urnResolver = $this->_objectHelper->getObject(\Magento\Framework\Config\Dom\UrnResolver::class); - $model = $this->_objectHelper->getObject( - \Magento\Framework\View\Model\Layout\Update\Validator::class, - ['domConfigFactory' => $domConfigFactory, 'urnResolver' => $urnResolver] - ); + )->willReturnSelf(); - return $model; + return $this->model; } /** * @dataProvider testIsValidNotSecurityCheckDataProvider * @param string $layoutUpdate - * @param boolean $isValid * @param boolean $expectedResult * @param array $messages */ - public function testIsValidNotSecurityCheck($layoutUpdate, $isValid, $expectedResult, $messages) + public function testIsValidNotSecurityCheck($layoutUpdate, $expectedResult, $messages) { - $model = $this->_createValidator($layoutUpdate, $isValid); + $model = $this->_createValidator($layoutUpdate); $this->assertEquals( $expectedResult, $model->isValid( @@ -85,15 +95,7 @@ public function testIsValidNotSecurityCheck($layoutUpdate, $isValid, $expectedRe public function testIsValidNotSecurityCheckDataProvider() { return [ - ['test', true, true, []], - [ - 'test', - false, - false, - [ - Validator::XML_INVALID => 'Please correct the XML data and try again. validation exception' - ] - ] + ['test', true, []] ]; } @@ -176,4 +178,42 @@ public function testIsValidSecurityCheckDataProvider() [$secureLayout, true, []] ]; } + + /** + * @expectedException \Magento\Framework\Config\Dom\ValidationException + * @expectedExceptionMessage Please correct the XML data and try again. + */ + public function testIsValidThrowsValidationException() + { + $this->domConfigFactory->expects($this->once())->method('createDom')->willThrowException( + new \Magento\Framework\Config\Dom\ValidationException('Please correct the XML data and try again.') + ); + $this->model->isValid('test'); + } + + /** + * @expectedException \Magento\Framework\Config\Dom\ValidationSchemaException + * @expectedExceptionMessage Please correct the XSD data and try again. + */ + public function testIsValidThrowsValidationSchemaException() + { + $this->domConfigFactory->expects($this->once())->method('createDom')->willThrowException( + new \Magento\Framework\Config\Dom\ValidationSchemaException( + new Phrase('Please correct the XSD data and try again.') + ) + ); + $this->model->isValid('test'); + } + + /** + * @expectedException \Exception + * @expectedExceptionMessage Exception. + */ + public function testIsValidThrowsException() + { + $this->domConfigFactory->expects($this->once())->method('createDom')->willThrowException( + new \Exception('Exception.') + ); + $this->model->isValid('test'); + } }