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 <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
