r61101 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r61100‎ | r61101 | r61102 >
Date:19:14, 15 January 2010
Author:mah
Status:resolved (Comments)
Tags:
Comment:
follow up r60832 and follow up r60763

* Don't set Parser::$mTitle to random garbage.
* Remove ParserOutput::$displayTitle, make setDisplayTitle() and
getDisplayTitle() wrappers for their *TitleText() equivalents.
* Remove Parser::$mDo*Convert member variables, move test for
$mDoubleUnderScores[] directives closer to the action.
* Remove bogus "global $wgContLang".
* Use accessor to get at $mConvRuleTitle
* Fix up showtitle option in parserTests.inc
* TODO: refactor FakeConverter class away
Modified paths:
  • /trunk/phase3/includes/OutputPage.php (modified) (history)
  • /trunk/phase3/includes/parser/Parser.php (modified) (history)
  • /trunk/phase3/includes/parser/ParserOutput.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
@@ -28,7 +28,9 @@
2929 $options = array( 'quick', 'color', 'quiet', 'help', 'show-output', 'record'. 'run-disabled' );
3030 $optionsWithArgs = array( 'regex', 'seed', 'setversion' );
3131
32 -require_once( dirname(__FILE__) . '/commandLine.inc' );
 32+if ( !defined( "NO_COMMAND_LINE" ) ) {
 33+ require_once( dirname(__FILE__) . '/commandLine.inc' );
 34+}
3335 require_once( "$IP/maintenance/parserTestsParserHook.php" );
3436 require_once( "$IP/maintenance/parserTestsStaticParserHook.php" );
3537 require_once( "$IP/maintenance/parserTestsParserTime.php" );
@@ -97,7 +99,7 @@
98100 isset( $options['quiet'] )
99101 && ( isset( $options['record'] )
100102 || isset( $options['compare'] ) ) ); // redundant output
101 -
 103+
102104 $this->showOutput = isset( $options['show-output'] );
103105
104106
@@ -359,7 +361,7 @@
360362 }
361363 if (!isset( $data['config'] ) )
362364 $data['config'] = '';
363 -
 365+
364366 if ( (preg_match('/\\bdisabled\\b/i', $data['options'])
365367 || !preg_match("/{$this->regex}/i", $data['test'])) && !$this->runDisabled ) {
366368 # disabled test
@@ -477,6 +479,9 @@
478480 $output = $parser->parse( $input, $title, $options, true, true, 1337 );
479481 $out = $output->getText();
480482
 483+ if ( isset( $opts['showtitle'] ) ) {
 484+ $out = $output->getTitleText() . "\n$out";
 485+ }
481486 if (isset( $opts['ill'] ) ) {
482487 $out = $this->tidy( implode( ' ', $output->getLanguageLinks() ) );
483488 } elseif( isset( $opts['cat'] ) ) {
@@ -493,9 +498,6 @@
494499 $result = $this->tidy($result);
495500 }
496501
497 - if ( isset( $opts['showtitle'] ) ) {
498 - $out = $parser->mTitle . "\n$out";
499 - }
500502
501503 $this->teardownGlobals();
502504
@@ -521,7 +523,7 @@
522524 return $default;
523525 }
524526 }
525 -
 527+
526528 private function parseOptions( $instring ) {
527529 $opts = array();
528530 $lines = explode( "\n", $instring );
@@ -577,7 +579,7 @@
578580 }
579581 return $opts;
580582 }
581 -
 583+
582584 private function cleanupOption( $opt ) {
583585 if( substr( $opt, 0, 1 ) == '"' ) {
584586 return substr( $opt, 1, -1 );
@@ -605,7 +607,7 @@
606608 self::getOptionValue( 'variant', $opts, false );
607609 $maxtoclevel =
608610 self::getOptionValue( 'wgMaxTocLevel', $opts, 999 );
609 - $linkHolderBatchSize =
 611+ $linkHolderBatchSize =
610612 self::getOptionValue( 'wgLinkHolderBatchSize', $opts, 1000 );
611613
612614 $settings = array(
@@ -664,17 +666,19 @@
665667
666668 if ($config) {
667669 $configLines = explode( "\n", $config );
668 -
 670+
669671 foreach( $configLines as $line ) {
670672 list( $var, $value ) = explode( '=', $line, 2 );
671 -
 673+
672674 $settings[$var] = eval("return $value;" );
673675 }
674676 }
675 -
 677+
676678 $this->savedGlobals = array();
677679 foreach( $settings as $var => $val ) {
678 - $this->savedGlobals[$var] = $GLOBALS[$var];
 680+ if( array_key_exists( $var, $GLOBALS ) ) {
 681+ $this->savedGlobals[$var] = $GLOBALS[$var];
 682+ }
679683 $GLOBALS[$var] = $val;
680684 }
681685 $langObj = Language::factory( $lang );
@@ -706,9 +710,9 @@
707711 'archive', 'user_groups', 'page_props', 'category'
708712 );
709713
710 - if ($wgDBtype === 'mysql')
 714+ if ($wgDBtype === 'mysql')
711715 array_push( $tables, 'searchindex' );
712 -
 716+
713717 // Allow extensions to add to the list of tables to duplicate;
714718 // may be necessary if they hook into page save or other code
715719 // which will require them while running tests.
@@ -791,7 +795,7 @@
792796
793797 if ($wgDBtype == 'oracle') {
794798 # Insert 0 and 1 user_ids to prevent FK violations
795 -
 799+
796800 #Anonymous user
797801 $db->insert( 'user', array(
798802 'user_id' => 0,
@@ -867,7 +871,7 @@
868872 $db->query('BEGIN FILL_WIKI_INFO; END;');
869873 */
870874 }
871 -
 875+
872876 /**
873877 * Create a dummy uploads directory which will contain a couple
874878 * of files in order to pass existence tests.
@@ -1310,8 +1314,8 @@
13111315 global $wgDBtype, $wgDBprefix;
13121316 parent::start();
13131317
1314 - if( ! $this->db->tableExists( 'testrun' )
1315 - or ! $this->db->tableExists( 'testitem' ) )
 1318+ if( ! $this->db->tableExists( 'testrun' )
 1319+ or ! $this->db->tableExists( 'testitem' ) )
13161320 {
13171321 print "WARNING> `testrun` table not found in database.\n";
13181322 $this->prevRun = false;
@@ -1347,7 +1351,7 @@
13481352 $res = $this->db->select( 'testitem', array( 'ti_name', 'ti_success' ),
13491353 array( 'ti_run' => $this->prevRun ), __METHOD__ );
13501354 foreach ( $res as $row ) {
1351 - if ( !$this->parent->regex
 1355+ if ( !$this->parent->regex
13521356 || preg_match( "/{$this->parent->regex}/i", $row->ti_name ) )
13531357 {
13541358 $prevResults[$row->ti_name] = $row->ti_success;
@@ -1420,7 +1424,7 @@
14211425
14221426 // Otherwise, this test has previous recorded results.
14231427 // See when this test last had a different result to what we're seeing now.
1424 - $conds = array(
 1428+ $conds = array(
14251429 'ti_name' => $testname,
14261430 'ti_success' => ($after == 'f' ? "1" : "0") );
14271431 if ( $this->curRun ) {
@@ -1483,8 +1487,8 @@
14841488 global $wgDBtype, $wgDBprefix, $options;
14851489 $this->db->begin();
14861490
1487 - if( ! $this->db->tableExists( 'testrun' )
1488 - or ! $this->db->tableExists( 'testitem' ) )
 1491+ if( ! $this->db->tableExists( 'testrun' )
 1492+ or ! $this->db->tableExists( 'testitem' ) )
14891493 {
14901494 print "WARNING> `testrun` table not found in database. Trying to create table.\n";
14911495 if ($wgDBtype === 'postgres')
@@ -1495,7 +1499,7 @@
14961500 $this->db->sourceFile( dirname(__FILE__) . '/testRunner.sql' );
14971501 echo "OK, resuming.\n";
14981502 }
1499 -
 1503+
15001504 parent::start();
15011505
15021506 $this->db->insert( 'testrun',
@@ -1537,17 +1541,17 @@
15381542 $this->results = array();
15391543 $this->ping( 'running' );
15401544 }
1541 -
 1545+
15421546 function record( $test, $result ) {
15431547 parent::record( $test, $result );
15441548 $this->results[$test] = (bool)$result;
15451549 }
1546 -
 1550+
15471551 function end() {
15481552 $this->ping( 'complete', $this->results );
15491553 parent::end();
15501554 }
1551 -
 1555+
15521556 /**
15531557 * Inform a CodeReview instance that we've started or completed a test run...
15541558 * @param $remote array: info on remote target
@@ -1558,16 +1562,16 @@
15591563 */
15601564 function ping( $status, $results=false ) {
15611565 global $wgParserTestRemote, $IP;
1562 -
 1566+
15631567 $remote = $wgParserTestRemote;
15641568 $revId = SpecialVersion::getSvnRevision( $IP );
15651569 $jsonResults = json_encode( $results );
1566 -
 1570+
15671571 if( !$remote ) {
15681572 print "Can't do remote upload without configuring \$wgParserTestRemote!\n";
15691573 exit( 1 );
15701574 }
1571 -
 1575+
15721576 // Generate a hash MAC to validate our credentials
15731577 $message = array(
15741578 $remote['repo'],
@@ -1579,7 +1583,7 @@
15801584 $message[] = $jsonResults;
15811585 }
15821586 $hmac = hash_hmac( "sha1", implode( "|", $message ), $remote['secret'] );
1583 -
 1587+
15841588 $postData = array(
15851589 'action' => 'codetestupload',
15861590 'format' => 'json',
@@ -1593,7 +1597,7 @@
15941598 $postData['results'] = $jsonResults;
15951599 }
15961600 $response = $this->post( $remote['api-url'], $postData );
1597 -
 1601+
15981602 if( $response === false ) {
15991603 print "CodeReview info upload failed to reach server.\n";
16001604 exit( 1 );
@@ -1611,7 +1615,7 @@
16121616 exit( 1 );
16131617 }
16141618 }
1615 -
 1619+
16161620 function post( $url, $data ) {
16171621 // @fixme: for whatever reason, I get a 417 fail when using CURL's multipart form submit.
16181622 // If we do form URL encoding ourselves, though, it should work.
Index: trunk/phase3/maintenance/parserTests.txt
@@ -21,6 +21,7 @@
2222 # language=XXX set content language to XXX for this test
2323 # variant=XXX set the variant of language for this test (eg zh-tw)
2424 # disabled do not run test
 25+# showtitle make the first line the title
2526 # comment run through Linker::formatComment() instead of main parser
2627 # local format section links in edit comment text as local links
2728 #
Index: trunk/phase3/includes/parser/Parser.php
@@ -93,7 +93,7 @@
9494 var $mTagHooks, $mTransparentTagHooks, $mFunctionHooks, $mFunctionSynonyms, $mVariables,
9595 $mImageParams, $mImageParamsMagicArray, $mStripList, $mMarkerIndex, $mPreprocessor,
9696 $mExtLinkBracketedRegex, $mUrlProtocols, $mDefaultStripList, $mVarCache, $mConf,
97 - $mFunctionTagHooks, $mDoTitleConvert, $mDoContentConvert;
 97+ $mFunctionTagHooks;
9898
9999
100100 # Cleared with clearState():
@@ -147,8 +147,6 @@
148148 }
149149 $this->mMarkerIndex = 0;
150150 $this->mFirstCall = true;
151 - $this->mDoTitleConvert = true;
152 - $this->mDoContentConvert = true;
153151 }
154152
155153 /**
@@ -258,19 +256,10 @@
259257 * Set the context title
260258 */
261259 function setTitle( $t ) {
262 - // If don't have a Title object, then convert what we have to
263 - // a string and then to Title. If you pass in an object, make
264 - // sure it has a __toString() method or you'll get a
265 - // "Catchable fatal error"
266 - if ( $t && !($t instanceOf FakeTitle)
267 - && !($t instanceOf Title) ) {
268 - $t = Title::newFromText( "$t" );
269 - }
 260+ if ( !$t || $t instanceof FakeTitle ) {
 261+ $t = Title::newFromText( 'NO TITLE' );
 262+ }
270263
271 - if ( !($t instanceOf Title) ) {
272 - $t = Title::newFromText( 'NO TITLE' );
273 - }
274 -
275264 if ( strval( $t->getFragment() ) !== '' ) {
276265 # Strip the fragment to avoid various odd effects
277266 $this->mTitle = clone $t;
@@ -316,7 +305,7 @@
317306 * to internalParse() which does all the real work.
318307 */
319308
320 - global $wgUseTidy, $wgAlwaysUseTidy, $wgContLang;
 309+ global $wgUseTidy, $wgAlwaysUseTidy, $wgContLang, $wgDisableLangConversion;
321310 $fname = __METHOD__.'-' . wfGetCaller();
322311 wfProfileIn( __METHOD__ );
323312 wfProfileIn( $fname );
@@ -360,7 +349,10 @@
361350 // The position of the convert() call should not be changed. it
362351 // assumes that the links are all replaced and the only thing left
363352 // is the <nowiki> mark.
364 - if ( $this->mDoContentConvert && !$this->mTitle->isConversionTable()) {
 353+ if ( !( $wgDisableLangConversion
 354+ || isset( $this->mDoubleUnderscores['nocontentconvert'] )
 355+ || $this->mTitle->isTalkPage()
 356+ || $this->mTitle->isConversionTable() ) ) {
365357 $text = $wgContLang->convert( $text );
366358 }
367359
@@ -369,9 +361,10 @@
370362 // rule but content conversion was not done, then the parser
371363 // won't pick it up. This is probably expected behavior.
372364 if ( $wgContLang->getConvRuleTitle() ) {
373 - $this->setTitle( $wgContLang->getConvRuleTitle() );
374 - } elseif ( $this->mDoTitleConvert && !$this->mTitle->isConversionTable() ) {
375 - $this->setTitle( $wgContLang->convert( $title ) );
 365+ $this->mOutput->setTitleText( $wgContLang->getConvRuleTitle() );
 366+ } elseif ( !( $wgDisableLangConversion
 367+ || isset( $this->mDoubleUnderscores['notitleconvert'] ) ) ) {
 368+ $this->mOutput->setTitleText( $wgContLang->convert( $title ) );
376369 }
377370
378371 $text = $this->mStripState->unstripNoWiki( $text );
@@ -451,6 +444,7 @@
452445 $text .= "\n<!-- \n$limitReport-->\n";
453446 }
454447 $this->mOutput->setText( $text );
 448+
455449 $this->mRevisionId = $oldRevisionId;
456450 $this->mRevisionTimestamp = $oldRevisionTimestamp;
457451 wfProfileOut( $fname );
@@ -3423,7 +3417,6 @@
34243418 * Fills $this->mDoubleUnderscores, returns the modified text
34253419 */
34263420 function doDoubleUnderscore( $text ) {
3427 - global $wgDisableLangConversion;
34283421 wfProfileIn( __METHOD__ );
34293422
34303423 // The position of __TOC__ needs to be recorded
@@ -3466,18 +3459,6 @@
34673460 $this->addTrackingCategory( 'index-category' );
34683461 }
34693462
3470 - if ( !$wgDisableLangConversion ) {
3471 - if( isset( $this->mDoubleUnderscores['notitleconvert'] ) ){
3472 - $this->mDoTitleConvert = false;
3473 - }
3474 -
3475 - // Don't convert talk pages
3476 - if( isset( $this->mDoubleUnderscores['nocontentconvert'] )
3477 - && !$this->mTitle->isTalkPage() ){
3478 - $this->mDoContentConvert = false;
3479 - }
3480 - }
3481 -
34823463 wfProfileOut( __METHOD__ );
34833464 return $text;
34843465 }
Index: trunk/phase3/includes/parser/ParserOutput.php
@@ -28,11 +28,6 @@
2929 $mTOCHTML = ''; # HTML of the TOC
3030 private $mIndexPolicy = ''; # 'index' or 'noindex'? Any other value will result in no change.
3131
32 - /**
33 - * Overridden title for display
34 - */
35 - private $displayTitle = false;
36 -
3732 function ParserOutput( $text = '', $languageLinks = array(), $categoryLinks = array(),
3833 $containsOldMagic = false, $titletext = '' )
3934 {
@@ -183,7 +178,7 @@
184179 * @param string $text Desired title text
185180 */
186181 public function setDisplayTitle( $text ) {
187 - $this->displayTitle = $text;
 182+ $this->setTitleText( $text );
188183 }
189184
190185 /**
@@ -192,7 +187,10 @@
193188 * @return string
194189 */
195190 public function getDisplayTitle() {
196 - return $this->displayTitle;
 191+ $t = $this->getTitleText( $text );
 192+ if( $t === '' ) {
 193+ return false;
 194+ }
197195 }
198196
199197 /**
Index: trunk/phase3/includes/OutputPage.php
@@ -347,8 +347,6 @@
348348 * Bad tags that were escaped in <h1> will still be escaped in <title>, and good tags like <i> will be dropped entirely.
349349 */
350350 public function setPageTitle( $name ) {
351 - global $wgContLang;
352 -
353351 # change "<script>foo&bar</script>" to "&lt;script&gt;foo&amp;bar&lt;/script&gt;"
354352 # but leave "<i>foobar</i>" alone
355353 $nameWithTags = Sanitizer::normalizeCharReferences( Sanitizer::removeHTMLtags( $name ) );
@@ -617,12 +615,8 @@
618616 }
619617 }
620618 // Page title
621 - $dt = $parserOutput->getDisplayTitle();
622619 $title = $parserOutput->getTitleText();
623 - if ( $dt !== false ) {
624 - $this->setPageTitle( $dt );
625 - }
626 - else if ( $title != '' ) {
 620+ if ( $title != '' ) {
627621 $this->setPageTitle( $title );
628622 }
629623
Index: trunk/phase3/languages/LanguageConverter.php
@@ -131,6 +131,14 @@
132132 }
133133
134134 /**
 135+ * Get the title produced by the conversion rule.
 136+ * @returns string
 137+ */
 138+ function getConvRuleTitle() {
 139+ return $this->mConvRuleTitle;
 140+ }
 141+
 142+ /**
135143 * Get preferred language variants.
136144 * @param boolean $fromUser Get it from $wgUser's preferences
137145 * @param boolean $fromHeader Get it from Accept-Language
Index: trunk/phase3/languages/Language.php
@@ -34,12 +34,12 @@
3535 */
3636 class FakeConverter {
3737 var $mLang;
38 - var $mConvRuleTitle;
3938 function FakeConverter($langobj) {$this->mLang = $langobj;}
4039 function autoConvertToAllVariants($text) {return $text;}
4140 function convert($t, $i) {return $t;}
4241 function getVariants() { return array( $this->mLang->getCode() ); }
4342 function getPreferredVariant() { return $this->mLang->getCode(); }
 43+ function getConvRuleTitle() { return false; }
4444 function findVariantLink(&$l, &$n, $ignoreOtherCond = false) {}
4545 function getExtraHashOptions() {return '';}
4646 function getParsedTitle() {return '';}
@@ -2670,6 +2670,6 @@
26712671 * Get the conversion rule title, if any.
26722672 */
26732673 function getConvRuleTitle() {
2674 - return $this->mConverter->mConvRuleTitle;
 2674+ return $this->mConverter->getConvRuleTitle();
26752675 }
26762676 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r61102follow up r61101, fix cut-n-paste errormah19:48, 15 January 2010
r62502follow up r61101 for special pages like Special:Version and Special:RecentCha...mah08:36, 15 February 2010
r62539fixes #22501 follow up r61101 remove superfluous, buggy code that was over-ri...mah20:18, 15 February 2010
r64814Follow-up r64811 on fixing bug 23115....platonides17:56, 9 April 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
r60832follow up r60763...mah08:22, 8 January 2010

Comments

#Comment by Raymond (talk | contribs)   19:39, 15 January 2010

Seen on translatewiki:

PHP Notice: Undefined variable: text in /var/www/w/includes/parser/ParserOutput.php on line 190

#Comment by Raymond (talk | contribs)   19:50, 15 January 2010

Fixed with r61102.

#Comment by Nikerabbit (talk | contribs)   17:25, 13 February 2010

This seems to break some special page page titles, at least Special:RecentChanges on translatewiki.net

#Comment by Conrad.Irwin (talk | contribs)   16:55, 15 February 2010

This seems to have broken DISPLAYTITLE too bugzilla:22501

#Comment by Simetrical (talk | contribs)   21:44, 15 February 2010

Marking fixme because of bug 22501 . . . why are these changes needed, exactly?

#Comment by MarkAHershberger (talk | contribs)   22:45, 15 February 2010

I should have posted here that this was fixed in the follow-up r62539, but I thought the commit message and taking off the "fixme" was enough.

The changes to Parser.php were part of a bug fix to the LanguageConverter.php code that didn't correctly obey the -{T}- directive that I did at TimStarling's request.

Status & tagging log