r49350 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r49349‎ | r49350 | r49351 >
Date:19:06, 9 April 2009
Author:rememberthedot
Status:resolved (Comments)
Tags:
Comment:
Follow-up to r49331, reduce redundancy in language attribute generation in MonoBook.php
Modified paths:
  • /trunk/phase3/skins/MonoBook.php (modified) (history)

Diff [purge]

Index: trunk/phase3/skins/MonoBook.php
——@@ -64,9 +64,7 @@
6565 * @access private
6666 */
6767 function execute() {
68 - global $wgLang;
6968 global $wgRequest;
70 - $wgLangCode = $wgLang->getCode();
7169 $this->skin = $skin = $this->data['skin'];
7270 $action = $wgRequest->getText( 'action' );
7371
——@@ -137,7 +135,7 @@
138136 <div id="p-cactions" class="portlet">
139137 <h5><?php $this->msg('views') ?></h5>
140138 <div class="pBody">
141 - <ul lang="<?php echo $wgLangCode; ?>" xml:lang="<?php echo $wgLangCode; ?>">
 139+ <ul <?php echo $this->langAttributes() ?>>
142140 <?php foreach($this->data['content_actions'] as $key => $tab) {
143141 echo '
144142 <li id="' . Sanitizer::escapeId( "ca-$key" ) . '"';
——@@ -165,7 +163,7 @@
166164 <div class="portlet" id="p-personal">
167165 <h5><?php $this->msg('personaltools') ?></h5>
168166 <div class="pBody">
169 - <ul lang="<?php echo $wgLangCode; ?>" xml:lang="<?php echo $wgLangCode; ?>">
 167+ <ul <?php echo $this->langAttributes() ?>>
170168 <?php foreach($this->data['personal_urls'] as $key => $item) { ?>
171169 <li id="<?php echo Sanitizer::escapeId( "pt-$key" ) ?>"<?php
172170 if ($item['active']) { ?> class="active"<?php } ?>><a href="<?php
——@@ -227,7 +225,7 @@
228226 <?php
229227 foreach( $validFooterLinks as $aLink ) {
230228 if( isset( $this->data[$aLink] ) && $this->data[$aLink] ) {
231 -?> <li id="<?php echo$aLink?>"><?php $this->html($aLink) ?></li>
 229+?> <li id="<?php echo $aLink ?>"><?php $this->html($aLink) ?></li>
232230 <?php }
233231 }
234232 ?>
——@@ -251,12 +249,10 @@
252250
253251 /*************************************************************************************************/
254252 function searchBox() {
255 - global $wgLang;
256253 global $wgUseTwoButtonsSearchForm;
257 - $wgLangCode = $wgLang->getCode();
258254 ?>
259255 <div id="p-search" class="portlet">
260 - <h5 lang="<?php echo $wgLangCode; ?>" xml:lang="<?php echo $wgLangCode; ?>"><label for="searchInput"><?php $this->msg('search') ?></label></h5>
 256+ <h5 <?php echo $this->langAttributes() ?>><label for="searchInput"><?php $this->msg('search') ?></label></h5>
261257 <div id="searchBody" class="pBody">
262258 <form action="<?php $this->text('wgScript') ?>" id="searchform"><div>
263259 <input type='hidden' name="title" value="<?php $this->text('searchtitle') ?>"/>
——@@ -276,11 +272,9 @@
277273
278274 /*************************************************************************************************/
279275 function toolbox() {
280 - global $wgLang;
281 - $wgLangCode = $wgLang->getCode();
282276 ?>
283277 <div class="portlet" id="p-tb">
284 - <h5 lang="<?php echo $wgLangCode; ?>" xml:lang="<?php echo $wgLangCode; ?>"><?php $this->msg('toolbox') ?></h5>
 278+ <h5 <?php echo $this->langAttributes() ?>><?php $this->msg('toolbox') ?></h5>
285279 <div class="pBody">
286280 <ul>
287281 <?php
——@@ -338,12 +332,10 @@
339333
340334 /*************************************************************************************************/
341335 function languageBox() {
342 - global $wgLang;
343 - $wgLangCode = $wgLangCode;
344336 if( $this->data['language_urls'] ) {
345337 ?>
346338 <div id="p-lang" class="portlet">
347 - <h5 lang="<?php echo $wgLangCode; ?>" xml:lang="<?php echo $wgLangCode; ?>"><?php $this->msg('otherlanguages') ?></h5>
 339+ <h5 <?php echo $this->langAttributes() ?>><?php $this->msg('otherlanguages') ?></h5>
348340 <div class="pBody">
349341 <ul>
350342 <?php foreach($this->data['language_urls'] as $langlink) { ?>
——@@ -359,11 +351,9 @@
360352
361353 /*************************************************************************************************/
362354 function customBox( $bar, $cont ) {
363 - global $wgLang;
364 - $wgLangCode = $wgLang->getCode();
365355 ?>
366356 <div class='generated-sidebar portlet' id='<?php echo Sanitizer::escapeId( "p-$bar" ) ?>'<?php echo $this->skin->tooltip('p-'.$bar) ?>>
367 - <h5 lang="<?php echo $wgLangCode; ?>" xml:lang="<?php echo $wgLangCode; ?>"><?php $out = wfMsg( $bar ); if (wfEmptyMsg($bar, $out)) echo htmlspecialchars($bar); else echo htmlspecialchars($out); ?></h5>
 357+ <h5 <?php echo $this->langAttributes() ?>><?php $out = wfMsg( $bar ); if (wfEmptyMsg($bar, $out)) echo htmlspecialchars($bar); else echo htmlspecialchars($out); ?></h5>
368358 <div class='pBody'>
369359 <?php if ( is_array( $cont ) ) { ?>
370360 <ul>
——@@ -383,6 +373,12 @@
384374 <?php
385375 }
386376
 377+ private function langAttributes()
 378+ {
 379+ global $wgLang;
 380+ $wgLangCode = $wgLang->getCode();
 381+ return 'lang="' . $wgLangCode . '" xml:lang="' . $wgLangCode . '"';
 382+ }
387383 } // end of class
388384
389385

Follow-up revisions

RevisionCommit summaryAuthorDate
r49359Follow-up to r49350, corrected code to follow coding conventions for braces a...rememberthedot23:38, 9 April 2009
r51448Moved language attribute generation to SkinTemplate.php as requested by Tim S...rememberthedot03:14, 4 June 2009

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r49331(bug 1553) Lowercase navigation headings in Germanrememberthedot05:27, 9 April 2009

Comments

#Comment by Jack Phoenix (talk | contribs)   20:16, 9 April 2009 
+	private function langAttributes()
+	{

Manual:Coding conventions#Indentation and spacing states that braces are added to the end of lines for function and class declarations, not on a new line.

Brion also commented on r49331 that the "use of a $wgLangCode *local* variable seems very poor form and is very confusing". Furthermore, Manual:Coding conventions#Variables states that a wg variable is a global variable, not a local one. Perhaps $wgLangCode should be renamed to something less confusing, such as $languageCode?

#Comment by Remember the dot (talk | contribs)   23:40, 9 April 2009

Ah, that's what he meant. Fixed in r49359.

#Comment by Tim Starling (talk | contribs)   16:25, 3 June 2009

Looks slow. The skin is a performance hotspot in terms of throughput. Please assign it to a member variable on initialisation and echo the variable each time. And use htmlspecialchars(), you don't know where that variable has come from, it's dangerous and lazy.

#Comment by Remember the dot (talk | contribs)   03:22, 4 June 2009

Separation to a member variable done in r51448. If you feel that sending the language codes through htmlspecialchars is necessary, then please make sure the same is done to the language codes on the root <html> tag. The root language codes are not (and to my knowledge never have been) sent through htmlspecialchars, so I didn't bother sending the new language codes through htmlspecialchars either.

#Comment by Tim Starling (talk | contribs)   15:09, 6 June 2009

It's not a vulnerability, if it was I would fix it and push out a release. But it's poor coding style which can easily lead to vulnerabilities. I'm telling you because I don't want you to repeat the mistakes of past developers, and because they're not around anymore for me to chastise. When you refactor ancient code, that is your opportunity to get it up to a modern standard.

Status & tagging log