r60832 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r60831‎ | r60832 | r60833 >
Date:08:22, 8 January 2010
Author:mah
Status:resolved (Comments)
Tags:
Comment:
follow up r60763
Recover the -{T| }- rule. Add the ability to test for it to the parserTests and add a test for it. Add a couple of disabled tests that I think demonstrate bugs in the LanguageTranslator
Modified paths:
  • /trunk/phase3/includes/parser/Parser.php (modified) (history)
  • /trunk/phase3/languages/Language.php (modified) (history)
  • /trunk/phase3/languages/LanguageConverter.php (modified) (history)
  • /trunk/phase3/maintenance/parserTests.inc (modified) (history)
  • /trunk/phase3/maintenance/parserTests.txt (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/parserTests.inc
——@@ -493,6 +493,10 @@
494494 $result = $this->tidy($result);
495495 }
496496
 497+ if ( isset( $opts['showtitle'] ) ) {
 498+ $out = $parser->mTitle . "\n$out";
 499+ }
 500+
497501 $this->teardownGlobals();
498502
499503 if( $result === $out && ( $noxml === true || $this->wellFormed( $out ) ) ) {
Index: trunk/phase3/maintenance/parserTests.txt
——@@ -6929,15 +6929,15 @@
69306930 !! test
69316931 Adding explicit conversion rule for title (T flag)
69326932 !! options
6933 -language=zh variant=zh-tw
 6933+language=zh variant=zh-tw showtitle
69346934 !! input
69356935 Should be stripped-{T|zh:China;zh-tw:Taiwan}-!
69366936 !! result
 6937+Taiwan
69376938 <p>Should be stripped!
69386939 </p>
69396940 !! end
69406941
6941 -
69426942 !! test
69436943 Raw output of variant escape tags (R flag)
69446944 !! options
——@@ -6949,7 +6949,6 @@
69506950 </p>
69516951 !! end
69526952
6953 -
69546953 !! test
69556954 Do not convert roman numbers to language variants
69566955 !! options
——@@ -7515,6 +7514,47 @@
75167515 <a href="https://www.mediawiki.org/wiki/Main_Page#section" title="Main Page">#section</a>
75177516 !! end
75187517
 7518+!! test
 7519+Adding explicit conversion rule for title (T flag)
 7520+!! options
 7521+language=zh variant=zh-tw showtitle **** disabled (for now) because I think it uncovers a bug and I want a reminder
 7522+!! input
 7523+Should be stripped-{T|zh:China;zh-tw:Taiwan}-!
 7524+!! result
 7525+Taiwan
 7526+<p>Should be stripped!
 7527+</p>
 7528+!! end
 7529+
 7530+!! test
 7531+Adding explicit conversion rule for title (T flag)
 7532+!! options
 7533+language=zh variant=zh showtitle **** disabled (for now) because I think it uncovers a bug and I want a reminder
 7534+!! input
 7535+Should be stripped-{T|zh:China;zh-tw:Taiwan}-!
 7536+!! result
 7537+China
 7538+<p>Should be stripped!
 7539+</p>
 7540+!! end
 7541+
 7542+!! test
 7543+Explicit session-wise language variant mapping (A flag and - flag)
 7544+!! options
 7545+language=zh variant=zh **** disabled (for now) because I think it uncovers a bug and I want a reminder
 7546+!! input
 7547+Taiwan is not China.
 7548+But -{zh:China;zh-tw:Taiwan}- is China,
 7549+(This-{-|zh:China;zh-tw:Taiwan}- should be stripped!)
 7550+and -{China}- is China.
 7551+!! result
 7552+<p>Taiwan is not China.
 7553+But Taiwan is Taiwan,
 7554+(This should be stripped!)
 7555+and China is China.
 7556+</p>
 7557+!! end
 7558+
75197559 TODO:
75207560 more images
75217561 more tables
Index: trunk/phase3/includes/parser/Parser.php
——@@ -261,6 +261,12 @@
262262 if ( !$t || $t instanceof FakeTitle ) {
263263 $t = Title::newFromText( 'NO TITLE' );
264264 }
 265+ // If we still don't have a Title object, make sure we can
 266+ // convert whatever we've been passed to a string.
 267+ if ( !$t instanceOf Title && is_string( "$t" ) ) {
 268+ $t = Title::newFromText( "$t" );
 269+ }
 270+
265271 if ( strval( $t->getFragment() ) !== '' ) {
266272 # Strip the fragment to avoid various odd effects
267273 $this->mTitle = clone $t;
——@@ -329,14 +335,6 @@
330336 # No more strip!
331337 wfRunHooks( 'ParserAfterStrip', array( &$this, &$text, &$this->mStripState ) );
332338 $text = $this->internalParse( $text );
333 - // internalParse took care of the notitleconvert bit, so title conversion is here.
334 - if ( $this->mDoTitleConvert && !$this->mTitle->isConversionTable()) {
335 - $converted = $wgContLang->convert( $title );
336 - if ( !$converted instanceOf Title ) {
337 - $converted = Title::newFromText( $converted );
338 - }
339 - $this->setTitle( $converted );
340 - }
341339
342340 $text = $this->mStripState->unstripGeneral( $text );
343341
——@@ -355,13 +353,23 @@
356354
357355 $this->replaceLinkHolders( $text );
358356
359 - # the position of the convert() call should not be changed. it
360 - # assumes that the links are all replaced and the only thing left
361 - # is the <nowiki> mark.
 357+ // The position of the convert() call should not be changed. it
 358+ // assumes that the links are all replaced and the only thing left
 359+ // is the <nowiki> mark.
362360 if ( $this->mDoContentConvert && !$this->mTitle->isConversionTable()) {
363361 $text = $wgContLang->convert( $text );
364362 }
365363
 364+ // A title may have been set in a conversion rule.
 365+ // Note that if a user tries to set a title in a conversion
 366+ // rule but content conversion was not done, then the parser
 367+ // won't pick it up. This is probably expected behavior.
 368+ if ( $wgContLang->getConvRuleTitle() ) {
 369+ $this->setTitle( $wgContLang->getConvRuleTitle() );
 370+ } elseif ( $this->mDoTitleConvert && !$this->mTitle->isConversionTable() ) {
 371+ $this->setTitle( $wgContLang->convert( $title ) );
 372+ }
 373+
366374 $text = $this->mStripState->unstripNoWiki( $text );
367375
368376 wfRunHooks( 'ParserBeforeTidy', array( &$this, &$text ) );
Index: trunk/phase3/languages/LanguageConverter.php
——@@ -31,6 +31,7 @@
3232 var $mDescCodeSep = ':', $mDescVarSep = ';';
3333 var $mUcfirst = false;
3434 var $mHeaderVariant;
 35+ var $mConvRuleTitle = false;
3536
3637 const CACHE_VERSION_KEY = 'VERSION 6';
3738
——@@ -459,8 +460,8 @@
460461 */
461462 function applyManualConv( $convRule ) {
462463 // use syntax -{T|zh:TitleZh;zh-tw:TitleTw}- for custom
463 - // conversion in title
464 - $title = $convRule->getTitle();
 464+ // conversion in title
 465+ $this->mConvRuleTitle = $convRule->getTitle();
465466
466467 // apply manual conversion table to global table
467468 $convTable = $convRule->getConvTable();
——@@ -544,7 +545,7 @@
545546 * @return string converted text
546547 * @public
547548 */
548 - function convert( $text, $isTitle ) {
 549+ function convert( $text ) {
549550 global $wgDisableLangConversion;
550551 if ( $wgDisableLangConversion ) return $text;
551552
——@@ -558,10 +559,6 @@
559560
560561 // Remove the last delimiter (wasn't real)
561562 $converted = substr( $converted, 0, - strlen( $this->mMarkup['end'] ) );
562 - if ( $isTitle ) {
563 - error_log("title2: $converted\n");
564 - $this->mConvertedTitle = $converted;
565 - }
566563 return $converted;
567564 }
568565
——@@ -1110,13 +1107,13 @@
11111108 * Parse rules conversion.
11121109 * @private
11131110 */
1114 - function getRuleConvertedStr( $variant, $doConvert ) {
 1111+ function getRuleConvertedStr( $variant ) {
11151112 $bidtable = $this->mBidtable;
11161113 $unidtable = $this->mUnidtable;
11171114
11181115 if ( count( $bidtable ) + count( $unidtable ) == 0 ) {
11191116 return $this->mRules;
1120 - } elseif ( $doConvert ) { // the text converted
 1117+ } else {
11211118 // display current variant in bidirectional array
11221119 $disp = $this->getTextInBidtable( $variant );
11231120 // or display current variant in fallbacks
——@@ -1142,8 +1139,6 @@
11431140 }
11441141 }
11451142 return $disp;
1146 - } else { // no convert
1147 - return $this->mRules;
11481143 }
11491144 }
11501145
——@@ -1271,15 +1266,13 @@
12721267 // proces H,- flag or T only: output nothing
12731268 $this->mRuleDisplay = '';
12741269 } elseif ( in_array( 'S', $flags ) ) {
1275 - // true hard-coded now since we shouldn't be called if we're not converting
1276 - $this->mRuleDisplay = $this->getRuleConvertedStr( $variant, true );
 1270+ $this->mRuleDisplay = $this->getRuleConvertedStr( $variant );
12771271 } else {
12781272 $this->mRuleDisplay = $this->mManualCodeError;
12791273 }
12801274 // process T flag
12811275 if ( in_array( 'T', $flags ) ) {
1282 - // true hard-coded now since we shouldn't be called if we're not converting
1283 - $this->mRuleTitle = $this->getRuleConvertedStr( $variant, true );
 1276+ $this->mRuleTitle = $this->getRuleConvertedStr( $variant );
12841277 }
12851278
12861279 if ( in_array( '-', $flags ) ) {
Index: trunk/phase3/languages/Language.php
——@@ -34,11 +34,12 @@
3535 */
3636 class FakeConverter {
3737 var $mLang;
 38+ var $mConvRuleTitle;
3839 function FakeConverter($langobj) {$this->mLang = $langobj;}
3940 function autoConvertToAllVariants($text) {return $text;}
4041 function convert($t, $i) {return $t;}
4142 function getVariants() { return array( $this->mLang->getCode() ); }
42 - function getPreferredVariant() {return $this->mLang->getCode(); }
 43+ function getPreferredVariant() { return $this->mLang->getCode(); }
4344 function findVariantLink(&$l, &$n, $ignoreOtherCond = false) {}
4445 function getExtraHashOptions() {return '';}
4546 function getParsedTitle() {return '';}
——@@ -2664,4 +2665,11 @@
26652666 $text = $this->getMessageFromDB( $msg );
26662667 return str_replace( '$1', $this->formatNum( $size ), $text );
26672668 }
 2669+
 2670+ /**
 2671+ * Get the conversion rule title, if any.
 2672+ */
 2673+ function getConvRuleTitle() {
 2674+ return $this->mConverter->mConvRuleTitle;
 2675+ }
26682676 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r60998follow-up r60832: make sure $t always ends up a Title.mah02:41, 13 January 2010
r61043follow up r60832: Remove un-necessary test for stringiness since "" converts it.mah05:02, 14 January 2010
r61101follow up r60832 and follow up r60763...mah19:14, 15 January 2010

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r60763Refactor LanguageConversion so that title conversion isn't so flimsy. Pull Ma...mah04:13, 7 January 2010

Comments

#Comment by PhiLiP (talk | contribs)   22:03, 12 January 2010

Fatal error: Call to a member function getFragment() on a non-object in E:\mwnew\includes\parser\Parser.php on line 270

#Comment by MarkAHershberger (talk | contribs)   02:27, 13 January 2010

do you have an example of how to cause this?

#Comment by PhiLiP (talk | contribs)   10:47, 13 January 2010

seems you have fixed it in r60998.

#Comment by 😂 (talk | contribs)   05:53, 13 January 2010

+ if ( !$t instanceOf Title && is_string( "$t" ) ) { + $t = Title::newFromText( "$t" ); + }

Any reason you're quoting the $t?

#Comment by MarkAHershberger (talk | contribs)   01:23, 14 January 2010

Quoting $t: see http://php.net/manual/en/language.types.type-juggling.php, "Instead of casting a variable to a string, it is also possible to enclose the variable in double quotes."

but it might be redundant to do is_string("$t")

#Comment by PhiLiP (talk | contribs)   03:25, 14 January 2010

"$t" is always a string, because it was enclosed in double quotes. So, is it really necessary to use is_string() here?

#Comment by MarkAHershberger (talk | contribs)   04:27, 14 January 2010

better would be to catch the error if $t is an object that can't be converted to a string

#Comment by Tim Starling (talk | contribs)   05:55, 15 January 2010 
+		// If we still don't have a Title object, make sure we can
+		// convert whatever we've been passed to a string.
+		if ( !$t instanceOf Title && is_string( "$t" ) ) {
+			$t = Title::newFromText( "$t" );
+		}

This section should be removed entirely, when (as requested on r60763) you stop calling setTitle() when you shouldn't be. Usually we use strval() to convert things to strings, instead of interpolation, since it causes less confusion. 

 class FakeConverter {
 	var $mLang;
+	var $mConvRuleTitle;

+	function getConvRuleTitle() {
+		return $this->mConverter->mConvRuleTitle;
+	}

Use an accessor, don't access member variables directly. By the way, I'd like to see FakeConverter disappear entirely, replaced by things like: 

	function convert( $text, $isTitle = false) {
		if ( $this->mConverter ) {
			return $this->mConverter->convert($text, $isTitle);
		} else {
			return $text;
		}
	}

I think that would be neater.

#Comment by Tim Starling (talk | contribs)   08:29, 20 January 2010

If you're going to remove the $isTitle parameter from LanguageConverter::convert(), you should probably also remove it in Language::convert(), Language::convertHtml() and FakeConverter::convert(). Presumably that's why Philip re-added it in r60797.

#Comment by IAlex (talk | contribs)   22:19, 12 February 2010

Also this or r61101 is causing bug 22501 since line 370 of Parser.php will override the result of the displaytitle parser function.

#Comment by MarkAHershberger (talk | contribs)   01:46, 22 February 2010

See r62539

Status & tagging log