diff --git a/src/Controller/ApiController.php b/src/Controller/ApiController.php index b8d328eb..c3bcb35b 100644 --- a/src/Controller/ApiController.php +++ b/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); diff --git a/src/Controller/TranslateController.php b/src/Controller/TranslateController.php index e057fc8c..c182b3db 100644 --- a/src/Controller/TranslateController.php +++ b/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) { diff --git a/src/Model/Svg/SvgFile.php b/src/Model/Svg/SvgFile.php index d8ec6b4b..9e4b3748 100644 --- a/src/Model/Svg/SvgFile.php +++ b/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 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 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 @@ -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; diff --git a/src/Service/SvgFileFactory.php b/src/Service/SvgFileFactory.php new file mode 100644 index 00000000..045e0704 --- /dev/null +++ b/src/Service/SvgFileFactory.php @@ -0,0 +1,34 @@ +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); + } +} diff --git a/tests/Controller/ApiControllerTest.php b/tests/Controller/ApiControllerTest.php index ce622d2d..6c0b49d9 100644 --- a/tests/Controller/ApiControllerTest.php +++ b/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; diff --git a/tests/Service/RendererTest.php b/tests/Service/RendererTest.php index b27e924e..33e566ea 100644 --- a/tests/Service/RendererTest.php +++ b/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'); } }