Merge pull request #188 from wikimedia/logging

Add logging of SvgFile issues

src/Controller/ApiController.php

@@ -7,6 +7,9 @@
 use App\Model\Title;
 use App\Service\FileCache;
 use App\Service\Renderer;
+use App\Service\SvgFileFactory;
+use Psr\Log\LoggerAwareTrait;
+use Psr\Log\LoggerInterface;
 use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
 use Symfony\Component\HttpFoundation\BinaryFileResponse;
 use Symfony\Component\HttpFoundation\JsonResponse;
@@ -20,17 +23,20 @@
  */
 class ApiController extends AbstractController
 {
-
     /** @var FileCache */
     private $cache;
 
     /** @var Renderer */
     protected $svgRenderer;
 
-    public function __construct(FileCache $cache, Renderer $svgRenderer)
+    /** @var SvgFileFactory */
+    protected $svgFactory;
+
+    public function __construct(FileCache $cache, Renderer $svgRenderer, SvgFileFactory $svgFactory)
     {
         $this->cache = $cache;
         $this->svgRenderer = $svgRenderer;
+        $this->svgFactory = $svgFactory;
     }
 
     /**
@@ -80,7 +86,7 @@ public function requestFileWithTranslations(string $filename, string $lang, Requ
         // receiving one language at a time).
         $filename = Title::normalize($filename);
         $path = $this->cache->getPath($filename);
-        $file = new SvgFile($path);
+        $file = $this->svgFactory->create($path);
         $translations = $request->request->all();
         $file->setTranslations($lang, $translations);
 
@@ -132,7 +138,7 @@ public function getTranslations(string $fileName): Response
     {
         $fileName = Title::normalize($fileName);
         $path = $this->cache->getPath($fileName);
-        $file = new SvgFile($path);
+        $file = $this->svgFactory->create($path);
 
         return $this->json($file->getInFileTranslations());
     }
@@ -147,7 +153,7 @@ public function getLanguages(string $fileName): Response
     {
         $fileName = Title::normalize($fileName);
         $path = $this->cache->getPath($fileName);
-        $file = new SvgFile($path);
+        $file = $this->svgFactory->create($path);
         $langs = $file->getSavedLanguages();
         sort($langs);
 

src/Controller/TranslateController.php

@@ -10,6 +10,7 @@
 use App\Model\Title;
 use App\OOUI\TranslationsFieldset;
 use App\Service\FileCache;
+use App\Service\SvgFileFactory;
 use App\Service\Uploader;
 use GuzzleHttp\Exception\RequestException;
 use Krinkle\Intuition\Intuition;
@@ -28,6 +29,12 @@
 
 class TranslateController extends AbstractController
 {
+    protected $svgFactory;
+
+    public function __construct(SvgFileFactory $svgFactory)
+    {
+        $this->svgFactory = $svgFactory;
+    }
 
     /**
      * The translate page.
@@ -60,7 +67,7 @@ public function translate(
         try {
             $this->assertFileType($normalizedFilename);
             $path = $cache->getPath($filename);
-            $svgFile = new SvgFile($path);
+            $svgFile = $this->svgFactory->create($path);
         } catch (ImageNotFoundException $exception) {
             return $this->showError('not-found', $normalizedFilename);
         } catch (InvalidFormatException $exception) {

src/Model/Svg/SvgFile.php

@@ -16,9 +16,19 @@
 use DOMNode;
 use DOMXpath;
 use Krinkle\Intuition\Intuition;
+use Psr\Log\LoggerAwareTrait;
+use Psr\Log\LoggerInterface;
+use Psr\Log\NullLogger;
 
 class SvgFile
 {
+    use LoggerAwareTrait;
+
+    /**
+     * @var string
+     */
+    private $fileName;
+
     /**
      * @var DOMDocument
      */
@@ -54,14 +64,22 @@ class SvgFile
      */
     private $fallbackLanguage;
 
+    /**
+     * @var bool[]
+     */
+    private $usedMessages = [];
+
     /**
      * Construct an SvgFile object.
      *
      * @param string $path
+     * @param LoggerInterface|null $logger
      * @throws SvgLoadException
      */
-    public function __construct(string $path)
+    public function __construct(string $path, ?LoggerInterface $logger = null)
     {
+        $this->fileName = $path;
+        $this->logger = $logger ?? new NullLogger();
         $this->fallbackLanguage = 'fallback';
 
         $this->document = new DOMDocument('1.0');
@@ -80,6 +98,22 @@ public function __construct(string $path)
         $this->makeTranslationReady();
     }
 
+    /**
+     * Logs issues with the file currently being processed
+     *
+     * @param string $message
+     * @param mixed[] $extraOptions
+     */
+    private function logFileProblem(string $message, array $extraOptions = []): void
+    {
+        if (isset($this->usedMessages[$message])) {
+            return;
+        }
+        $this->usedMessages[$message] = true;
+        $options = ['file' => basename($this->fileName)] + $extraOptions;
+        $this->logger->warning("SvgFile: $message", $options);
+    }
+
     /**
      * Was the file successfully made translation ready i.e. is it translatable?
      *
@@ -106,6 +140,7 @@ protected function makeTranslationReady(): bool
 
         if (null === $this->document->documentElement) {
             // Empty or malformed file
+            $this->logFileProblem('File {file} looks malformed');
             return false;
         }
 
@@ -126,6 +161,7 @@ protected function makeTranslationReady(): bool
         $textLength = $texts->length;
         if (0 === $textLength) {
             // Nothing to translate!
+            $this->logFileProblem('File {file} has nothing to translate');
             return false;
         }
 
@@ -137,12 +173,14 @@ protected function makeTranslationReady(): bool
             if (false !== strpos($CSS, '#')) {
                 if (!preg_match('/^([^{]+\{[^}]*\})*[^{]+$/', $CSS)) {
                     // Can't easily understand the CSS to check it, so exit
+                    $this->logFileProblem('File {file} has CSS too complex to parse');
                     return false;
                 }
                 $selectors = preg_split('/\{[^}]+\}/', $CSS);
                 foreach ($selectors as $selector) {
                     if (false !== strpos($selector, '#')) {
                         // IDs in CSS will break when we clone things, should be classes
+                        $this->logFileProblem('File {file} has IDs in CSS');
                         return false;
                     }
                 }
@@ -151,6 +189,7 @@ protected function makeTranslationReady(): bool
 
         if (0 !== $this->document->getElementsByTagName('tref')->length) {
             // Tref tags not (yet) supported
+            $this->logFileProblem('File {file} has <tref> tags');
             return false;
         }
 
@@ -163,6 +202,7 @@ protected function makeTranslationReady(): bool
             if ($tspan->childNodes->length > 1
                 || ( 1 == $tspan->childNodes->length && XML_TEXT_NODE !== $tspan->childNodes->item(0)->nodeType )
             ) {
+                $this->logFileProblem('File {file} has nested <tspan> tags');
                 return false; // Nested tspans not (yet) supported
             }
             $translatableNodes[] = $tspan;
@@ -178,10 +218,12 @@ protected function makeTranslationReady(): bool
                 }
                 if (XML_TEXT_NODE !== $node->nodeType) {
                     // Anything but tspans and text nodes is unexpected
+                    $this->logFileProblem('File {file} has an unexpected tag in translatable content');
                     return false;
                 }
                 if ('' === trim($node->nodeValue)) {
                     // Don't bother with whitespace-only nodes
+                    $this->logFileProblem('File {file} has a whitespace-only text node');
                     continue;
                 }
                 // Wrap text in <tspan>
@@ -243,6 +285,7 @@ protected function makeTranslationReady(): bool
             // self::replaceIndicesRecursive() will try to replace them
             // with (non-existent) child nodes.
             if (preg_match('/$[0-9]/', $text->textContent)) {
+                $this->logFileProblem('File {file} has text with $-numbers');
                 return false;
             }
 
@@ -271,6 +314,7 @@ protected function makeTranslationReady(): bool
                     && 'svg:tspan' !== $child->nodeName
                 ) {
                     // Tags other than tspan inside text tags are not (yet) supported
+                    $this->logFileProblem('File {file} has a tag other than tspan inside a text tag');
                     return false;
                 }
             }
@@ -287,6 +331,7 @@ protected function makeTranslationReady(): bool
                 if (XML_TEXT_NODE === $sibling->nodeType) {
                     if ('' !== trim($sibling->textContent)) {
                         // Text content inside switch but outside text tags is awkward.
+                        $this->logFileProblem('File {file} has text content inside switch but outside of a text tag');
                         return false;
                     }
                     continue;
@@ -296,6 +341,9 @@ protected function makeTranslationReady(): bool
                 }
 
                 if ('text' !== $sibling->nodeName && 'svg:text' !== $sibling->nodeName) {
+                    $this->logFileProblem('Encountered unexpected element {elementName} in file {file}',
+                        ['elementName' => $sibling->nodeName]
+                    );
                     return false;
                 }
 
@@ -305,6 +353,7 @@ protected function makeTranslationReady(): bool
                 foreach ($realLangs as $realLang) {
                     if (in_array($realLang, $languagesPresent)) {
                         // Two tags for the same language
+                        $this->logFileProblem('File {file} has 2 tags');
                         return false;
                     }
                     $languagesPresent[] = $realLang;

src/Service/SvgFileFactory.php

@@ -0,0 +1,34 @@
+<?php
+
+
+namespace App\Service;
+
+
+use App\Model\Svg\SvgFile;
+use Psr\Log\LoggerInterface;
+
+/**
+ * Instantiates SvgFile objects
+ */
+class SvgFileFactory
+{
+    /** @var LoggerInterface */
+    private $logger;
+
+    public function __construct(LoggerInterface $logger)
+    {
+        $this->logger = $logger;
+    }
+
+    /**
+     * Creates an instance of SvgFile for the given path
+     *
+     * @param string $path
+     * @return SvgFile
+     * @throws \App\Exception\SvgLoadException
+     */
+    public function create(string $path): SvgFile
+    {
+        return new SvgFile($path, $this->logger);
+    }
+}

tests/Controller/ApiControllerTest.php

@@ -6,9 +6,11 @@
 use App\Controller\ApiController;
 use App\Service\FileCache;
 use App\Service\Renderer;
+use App\Service\SvgFileFactory;
 use App\Tests\Model\Svg\SvgFileTest;
 use PHPUnit\Framework\TestCase;
 use Psr\Container\ContainerInterface;
+use Psr\Log\NullLogger;
 use Symfony\Component\HttpFoundation\Request;
 
 class ApiControllerTest extends TestCase
@@ -75,8 +77,10 @@ private function makeController(): ApiController
             ->with('Foo.svg')
             ->willReturn('test content');
 
+        $factory = new SvgFileFactory(new NullLogger());
+
         /** @var FileCache $cache */
-        $controller = new ApiController($cache, new Renderer('rsvg-convert'));
+        $controller = new ApiController($cache, new Renderer('rsvg-convert'), $factory);
         $controller->setContainer($container);
 
         return $controller;

tests/Service/RendererTest.php

@@ -14,7 +14,7 @@ public function testInvalidCommand() : void
     {
         $renderer = new Renderer('foo');
         static::expectException(ProcessFailedException::class);
-        static::expectExceptionMessage('foo: not found');
+        static::expectExceptionMessageRegExp('/foo:( command)? not found/');
         $renderer->render('foo.svg', 'fr');
     }
 }