r90337 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r90336‎ | r90337 | r90338 >
Date:14:49, 18 June 2011
Author:robin
Status:resolved (Comments)
Tags:
Comment:
Address comment by Platonides on r90320:
* undefined variable $list in CategoryPage.php
* move code in ParserOptions to a new member of Title class, which falls back to $wgContLang
Modified paths:
  • /trunk/phase3/includes/CategoryPage.php (modified) (history)
  • /trunk/phase3/includes/EditPage.php (modified) (history)
  • /trunk/phase3/includes/SkinTemplate.php (modified) (history)
  • /trunk/phase3/includes/Title.php (modified) (history)
  • /trunk/phase3/includes/diff/DifferenceEngine.php (modified) (history)
  • /trunk/phase3/includes/parser/ParserOptions.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/CategoryPage.php
——@@ -506,19 +506,18 @@
507507 * @private
508508 */
509509 function formatList( $articles, $articles_start_char, $cutoff = 6 ) {
 510+ $list = '';
510511 if ( count ( $articles ) > $cutoff ) {
511512 $list = self::columnList( $articles, $articles_start_char );
512513 } elseif ( count( $articles ) > 0 ) {
513514 // for short lists of articles in categories.
514515 $list = self::shortList( $articles, $articles_start_char );
515516 }
516 - global $wgBetterDirectionality;
 517+ global $wgBetterDirectionality, $wgTitle;
517518 if( $wgBetterDirectionality ) {
518 - global $wgOut, $wgContLang;
519 - $getPageLang = $wgOut->parserOptions()->getTargetLanguage();
520 - $pageLang = ( $getPageLang ? Language::factory( $getPageLang ) : $wgContLang );
521 - $realBodyAttribs = array( 'lang' => $pageLang->getCode(), 'dir' => $pageLang->getDir() );
522 - $list = Html::rawElement( 'div', $realBodyAttribs, $list );
 519+ $pageLang = $wgTitle->getPageLanguage();
 520+ $attribs = array( 'lang' => $pageLang->getCode(), 'dir' => $pageLang->getDir() );
 521+ $list = Html::rawElement( 'div', $attribs, $list );
523522 }
524523
525524 return $list;
Index: trunk/phase3/includes/diff/DifferenceEngine.php
——@@ -936,14 +936,12 @@
937937 * @return string
938938 */
939939 static function addHeader( $diff, $otitle, $ntitle, $multi = '', $notice = '' ) {
940 - global $wgBetterDirectionality;
 940+ global $wgBetterDirectionality, $wgTitle;
941941 $dirclass = '';
942942 if( $wgBetterDirectionality ) {
943 - global $wgContLang, $wgOut, $wgTitle;
944943 // shared.css sets diff in interface language/dir,
945944 // but the actual content should be in the page language/dir
946 - $getPageLang = $wgOut->parserOptions()->getTargetLanguage( $wgTitle );
947 - $pageLang = ( $getPageLang ? $getPageLang : $wgContLang );
 945+ $pageLang = $wgTitle->getPageLanguage();
948946 $dirclass = ' diff-contentalign-'.$pageLang->alignStart();
949947 }
950948 $header = "<table class='diff $dirclass'>";
Index: trunk/phase3/includes/parser/ParserOptions.php
——@@ -66,17 +66,7 @@
6767 function getAllowSpecialInclusion() { return $this->mAllowSpecialInclusion; }
6868 function getTidy() { return $this->mTidy; }
6969 function getInterfaceMessage() { return $this->mInterfaceMessage; }
70 - function getTargetLanguage( $title = null ) {
71 - if ( $title && $title->isCssOrJsPage() ) {
72 - return Language::factory( 'en' ); // css/js should always be LTR
73 - } elseif ( $title && $title->getNamespace() == NS_MEDIAWIKI ) {
74 - // Parse mediawiki messages with correct target language
75 - list( /* $unused */, $lang ) = MessageCache::singleton()->figureMessage( $title->getText() );
76 - $obj = wfGetLangObj( $lang );
77 - return $obj;
78 - }
79 - return $this->mTargetLanguage;
80 -}
 70+ function getTargetLanguage() { return $this->mTargetLanguage; }
8171 function getMaxIncludeSize() { return $this->mMaxIncludeSize; }
8272 function getMaxPPNodeCount() { return $this->mMaxPPNodeCount; }
8373 function getMaxPPExpandDepth() { return $this->mMaxPPExpandDepth; }
Index: trunk/phase3/includes/EditPage.php
——@@ -1817,8 +1817,7 @@
18181818
18191819 global $wgBetterDirectionality, $wgContLang;
18201820 if( $wgBetterDirectionality ) {
1821 - $getPageLang = $wgOut->parserOptions()->getTargetLanguage( $this->mTitle );
1822 - $pageLang = ( $getPageLang ? $getPageLang : $wgContLang );
 1821+ $pageLang = $this->mTitle->getPageLanguage();
18231822 $attribs['lang'] = $pageLang->getCode();
18241823 $attribs['dir'] = $pageLang->getDir();
18251824 }
——@@ -2066,6 +2065,12 @@
20672066
20682067 wfRunHooks( 'EditPageGetPreviewText', array( $this, &$toparse ) );
20692068
 2069+ // In which language to parse the page
 2070+ // (Should this still be only for MediaWiki pages, or for all pages?)
 2071+ if ( $this->mTitle->getNamespace() == NS_MEDIAWIKI ) {
 2072+ $parserOptions->setTargetLanguage( $this->mTitle->getPageLanguage() );
 2073+ }
 2074+ $parserOptions->setTargetLanguage( $this->mTitle->getPageLanguage() );
20702075 $parserOptions->setTidy( true );
20712076 $parserOptions->enableLimitReport();
20722077 $parserOutput = $wgParser->parse( $this->mArticle->preSaveTransform( $toparse ),
——@@ -2091,12 +2096,11 @@
20922097 '<h2 id="mw-previewheader">' . htmlspecialchars( wfMsg( 'preview' ) ) . "</h2>" .
20932098 $wgOut->parse( $note ) . $conflict . "</div>\n";
20942099
2095 - global $wgBetterDirectionality, $wgContLang;
 2100+ global $wgBetterDirectionality;
20962101 if( $wgBetterDirectionality ) {
2097 - $getPageLang = $wgOut->parserOptions()->getTargetLanguage( $this->mTitle );
2098 - $pageLang = ( $getPageLang ? $getPageLang : $wgContLang );
2099 - $realBodyAttribs = array( 'lang' => $pageLang->getCode(), 'dir' => $pageLang->getDir() );
2100 - $previewHTML = Html::rawElement( 'div', $realBodyAttribs, $previewHTML );
 2102+ $pageLang = $this->mTitle->getPageLanguage();
 2103+ $attribs = array( 'lang' => $pageLang->getCode(), 'dir' => $pageLang->getDir() );
 2104+ $previewHTML = Html::rawElement( 'div', $attribs, $previewHTML );
21012105 }
21022106 wfProfileOut( __METHOD__ );
21032107 return $previewhead . $previewHTML . $this->previewTextAfterContent;
Index: trunk/phase3/includes/Title.php
——@@ -67,6 +67,7 @@
6868 var $mRedirect = null; // /< Is the article at this title a redirect?
6969 var $mNotificationTimestamp = array(); // /< Associative array of user ID -> timestamp/false
7070 var $mBacklinkCache = null; // /< Cache of links to this title
 71+ var $mPageLanguage;
7172 // @}
7273
7374
——@@ -4221,6 +4222,26 @@
42224223 }
42234224 return $unprefixed;
42244225 }
 4226+
 4227+ /**
 4228+ * Get the language this page is written in
 4229+ * Defaults to $wgContLang
 4230+ *
 4231+ * @return object Language
 4232+ */
 4233+ public function getPageLanguage() {
 4234+ global $wgContLang;
 4235+ $this->mPageLanguage = $wgContLang;
 4236+ if ( $this->isCssOrJsPage() ) {
 4237+ // css/js should always be LTR and is, in fact, English
 4238+ $this->mPageLanguage = Language::factory( 'en' );
 4239+ } elseif ( $this->getNamespace() == NS_MEDIAWIKI ) {
 4240+ // Parse mediawiki messages with correct target language
 4241+ list( /* $unused */, $lang ) = MessageCache::singleton()->figureMessage( $this->getText() );
 4242+ $this->mPageLanguage = wfGetLangObj( $lang );
 4243+ }
 4244+ return $this->mPageLanguage;
 4245+ }
42254246 }
42264247
42274248 /**
Index: trunk/phase3/includes/SkinTemplate.php
——@@ -461,8 +461,7 @@
462462 if( $this->getTitle()->getNamespace() != NS_SPECIAL &&
463463 in_array( $action, array( 'view', 'render', 'print' ) ) &&
464464 ( $this->getTitle()->exists() || $this->getTitle()->getNamespace() == NS_MEDIAWIKI ) ) {
465 - $getPageLang = $out->parserOptions()->getTargetLanguage( $this->getTitle() );
466 - $pageLang = ( $getPageLang ? $getPageLang : $wgContLang );
 465+ $pageLang = $this->getTitle()->getPageLanguage();
467466 $realBodyAttribs = array( 'lang' => $pageLang->getCode(), 'dir' => $pageLang->getDir() );
468467 $out->mBodytext = Html::rawElement( 'div', $realBodyAttribs, $out->mBodytext );
469468 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r90342Fix double setTargetLanguage of r90337robin15:08, 18 June 2011
r90515Per comment by Platonides on r90337: use wfGetLangObj() instead of Language::...robin09:45, 21 June 2011
r91066Follow-up r90334 and r90337: made DifferenceEngine::addHeader() non-static to...ialex10:51, 29 June 2011
r91401Fix for r90337 and r90743: Use $this->title instead of $wgTitle, also grouppe...ialex08:40, 4 July 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r90320Follow-up to r90265: directionality improvements as part of bug 6100 (under $...robin21:48, 17 June 2011

Comments

#Comment by Platonides (talk | contribs)   18:42, 19 June 2011

Looks much better. There's the double setTargetLanguage that you already noticed in r90342

I'm not sure about "css/js is, in fact, English", perhaps we should conceptually create a x-js language, but it's perfectly good for now.

On Title::getPageLanguage() you don't need this->mPageLanguage. If you are calculating it on each time, there's no point in storing it in a property. If it was costly (eg. benchmark shows figureMessage() to be slow) then you could keep the result in mPageLanguage as a cache, but further calls would then directly use the calculated value.

#Comment by Platonides (talk | contribs)   18:50, 19 June 2011

You should call wfGetLangObj() instead of Language::factory(), so it doesn't create a new Language object if there's already one.

#Comment by SPQRobin (talk | contribs)   09:47, 21 June 2011

Ok, done in r90515.

#Comment by Nikerabbit (talk | contribs)   11:21, 21 June 2011

pagelanguage is ambiguous - does it refer to content or the interface (since those two can be different).

#Comment by SPQRobin (talk | contribs)   11:48, 21 June 2011

'page language' refers to language in which the content of the current page is written. If it isn't clear enough, then we should change the description above the function.

(See also bug 28970.)

#Comment by Nikerabbit (talk | contribs)   17:46, 21 June 2011

What's wrong with calling it content language if that is what it is about?

#Comment by SPQRobin (talk | contribs)   17:59, 21 June 2011

To make a distinction between the site content language and the page content language.

#Comment by Nikerabbit (talk | contribs)   18:00, 21 June 2011

PageContentLanguage then? :)

#Comment by SPQRobin (talk | contribs)   18:04, 21 June 2011

Yes. I chose for a shorter name, but it can be changed if it's not clear enough.

#Comment by SPQRobin (talk | contribs)   01:42, 15 August 2011

Marking as new. Dunno why this was still marked fixme.

Status & tagging log