Merge pull request #108 from wikimedia/borkage

Fix crashes when handling some <text>s consisting of <tspan>s
wikimedia · Apr 24, 2019 · 8f145b3 · 8f145b3
2 parents 05cdbc6 + d9e6d4d
commit 8f145b3
Show file tree

Hide file tree

Showing 4 changed files with 42 additions and 42 deletions.
diff --git a/src/Model/Svg/SvgFile.php b/src/Model/Svg/SvgFile.php
@@ -375,7 +375,6 @@ protected function analyse(): void
                 /** @var DOMElement $text */
                 $text = $actualNode->cloneNode(true);
                 $numChildren = $text->childNodes->length;
-                $hasActualTextContent = self::hasActualTextContent($text);
                 $lang = $text->hasAttribute('systemLanguage') ? $text->getAttribute('systemLanguage') : 'fallback';
                 $langCode = str_replace('_', '-', strtolower($lang));
 
@@ -407,13 +406,7 @@ protected function analyse(): void
                         $counter++;
                     }
                 }
-                if ($hasActualTextContent) {
-                    // If the <text> has *its own* text content, rather than just <tspan>s, register it
-                    // for translation.
-                    $translations[$fallbackTextId][$langCode] = $this->nodeToArray($text);
-                } else {
-                    $this->filteredTextNodes[$fallbackTextId][$langCode] = $this->nodeToArray($text);
-                }
+                $this->filteredTextNodes[$fallbackTextId][$langCode] = $this->nodeToArray($text);
                 $savedLang = 'fallback' === $langCode ? $this->fallbackLanguage : $langCode;
                 $this->savedLanguages[] = $savedLang;
             }
@@ -704,40 +697,6 @@ public function arrayToNode(array $array, string $nodeName = 'text'): DOMNode
         return $newNode;
     }
 
-    /**
-     * Checks whether a given DOMNode has some non-negligible text content (as
-     * opposed to just whitespace or other tags. Whitespace *between* tags
-     * counts, as it does get rendered.
-     *
-     * @param DOMNode $node The node to check for text content
-     * @return bool True if content found, false if not
-     */
-    public static function hasActualTextContent(DOMNode $node): bool
-    {
-        // No text nodes means no text content
-        if (!$node->hasChildNodes()) {
-            return false;
-        }
-
-        // Search child nodes looking for matching content
-        $children = $node->childNodes;
-        $numChildren = $children->length;
-        for ($i = 0; $i < $numChildren; $i++) {
-            if (XML_TEXT_NODE == $children->item($i)->nodeType) {
-                // Whitespace at beginning and end doesn't count, but
-                // otherwise we have a match
-                if (!0 === $i || $i === ( $numChildren - 1 )
-                    || !0 === strlen(trim($children->item($i)->textContent))
-                ) {
-                    return true;
-                }
-            }
-        }
-
-        // Didn't find any
-        return false;
-    }
-
     /**
      * Recursively replaces $1, $2, etc. with text tags, if required. Text content
      * is formalised as actual text nodes

diff --git a/tests/Model/Svg/SvgFileTest.php b/tests/Model/Svg/SvgFileTest.php
@@ -549,4 +549,15 @@ public function testMixedTextContent(): void
         $svg = $this->getSvg('mixed.svg');
         $this->assertCount(6, $svg->getInFileTranslations());
     }
+
+    /**
+     * https://phabricator.wikimedia.org/T220522
+     */
+    public function testChildOnly(): void
+    {
+        $svg = $this->getSvg('child-only.svg');
+        $svg->setTranslations('ru', ['trsvg1' => 'foo', 'trsvg2' => '']);
+        // Dummy assertion to avoid this test being marked as risky; the measure of success here is no crash.
+        static::assertTrue(true);
+    }
 }
diff --git a/tests/OOUI/TranslationsFieldsetTest.php b/tests/OOUI/TranslationsFieldsetTest.php
@@ -3,6 +3,7 @@
 
 namespace App\Tests\OOUI;
 
+use App\Model\Svg\SvgFile;
 use App\OOUI\TranslationsFieldset;
 use OOUI\FieldsetLayout;
 use PHPUnit\Framework\TestCase;
@@ -64,4 +65,21 @@ public function testMissingSourceLanguage(): void
         $items = $fieldset->getItems();
         static::assertCount(0, $items[0]->getItems());
     }
+
+    /**
+     * Tests for undefined index warning
+     */
+    public function testChildOnlyTranslations(): void
+    {
+        $svg = new SvgFile(dirname(__DIR__).'/data/child-only.svg');
+
+        $fieldset = new TranslationsFieldset([
+            'translations' => $svg->getInFileTranslations(),
+            'source_lang_code' => 'fallback',
+            'target_lang_code' => 'ru',
+        ]);
+
+        // One group
+        static::assertCount(1, $fieldset->getItems());
+    }
 }
diff --git a/tests/data/child-only.svg b/tests/data/child-only.svg