Merge pull request #138 from wikimedia/xmlerrors

Handle SVG format errors

i18n/en.json

@@ -26,6 +26,7 @@
   "no-translations": "This file does not have any labels available for translation. Please pick another image.",
   "not-found": "The file you requested could not be found. Please pick another image.",
   "invalid-format": "Only SVG files are supported.",
+  "invalid-svg": "Error reading file.",
 
   "pick-another": "← pick another file",
   "view-on-commons": "View on Commons",

i18n/qqq.json

@@ -30,6 +30,7 @@
 	"no-translations": "Error message displayed for SVGs that have nothing to translate.",
 	"not-found": "Error message displayed if the requested file could not be found.",
 	"invalid-format": "Error message displayed if the requested file is not an SVG image.",
+	"invalid-svg": "Error message displayed if the requested file is an SVG image but software can't interpret it.",
 	"pick-another": "Label for backlink from the translate page to the search page.",
 	"view-on-commons": "Link to view the file on Commons. The whole phrase is linked.",
 	"opens-in-new-tab": "Tooltip text for links that will open in a new browser tab.",

public/assets/i18n/app/en.json

@@ -26,6 +26,7 @@
   "no-translations": "This file does not have any labels available for translation. Please pick another image.",
   "not-found": "The file you requested could not be found. Please pick another image.",
   "invalid-format": "Only SVG files are supported.",
+  "invalid-svg": "Error reading file.",
 
   "pick-another": "← pick another file",
   "view-on-commons": "View on Commons",

public/assets/i18n/app/qqq.json

@@ -30,6 +30,7 @@
 	"no-translations": "Error message displayed for SVGs that have nothing to translate.",
 	"not-found": "Error message displayed if the requested file could not be found.",
 	"invalid-format": "Error message displayed if the requested file is not an SVG image.",
+	"invalid-svg": "Error message displayed if the requested file is an SVG image but software can't interpret it.",
 	"pick-another": "Label for backlink from the translate page to the search page.",
 	"view-on-commons": "Link to view the file on Commons. The whole phrase is linked.",
 	"opens-in-new-tab": "Tooltip text for links that will open in a new browser tab.",

src/Controller/TranslateController.php

@@ -5,6 +5,7 @@
 
 use App\Exception\ImageNotFoundException;
 use App\Exception\InvalidFormatException;
+use App\Exception\SvgLoadException;
 use App\Model\Svg\SvgFile;
 use App\Model\Title;
 use App\OOUI\TranslationsFieldset;
@@ -58,15 +59,15 @@ public function translate(
         try {
             $this->assertFileType($normalizedFilename);
             $path = $cache->getPath($filename);
+            $svgFile = new SvgFile($path);
         } catch (ImageNotFoundException $exception) {
             return $this->showError('not-found', $normalizedFilename);
         } catch (InvalidFormatException $exception) {
             return $this->showError('invalid-format', $normalizedFilename);
+        } catch (SvgLoadException $exception) {
+            return $this->showError('invalid-svg', $normalizedFilename);
         }
 
-
-        $svgFile = new SvgFile($path);
-
         // If there are no strings to translate, tell the user.
         //   - If they've come from the search form redirect then back there with an error.
         //   - If they've come directly to this page, just show the message here.

src/Exception/SvgLoadException.php

@@ -5,6 +5,7 @@
 namespace App\Exception;
 
 use Exception;
+use LibXMLError;
 use Throwable;
 
 /**
@@ -15,9 +16,11 @@ class SvgLoadException extends Exception
 {
     public function __construct(string $message = '', ?Throwable $previous = null)
     {
-        $error = error_get_last();
-        if ($error && isset($error['message'])) {
-            $message = $error['message'];
+        if ('' === $message) {
+            $errors = libxml_get_errors();
+            $message = implode("\n", array_map(function(LibXMLError $e) {
+                return trim($e->message) . ' in ' . basename($e->file) . " line {$e->line}";
+            }, $errors));
         }
         parent::__construct($message, 0, $previous);
     }

src/Model/Svg/SvgFile.php

@@ -66,6 +66,8 @@ public function __construct(string $path)
 
         $this->document = new DOMDocument('1.0');
 
+        libxml_use_internal_errors(true);
+        libxml_clear_errors();
         // Warnings need to be suppressed in case there are DOM warnings
         if (!$this->document->load($path, LIBXML_NOWARNING)) {
             throw new SvgLoadException();

tests/Model/Svg/SvgFileTest.php

@@ -9,6 +9,7 @@
 
 namespace App\Tests\Model\Svg;
 
+use App\Exception\SvgLoadException;
 use App\Model\Svg\SvgFile;
 use DOMDocument;
 use PHPUnit\Framework\TestCase;
@@ -560,4 +561,11 @@ public function testChildOnly(): void
         // Dummy assertion to avoid this test being marked as risky; the measure of success here is no crash.
         static::assertTrue(true);
     }
+
+    public function testInvalidFileHandling(): void
+    {
+        self::expectException(SvgLoadException::class);
+        self::expectExceptionMessage("Start tag expected, '<' not found in invalid.svg line 1");
+        $this->getSvg('invalid.svg');
+    }
 }