r96669 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r96668‎ | r96669 | r96670 >
Date:15:41, 9 September 2011
Author:dantman
Status:ok (Comments)
Tags:
Comment:
Kill off $wg{Request,User,Lang,Output} usage within skins/ and redundant $this->skin local vars.
Modified paths:
  • /trunk/phase3/skins/CologneBlue.php (modified) (history)
  • /trunk/phase3/skins/Modern.php (modified) (history)
  • /trunk/phase3/skins/Nostalgia.php (modified) (history)
  • /trunk/phase3/skins/Simple.php (modified) (history)
  • /trunk/phase3/skins/Standard.php (modified) (history)
  • /trunk/phase3/skins/Vector.php (modified) (history)

Diff [purge]

Index: trunk/phase3/skins/Simple.php
——@@ -30,16 +30,15 @@
3131 $out->addModuleStyles( 'skins.simple' );
3232
3333 /* Add some userprefs specific CSS styling */
34 - global $wgUser;
3534 $rules = array();
3635 $underline = "";
3736
38 - if ( $wgUser->getOption( 'underline' ) < 2 ) {
39 - $underline = "text-decoration: " . $wgUser->getOption( 'underline' ) ? 'underline !important' : 'none' . ";";
 37+ if ( $this->getSkin()->getUser()->getOption( 'underline' ) < 2 ) {
 38+ $underline = "text-decoration: " . $this->getSkin()->getUser()->getOption( 'underline' ) ? 'underline !important' : 'none' . ";";
4039 }
4140
4241 /* Also inherits from resourceloader */
43 - if( !$wgUser->getOption( 'highlightbroken' ) ) {
 42+ if( !$this->getSkin()->getUser()->getOption( 'highlightbroken' ) ) {
4443 $rules[] = "a.new, a.stub { color: inherit; text-decoration: inherit;}";
4544 $rules[] = "a.new:after { color: #CC2200; $underline;}";
4645 $rules[] = "a.stub:after { $underline; }";
Index: trunk/phase3/skins/CologneBlue.php
——@@ -101,8 +101,6 @@
102102 * @return string
103103 */
104104 function doAfterContent(){
105 - global $wgLang;
106 -
107105 $s = "\n</div><br clear='all' />\n";
108106
109107 $s .= "\n<div id='footer'>";
——@@ -115,7 +113,7 @@
116114 $s .= '<td class="bottom">';
117115
118116 $s .= $this->bottomLinks();
119 - $s .= $wgLang->pipeList( array(
 117+ $s .= $this->getSkin()->getLang()->pipeList( array(
120118 "\n<br />" . Linker::link(
121119 Title::newMainPage(),
122120 null,
——@@ -145,7 +143,6 @@
146144 * @return string
147145 */
148146 function sysLinks() {
149 - global $wgUser, $wgLang;
150147 $li = SpecialPage::getTitleFor( 'Userlogin' );
151148 $lo = SpecialPage::getTitleFor( 'Userlogout' );
152149
——@@ -180,7 +177,7 @@
181178 if( $this->extensionTabLinks() ) {
182179 $s[] = $this->extensionTabLinks();
183180 }
184 - if ( $wgUser->isLoggedIn() ) {
 181+ if ( $this->data['loggedin'] ) {
185182 $s[] = Linker::linkKnown(
186183 $lo,
187184 wfMsg( 'logout' ),
——@@ -196,7 +193,7 @@
197194 );
198195 }
199196
200 - return $wgLang->pipeList( $s );
 197+ return $this->getSkin()->getLang()->pipeList( $s );
201198 }
202199
203200 /**
——@@ -206,8 +203,6 @@
207204 * @return string
208205 */
209206 function quickBar(){
210 - global $wgOut, $wgUser;
211 -
212207 $tns = $this->getSkin()->getTitle()->getNamespace();
213208
214209 $s = "\n<div id='quickbar'>";
——@@ -246,7 +241,7 @@
247242 $barnumber++;
248243 }
249244
250 - if ( $wgOut->isArticle() ) {
 245+ if ( $this->data['isarticle'] ) {
251246 $s .= $this->menuHead( 'qbedit' );
252247 $s .= '<strong>' . $this->editThisPage() . '</strong>';
253248
——@@ -255,16 +250,16 @@
256251 wfMsg( 'edithelp' )
257252 );
258253
259 - if( $wgUser->isLoggedIn() ) {
 254+ if( $this->data['loggedin'] ) {
260255 $s .= $sep . $this->moveThisPage();
261256 }
262 - if ( $wgUser->isAllowed( 'delete' ) ) {
 257+ if ( $this->getSkin()->getUser()->isAllowed( 'delete' ) ) {
263258 $dtp = $this->deleteThisPage();
264259 if ( $dtp != '' ) {
265260 $s .= $sep . $dtp;
266261 }
267262 }
268 - if ( $wgUser->isAllowed( 'protect' ) ) {
 263+ if ( $this->getSkin()->getUser()->isAllowed( 'protect' ) ) {
269264 $ptp = $this->protectThisPage();
270265 if ( $ptp != '' ) {
271266 $s .= $sep . $ptp;
——@@ -276,7 +271,7 @@
277272 $s .= $this->talkLink()
278273 . $sep . $this->commentLink()
279274 . $sep . $this->printableLink();
280 - if ( $wgUser->isLoggedIn() ) {
 275+ if ( $this->data['loggedin'] ) {
281276 $s .= $sep . $this->watchThisPage();
282277 }
283278
——@@ -300,20 +295,20 @@
301296 }
302297
303298 $s .= $this->menuHead( 'qbmyoptions' );
304 - if ( $wgUser->isLoggedIn() ) {
 299+ if ( $this->data['loggedin'] ) {
305300 $tl = Linker::link(
306 - $wgUser->getTalkPage(),
 301+ $this->getSkin()->getUser()->getTalkPage(),
307302 wfMsg( 'mytalk' ),
308303 array(),
309304 array(),
310305 array( 'known', 'noclasses' )
311306 );
312 - if ( $wgUser->getNewtalk() ) {
 307+ if ( $this->getSkin()->getUser()->getNewtalk() ) {
313308 $tl .= ' *';
314309 }
315310
316311 $s .= Linker::link(
317 - $wgUser->getUserPage(),
 312+ $this->getSkin()->getUser()->getUserPage(),
318313 wfMsg( 'mypage' ),
319314 array(),
320315 array(),
——@@ -321,7 +316,7 @@
322317 ) . $sep . $tl . $sep . Linker::specialLink( 'Watchlist' )
323318 . $sep .
324319 Linker::link(
325 - SpecialPage::getSafeTitleFor( 'Contributions', $wgUser->getName() ),
 320+ SpecialPage::getSafeTitleFor( 'Contributions', $this->getSkin()->getUser()->getName() ),
326321 wfMsg( 'mycontris' ),
327322 array(),
328323 array(),
——@@ -336,7 +331,7 @@
337332 . Linker::specialLink( 'Newpages' )
338333 . $sep . Linker::specialLink( 'Listfiles' )
339334 . $sep . Linker::specialLink( 'Statistics' );
340 - if( UploadBase::isEnabled() && UploadBase::isAllowed( $wgUser ) === true ) {
 335+ if( UploadBase::isEnabled() && UploadBase::isAllowed( $this->getSkin()->getUser() ) === true ) {
341336 $s .= $sep . $this->getUploadLink();
342337 }
343338
——@@ -373,9 +368,9 @@
374369 * @return string
375370 */
376371 function searchForm( $label = '' ) {
377 - global $wgRequest, $wgUseTwoButtonsSearchForm;
 372+ global $wgUseTwoButtonsSearchForm;
378373
379 - $search = $wgRequest->getText( 'search' );
 374+ $search = $this->getSkin()->getRequest()->getText( 'search' );
380375 $action = $this->data['searchaction'];
381376 $s = "<form id=\"searchform{$this->searchboxes}\" method=\"get\" class=\"inline\" action=\"$action\">";
382377 if( $label != '' ) {
Index: trunk/phase3/skins/Standard.php
——@@ -54,7 +54,7 @@
5555 * @return string
5656 */
5757 function doAfterContent() {
58 - global $wgContLang, $wgLang;
 58+ global $wgContLang;
5959 wfProfileIn( __METHOD__ );
6060 wfProfileIn( __METHOD__ . '-1' );
6161
——@@ -78,7 +78,7 @@
7979 $s .= "<td class='bottom' align='$l' valign='top'>";
8080
8181 $s .= $this->bottomLinks();
82 - $s .= "\n<br />" . $wgLang->pipeList( array(
 82+ $s .= "\n<br />" . $this->getSkin()->getLang()->pipeList( array(
8383 $this->getSkin()->mainPageLink(),
8484 $this->getSkin()->aboutLink(),
8585 Linker::specialLink( 'Recentchanges' ),
——@@ -105,12 +105,12 @@
106106 * @return string
107107 */
108108 function quickBar() {
109 - global $wgOut, $wgUser, $wgRequest, $wgContLang;
 109+ global $wgContLang;
110110
111111 wfProfileIn( __METHOD__ );
112112
113 - $action = $wgRequest->getText( 'action' );
114 - $wpPreview = $wgRequest->getBool( 'wpPreview' );
 113+ $action = $this->getSkin()->getRequest()->getText( 'action' );
 114+ $wpPreview = $this->getSkin()->getRequest()->getBool( 'wpPreview' );
115115 $title = $this->getSkin()->getTitle();
116116 $tns = $title->getNamespace();
117117
——@@ -138,13 +138,13 @@
139139 }
140140 if ( $barnumber == 1 ) {
141141 // only show watchlist link if logged in
142 - if( $wgUser->isLoggedIn() ) {
 142+ if( $this->data['loggedin'] ) {
143143 $s.= Linker::specialLink( 'Watchlist' ) ;
144144 $s .= $sep . Linker::linkKnown(
145145 SpecialPage::getTitleFor( 'Contributions' ),
146146 wfMsg( 'mycontris' ),
147147 array(),
148 - array( 'target' => $wgUser->getName() )
 148+ array( 'target' => $this->data['username'] )
149149 );
150150 }
151151 }
——@@ -153,8 +153,8 @@
154154
155155 $s .= "\n<hr class='sep' />";
156156 $articleExists = $title->getArticleId();
157 - if ( $wgOut->isArticle() || $action == 'edit' || $action == 'history' || $wpPreview ) {
158 - if( $wgOut->isArticle() ) {
 157+ if ( $this->data['isarticle'] || $action == 'edit' || $action == 'history' || $wpPreview ) {
 158+ if( $this->data['isarticle'] ) {
159159 $s .= '<strong>' . $this->editThisPage() . '</strong>';
160160 } else { # backlink to the article in edit or history mode
161161 if( $articleExists ){ # no backlink if no article
——@@ -212,7 +212,7 @@
213213 }
214214
215215 # "Post a comment" link
216 - if( ( $title->isTalkPage() || $wgOut->showNewSectionLink() ) && $action != 'edit' && !$wpPreview )
 216+ if( ( $title->isTalkPage() || $this->getSkin()->getOutput()->showNewSectionLink() ) && $action != 'edit' && !$wpPreview )
217217 $s .= '<br />' . Linker::link(
218218 $title,
219219 wfMsg( 'postcomment' ),
——@@ -229,14 +229,14 @@
230230 article with "Watch this article" checkbox disabled, the article is transparently
231231 unwatched. Therefore we do not show the "Watch this page" link in edit mode
232232 */
233 - if ( $wgUser->isLoggedIn() && $articleExists ) {
 233+ if ( $this->data['loggedin'] && $articleExists ) {
234234 if( $action != 'edit' && $action != 'submit' ) {
235235 $s .= $sep . $this->watchThisPage();
236236 }
237237 if ( $title->userCan( 'edit' ) )
238238 $s .= $sep . $this->moveThisPage();
239239 }
240 - if ( $wgUser->isAllowed( 'delete' ) && $articleExists ) {
 240+ if ( $this->getSkin()->getUser()->isAllowed( 'delete' ) && $articleExists ) {
241241 $s .= $sep . $this->deleteThisPage() .
242242 $sep . $this->protectThisPage();
243243 }
——@@ -246,7 +246,7 @@
247247 }
248248 $s .= $sep . $this->whatLinksHere();
249249
250 - if( $wgOut->isArticleRelated() ) {
 250+ if( $this->getSkin()->getOutput()->isArticleRelated() ) {
251251 $s .= $sep . $this->watchPageLinksLink();
252252 }
253253
——@@ -268,7 +268,7 @@
269269 $s .= "\n<br /><hr class='sep' />";
270270 }
271271
272 - if( UploadBase::isEnabled() && UploadBase::isAllowed( $wgUser ) === true ) {
 272+ if( UploadBase::isEnabled() && UploadBase::isAllowed( $this->getSkin()->getUser() ) === true ) {
273273 $s .= $this->getUploadLink() . $sep;
274274 }
275275
Index: trunk/phase3/skins/Vector.php
——@@ -26,14 +26,14 @@
2727 * @param $out OutputPage object to initialize
2828 */
2929 public function initPage( OutputPage $out ) {
30 - global $wgLocalStylePath, $wgRequest;
 30+ global $wgLocalStylePath;
3131
3232 parent::initPage( $out );
3333
3434 // Append CSS which includes IE only behavior fixes for hover support -
3535 // this is better than including this in a CSS fille since it doesn't
3636 // wait for the CSS file to load before fetching the HTC file.
37 - $min = $wgRequest->getFuzzyBool( 'debug' ) ? '' : '.min';
 37+ $min = $this->getRequest()->getFuzzyBool( 'debug' ) ? '' : '.min';
3838 $out->addHeadItem( 'csshover',
3939 '<!--[if lt IE 7]><style type="text/css">body{behavior:url("' .
4040 htmlspecialchars( $wgLocalStylePath ) .
——@@ -60,29 +60,19 @@
6161 */
6262 class VectorTemplate extends BaseTemplate {
6363
64 - /* Members */
65 -
66 - /**
67 - * @var Skin Cached skin object
68 - */
69 - var $skin;
70 -
7164 /* Functions */
7265
7366 /**
7467 * Outputs the entire contents of the (X)HTML page
7568 */
7669 public function execute() {
77 - global $wgLang, $wgVectorUseIconWatch;
 70+ global $wgVectorUseIconWatch;
7871
79 - $this->skin = $this->data['skin'];
80 -
8172 // Build additional attributes for navigation urls
82 - //$nav = $this->skin->buildNavigationUrls();
8373 $nav = $this->data['content_navigation'];
8474
8575 if ( $wgVectorUseIconWatch ) {
86 - $mode = $this->skin->getTitle()->userIsWatching() ? 'unwatch' : 'watch';
 76+ $mode = $this->getSkin()->getTitle()->userIsWatching() ? 'unwatch' : 'watch';
8777 if ( isset( $nav['actions'][$mode] ) ) {
8878 $nav['views'][$mode] = $nav['actions'][$mode];
8979 $nav['views'][$mode]['class'] = rtrim( 'icon ' . $nav['views'][$mode]['class'], ' ' );
——@@ -121,7 +111,7 @@
122112 $this->data['variant_urls'] = $nav['variants'];
123113
124114 // Reverse horizontally rendered navigation elements
125 - if ( $wgLang->isRTL() ) {
 115+ if ( $this->data['rtl'] ) {
126116 $this->data['view_urls'] =
127117 array_reverse( $this->data['view_urls'] );
128118 $this->data['namespace_urls'] =
——@@ -236,7 +226,7 @@
237227 <?php foreach ( $footericons as $blockName => $footerIcons ): ?>
238228 <li id="footer-<?php echo htmlspecialchars( $blockName ); ?>ico">
239229 <?php foreach ( $footerIcons as $icon ): ?>
240 - <?php echo $this->skin->makeFooterIcon( $icon ); ?>
 230+ <?php echo $this->getSkin()->makeFooterIcon( $icon ); ?>
241231
242232 <?php endforeach; ?>
243233 </li>
——@@ -336,14 +326,14 @@
337327 * @param $elements array
338328 */
339329 private function renderNavigation( $elements ) {
340 - global $wgVectorUseSimpleSearch, $wgVectorShowVariantName, $wgUser, $wgLang;
 330+ global $wgVectorUseSimpleSearch, $wgVectorShowVariantName;
341331
342332 // If only one element was given, wrap it in an array, allowing more
343333 // flexible arguments
344334 if ( !is_array( $elements ) ) {
345335 $elements = array( $elements );
346336 // If there's a series of elements, reverse them when in RTL mode
347 - } elseif ( $wgLang->isRTL() ) {
 337+ } elseif ( $this->data['rtl'] ) {
348338 $elements = array_reverse( $elements );
349339 }
350340 // Render elements
——@@ -435,14 +425,14 @@
436426 <h5<?php $this->html( 'userlangattributes' ) ?>><label for="searchInput"><?php $this->msg( 'search' ) ?></label></h5>
437427 <form action="<?php $this->text( 'wgScript' ) ?>" id="searchform">
438428 <input type='hidden' name="title" value="<?php $this->text( 'searchtitle' ) ?>"/>
439 - <?php if ( $wgVectorUseSimpleSearch && $wgUser->getOption( 'vector-simplesearch' ) ): ?>
 429+ <?php if ( $wgVectorUseSimpleSearch && $this->getSkin()->getUser()->getOption( 'vector-simplesearch' ) ): ?>
440430 <div id="simpleSearch">
441431 <?php if ( $this->data['rtl'] ): ?>
442 - <?php echo $this->makeSearchButton( 'image', array( 'id' => 'searchButton', 'src' => $this->skin->getSkinStylePath( 'images/search-rtl.png' ) ) ); ?>
 432+ <?php echo $this->makeSearchButton( 'image', array( 'id' => 'searchButton', 'src' => $this->getSkin()->getSkinStylePath( 'images/search-rtl.png' ) ) ); ?>
443433 <?php endif; ?>
444434 <?php echo $this->makeSearchInput( array( 'id' => 'searchInput', 'type' => 'text' ) ); ?>
445435 <?php if ( !$this->data['rtl'] ): ?>
446 - <?php echo $this->makeSearchButton( 'image', array( 'id' => 'searchButton', 'src' => $this->skin->getSkinStylePath( 'images/search-ltr.png' ) ) ); ?>
 436+ <?php echo $this->makeSearchButton( 'image', array( 'id' => 'searchButton', 'src' => $this->getSkin()->getSkinStylePath( 'images/search-ltr.png' ) ) ); ?>
447437 <?php endif; ?>
448438 </div>
449439 <?php else: ?>
Index: trunk/phase3/skins/Nostalgia.php
——@@ -69,13 +69,12 @@
7070 * @return string
7171 */
7272 function topLinks() {
73 - global $wgOut, $wgUser;
7473 $sep = " |\n";
7574
7675 $s = $this->getSkin()->mainPageLink() . $sep
7776 . Linker::specialLink( 'Recentchanges' );
7877
79 - if ( $wgOut->isArticle() ) {
 78+ if ( $this->data['isarticle'] ) {
8079 $s .= $sep . '<strong>' . $this->editThisPage() . '</strong>' . $sep . $this->talkLink() .
8180 $sep . $this->historyLink();
8281 }
——@@ -83,25 +82,25 @@
8483 /* show links to different language variants */
8584 $s .= $this->variantLinks();
8685 $s .= $this->extensionTabLinks();
87 - if ( $wgUser->isAnon() ) {
 86+ if ( !$this->data['loggedin'] ) {
8887 $s .= $sep . Linker::specialLink( 'Userlogin' );
8988 } else {
9089 /* show user page and user talk links */
91 - $s .= $sep . Linker::link( $wgUser->getUserPage(), wfMsgHtml( 'mypage' ) );
92 - $s .= $sep . Linker::link( $wgUser->getTalkPage(), wfMsgHtml( 'mytalk' ) );
93 - if ( $wgUser->getNewtalk() ) {
 90+ $s .= $sep . Linker::link( $this->getSkin()->getUser()->getUserPage(), wfMsgHtml( 'mypage' ) );
 91+ $s .= $sep . Linker::link( $this->getSkin()->getUser()->getTalkPage(), wfMsgHtml( 'mytalk' ) );
 92+ if ( $this->getSkin()->getUser()->getNewtalk() ) {
9493 $s .= ' *';
9594 }
9695 /* show watchlist link */
9796 $s .= $sep . Linker::specialLink( 'Watchlist' );
9897 /* show my contributions link */
9998 $s .= $sep . Linker::link(
100 - SpecialPage::getSafeTitleFor( 'Contributions', $wgUser->getName() ),
 99+ SpecialPage::getSafeTitleFor( 'Contributions', $this->data['username'] ),
101100 wfMsgHtml( 'mycontris' ) );
102101 /* show my preferences link */
103102 $s .= $sep . Linker::specialLink( 'Preferences' );
104103 /* show upload file link */
105 - if( UploadBase::isEnabled() && UploadBase::isAllowed( $wgUser ) === true ) {
 104+ if( UploadBase::isEnabled() && UploadBase::isAllowed( $this->getSkin()->getUser() ) === true ) {
106105 $s .= $sep . $this->getUploadLink();
107106 }
108107
Index: trunk/phase3/skins/Modern.php
——@@ -37,10 +37,6 @@
3838 class ModernTemplate extends MonoBookTemplate {
3939
4040 /**
41 - * @var Skin
42 - */
43 - var $skin;
44 - /**
4541 * Template filter callback for Modern skin.
4642 * Takes an associative array of data set from a SkinTemplate-based
4743 * class, and a wrapper for MediaWiki's localization database, and
——@@ -49,8 +45,6 @@
5046 * @access private
5147 */
5248 function execute() {
53 - $this->skin = $skin = $this->data['skin'];
54 -
5549 // Suppress warnings to prevent notices about missing indexes in $this->data
5650 wfSuppressWarnings();
5751
——@@ -137,7 +131,7 @@
138132 <div id="mw_<?php echo htmlspecialchars($blockName); ?>">
139133 <?php
140134 foreach ( $footerIcons as $icon ) { ?>
141 - <?php echo $this->skin->makeFooterIcon( $icon, 'withoutImage' ); ?>
 135+ <?php echo $this->getSkin()->makeFooterIcon( $icon, 'withoutImage' ); ?>
142136
143137 <?php
144138 } ?>

Follow-up revisions

RevisionCommit summaryAuthorDate
r96750Simplify some contextsreedy22:09, 10 September 2011
r97915Factorise calls to $this->getSkin()->getUser() in SkinCologneBlue::quickBar()...ialex12:55, 23 September 2011

Comments

#Comment by Reedy (talk | contribs)   16:00, 9 September 2011 
-		if ( $wgUser->getOption( 'underline' ) < 2 ) {
-			$underline = "text-decoration: " . $wgUser->getOption( 'underline' ) ? 'underline !important' : 'none' . ";";
+		if ( $this->getSkin()->getUser()->getOption( 'underline' ) < 2 ) {
+			$underline = "text-decoration: " . $this->getSkin()->getUser()->getOption( 'underline' ) ? 'underline !important' : 'none' . ";";
 		}

Can we just not use $this->getUser() ?

#Comment by Dantman (talk | contribs)   16:02, 9 September 2011

...there's no getUser() on QuickTemplate.

#Comment by Reedy (talk | contribs)   16:01, 9 September 2011

Similar here 

-			$mode = $this->skin->getTitle()->userIsWatching() ? 'unwatch' : 'watch';
+			$mode = $this->getSkin()->getTitle()->userIsWatching() ? 'unwatch' : 'watch';

What's wrong with $this->getTitle() ?

#Comment by Reedy (talk | contribs)   16:02, 9 September 2011

And here 

-		$s .= $wgLang->pipeList( array(
+		$s .= $this->getSkin()->getLang()->pipeList( array(

$this->getLang() ?

#Comment by Reedy (talk | contribs)   16:04, 9 September 2011 
class SkinSimple extends SkinTemplate {

class SkinTemplate extends Skin {

abstract class Skin extends ContextSource {

class RequestContext implements IContextSource {
<pre>

interface IContextSource {

/** * Get the WebRequest object * * @return WebRequest */ public function getRequest();

/** * Get the Title object * * @return Title */ public function getTitle();

/** * Get the OutputPage object * * @return OutputPage object */ public function getOutput();

/** * Get the User object * * @return User */ public function getUser();

/** * Get the Language object * * @return Language */ public function getLang();

/** * Get the Skin object * * @return Skin */ public function getSkin(); }


Yes there is

#Comment by Dantman (talk | contribs)   16:13, 9 September 2011

You could have just said that I accidentally thought I was in template context when I was in skin context.

#Comment by Reedy (talk | contribs)   16:05, 9 September 2011 
abstract class ContextSource implements IContextSource {

even for the last one

Status & tagging log