Merge remote-tracking branch 'ogresce/MAGETWO-31422-file-xml-syntax-error-handling' into develop

Author: mazhalai

dev/tests/unit/testsuite/Magento/Framework/Module/ModuleList/LoaderTest.php

@@ -7,6 +7,7 @@
 namespace Magento\Framework\Module\ModuleList;
 
 use Magento\Framework\App\Filesystem\DirectoryList;
+use Magento\Framework\Xml\Parser;
 
 class LoaderTest extends \PHPUnit_Framework_TestCase
 {
@@ -32,6 +33,11 @@ class LoaderTest extends \PHPUnit_Framework_TestCase
      */
     private $converter;
 
+    /*
+     * @var \PHPUnit_Framework_MockObject_MockObject
+     */
+    private $parser;
+
     protected function setUp()
     {
         $this->filesystem = $this->getMock('Magento\Framework\Filesystem', [], [], '', false);
@@ -41,6 +47,7 @@ protected function setUp()
             ->with(DirectoryList::MODULES)
             ->willReturn($this->dir);
         $this->converter = $this->getMock('Magento\Framework\Module\Declaration\Converter\Dom', [], [], '', false);
+        $this->parser = $this->getMock('Magento\Framework\Xml\Parser', [], [], '', false);
     }
 
     public function testLoad()
@@ -57,11 +64,13 @@ public function testLoad()
             ['b', null, null, self::$sampleXml],
             ['c', null, null, self::$sampleXml],
         ]));
-        $dom = new \PHPUnit_Framework_Constraint_IsInstanceOf('DOMDocument');
-        $this->converter->expects($this->at(0))->method('convert')->with($dom)->willReturn(['a' => $fixture['a']]);
-        $this->converter->expects($this->at(1))->method('convert')->with($dom)->willReturn(['b' => $fixture['b']]);
-        $this->converter->expects($this->at(2))->method('convert')->with($dom)->willReturn(['c' => $fixture['c']]);
-        $object = new Loader($this->filesystem, $this->converter);
+        $this->converter->expects($this->at(0))->method('convert')->willReturn(['a' => $fixture['a']]);
+        $this->converter->expects($this->at(1))->method('convert')->willReturn(['b' => $fixture['b']]);
+        $this->converter->expects($this->at(2))->method('convert')->willReturn(['c' => $fixture['c']]);
+        $this->parser->expects($this->once())->method('initErrorHandler');
+        $this->parser->expects($this->atLeastOnce())->method('loadXML');
+        $this->parser->expects($this->atLeastOnce())->method('getDom');
+        $object = new Loader($this->filesystem, $this->converter, $this->parser);
         $result = $object->load();
         $this->assertSame(['a', 'c', 'b'], array_keys($result));
         $this->assertSame($fixture['a'], $result['a']);
@@ -84,10 +93,9 @@ public function testLoadCircular()
             ['a', null, null, self::$sampleXml],
             ['b', null, null, self::$sampleXml],
         ]));
-        $dom = new \PHPUnit_Framework_Constraint_IsInstanceOf('DOMDocument');
-        $this->converter->expects($this->at(0))->method('convert')->with($dom)->willReturn(['a' => $fixture['a']]);
-        $this->converter->expects($this->at(1))->method('convert')->with($dom)->willReturn(['b' => $fixture['b']]);
-        $object = new Loader($this->filesystem, $this->converter);
+        $this->converter->expects($this->at(0))->method('convert')->willReturn(['a' => $fixture['a']]);
+        $this->converter->expects($this->at(1))->method('convert')->willReturn(['b' => $fixture['b']]);
+        $object = new Loader($this->filesystem, $this->converter, $this->parser);
         $object->load();
     }
 }

dev/tests/unit/testsuite/Magento/Framework/Xml/ParserTest.php

@@ -31,4 +31,15 @@ public function testGetXml()
             $this->parser->load(__DIR__ . '/_files/data.xml')->xmlToArray()
         );
     }
+
+    /**
+     * @expectedException \Magento\Framework\Exception
+     * @expectedExceptionMessage DOMDocument::loadXML(): Opening and ending tag mismatch
+     */
+    public function testLoadXmlInvalid()
+    {
+        $sampleInvalidXml = '<?xml version="1.0"?><config></onfig>';
+        $this->parser->initErrorHandler();
+        $this->parser->loadXML($sampleInvalidXml);
+    }
 }

lib/internal/Magento/Framework/Module/ModuleList/Loader.php

@@ -9,6 +9,7 @@
 use Magento\Framework\App\Filesystem\DirectoryList;
 use Magento\Framework\Filesystem;
 use Magento\Framework\Module\Declaration\Converter\Dom;
+use Magento\Framework\Xml\Parser;
 
 /**
  * Loader of module list information from the filesystem
@@ -29,21 +30,32 @@ class Loader
      */
     private $converter;
 
+    /**
+     * Parser
+     *
+     * @var \Magento\Framework\Xml\Parser
+     */
+    private $parser;
+
     /**
      * Constructor
      *
      * @param Filesystem $filesystem
      * @param Dom $converter
+     * @param Parser $parser
      */
-    public function __construct(Filesystem $filesystem, Dom $converter)
+    public function __construct(Filesystem $filesystem, Dom $converter, Parser $parser)
     {
         $this->filesystem = $filesystem;
         $this->converter = $converter;
+        $this->parser = $parser;
+        $this->parser->initErrorHandler();
     }
 
     /**
      * Loads the full module list information
      *
+     * @throws \Magento\Framework\Exception
      * @return array
      */
     public function load()
@@ -52,9 +64,18 @@ public function load()
         $dir = $this->filesystem->getDirectoryRead(DirectoryList::MODULES);
         foreach ($dir->search('*/*/etc/module.xml') as $file) {
             $contents = $dir->readFile($file);
-            $dom = new \DOMDocument();
-            $dom->loadXML($contents);
-            $data = $this->converter->convert($dom);
+
+            try {
+                $this->parser->loadXML($contents);
+            } catch (\Magento\Framework\Exception $e) {
+                throw new \Magento\Framework\Exception(
+                    'Invalid Document: ' . $file . PHP_EOL . ' Error: ' . $e->getMessage(),
+                    $e->getCode(),
+                    $e
+                );
+            }
+
+            $data = $this->converter->convert($this->parser->getDom());
             $name = key($data);
             $result[$name] = $data[$name];
         }

lib/internal/Magento/Framework/Xml/Parser.php

@@ -5,6 +5,8 @@
  */
 namespace Magento\Framework\Xml;
 
+use \Magento\Framework\Exception;
+
 class Parser
 {
     /**
@@ -22,6 +24,11 @@ class Parser
      */
     protected $_content = [];
 
+    /**
+     * @var boolean
+     */
+    protected $errorHandlerIsActive = false;
+
     /**
      *
      */
@@ -32,6 +39,16 @@ public function __construct()
         return $this;
     }
 
+    /**
+     * Initializes error handler
+     *
+     * @return void
+     */
+    public function initErrorHandler()
+    {
+        $this->errorHandlerIsActive = true;
+    }
+
     /**
      * @return \DOMDocument|null
      */
@@ -135,7 +152,39 @@ public function load($file)
      */
     public function loadXML($string)
     {
-        $this->getDom()->loadXML($string);
+        if ($this->errorHandlerIsActive) {
+            set_error_handler([$this, 'errorHandler']);
+        }
+
+        try {
+            $this->getDom()->loadXML($string);
+        } catch (\Magento\Framework\Exception $e) {
+            restore_error_handler();
+            throw new \Magento\Framework\Exception($e->getMessage(), $e->getCode(), $e);
+        }
+
+        if ($this->errorHandlerIsActive) {
+            restore_error_handler();
+        }
+
         return $this;
     }
+
+    /**
+     * Custom XML lib error handler
+     *
+     * @param int $errorNo
+     * @param string $errorStr
+     * @param string $errorFile
+     * @param int $errorLine
+     * @throws \Magento\Framework\Exception
+     * @return void
+     */
+    public function errorHandler($errorNo, $errorStr, $errorFile, $errorLine)
+    {
+        if ($errorNo != 0) {
+            $message = "{$errorStr} in {$errorFile} on line {$errorLine}";
+            throw new \Magento\Framework\Exception($message);
+        }
+    }
 }