r82307 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r82306‎ | r82307 | r82308 >
Date:22:45, 16 February 2011
Author:hartman
Status:reverted (Comments)
Tags:
Comment:
Add support for namespace prefixed elements in svg. ie. <svg:svg>

Fixes bug 27465
Modified paths:
  • /trunk/phase3/includes/media/SVGMetadataExtractor.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/media/SVGMetadataExtractor.php
@@ -74,7 +74,7 @@
7575 $keepReading = $this->reader->read();
7676 }
7777
78 - if ( $this->reader->name != 'svg' ) {
 78+ if ( !$this->qualifiedNameEquals( $this->reader->name, 'svg', 'svg' ) ) {
7979 throw new MWException( "Expected <svg> tag, got ".
8080 $this->reader->name );
8181 }
@@ -90,13 +90,13 @@
9191
9292 $this->debug( "$tag" );
9393
94 - if ( $tag == 'svg' && $type == XmlReader::END_ELEMENT && $this->reader->depth <= $exitDepth ) {
 94+ if ( $this->qualifiedNameEquals( $tag, 'svg', 'svg' ) && $type == XmlReader::END_ELEMENT && $this->reader->depth <= $exitDepth ) {
9595 break;
96 - } elseif ( $tag == 'title' ) {
 96+ } elseif ( $this->qualifiedNameEquals( $tag, 'svg', 'title' ) ) {
9797 $this->readField( $tag, 'title' );
98 - } elseif ( $tag == 'desc' ) {
 98+ } elseif ( $this->qualifiedNameEquals( $tag, 'svg', 'desc' ) ) {
9999 $this->readField( $tag, 'description' );
100 - } elseif ( $tag == 'metadata' && $type == XmlReader::ELEMENT ) {
 100+ } elseif ( $this->qualifiedNameEquals( $tag, 'svg', 'metadata' ) && $type == XmlReader::ELEMENT ) {
101101 $this->readXml( $tag, 'metadata' );
102102 } elseif ( $tag !== '#text' ) {
103103 $this->debug( "Unhandled top-level XML tag $tag" );
@@ -172,10 +172,15 @@
173173 } elseif ( $this->reader->nodeType == XmlReader::ELEMENT ) {
174174 switch( $this->reader->name ) {
175175 case 'animate':
 176+ case 'svg:animate':
176177 case 'set':
 178+ case 'svg:set':
177179 case 'animateMotion':
 180+ case 'svg:animateMotion':
178181 case 'animateColor':
 182+ case 'svg:animateColor':
179183 case 'animateTransform':
 184+ case 'svg:animateTransform':
180185 $this->debug( "HOUSTON WE HAVE ANIMATION" );
181186 $this->metadata['animated'] = true;
182187 break;
@@ -284,4 +289,22 @@
285290 return floatval( $length );
286291 }
287292 }
 293+
 294+ /**
 295+ * XML namespace check
 296+ *
 297+ * Check if a read node name matches the expected nodeName
 298+ * @param $qualifiedName as read by XMLReader
 299+ * @param $prefix the namespace prefix that you expect for this element, defaults to svg namespace
 300+ * @param $localName the localName part of the element that you want to match
 301+ *
 302+ * @return boolean
 303+ */
 304+ private function qualifiedNameEquals( $qualifiedName, $prefix="svg", $localName ) {
 305+ if( ($qualifiedName == $localName && $prefix == "svg" ) ||
 306+ $qualifiedName == ($prefix . ":" . $localName) ) {
 307+ return true;
 308+ }
 309+ return false;
 310+ }
288311 }

Sign-offs

UserFlagDate
Bryaninspected08:39, 17 February 2011
Krinkleinspected12:40, 17 February 2011

Follow-up revisions

RevisionCommit summaryAuthorDate
r82336MFT r82307reedy16:01, 17 February 2011
r85211MFT: r82297, r82307, r82309, r82312, r82315, r82337, r82391, r82392, r82403, ...demon21:01, 2 April 2011
r88865Test cases for bug 27465: problems with XML namespace handling break SVGMetad...brion23:34, 25 May 2011
r88870Reverting r82307 (bug 27465) as initial step to recommitting a cleaner fix.brion23:57, 25 May 2011
r88871* (bug 27465) Fix metadata extraction for SVG files using unusual namespace n...brion23:59, 25 May 2011

Comments

#Comment by Brion VIBBER (talk | contribs)   23:08, 25 May 2011

This looks really wrong; it seems to be assuming that namespaces have particular *names*, whereas namespace names are in fact completely arbitrary, and can only be validated by matching the URL specified in the xmlns declaration.

On a local copy of https://secure.wikimedia.org/wikipedia/en/wiki/File:US_states_by_total_state_tax_revenue.svg I can confirm that I'm getting errors like this:

 SvgHandler::getMetadata: Expected <svg> tag, got ns0:svg

Although the file looks like it defines the ns0 namespace as SVG just fine:

 <ns0:svg ... xmlns:ns0="http://www.w3.org/2000/svg" ... >

As a result, although it passes verification for upload, we can't extract metadata so we don't get width/height info and we end up not being able to render it.

#Comment by Brion VIBBER (talk | contribs)   00:08, 26 May 2011

Reverted in r88870 in favor of new code in r88871 (plus test cases from r88865)

Status & tagging log