Merge pull request #216 from magento-tango/MAGETWO-52625

[Tango] MAGETWO-52625: Fail fast for layout XML errors
magento · Aug 4, 2016 · 6806aec · 6806aec
2 parents 644e60d + 88118f8
commit 6806aec
Show file tree

Hide file tree

Showing 9 changed files with 323 additions and 88 deletions.
diff --git 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
@@ -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
@@ -0,0 +1,16 @@
+<?php
+/**
+ * Copyright © 2016 Magento. All rights reserved.
+ * See COPYING.txt for license details.
+ */
+
+/**
+ * Exception that should be thrown by DOM model when incoming xsd is not valid.
+ */
+namespace Magento\Framework\Config\Dom;
+
+use Magento\Framework\Exception\LocalizedException;
+
+class ValidationSchemaException extends LocalizedException
+{
+}
diff --git a/lib/internal/Magento/Framework/Config/Test/Unit/DomTest.php b/lib/internal/Magento/Framework/Config/Test/Unit/DomTest.php
@@ -8,18 +8,14 @@
 class DomTest extends \PHPUnit_Framework_TestCase
 {
     /**
-     * @var \Magento\Framework\Config\ValidationStateInterface
+     * @var \Magento\Framework\Config\ValidationStateInterface|\PHPUnit_Framework_MockObject_MockObject
      */
     protected $validationStateMock;
 
     protected function setUp()
     {
-        $this->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 = '<root><node id="id1"/><node id="id2"/></root>';
+        $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
@@ -7,6 +7,8 @@
 -->
 <xs:schema xmlns:xs="http://www.w3.org/2001/XMLSchema">
     <xs:include schemaLocation="urn:magento:framework:View/Layout/etc/elements.xsd"/>
+    <xs:include schemaLocation="urn:magento:framework:View/Layout/etc/head.xsd"/>
+    <xs:include schemaLocation="urn:magento:framework:View/Layout/etc/body.xsd"/>
 
     <xs:element name="layout">
         <xs:annotation>
@@ -20,29 +22,31 @@
             </xs:sequence>
         </xs:complexType>
         <xs:key name="handleName">
-            <xs:selector xpath="handle"></xs:selector>
-            <xs:field xpath="@id"></xs:field>
+            <xs:selector xpath="handle"/>
+            <xs:field xpath="@id"/>
         </xs:key>
     </xs:element>
 
-    <xs:element name="handle" type="handleType">
-        <xs:unique name="blockKey">
-            <xs:selector xpath=".//block"/>
-            <xs:field xpath="@name"/>
-        </xs:unique>
-        <xs:unique name="containerKey">
-            <xs:selector xpath=".//container"/>
-            <xs:field xpath="@name"/>
-        </xs:unique>
-        <xs:keyref name="blockReference" refer="blockKey">
-            <xs:selector xpath=".//referenceBlock"/>
-            <xs:field xpath="@name"/>
-        </xs:keyref>
-        <xs:keyref name="containerReference" refer="containerKey">
-            <xs:selector xpath=".//referenceContainer"/>
-            <xs:field xpath="@name"/>
-        </xs:keyref>
-    </xs:element>
+    <xs:element name="handle" type="handleType" />
+
+    <xs:complexType name="layoutType">
+        <xs:annotation>
+            <xs:documentation>
+                Layout Type definition
+            </xs:documentation>
+        </xs:annotation>
+        <xs:choice minOccurs="0" maxOccurs="unbounded">
+            <xs:element ref="referenceContainer" minOccurs="0" maxOccurs="unbounded"/>
+            <xs:element ref="container" minOccurs="0" maxOccurs="unbounded"/>
+            <xs:element ref="update" minOccurs="0" maxOccurs="unbounded"/>
+            <xs:element ref="remove" minOccurs="0" maxOccurs="unbounded"/>
+            <xs:element ref="move" minOccurs="0" maxOccurs="unbounded"/>
+            <xs:element ref="block" minOccurs="0" maxOccurs="unbounded"/>
+            <xs:element ref="referenceBlock" minOccurs="0" maxOccurs="unbounded"/>
+            <xs:element name="body" type="bodyType" minOccurs="0" maxOccurs="unbounded"/>
+            <xs:element name="head" type="headType" minOccurs="0" maxOccurs="unbounded"/>
+        </xs:choice>
+    </xs:complexType>
 
     <xs:complexType name="handleType">
         <xs:annotation>

diff --git 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 = '<handle id="handle">' . $layout . '</handle>';
 
-        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
@@ -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,14 +107,15 @@ 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.
      *
      * @param string $value
      * @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;
     }