Jarry1250 has uploaded a new change for review. https://gerrit.wikimedia.org/r/213798
Change subject: Add tests for each code path in makeTranslationReady ...................................................................... Add tests for each code path in makeTranslationReady Implement constants for each rather than true/false dichotomy. This would allow custom error messages in the eventual interface. Change-Id: I39c6a432e85346433ec1d3372ba2ce49bce2690a --- M SVGFile.php M TranslateSvgHooks.php M tests/phpunit/SVGFileTest.php 3 files changed, 86 insertions(+), 30 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/TranslateSvg refs/changes/98/213798/1 diff --git a/SVGFile.php b/SVGFile.php index 214360e..4d07d68 100644 --- a/SVGFile.php +++ b/SVGFile.php @@ -1,4 +1,5 @@ <?php + /** * This file contains classes for manipulating the contents of an SVG file. * Intended to centralise references to PHP's byzantine DOM manipulation system. @@ -25,6 +26,20 @@ protected $filteredTextNodes; protected $fallbackLanguage; + const CAN_TRANSLATE = 1; + const DOCUMENT_MALFORMED = 2; + const NOTHING_TO_TRANSLATE = 3; + const CANNOT_PARSE_CSS = 4; + const HAS_CSS_IDS = 5; + const HAS_TREF = 6; + const HAS_NESTED_TSPANS = 7; + const ID_HAS_BAD_CHARS = 8; + const HAS_DOLLAR_SIGNS = 9; + const TEXT_HAS_NON_TSPAN = 10; + const SWITCH_HAS_LOOSE_TEXT = 11; + const SWITCH_HAS_NON_TEXT_ELEMENT = 12; + const SWITCH_HAS_DUPLICATE_TRANSLATIONS = 13; + /** * Construct an SVGFile object. * @@ -48,7 +63,7 @@ $this->xpath->registerNamespace( 'svg', 'http://www.w3.org/2000/svg' ); // $this->isTranslationReady() can be used to test if construction was a success - $this->makeTranslationReady(); + $this->isTranslationReady = $this->makeTranslationReady(); } /** @@ -66,16 +81,16 @@ * * @todo: Find a way of making isTranslationReady a proper check * @todo: add interlanguage consistency check - * @return bool False on failure, DOMDocument on success + * @return int Error code on failure, self::CAN_TRANSLATE on success */ protected function makeTranslationReady() { - if( $this->isTranslationReady ) { - return true; + if ( $this->isTranslationReady ) { + return self::CAN_TRANSLATE; } if ( $this->document->documentElement === null ) { // Empty or malformed file - return false; + return self::DOCUMENT_MALFORMED; } // Automated editors have a habit of using XML entity references in the SVG namespace @@ -95,24 +110,24 @@ $textLength = $texts->length; if ( $textLength === 0 ) { // Nothing to translate! - return false; + return self::NOTHING_TO_TRANSLATE; } $styles = $this->document->getElementsByTagName( 'style' ); $styleLength = $styles->length; for ( $i = 0; $i < $styleLength; $i++ ) { $style = $styles->item( $i ); - $CSS = $style->textContent; - if ( strpos( $CSS, '#' ) !== false ) { - if ( !preg_match( '/^([^{]+\{[^}]*\})*[^{]+$/', $CSS ) ) { + $css = $style->textContent; + if ( strpos( $css, '#' ) !== false ) { + if ( !preg_match( '/^([^{]+\{[^}]*\})*[^{]*$/', $css ) ) { // Can't easily understand the CSS to check it, so exit - return false; + return self::CANNOT_PARSE_CSS; } - $selectors = preg_split( '/\{[^}]+\}/', $CSS ); + $selectors = preg_split( '/\{[^}]+\}/', $css ); foreach ( $selectors as $selector ) { if ( strpos( $selector, '#' ) !== false ) { // IDs in CSS will break when we clone things, should be classes - return false; + return self::HAS_CSS_IDS; } } } @@ -120,7 +135,7 @@ if ( $this->document->getElementsByTagName( 'tref' )->length !== 0 ) { // Tref tags not (yet) supported - return false; + return self::HAS_TREF; } // Strip empty tspans, texts, fill $idsInUse @@ -129,8 +144,8 @@ $tspans = $this->document->getElementsByTagName( 'tspan' ); $texts = $this->document->getElementsByTagName( 'text' ); foreach ( $tspans as $tspan ) { - if ( $tspan->childNodes->length > 1 ) { - return false; // Nested tspans not (yet) supported + if ( $tspan->childNodes->length > 1 || $tspan->childNodes->item(0)->nodeType !== XML_TEXT_NODE ) { + return self::HAS_NESTED_TSPANS; // @todo: Nested tspans not (yet) supported } $translatableNodes[] = $tspan; } @@ -144,7 +159,7 @@ $translatableNode->setAttribute( 'id', $id ); if ( strpos( $id, '|' ) !== false || strpos( $id, '/' ) !== false ) { // Will cause problems later - return false; + return self::ID_HAS_BAD_CHARS; } if ( preg_match( '/^trsvg([0-9]+)/', $id, $matches ) ) { $idsInUse[] = $matches[1]; @@ -157,6 +172,13 @@ // Empty tag, will just confuse translators if we leave it in $translatableNode->parentNode->removeChild( $translatableNode ); } + } + + $texts = $this->document->getElementsByTagName( 'text' ); + $textLength = $texts->length; + if ( $textLength === 0 ) { + // Nothing to translate! + return self::NOTHING_TO_TRANSLATE; } // Reset $translatableNodes @@ -176,6 +198,13 @@ $newId = ( max( $idsInUse ) + 1 ); $translatableNode->setAttribute( 'id', 'trsvg' . $newId ); $idsInUse[] = $newId; + } + + // Text strings like $1, $2 will cause problems later because + // self::replaceIndicesRecursive() will try to replace them + // with (non-existent) child nodes. + if ( preg_match( '/\$[0-9]/', $translatableNode->textContent ) ) { + return self::HAS_DOLLAR_SIGNS; } } @@ -222,7 +251,7 @@ && $child->nodeName !== 'svg:tspan' ) { // Tags other than tspan inside text tags are not (yet) supported - return false; + return self::TEXT_HAS_NON_TSPAN; } } } @@ -231,23 +260,19 @@ for ( $i = 0; $i < $switchLength; $i++ ) { $switch = $this->document->getElementsByTagName( 'switch' )->item( $i ); $siblings = $switch->childNodes; + $languagesPresent = array(); foreach ( $siblings as $sibling ) { /** @var DOMElement $sibling */ - - $languagesPresent = array(); if ( $sibling->nodeType === XML_TEXT_NODE ) { if ( trim( $sibling->textContent ) !== '' ) { // Text content inside switch but outside text tags is awkward. - return false; + return self::SWITCH_HAS_LOOSE_TEXT; } continue; - } elseif ( $sibling->nodeType !== XML_ELEMENT_NODE ) { + } elseif ( $sibling->nodeType !== XML_ELEMENT_NODE + || ( $sibling->nodeName !== 'text' && $sibling->nodeName !== 'svg:text' ) ) { // Only text tags are allowed inside switches - return false; - } - - if ( $sibling->nodeName !== 'text' && $sibling->nodeName !== 'svg:text' ) { - return false; + return self::SWITCH_HAS_NON_TEXT_ELEMENT; } $language = $sibling->hasAttribute( 'systemLanguage' ) ? @@ -256,7 +281,7 @@ foreach ( $realLangs as $realLang ) { if ( in_array( $realLang, $languagesPresent ) ) { // Two tags for the same language - return false; + return self::SWITCH_HAS_DUPLICATE_TRANSLATIONS; } $languagesPresent[] = $realLang; } @@ -283,8 +308,7 @@ $this->reorderTexts(); - $this->isTranslationReady = true; - return true; + return self::CAN_TRANSLATE; } diff --git a/TranslateSvgHooks.php b/TranslateSvgHooks.php index 8deee7a..5524a55 100644 --- a/TranslateSvgHooks.php +++ b/TranslateSvgHooks.php @@ -418,7 +418,7 @@ $messageGroup = new SVGMessageGroup( $id ); $svg = SVGFile::newFromMessageGroup( $messageGroup ); $vars['wgFileCanBeTranslated'] = ( $svg->isTranslationReady() ); - if ( !$vars['wgFileCanBeTranslated'] || MessageGroups::getGroup( $id ) === null ) { + if ( $vars['wgFileCanBeTranslated'] !== SVGFile::CAN_TRANSLATE || MessageGroups::getGroup( $id ) === null ) { // Not translatable or not yet translated, let's save time and return immediately $vars['wgFileTranslationStarted'] = false; $vars['wgFileFullTranslations'] = array(); diff --git a/tests/phpunit/SVGFileTest.php b/tests/phpunit/SVGFileTest.php index 116e389..49dfde9 100644 --- a/tests/phpunit/SVGFileTest.php +++ b/tests/phpunit/SVGFileTest.php @@ -25,6 +25,38 @@ $this->svg = new SVGFile( __DIR__ . '/../data/Speech_bubbles.svg', 'en' ); } + // todo: throws Namespace error on HHVM + public function testMakeTranslationReady() { + // Start with the easy case, Speech_Bubbles should have passed + $this->assertEquals( SVGFile::CAN_TRANSLATE, $this->svg->isTranslationReady() ); + + // Now to construct examples which don't + $tempPath = tempnam( wfTempDir(), 'test' ); + $tests = array( + array( '</text>', SVGFile::DOCUMENT_MALFORMED ), + array( '<svg></svg>', SVGFile::NOTHING_TO_TRANSLATE ), + array( '<svg><text></text></svg>', SVGFile::NOTHING_TO_TRANSLATE ), + array( '<svg><style type="text/css">#someId{font-weight:bold;</style><text></text></svg>', SVGFile::CANNOT_PARSE_CSS ), + array( '<svg><style type="text/css">#someId{font-weight:bold;}</style><text></text></svg>', SVGFile::HAS_CSS_IDS ), + array( '<svg><tref></tref><text>Foo</text></svg>', SVGFile::HAS_TREF ), + array( '<svg><text><tspan><tspan></tspan></tspan></text></svg>', SVGFile::HAS_NESTED_TSPANS ), + array( '<svg><text id="foo|bar"></text></svg>', SVGFile::ID_HAS_BAD_CHARS ), + array( '<svg><text>$$$</text></svg>', SVGFile::CAN_TRANSLATE ), + array( '<svg><text>Only $50!</text></svg>', SVGFile::HAS_DOLLAR_SIGNS ), + array( '<svg><text><b>Foo</b></text></svg>', SVGFile::TEXT_HAS_NON_TSPAN ), + array( '<svg><switch>Foo<text>Foo</text></switch></svg>', SVGFile::SWITCH_HAS_LOOSE_TEXT ), + array( '<svg><switch><tspan>Foo</tspan><text>Foo</text></switch></svg>', SVGFile::SWITCH_HAS_NON_TEXT_ELEMENT ), + array( '<svg><switch><text systemLanguage="fr">Foo</text><text systemLanguage="fr">Foo</text></switch></svg>', SVGFile::SWITCH_HAS_DUPLICATE_TRANSLATIONS ), + + ); + foreach( $tests as $test ) { + list( $xml, $expectedResponse ) = $test; + file_put_contents( $tempPath, $xml ); + $svg = new SVGFile( $tempPath, 'en' ); + $this->assertEquals( $expectedResponse, $svg->isTranslationReady() ); + } + } + public function testGetInFileTranslations() { $expected = array( 'tspan2987' => -- To view, visit https://gerrit.wikimedia.org/r/213798 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I39c6a432e85346433ec1d3372ba2ce49bce2690a Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/extensions/TranslateSvg Gerrit-Branch: master Gerrit-Owner: Jarry1250 <jarry1...@gmail.com> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits