Merge pull request #213 from cleverage/202

#202 - Fix XmlReaderTask

Author: njoubert-cleverage

CHANGELOG.md

@@ -12,6 +12,7 @@ Latest
 * [#198](https://github.com/cleverage/process-bundle/issues/198) Fix DateParserTransformer: a `\DateTimeImmutable` input is converted to a `\DateTime` instead of throwing a `TypeError`. Update documentation, add tests.
 * [#199](https://github.com/cleverage/process-bundle/issues/199) Fix PregFilterTransformer: an array `replacement` is no longer cast to the string `"Array"`, and requires an array `pattern`. Update documentation, add tests.
 * [#200](https://github.com/cleverage/process-bundle/issues/200) Fix GenericTransformer: a contextual option declared with `required: false` and no default can now be used; its placeholders are replaced by `null` when omitted. Update documentation, add tests.
+* [#202](https://github.com/cleverage/process-bundle/issues/202) Fix XmlReaderTask: throw an explicit `\UnexpectedValueException` on an empty file or invalid XML (with the libxml error messages) instead of a `ValueError` or a silent empty `\DOMDocument`. Update documentation, add tests.
 
 ## Deprecated
 * [#189](https://github.com/cleverage/process-bundle/issues/189) EventDispatcherTask: when `event_name` is set, listening to `CleverAge\ProcessBundle\Event\EventDispatcherTaskEvent` is deprecated (the event is still dispatched under its class name, with an `E_USER_DEPRECATED` error, if it has listeners). Listen to the configured `event_name` instead: the BC layer will be removed in v6.0.

docs/reference/tasks/xml_reader_task.md

@@ -43,6 +43,7 @@ read_xml:
 Notes
 -----
 
-* The result of `loadXML()` is not checked: an invalid XML content raises libxml warnings (which may be converted to
-  exceptions by the Symfony error handler) and produces an empty or partial document.
-* An empty file raises a `\ValueError` (the file content is read with `fread()` using the file size as length).
+* An empty file throws an `\UnexpectedValueException`.
+* An invalid XML content (not well-formed, undefined namespace prefix...) throws an `\UnexpectedValueException` whose
+  message contains the libxml error(s) with their line and column. libxml warnings are tolerated. The libxml internal
+  errors setting is restored after loading.

src/Filesystem/XmlFile.php

@@ -27,12 +27,39 @@ public function __construct(string $path, string $mode = 'rb')
 
     public function read(): \DOMDocument
     {
-        $dom = new \DOMDocument();
         $this->file->rewind();
-        $fileSize = $this->file->getSize();
+        // fstat() on the open handle, as getSize() may return a stale size from the stat cache
+        $fileSize = $this->file->fstat()['size'];
+        if (0 === $fileSize) {
+            throw new \UnexpectedValueException(\sprintf('XML file "%s" is empty', $this->file->getPathname()));
+        }
+
         $fileContent = $this->file->fread($fileSize);
+        if (false === $fileContent) {
+            throw new \RuntimeException(\sprintf('Could not read content from XML file "%s"', $this->file->getPathname()));
+        }
 
-        $dom->loadXML($fileContent);
+        $dom = new \DOMDocument();
+        $previousUseErrors = libxml_use_internal_errors(true);
+        libxml_clear_errors();
+        try {
+            $loaded = $dom->loadXML($fileContent);
+            $errors = libxml_get_errors();
+        } finally {
+            libxml_clear_errors();
+            libxml_use_internal_errors($previousUseErrors);
+        }
+
+        // Warnings are tolerated, errors (e.g. undefined namespace prefix) and fatal errors are not
+        $errors = array_filter($errors, static fn (\LibXMLError $error): bool => \LIBXML_ERR_WARNING !== $error->level);
+        if (!$loaded || [] !== $errors) {
+            $messages = array_map(
+                static fn (\LibXMLError $error): string => \sprintf('%s (line %d, column %d)', trim($error->message), $error->line, $error->column),
+                $errors,
+            );
+
+            throw new \UnexpectedValueException(\sprintf('Invalid XML in file "%s": %s', $this->file->getPathname(), [] !== $messages ? implode('; ', $messages) : 'unknown error'));
+        }
 
         return $dom;
     }

tests/Filesystem/XmlFileTest.php

@@ -0,0 +1,118 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of the CleverAge/ProcessBundle package.
+ *
+ * Copyright (c) Clever-Age
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace CleverAge\ProcessBundle\Tests\Filesystem;
+
+use CleverAge\ProcessBundle\Filesystem\XmlFile;
+use PHPUnit\Framework\TestCase;
+
+#[\PHPUnit\Framework\Attributes\CoversClass(XmlFile::class)]
+class XmlFileTest extends TestCase
+{
+    private string $path;
+
+    protected function setUp(): void
+    {
+        $path = tempnam(sys_get_temp_dir(), 'xml_file_test_');
+        self::assertIsString($path);
+        $this->path = $path;
+    }
+
+    protected function tearDown(): void
+    {
+        if (is_file($this->path)) {
+            unlink($this->path);
+        }
+    }
+
+    public function testReadValidXml(): void
+    {
+        file_put_contents($this->path, '<root><a>1</a></root>');
+
+        $dom = (new XmlFile($this->path))->read();
+
+        self::assertNotNull($dom->documentElement);
+        self::assertSame('root', $dom->documentElement->nodeName);
+        self::assertSame('1', $dom->documentElement->textContent);
+    }
+
+    public function testReadEmptyFileThrows(): void
+    {
+        file_put_contents($this->path, '');
+
+        $this->expectException(\UnexpectedValueException::class);
+        $this->expectExceptionMessage(\sprintf('XML file "%s" is empty', $this->path));
+
+        (new XmlFile($this->path))->read();
+    }
+
+    public function testReadMalformedXmlThrowsWithLibxmlMessage(): void
+    {
+        file_put_contents($this->path, '<root><a>1</a>');
+
+        try {
+            (new XmlFile($this->path))->read();
+            self::fail('An exception should have been thrown');
+        } catch (\UnexpectedValueException $e) {
+            self::assertStringContainsString(\sprintf('Invalid XML in file "%s"', $this->path), $e->getMessage());
+            self::assertStringContainsString('Premature end of data in tag root', $e->getMessage());
+        }
+    }
+
+    public function testReadNotXmlThrows(): void
+    {
+        file_put_contents($this->path, 'not xml at all');
+
+        $this->expectException(\UnexpectedValueException::class);
+        $this->expectExceptionMessage("Start tag expected, '<' not found");
+
+        (new XmlFile($this->path))->read();
+    }
+
+    public function testReadUndefinedNamespacePrefixThrows(): void
+    {
+        file_put_contents($this->path, '<root><x:a/></root>');
+
+        $this->expectException(\UnexpectedValueException::class);
+        $this->expectExceptionMessage('Namespace prefix x on a is not defined');
+
+        (new XmlFile($this->path))->read();
+    }
+
+    public function testReadRestoresLibxmlErrorHandling(): void
+    {
+        file_put_contents($this->path, '<root>');
+        $previous = libxml_use_internal_errors(false);
+
+        try {
+            (new XmlFile($this->path))->read();
+            self::fail('An exception should have been thrown');
+        } catch (\UnexpectedValueException) {
+            self::assertFalse(libxml_use_internal_errors());
+            self::assertSame([], libxml_get_errors());
+        } finally {
+            libxml_use_internal_errors($previous);
+        }
+    }
+
+    public function testWriteThenRead(): void
+    {
+        $dom = new \DOMDocument();
+        $dom->loadXML('<root><a>ok</a></root>');
+
+        (new XmlFile($this->path, 'wb'))->write($dom);
+        $read = (new XmlFile($this->path))->read();
+
+        self::assertSame('ok', $read->documentElement?->textContent);
+    }
+}