Cover, fix and cleanup XML unserializer

lib/PHPParser/NodeVisitorAbstract.php

@@ -1,5 +1,8 @@
 <?php
 
+/**
+ * @codeCoverageIgnore
+ */
 class PHPParser_NodeVisitorAbstract implements PHPParser_NodeVisitor
 {
     public function beforeTraverse(array $nodes)    { }

lib/PHPParser/Unserializer/XML.php

@@ -16,83 +16,103 @@ class PHPParser_Unserializer_XML implements PHPParser_Unserializer
             throw new DomainException('AST root element not found');
         }
 
-        return $this->read();
+        return $this->read($this->reader->depth);
     }
 
-    protected function read() {
-        $depth = $this->reader->depth;
-        while ($this->reader->read() && $depth <= $this->reader->depth) {
+    protected function read($depthLimit, $throw = true, &$nodeFound = null) {
+        $nodeFound = true;
+        while ($this->reader->read() && $depthLimit < $this->reader->depth) {
             if (XMLReader::ELEMENT !== $this->reader->nodeType) {
                 continue;
             }
 
             if ('node' === $this->reader->prefix) {
-                $className = 'PHPParser_Node_' . $this->reader->localName;
-
-                // create the node without calling it's constructor
-                $node = unserialize(
-                    sprintf('O:%d:"%s":0:{}', strlen($className), $className)
-                );
-
-                $line = $this->reader->getAttribute('line');
-                $node->setLine(null !== $line ? $line : -1);
-
-                $docComment = $this->reader->getAttribute('docComment');
-                $node->setDocComment($docComment);
-
-                $depth2 = $this->reader->depth;
-                while ($this->reader->read() && $depth2 < $this->reader->depth) {
-                    if (XMLReader::ELEMENT !== $this->reader->nodeType) {
-                        continue;
-                    }
-
-                    if ('subNode' !== $this->reader->prefix) {
-                        throw new Exception('Expected sub node');
-                    }
-
-                    $subNodeName    = $this->reader->localName;
-                    $subNodeContent = $this->read();
-
-                    $node->$subNodeName = $subNodeContent;
-                }
-
-                return $node;
+                return $this->readNode();
             } elseif ('scalar' === $this->reader->prefix) {
-                if ('array' === $this->reader->localName) {
-                    $array = array();
-                    while ($node = $this->read()) {
-                        $array[] = $node;
-                    }
-                    return $array;
-                } elseif ('string' === $this->reader->localName) {
-                    return $this->readText();
-                } elseif ('int' === $this->reader->localName) {
-                    return (int) $this->readText();
-                } elseif ('float' === $this->reader->localName) {
-                    return (float) $this->readText();
-                } elseif ('false' === $this->reader->localName
-                          || 'true' === $this->reader->localName
-                          || 'null' === $this->reader->localName
-                ) {
-                    if ($this->reader->hasValue) {
-                        throw new Exception('false, true and null nodes cannot have a value');
-                    }
-
-                    return constant($this->reader->localName);
-                } else {
-                    throw new Exception('Unexpected scalar type');
-                }
+                return $this->readScalar();
             } else {
-                throw new Exception('Unexpected node type');
+                throw new DomainException(sprintf('Unexpected node of type "%s"', $this->reader->name));
             }
         }
+
+        $nodeFound = false;
+        if ($throw) {
+            throw new DomainException('Expected node or scalar');
+        }
     }
 
-    protected function readText() {
-        if (!$this->reader->read() || XMLReader::TEXT !== $this->reader->nodeType) {
-            throw new Exception('Expected text node');
+    protected function readNode()
+    {
+        $className = 'PHPParser_Node_' . $this->reader->localName;
+
+        // create the node without calling it's constructor
+        $node = unserialize(
+            sprintf('O:%d:"%s":0:{}', strlen($className), $className)
+        );
+
+        $line = $this->reader->getAttribute('line');
+        $node->setLine(null !== $line ? $line : -1);
+
+        $docComment = $this->reader->getAttribute('docComment');
+        $node->setDocComment($docComment);
+
+        $depthLimit = $this->reader->depth;
+        while ($this->reader->read() && $depthLimit < $this->reader->depth) {
+            if (XMLReader::ELEMENT !== $this->reader->nodeType) {
+                continue;
+            }
+
+            if ('subNode' !== $this->reader->prefix) {
+                throw new DomainException(
+                    sprintf('Expected sub node, got node of type "%s"', $this->reader->name)
+                );
+            }
+
+            $subNodeName = $this->reader->localName;
+            $subNodeContent = $this->read($this->reader->depth);
+
+            $node->$subNodeName = $subNodeContent;
         }
 
-        return $this->reader->value;
+        return $node;
+    }
+
+    protected function readScalar() {
+        switch ($name = $this->reader->localName) {
+            case 'array':
+                $depth = $this->reader->depth;
+                $array = array();
+                while (true) {
+                    $node = $this->read($depth, false, $nodeFound);
+                    if (!$nodeFound) {
+                        break;
+                    }
+                    $array[] = $node;
+                }
+                return $array;
+            case 'string':
+                return $this->reader->readString();
+            case 'int':
+                $text = $this->reader->readString();
+                if (false === $int = filter_var($text, FILTER_VALIDATE_INT)) {
+                    throw new DomainException(sprintf('"%s" is not a valid integer', $text));
+                }
+                return $int;
+            case 'float':
+                $text = $this->reader->readString();
+                if (false === $float = filter_var($text, FILTER_VALIDATE_FLOAT)) {
+                    throw new DomainException(sprintf('"%s" is not a valid float', $text));
+                }
+                return $float;
+            case 'true':
+            case 'false':
+            case 'null':
+                if (!$this->reader->isEmptyElement) {
+                    throw new DomainException(sprintf('"%s" scalar must be empty', $name));
+                }
+                return constant($name);
+            default:
+                throw new DomainException(sprintf('Unknown scalar type "%s"', $name));
+        }
     }
 }

test/PHPParser/Tests/Unserializer/XMLTest.php

@@ -2,93 +2,54 @@
 
 class PHPParser_Tests_Unserializer_XMLTest extends PHPUnit_Framework_TestCase
 {
-    /**
-     * @covers PHPParser_Unserializer_XML<extended>
-     */
-    public function testUnserialize() {
+    public function testNode() {
         $xml = <<<XML
 <?xml version="1.0" encoding="UTF-8"?>
 <AST xmlns:node="http://nikic.github.com/PHPParser/XML/node" xmlns:subNode="http://nikic.github.com/PHPParser/XML/subNode" xmlns:scalar="http://nikic.github.com/PHPParser/XML/scalar">
-  <scalar:array>
-    <node:Stmt_Function line="3" docComment="/** doc comment */">
-      <subNode:byRef>
-        <scalar:false/>
-      </subNode:byRef>
-      <subNode:params>
-        <scalar:array>
-          <node:Param line="3">
-            <subNode:name>
-              <scalar:string>a</scalar:string>
-            </subNode:name>
-            <subNode:default>
-              <node:Scalar_LNumber line="3">
-                <subNode:value>
-                  <scalar:int>0</scalar:int>
-                </subNode:value>
-              </node:Scalar_LNumber>
-            </subNode:default>
-            <subNode:type>
-              <scalar:null/>
-            </subNode:type>
-            <subNode:byRef>
-              <scalar:true/>
-            </subNode:byRef>
-          </node:Param>
-          <node:Param line="3">
-            <subNode:name>
-              <scalar:string>b</scalar:string>
-            </subNode:name>
-            <subNode:default>
-              <node:Scalar_DNumber line="3">
-                <subNode:value>
-                  <scalar:float>1</scalar:float>
-                </subNode:value>
-              </node:Scalar_DNumber>
-            </subNode:default>
-            <subNode:type>
-              <scalar:null/>
-            </subNode:type>
-            <subNode:byRef>
-              <scalar:false/>
-            </subNode:byRef>
-          </node:Param>
-        </scalar:array>
-      </subNode:params>
-      <subNode:stmts>
-        <scalar:array>
-          <node:Stmt_Echo line="4">
-            <subNode:exprs>
-              <scalar:array>
-                <node:Scalar_String line="4">
-                  <subNode:value>
-                    <scalar:string>Foo</scalar:string>
-                  </subNode:value>
-                </node:Scalar_String>
-              </scalar:array>
-            </subNode:exprs>
-          </node:Stmt_Echo>
-        </scalar:array>
-      </subNode:stmts>
-      <subNode:name>
-        <scalar:string>functionName</scalar:string>
-      </subNode:name>
-    </node:Stmt_Function>
-  </scalar:array>
+ <node:Scalar_String line="1" docComment="/** doc comment */">
+  <subNode:value>
+   <scalar:string>Test</scalar:string>
+  </subNode:value>
+ </node:Scalar_String>
 </AST>
 XML;
-        $code = <<<'CODE'
-/** doc comment */
-function functionName(&$a = 0, $b = 1.0)
-{
-    echo 'Foo';
-}
-CODE;
 
         $unserializer  = new PHPParser_Unserializer_XML;
-        $prettyPrinter = new PHPParser_PrettyPrinter_Zend;
+        $this->assertEquals(
+            new PHPParser_Node_Scalar_String('Test', 1, '/** doc comment */'),
+            $unserializer->unserialize($xml)
+        );
+    }
 
-        $stmts = $unserializer->unserialize($xml);
-        $this->assertEquals($code, $prettyPrinter->prettyPrint($stmts), '', 0, 10, true);
+    public function testScalars() {
+        $xml = <<<XML
+<?xml version="1.0" encoding="UTF-8"?>
+<AST xmlns:scalar="http://nikic.github.com/PHPParser/XML/scalar">
+ <scalar:array>
+  <scalar:array></scalar:array>
+  <scalar:array/>
+  <scalar:string>test</scalar:string>
+  <scalar:string></scalar:string>
+  <scalar:string/>
+  <scalar:int>1</scalar:int>
+  <scalar:float>1</scalar:float>
+  <scalar:float>1.5</scalar:float>
+  <scalar:true/>
+  <scalar:false/>
+  <scalar:null/>
+ </scalar:array>
+</AST>
+XML;
+        $result = array(
+            array(), array(),
+            'test', '', '',
+            1,
+            1, 1.5,
+            true, false, null
+        );
+
+        $unserializer  = new PHPParser_Unserializer_XML;
+        $this->assertEquals($result, $unserializer->unserialize($xml));
     }
 
     /**
@@ -104,4 +65,47 @@ XML;
         $unserializer = new PHPParser_Unserializer_XML;
         $unserializer->unserialize($xml);
     }
+
+    /**
+     * @dataProvider             provideTestErrors
+     * @expectedException        DomainException
+     * @expectedExceptionMessage false, true and null scalars must be empty elements
+     */
+    public function testErrors($xml, $errorMsg) {
+        $this->setExpectedException('DomainException', $errorMsg);
+
+        $xml = <<<XML
+<?xml version="1.0" encoding="UTF-8"?>
+<AST xmlns:scalar="http://nikic.github.com/PHPParser/XML/scalar"
+     xmlns:node="http://nikic.github.com/PHPParser/XML/node"
+     xmlns:subNode="http://nikic.github.com/PHPParser/XML/subNode"
+     xmlns:foo="http://nikic.github.com/PHPParser/XML/foo">
+ $xml
+</AST>
+XML;
+
+        $unserializer = new PHPParser_Unserializer_XML;
+        $unserializer->unserialize($xml);
+    }
+
+    public function provideTestErrors() {
+        return array(
+            array('<scalar:true>test</scalar:true>',   '"true" scalar must be empty'),
+            array('<scalar:false>test</scalar:false>', '"false" scalar must be empty'),
+            array('<scalar:null>test</scalar:null>',   '"null" scalar must be empty'),
+            array('<scalar:foo>bar</scalar:foo>',      'Unknown scalar type "foo"'),
+            array('<scalar:int>x</scalar:int>',        '"x" is not a valid int'),
+            array('<scalar:float>x</scalar:float>',    '"x" is not a valid float'),
+            array('',                                  'Expected node or scalar'),
+            array('<foo:bar>test</foo:bar>',           'Unexpected node of type "foo:bar"'),
+            array(
+                '<node:Scalar_String><foo:bar>test</foo:bar></node:Scalar_String>',
+                'Expected sub node, got node of type "foo:bar"'
+            ),
+            array(
+                '<node:Scalar_String><subNode:value/></node:Scalar_String>',
+                'Expected node or scalar'
+            ),
+        );
+    }
 }