Merge pull request #657 from wikimedia/multiple-same-lang

Fail earlier for multiple-text-same-lang structure

i18n/en.json

@@ -30,7 +30,7 @@
 
   "structure-error-no-doc-element": "No document element found.",
   "structure-error-nested-tspans-not-supported": "This file can not be translated because it contains nested tspan elements in $1.",
-  "structure-error-multiple-text-same-lang": "Multiple text elements found with language '$1'",
+  "structure-error-multiple-text-same-lang": "Multiple text elements found with language code '$2' (in element with ID '$1').",
   "structure-error-contains-tref": "This file contains tref tags, which are not supported by this tool.",
   "structure-error-css-too-complex": "This file contains CSS that is too complicated to parse.",
   "structure-error-css-has-ids": "This file uses element IDs in the CSS, which may break when SVG Translate adds new IDs. It should use classes instead, if possible.",

i18n/qqq.json

@@ -43,7 +43,7 @@
 	"structure-error-css-has-ids": "Message displayed when an SVG file has an error that renders it untranslatable.",
 	"structure-error-unexpected-node-in-text": "Message displayed when an SVG file has an error that renders it untranslatable.",
 	"structure-error-invalid-node-id": "Message displayed when an SVG file has an error that renders it untranslatable.\n\nParameters:\n* $1 — Closest parent element ID.",
-	"structure-error-text-contains-dollar": "Message displayed when an SVG file has an error that renders it untranslatable.\n\nParameters:\n* $1 — Closest parent element ID.",
+	"structure-error-text-contains-dollar": "Message displayed when an SVG file has an error that renders it untranslatable.\n\nParameters:\n* $1 — Closest parent element ID.\n* $2 — Offending text content.",
 	"structure-error-non-tspan-inside-text": "Message displayed when an SVG file has an error that renders it untranslatable.",
 	"structure-error-switch-text-is-not-node": "Message displayed when an SVG file has an error that renders it untranslatable.",
 	"structure-error-switch-text-content-outside-text": "Message displayed when an SVG file has an error that renders it untranslatable.",

src/Controller/TranslateController.php

@@ -80,9 +80,10 @@ public function translate(
             return $this->showError('network-error', $normalizedFilename);
         } catch (SvgStructureException $exception) {
             $msgParams = $exception->getMessageParams();
-            // Get the element ID, or fall back to the no-ID placeholder message.
-            $id = $exception->getClosestId();
-            array_unshift($msgParams, $id ?: $intuition->msg('structure-error-no-id'));
+            // If there's no element ID fall back to the no-ID placeholder message.
+            if ($msgParams[0] === null) {
+                $msgParams[0] = $intuition->msg('structure-error-no-id');
+            }
             return $this->showError(
                 $exception->getMessage(),
                 $normalizedFilename,

src/Model/Svg/SvgFile.php

@@ -274,10 +274,11 @@ protected function makeTranslationReady(): void
             }
         }
 
-        $textLength = $this->document->getElementsByTagName('text')->length;
+        $texts = $this->document->getElementsByTagName('text');
+        $textLength = $texts->length;
         for ($i = 0; $i < $textLength; $i++) {
             /** @var DOMElement $text */
-            $text = $this->document->getElementsByTagName('text')->item($i);
+            $text = $texts->item($i);
 
             // Text strings like $1, $2 will cause problems later because
             // self::replaceIndicesRecursive() will try to replace them
@@ -325,6 +326,7 @@ protected function makeTranslationReady(): void
         for ($i = 0; $i < $switchLength; $i++) {
             $switch = $this->document->getElementsByTagName('switch')->item($i);
             $siblings = $switch->childNodes;
+            $existingLangs = [];
             foreach ($siblings as $sibling) {
                 /** @var DOMElement $sibling */
 
@@ -344,6 +346,14 @@ protected function makeTranslationReady(): void
                     throw new SvgStructureException('structure-error-switch-child-not-text', $sibling);
                 }
 
+                // Make sure there's not more than one text element with the same lang.
+                $textLang = $sibling->getAttribute('systemLanguage');
+                if ( in_array( $textLang, $existingLangs ) ) {
+                    throw new SvgStructureException('structure-error-multiple-text-same-lang', $switch, [$textLang]);
+                }
+                $existingLangs[] = $textLang;
+
+                // Check for comma-separated language codes (and reject the file if any duplicates are found).
                 $language = $sibling->hasAttribute('systemLanguage') ?
                     $sibling->getAttribute('systemLanguage') : 'fallback';
                 $realLangs = preg_split('/, */', $language);
@@ -616,8 +626,9 @@ public function switchToTranslationSet(array $translations): array
                     // No matching text node for this language, so we'll create one
                     $switch->appendChild($newTextTag);
                 } else {
-                    // If there is more than existing text element in a switch (with the given lang), give up on this file.
-                    throw new SvgStructureException('multiple-text-same-lang', $switch, [$language]);
+                    // If there is more than one existing text element in a switch (with the given lang), give up on this file.
+                    // This should never happen, because this same check is done in makeTranslationReady().
+                    throw new SvgStructureException('structure-error-multiple-text-same-lang', $switch, [$language]);
                 }
 
                 // To have got this far, we must have either updated or started a new language

tests/Model/Svg/SvgFileTest.php

@@ -774,7 +774,7 @@ public function testAddsTextToSwitch() {
                 . '</svg>');
                 $svgFile3->setTranslations('la', ['trsvg3' => 'lang la (new)']);
         } catch (SvgStructureException $exception) {
-            $this->assertSame('multiple-text-same-lang', $exception->getMessage());
+            $this->assertSame('structure-error-multiple-text-same-lang', $exception->getMessage());
             $this->assertSame(['testswitch', 'la'], $exception->getMessageParams());
         }
     }