Last modified: 2012-04-15 04:22:08 UTC

Wikimedia Bugzilla is closed!

Wikimedia has migrated from Bugzilla to Phabricator. Bug reports should be created and updated in Wikimedia Phabricator instead. Please create an account in Phabricator and add your Bugzilla email address to it.
Wikimedia Bugzilla is read-only. If you try to edit or create any bug report in Bugzilla you will be shown an intentional error message.
In order to access the Phabricator task corresponding to a Bugzilla report, just remove "static-" from its URL.
You could still run searches in Bugzilla or access your list of votes but bug reports will obviously not be up-to-date in Bugzilla.
Bug 28532 - wfMsgExt() and wfMsgWikiHtml() use $wgOut->parse()
wfMsgExt() and wfMsgWikiHtml() use $wgOut->parse()
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
Internationalization (Other open bugs)
1.18.x
All All
: Low critical (vote)
: ---
Assigned To: Sam Reed (reedy)
: patch, patch-need-review
: 14959 (view as bug list)
Depends on:
Blocks: 16129
  Show dependency treegraph
 
Reported: 2011-04-14 02:12 UTC by Tim Starling
Modified: 2012-04-15 04:22 UTC (History)
1 user (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
Patch (3.82 KB, patch)
2011-04-15 17:42 UTC, Sam Reed (reedy)
Details

Description Tim Starling 2011-04-14 02:12:34 UTC
wfMsgExt() and wfMsgWikiHtml() use $wgOut->parse(). This is inappropriate and causes many bugs. For example, at the moment, if you try to view a wiki and your CentralAuth username is blacklisted, it will throw an exception "Empty $mTitle in OutputPage::parse" due to TitleBlacklistHooks::acceptNewUserName() calling wfMsgWikiHtml(). 

$wgMessageCache->transform() does this correctly. It uses a separate instance of the parser, it doesn't get titles or parser options from strange sources like $wgOut, it doesn't mind if $wgTitle is unset, and it fails gracefully if it is called re-entrantly.

I suggest MessageCache::transform() be refactored to provide a getParser() method, which should then be used by a new method "MessageCache::parse()", which should in turn be used by wfMsgWikiHtml() etc. instead of $wgOut->parse(). The guarding against re-entrant calls should be extended to cover both entry points.
Comment 1 Sam Reed (reedy) 2011-04-15 17:42:47 UTC
Created attachment 8406 [details]
Patch

Rather than committing and waiting for review, seemed sensible to create a patch and put here for you to look at Tim, being an area of code I'm not really familiar with
Comment 2 Niklas Laxström 2011-04-15 17:46:37 UTC
I'd rather see wfMessage being fixed and wfMsg* slowly converted to deprecated wrappers around wfMessage.
Comment 3 Tim Starling 2011-04-18 00:43:11 UTC
(In reply to comment #1)
> Created attachment 8406 [details]
> Patch
> 
> Rather than committing and waiting for review, seemed sensible to create a
> patch and put here for you to look at Tim, being an area of code I'm not really
> familiar with

That's the general idea, but MessageCache::parse() lacks protection against re-entrant calls. You need something like:

if ( $this->mInParser ) {
   return htmlspecialchars( $in );
}
$this->mInParser = true;
$this->mParser->parse(...);
$this->mInParser = false;

Both MessageCache::transform() and MessageCache::parse() should be protected using the same member variable, since they both access the same parser object. You could test it by making a couple of tag hook extensions that call $wgMessageCache->parse().

<foo>
<bar>
[[test]]
</bar>
</foo>

It's easy enough to make a tag hook extension in a few lines of code in LocalSettings.php, you don't need to make a separate file or anything.

Please also patch Message::parseText() to call MessageCache::singleton()->parse(), so that Niklas's idea can easily be done later.
Comment 4 Sam Reed (reedy) 2011-04-18 12:44:14 UTC
Comment on attachment 8406 [details]
Patch

>Index: GlobalFunctions.php
>===================================================================
>--- GlobalFunctions.php	(revision 86120)
>+++ GlobalFunctions.php	(working copy)
>@@ -716,10 +716,12 @@
>  * @return string
>  */
> function wfMsgWikiHtml( $key ) {
>-	global $wgOut;
>+	global $wgMessageCache;
> 	$args = func_get_args();
> 	array_shift( $args );
>-	return wfMsgReplaceArgs( $wgOut->parse( wfMsgGetKey( $key, true ), /* can't be set to false */ true ), $args );
>+	return wfMsgReplaceArgs(
>+		$wgMessageCache->parse( wfMsgGetKey( $key, true ), null, /* can't be set to false */ true ),
>+		$args );
> }
> 
> /**
>@@ -741,7 +743,7 @@
>  * Behavior for conflicting options (e.g., parse+parseinline) is undefined.
>  */
> function wfMsgExt( $key, $options ) {
>-	global $wgOut;
>+	global $wgMessageCache;
> 
> 	$args = func_get_args();
> 	array_shift( $args );
>@@ -781,9 +783,9 @@
> 	}
> 
> 	if( in_array( 'parse', $options, true ) ) {
>-		$string = $wgOut->parse( $string, true, !$forContent, $langCodeObj );
>+		$string = $wgMessageCache->parse( $string, null, true, !$forContent, $langCodeObj );
> 	} elseif ( in_array( 'parseinline', $options, true ) ) {
>-		$string = $wgOut->parse( $string, true, !$forContent, $langCodeObj );
>+		$string = $wgMessageCache->parse( $string, null, true, !$forContent, $langCodeObj );
> 		$m = array();
> 		if( preg_match( '/^<p>(.*)\n?<\/p>\n?$/sU', $string, $m ) ) {
> 			$string = $m[1];
>Index: MessageCache.php
>===================================================================
>--- MessageCache.php	(revision 86120)
>+++ MessageCache.php	(working copy)
>@@ -102,6 +102,8 @@
> 
> 	/**
> 	 * ParserOptions is lazy initialised.
>+	 *
>+	 * @return ParserOptions
> 	 */
> 	function getParserOptions() {
> 		if ( !$this->mParserOptions ) {
>@@ -220,6 +222,8 @@
> 
> 	/**
> 	 * Set the cache to $cache, if it is valid. Otherwise set the cache to false.
>+	 *
>+	 * @return bool
> 	 */
> 	function setCache( $cache, $code ) {
> 		if ( isset( $cache['VERSION'] ) && $cache['VERSION'] == MSG_CACHE_VERSION ) {
>@@ -734,6 +738,21 @@
> 			return $message;
> 		}
> 
>+		$parser = $this->getParser();
>+		if ( $parser ) {
>+			$popts = $this->getParserOptions();
>+			$popts->setInterfaceMessage( $interface );
>+			$popts->setTargetLanguage( $language );
>+			$popts->setUserLang( $language );
>+			$message = $parser->transformMsg( $message, $popts, $title );
>+		}
>+		return $message;
>+	}
>+
>+	/**
>+	 * @return Parser
>+	 */
>+	function getParser() {
> 		global $wgParser, $wgParserConf;
> 		if ( !$this->mParser && isset( $wgParser ) ) {
> 			# Do some initialisation so that we don't have to do it twice
>@@ -746,16 +765,28 @@
> 			} else {
> 				$this->mParser = clone $wgParser;
> 			}
>-			#wfDebug( __METHOD__ . ": following contents triggered transform: $message\n" );
> 		}
>-		if ( $this->mParser ) {
>-			$popts = $this->getParserOptions();
>-			$popts->setInterfaceMessage( $interface );
>+		return $this->mParser;
>+	}
>+
>+	/**
>+	 * @param $text string
>+	 * @param $title
>+	 * @param $linestart bool
>+	 * @return ParserOutput
>+	 */
>+	public function parse( $text, $title = null, $linestart = true, $interface = false, $language = null  ) {
>+		$parser = $this->getParser();
>+		$popts = $this->getParserOptions();
>+
>+		if ( $interface ) {
>+			$popts->setInterfaceMessage( true );
>+		}
>+		if ( $language !== null ) {
> 			$popts->setTargetLanguage( $language );
>-			$popts->setUserLang( $language );
>-			$message = $this->mParser->transformMsg( $message, $popts, $title );
> 		}
>-		return $message;
>+
>+		return $parser->parse( $text, $title, $popts, $linestart );
> 	}
> 
> 	function disable() {
>Index: OutputPage.php
>===================================================================
>--- OutputPage.php	(revision 86120)
>+++ OutputPage.php	(working copy)
>@@ -792,7 +792,7 @@
> 	 * @param $t Title object
> 	 */
> 	public function setTitle( $t ) {
>-		$this->getContext()->setTitle($t);
>+		$this->getContext()->setTitle( $t );
> 	}
> 
> 	/**
Comment 5 Sam Reed (reedy) 2011-04-18 12:44:37 UTC
r86304
Comment 6 Bawolff (Brian Wolff) 2012-04-15 04:22:08 UTC
*** Bug 14959 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links