Last modified: 2012-04-15 04:22:08 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.
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
I'd rather see wfMessage being fixed and wfMsg* slowly converted to deprecated wrappers around wfMessage.
(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 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 ); > } > > /**
r86304
*** Bug 14959 has been marked as a duplicate of this bug. ***