r86312 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r86311‎ | r86312 | r86313 >
Date:14:02, 18 April 2011
Author:reedy
Status:reverted (Comments)
Tags:needs-php-test 
Comment:
Followup rr86304

We need a title object for parsing, do one against the message key

Doesn't seem to be the best way, but it's the most applicable. If I abused $wgTitle, Chad would come and beat me too ;)
Modified paths:
  • /trunk/phase3/includes/GlobalFunctions.php (modified) (history)
  • /trunk/phase3/includes/Message.php (modified) (history)
  • /trunk/phase3/includes/MessageCache.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/GlobalFunctions.php
——@@ -719,7 +719,7 @@
720720 $args = func_get_args();
721721 array_shift( $args );
722722 return wfMsgReplaceArgs(
723 - MessageCache::singleton()->parse( wfMsgGetKey( $key, true ), null, /* can't be set to false */ true )->getText(),
 723+ MessageCache::singleton()->parse( wfMsgGetKey( $key, true ), $key, /* can't be set to false */ true )->getText(),
724724 $args );
725725 }
726726
——@@ -781,9 +781,9 @@
782782
783783 $messageCache = MessageCache::singleton();
784784 if( in_array( 'parse', $options, true ) ) {
785 - $string = $messageCache->parse( $string, null, true, !$forContent, $langCodeObj )->getText();
 785+ $string = $messageCache->parse( $string, $key, true, !$forContent, $langCodeObj )->getText();
786786 } elseif ( in_array( 'parseinline', $options, true ) ) {
787 - $string = $messageCache->parse( $string, null, true, !$forContent, $langCodeObj )->getText();
 787+ $string = $messageCache->parse( $string, $key, true, !$forContent, $langCodeObj )->getText();
788788 $m = array();
789789 if( preg_match( '/^<p>(.*)\n?<\/p>\n?$/sU', $string, $m ) ) {
790790 $string = $m[1];
Index: trunk/phase3/includes/MessageCache.php
——@@ -790,13 +790,13 @@
791791
792792 /**
793793 * @param $text string
794 - * @param $title Title
 794+ * @param $string Title|string
795795 * @param $interface bool
796796 * @param $linestart bool
797797 * @param $language
798798 * @return ParserOutput
799799 */
800 - public function parse( $text, $title = null, $linestart = true, $interface = false, $language = null ) {
 800+ public function parse( $text, $key, $linestart = true, $interface = false, $language = null ) {
801801 if ( $this->mInParser ) {
802802 return htmlspecialchars( $text );
803803 }
——@@ -811,6 +811,9 @@
812812 $popts->setTargetLanguage( $language );
813813 }
814814
 815+ if ( !($key instanceof Title) ) {
 816+ $title = Title::newFromText( $key, NS_MEDIAWIKI );
 817+ }
815818 $this->mInParser = true;
816819 $res = $parser->parse( $text, $title, $popts, $linestart );
817820 $this->mInParser = false;
Index: trunk/phase3/includes/Message.php
——@@ -432,7 +432,7 @@
433433 * @return string Wikitext parsed into HTML
434434 */
435435 protected function parseText( $string ) {
436 - return MessageCache::singleton()->parse( $string, null, /*linestart*/true, $this->interface, $this->language )->getText();
 436+ return MessageCache::singleton()->parse( $string, $this->key, /*linestart*/true, $this->interface, $this->language )->getText();
437437 }
438438
439439 /**

Follow-up revisions

RevisionCommit summaryAuthorDate
r86735Followup to r86312...reedy20:17, 22 April 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r86304* (bug 28532) wfMsgExt() and wfMsgWikiHtml() use $wgOut->parse()...reedy12:43, 18 April 2011

Comments

#Comment by 😂 (talk | contribs)   14:27, 18 April 2011

Should probably check to make sure we return a valid Title from newFromText(). I imagine the scenario of feeding an invalid title (or string that makes one) is fairly rare. Just throw an exception?

Also, you don't need () around the instanceof check, you can just do !$a instanceof B

#Comment by Reedy (talk | contribs)   14:33, 18 April 2011

Possibly, yeah. Not sure if there's a better/nicer/saner way to do this anyway...

#Comment by Nikerabbit (talk | contribs)   14:40, 18 April 2011

Constructing title object for nearly every call sounds a bit excessive. Maybe it is better just to pass some fake title until we are able to pass the correct one?

#Comment by Reedy (talk | contribs)   14:45, 18 April 2011

Indeed, it was the simplest fix without reverting.

What can we use for a fake title that isn't going to bork?

#Comment by IAlex (talk | contribs)   16:28, 18 April 2011

This is breaking some messages by displaying message name instead of page name, such as "noarticletext" in action=info, and maybe some others.

#Comment by Brion VIBBER (talk | contribs)   00:02, 15 June 2011

If we can confirm tests for this & r86735, should be ok to mark as resolved.

#Comment by Reedy (talk | contribs)   00:25, 8 September 2011

Can you provide me with some help/guidance on what I actually need to do test wise? I'm not overly familiar with the code, as it was fixed mainly against Tims suggestion on how to fix it.

Which therefore goes back to

  • (bug 28532) wfMsgExt() and wfMsgWikiHtml() use $wgOut->parse()
  • (bug 16129) Transcluded special pages expose strip markers when they output parsed messages
#Comment by Nikerabbit (talk | contribs)   05:37, 16 September 2011

This is reverted by r86735. Tagging it as such.

Status & tagging log