r59444 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r59443‎ | r59444 | r59445 >
Date:05:29, 26 November 2009
Author:yaron
Status:ok (Comments)
Tags:
Comment:
Error message returned for bad XML is now actual parsing error message
Modified paths:
  • /trunk/extensions/TemplateInfo/TemplateInfo.hooks.php (modified) (history)

Diff [purge]

Index: trunk/extensions/TemplateInfo/TemplateInfo.hooks.php
——@@ -19,7 +19,7 @@
2020 return true;
2121 }
2222
23 - public static function validateXML( $xml ) {
 23+ public static function validateXML( $xml, &$error_msg ) {
2424 $xmlDTD =<<<END
2525 <?xml version="1.0" encoding="utf-8"?>
2626 <!DOCTYPE template [
——@@ -49,22 +49,27 @@
5050 // hide parsing warnings
5151 libxml_use_internal_errors(true);
5252 $xml_success = simplexml_load_string($xmlDTD . $xml);
 53+ $errors = libxml_get_errors();
 54+ $message = $errors[0]->message;
 55+ //$line = $errors[0]->line;
 56+ $error_msg = "ERROR: $message";
5357 return $xml_success;
5458 }
5559
5660 // Render the displayed XML, if any
5761 public static function render( $input, $args, $parser, $frame ) {
5862 // if this call is contained in a transcluded page or template,
59 - // or if the inpur is empty, display nothing
 63+ // or if the input is empty, display nothing
6064 if ( !$frame->title->equals( $parser->getTitle() ) || $input == '' )
6165 return;
6266
6367 // Store XML in the page_props table
6468 // TODO: Do processing here, like parse to an array
65 - if ( TemplateInfoHooks::validateXML( $input ) )
 69+ $error_msg = null;
 70+ if ( TemplateInfoHooks::validateXML( $input, $error_msg ) )
6671 $parser->getOutput()->setProperty( 'templateinfo', $input );
6772 else
68 - $parser->getOutput()->setProperty( 'templateinfo', "Error: Invalid XML" );
 73+ $parser->getOutput()->setProperty( 'templateinfo', $error_msg );
6974
7075 // Return output
7176 $text = "<p>" . wfMsg( 'templateinfo-header' ) . "</p>\n";

Comments

#Comment by Catrope (talk | contribs)   12:48, 26 November 2009 
+	$error_msg = "ERROR: $message";
  1. It's cleaner to return $message and let the caller prepend "ERROR: " if they want to
  2. Hardcoded English like this is generally considered evil. The API is an exception, but a generic method like this should not be biased towards API usage
+		$parser->getOutput()->setProperty( 'templateinfo', $error_msg );

Storing the XML parse error in the same property that normally holds the XML (if it validates) feels kind of ugly for me, but I can't think of anything better offhand.

It would be cleaner if, in the API module, you output something like <templateinfo error="Error message here" /> instead of <templateinfo>ERROR: Error message here</templateinfo>. The former is also easier to detect on the client side, as it can be done by just checking for the error attribute (instead of having to check whether the XML starts with ERROR). Of course, this requires that the API module detect the difference between valid XML and an error message in the templateinfo property, which reinforces my earlier point about this double usage of the property being ugly. If you need help outputting this XML in the API, poke me on IRC.

#Comment by Yaron Koren (talk | contribs)   22:51, 3 December 2009

Good point - I changed both of these things. I probably didn't use the right function to add the "error=" attribute in the XML; but unfortunately I couldn't log in to IRC to ask you about it. Feel free to change it yourself if you want, or let me know here.

Status & tagging log