r77741 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r77740‎ | r77741 | r77742 >
Date:20:55, 4 December 2010
Author:dantman
Status:resolved (Comments)
Tags:brion 
Comment:
Implement $wgFooterIcons to replace copyrightico and poweredbyico with a flexible list of icons that can be customized by extensions, hosting, farms, wikimedia, etc...
Implementations for MonoBook, Vector, and Modern. Modern implementation uses text instead of icons as desired.
Modified paths:
  • /trunk/phase3/includes/DefaultSettings.php (modified) (history)
  • /trunk/phase3/includes/Setup.php (modified) (history)
  • /trunk/phase3/includes/SkinTemplate.php (modified) (history)
  • /trunk/phase3/skins/Modern.php (modified) (history)
  • /trunk/phase3/skins/MonoBook.php (modified) (history)
  • /trunk/phase3/skins/Vector.php (modified) (history)

Diff [purge]

Index: trunk/phase3/skins/Vector.php
@@ -413,13 +413,7 @@
414414
415415 // Generate additional footer links
416416 $footerlinks = $this->data["footerlinks"];
417 - // footerlinks doesn't include icons for now, so we'll just append the default
418 - $footerlinks["icons"] = array( 'poweredbyico', 'copyrightico', );
419417
420 - $footerlinksClasses = array(
421 - 'icons' => array( 'noprint' )
422 - );
423 -
424418 // Reduce footer links down to only those which are being used
425419 $validFooterLinks = array();
426420 foreach( $footerlinks as $category => $links ) {
@@ -430,6 +424,24 @@
431425 }
432426 }
433427 }
 428+
 429+ // Generate additional footer icons
 430+ $footericons = $this->data["footericons"];
 431+ // Unset any icons which don't have an image
 432+ foreach ( $footericons as $footerIconsKey => &$footerIconsBlock ) {
 433+ foreach ( $footerIconsBlock as $footerIconKey => $footerIcon ) {
 434+ if ( !is_string($footerIcon) && !isset($footerIcon["src"]) ) {
 435+ unset($footerIconsBlock[$footerIconKey]);
 436+ }
 437+ }
 438+ }
 439+ // Redo removal of any empty blocks
 440+ foreach ( $footericons as $footerIconsKey => &$footerIconsBlock ) {
 441+ if ( count($footerIconsBlock) <= 0 ) {
 442+ unset($footericons[$footerIconsKey]);
 443+ }
 444+ }
 445+
434446 // Reverse horizontally rendered navigation elements
435447 if ( $wgLang->isRTL() ) {
436448 $this->data['view_urls'] =
@@ -523,7 +535,7 @@
524536 <div id="footer"<?php $this->html('userlangattributes') ?>>
525537 <?php foreach( $validFooterLinks as $category => $links ): ?>
526538 <?php if ( count( $links ) > 0 ): ?>
527 - <ul id="footer-<?php echo $category ?>"<?php if (isset($footerlinksClasses[$category])) echo ' class="' . implode(" ", $footerlinksClasses[$category]) . '"'; ?>>
 539+ <ul id="footer-<?php echo $category ?>">
528540 <?php foreach( $links as $link ): ?>
529541 <?php if( isset( $this->data[$link] ) && $this->data[$link] ): ?>
530542 <li id="footer-<?php echo $category ?>-<?php echo $link ?>"><?php $this->html( $link ) ?></li>
@@ -532,6 +544,31 @@
533545 </ul>
534546 <?php endif; ?>
535547 <?php endforeach; ?>
 548+<?php if ( count( $footericons ) > 0 ): ?>
 549+ <ul id="footer-icons">
 550+<?php foreach ( $footericons as $blockName => $footerIcons ): ?>
 551+ <li id="footer-<?php echo htmlspecialchars($blockName); ?>ico" class="noprint">
 552+<?php foreach ( $footerIcons as $icon ):
 553+ if ( is_string($icon) ) {
 554+ $html = $icon;
 555+ } else {
 556+ $url = $icon["url"];
 557+ unset($icon["url"]);
 558+ if ( isset($icon["src"]) ) {
 559+ $html = Html::element( 'img', $icon ); // do this the lazy way, just pass icon data as an attribute array
 560+ } else {
 561+ $html = htmlspecialchars($icon["alt"]);
 562+ }
 563+ if ( $url ) {
 564+ $html = Html::rawElement( 'a', array( "href" => $url ), $html );
 565+ }
 566+ }
 567+ echo " $html\n";
 568+ endforeach; ?>
 569+ </li>
 570+<?php endforeach; ?>
 571+ </ul>
 572+ <?php endif; ?>
536573 <div style="clear:both"></div>
537574 </div>
538575 <!-- /footer -->
Index: trunk/phase3/skins/MonoBook.php
@@ -74,6 +74,22 @@
7575 $footerlinks = $this->data["footerlinks"];
7676 // fold footerlinks into a single array using a bit of trickery
7777 $footerlinks = call_user_func_array('array_merge', array_values($footerlinks));
 78+ // Generate additional footer icons
 79+ $footericons = $this->data["footericons"];
 80+ // Unset any icons which don't have an image
 81+ foreach ( $footericons as $footerIconsKey => &$footerIconsBlock ) {
 82+ foreach ( $footerIconsBlock as $footerIconKey => $footerIcon ) {
 83+ if ( !is_string($footerIcon) && !isset($footerIcon["src"]) ) {
 84+ unset($footerIconsBlock[$footerIconKey]);
 85+ }
 86+ }
 87+ }
 88+ // Redo removal of any empty blocks
 89+ foreach ( $footericons as $footerIconsKey => &$footerIconsBlock ) {
 90+ if ( count($footerIconsBlock) <= 0 ) {
 91+ unset($footericons[$footerIconsKey]);
 92+ }
 93+ }
7894
7995 $this->html( 'headelement' );
8096 ?><div id="globalWrapper">
@@ -171,13 +187,27 @@
172188 </div><!-- end of the left (by default at least) column -->
173189 <div class="visualClear"></div>
174190 <div id="footer"<?php $this->html('userlangattributes') ?>>
175 -<?php
176 -if($this->data['poweredbyico']) { ?>
177 - <div id="f-poweredbyico"><?php $this->html('poweredbyico') ?></div>
 191+<?php foreach ( $footericons as $blockName => $footerIcons ) { ?>
 192+ <div id="f-<?php echo htmlspecialchars($blockName); ?>ico">
 193+<?php foreach ( $footerIcons as $icon ) {
 194+ if ( is_string($icon) ) {
 195+ $html = $icon;
 196+ } else {
 197+ $url = $icon["url"];
 198+ unset($icon["url"]);
 199+ if ( isset($icon["src"]) ) {
 200+ $html = Html::element( 'img', $icon ); // do this the lazy way, just pass icon data as an attribute array
 201+ } else {
 202+ $html = htmlspecialchars($icon["alt"]);
 203+ }
 204+ if ( $url ) {
 205+ $html = Html::rawElement( 'a', array( "href" => $url ), $html );
 206+ }
 207+ }
 208+ echo " $html\n";
 209+ } ?>
 210+ </div>
178211 <?php }
179 -if($this->data['copyrightico']) { ?>
180 - <div id="f-copyrightico"><?php $this->html('copyrightico') ?></div>
181 -<?php }
182212
183213 // Generate additional footer links
184214 $validFooterLinks = array();
Index: trunk/phase3/skins/Modern.php
@@ -21,17 +21,6 @@
2222 var $skinname = 'modern', $stylename = 'modern',
2323 $template = 'ModernTemplate', $useHeadElement = true;
2424
25 - /*
26 - * We don't like the default getPoweredBy, the icon clashes with the
27 - * skin L&F.
28 - */
29 - function getPoweredBy() {
30 - global $wgVersion;
31 - $text = "<div class='mw_poweredby'>Powered by MediaWiki $wgVersion</div>";
32 - wfRunHooks( 'SkinGetPoweredBy', array( &$text, $this ) );
33 - return $text;
34 - }
35 -
3625 function setupSkinUserCss( OutputPage $out ){
3726 // Do not call parent::setupSkinUserCss(), we have our own print style
3827 $out->addStyle( 'common/shared.css', 'screen' );
@@ -63,6 +52,18 @@
6453 // Suppress warnings to prevent notices about missing indexes in $this->data
6554 wfSuppressWarnings();
6655
 56+ // Generate additional footer links
 57+ $footerlinks = $this->data["footerlinks"];
 58+ // fold footerlinks into a single array using a bit of trickery
 59+ $footerlinks = call_user_func_array('array_merge', array_values($footerlinks));
 60+ // Generate additional footer icons
 61+ $footericons = $this->data["footericons"];
 62+ // Unset copyright.copyright since we don't need the icon and already output a copyright from footerlinks
 63+ unset($footericons["copyright"]["copyright"]);
 64+ if ( count($footericons["copyright"]) <= 0 ) {
 65+ unset($footericons["copyright"]);
 66+ }
 67+
6768 $this->html( 'headelement' );
6869 ?>
6970
@@ -182,10 +183,6 @@
183184 <div id="footer"<?php $this->html('userlangattributes') ?>>
184185 <ul id="f-list">
185186 <?php
186 - $footerlinks = array(
187 - 'lastmod', 'viewcount', 'numberofwatchingusers', 'credits', 'copyright',
188 - 'privacy', 'about', 'disclaimer', 'tagline',
189 - );
190187 foreach( $footerlinks as $aLink ) {
191188 if( isset( $this->data[$aLink] ) && $this->data[$aLink] ) {
192189 ?> <li id="<?php echo$aLink?>"><?php $this->html($aLink) ?></li>
@@ -193,7 +190,25 @@
194191 }
195192 ?>
196193 </ul>
197 - <?php echo $this->html("poweredbyico"); ?>
 194+<?php
 195+ foreach ( $footericons as $blockName => $footerIcons ) { ?>
 196+ <div id="mw_<?php echo htmlspecialchars($blockName); ?>">
 197+<?php
 198+ foreach ( $footerIcons as $icon ) {
 199+ if ( is_string($icon) ) {
 200+ $html = $icon;
 201+ } else {
 202+ $html = htmlspecialchars($icon["alt"]);
 203+ if ( $icon["url"] ) {
 204+ $html = Html::element( 'a', array( "href" => $icon["url"] ), $html );
 205+ }
 206+ }
 207+ echo " $html\n";
 208+ } ?>
 209+ </div>
 210+<?php
 211+ }
 212+?>
198213 </div>
199214
200215 <?php $this->html('bottomscripts'); /* JS call to runBodyOnloadHook */ ?>
Index: trunk/phase3/includes/Setup.php
@@ -63,6 +63,28 @@
6464 $wgDeletedDirectory = $wgFileStore['deleted']['directory'];
6565 }
6666
 67+if( isset($wgFooterIcons["copyright"]) &&
 68+ isset($wgFooterIcons["copyright"]["copyright"]) &&
 69+ $wgFooterIcons["copyright"]["copyright"] === array() ) {
 70+ if ( isset( $wgCopyrightIcon ) && $wgCopyrightIcon ) {
 71+ $wgFooterIcons["copyright"]["copyright"] = $wgCopyrightIcon;
 72+ } elseif ( $wgRightsIcon || $wgRightsText ) {
 73+ $wgFooterIcons["copyright"]["copyright"] = array(
 74+ "url" => $wgRightsUrl,
 75+ "src" => $wgRightsIcon,
 76+ "alt" => $wgRightsText,
 77+ );
 78+ } else {
 79+ unset($wgFooterIcons["copyright"]["copyright"]);
 80+ }
 81+}
 82+
 83+if( isset($wgFooterIcons["poweredby"]) &&
 84+ isset($wgFooterIcons["poweredby"]["mediawiki"]) &&
 85+ $wgFooterIcons["poweredby"]["mediawiki"]["src"] === null ) {
 86+ $wgFooterIcons["poweredby"]["mediawiki"]["src"] = "$wgStylePath/common/images/poweredby_mediawiki_88x31.png";
 87+}
 88+
6789 /**
6890 * Unconditional protection for NS_MEDIAWIKI since otherwise it's too easy for a
6991 * sysadmin to set $wgNamespaceProtection incorrectly and leave the wiki insecure.
Index: trunk/phase3/includes/SkinTemplate.php
@@ -425,6 +425,21 @@
426426 'disclaimer',
427427 ),
428428 ) );
 429+
 430+ global $wgFooterIcons;
 431+ $tpl->set( 'footericons', $wgFooterIcons );
 432+ foreach ( $tpl->data["footericons"] as $footerIconsKey => &$footerIconsBlock ) {
 433+ if ( count($footerIconsBlock) > 0 ) {
 434+ foreach ( $footerIconsBlock as &$footerIcon ) {
 435+ if ( isset($footerIcon["src"]) ) {
 436+ if ( !isset($footerIcon["width"]) ) $footerIcon["width"] = 88;
 437+ if ( !isset($footerIcon["height"]) ) $footerIcon["height"] = 31;
 438+ }
 439+ }
 440+ } else {
 441+ unset($tpl->data["footericons"][$footerIconsKey]);
 442+ }
 443+ }
429444
430445 if ( $wgDebugComments ) {
431446 $tpl->setRef( 'debug', $out->mDebugtext );
Index: trunk/phase3/includes/DefaultSettings.php
@@ -2263,6 +2263,25 @@
22642264 $wgExperimentalHtmlIds = true;
22652265
22662266 /**
 2267+ * Abstract list of footer icons for skins in place of old copyrightico and poweredbyico code
 2268+ * You can add new icons to the built in copyright or poweredby, or you can create
 2269+ * a new block. Though note that you may need to add some custom css to get good styling
 2270+ * of new blocks in monobook. vector and modern should work without any special css.
 2271+ */
 2272+$wgFooterIcons = array(
 2273+ "copyright" => array(
 2274+ "copyright" => array(), // placeholder for the built in copyright icon
 2275+ ),
 2276+ "poweredby" => array(
 2277+ "mediawiki" => array(
 2278+ "src" => null, // Defaults to "$wgStylePath/common/images/poweredby_mediawiki_88x31.png"
 2279+ "url" => "http://www.mediawiki.org/",
 2280+ "alt" => "Powered by MediaWiki",
 2281+ )
 2282+ ),
 2283+);
 2284+
 2285+/**
22672286 * Search form behavior for Vector skin only
22682287 * true = use an icon search button
22692288 * false = use Go & Search buttons

Follow-up revisions

RevisionCommit summaryAuthorDate
r77763Take the footer icon html building common to all 3 skins using r77741 $wgFoot...dantman04:44, 5 December 2010
r77781Follow up for r77732 and r77741, add the missing RELEASE-NOTES which I forgot.dantman06:59, 5 December 2010
r81433Commit some fixes for comments on r77741dantman00:00, 3 February 2011
r81434Backport r81433's fixes for comments on r77741 which affect 1.17.dantman00:19, 3 February 2011

Comments

#Comment by Dantman (talk | contribs)   20:58, 4 December 2010

Oh right, I also fixed Modern's footerlinks to use the skin generic as well in this commit.

#Comment by Brion VIBBER (talk | contribs)   06:32, 19 January 2011

I like the basic idea, but it looks like there's still a few issues.

Settings

  • $wgFooterIcons doc comment should indicate that you can drop a raw HTML chunk in in place of an array for an item
  • $wgFooterIcons doc comment should clarify the format for arrays; they appear to be a set of HTML attributes for the img element, plus an extra 'url' parameter, and minus unspecified default width and height attributes
  • $wgCopyrightIcon documentation should be updated with a deprecation note

Skin code

  • SkinTemplate retains the original getCopyrightIcon() and getPoweredBy(), and they're still included as-is in the template data. If it's necessary to keep the template entries for skin compatibility (only relevant if 1.16 skins actually work correctly without modification -- there might be lots of changes from ResourceLoader stuff) they possibly should call into makeFooterIcon() on the relevant sections of $wgFooterIcons; otherwise I'd say drop em.
  • makeFooterIcon() as consolidated in r77763 will spew an E_NOTICE entry if a non-string icon doesn't have a URL entry
  • use of & references in foreach loops where not needed
  • use of unset() on an array parameter; while technically safe with regular arrays, it's confusing. Consider either copying to a local variable or isolating the attribute array from other parameters
#Comment by Dantman (talk | contribs)   07:20, 19 January 2011

I don't believe we've completely dropped back compat for old skins. In fact one of the skins I was working with was coded for 1.15, updated to 1.16 and it always worked fine even though I hadn't updated it to use the resource loader.

I don't really consider $wgCopyrightIcon deprecated. It still works, I don't see any reason to force people to update their localsettings code to use verbose icon code if they don't want to use more than one icon. That feels like dropping $wgUploadPath because we have $wgLocalFileRepo.

The & was required to modify the array. Is copying the entire array just to remove an item really the preferred method for coding in php?

#Comment by Brion VIBBER (talk | contribs)   05:37, 24 January 2011

If we haven't dropped support for old skins, then those old skins will not pick up changes to the copyright or powered-by icons made in $wgFooterIcons in LocalSettings with the current code here.

Deprecation doesn't mean something is broken or no longer works -- rather, it means that something that still works is not recommended because it's being superseded by something else. It might stop working in the future, or it might be left forever, but still deprecated because the newer way is more flexible, or easier to work with, or it's simply easier to be consistent and use the new way.

The & on the foreaches are only needed to make that array modification because you're modifying the copy you got on an inner-loop instead of the variable directly... though I think those might be mostly gone now with the function parameter to include or not include the images.

Where the $icon array bits are modified in-place, that's an oddity to begin with because two unrelated things are being squished into one associative array: arbitrary HTML attributes for an <img> tag, and specific array entries that are pulled to be used elsewhere (the 'url' entry, to be used to form a link).

#Comment by Brion VIBBER (talk | contribs)   03:13, 4 February 2011

With the deprecation marking now in, I'm happy enough with this for now; marking RESOLVED.

Future cleanup & modernization of the legacy skins can proceed at leisure.

Status & tagging log