r104384 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r104383‎ | r104384 | r104385 >
Date:00:51, 28 November 2011
Author:varnent
Status:deferred (Comments)
Tags:
Comment:
switched to arrays in response to r104381#c26594 - ty Johnduhart
Modified paths:
  • /trunk/extensions/AddThis/AddThis.body.php (modified) (history)
  • /trunk/extensions/AddThis/AddThis.php (modified) (history)

Diff [purge]

Index: trunk/extensions/AddThis/AddThis.body.php
——@@ -13,22 +13,21 @@
1414 *
1515 */
1616 public static function parserHook( $parser ) {
17 - global $wgOut, $wgAddThispubid, $wgAddThisHeader, $wgAddThisHServ1, $wgAddThisHServ1set, $wgAddThisHServ2, $wgAddThisHServ2set, $wgAddThisHServ3, $wgAddThisHServ3set, $wgAddThisHServ4, $wgAddThisHServ4set,
18 - $wgAddThisHServ5, $wgAddThisHServ5set, $wgAddThisHServ6, $wgAddThisHServ6set, $wgAddThisHServ7, $wgAddThisHServ7set, $wgAddThisHServ8, $wgAddThisHServ8set, $wgAddThisBackground, $wgAddThisBorder;
 17+ global $wgAddThispubid, $wgAddThisHeader, $wgAddThisMain, $wgAddThisMainpage, $wgRequest, $wgAddThisHServ, $wgAddThisBackground, $wgAddThisBorder;
1918 # Localisation for "Share"
2019 $share = wfMsg( 'addthis' );
2120 # Output AddThis widget
2221 $output .='<!-- AddThis Button BEGIN -->
2322 <div class="addthis_toolbox addthis_default_style" id="addthistoolbar" style="background:'.$wgAddThisBackground.'; border-color:'.$wgAddThisBorder.';">
2423 <a href="http://www.addthis.com/bookmark.php?v=250&amp;pubid=ra-4eb75def4ec6488b" class="addthis_button_compact">&nbsp;' . $share . '</a><span class="addthis_separator">&nbsp;</span>
25 - <a class="addthis_button_'.$wgAddThisHServ1.'" '.$wgAddThisHServ1set.'></a>
26 - <a class="addthis_button_'.$wgAddThisHServ2.'" '.$wgAddThisHServ2set.'></a>
27 - <a class="addthis_button_'.$wgAddThisHServ3.'" '.$wgAddThisHServ3set.'></a>
28 - <a class="addthis_button_'.$wgAddThisHServ4.'" '.$wgAddThisHServ4set.'></a>
29 - <a class="addthis_button_'.$wgAddThisHServ5.'" '.$wgAddThisHServ5set.'></a>
30 - <a class="addthis_button_'.$wgAddThisHServ6.'" '.$wgAddThisHServ6set.'></a>
31 - <a class="addthis_button_'.$wgAddThisHServ7.'" '.$wgAddThisHServ7set.'></a>
32 - <a class="addthis_button_'.$wgAddThisHServ8.'" '.$wgAddThisHServ8set.'></a>
 24+ <a class="addthis_button_'.$wgAddThisHServ[1].'" '.$wgAddThisHServ['1set'].'></a>
 25+ <a class="addthis_button_'.$wgAddThisHServ[2].'" '.$wgAddThisHServ['2set'].'></a>
 26+ <a class="addthis_button_'.$wgAddThisHServ[3].'" '.$wgAddThisHServ['3set'].'></a>
 27+ <a class="addthis_button_'.$wgAddThisHServ[4].'" '.$wgAddThisHServ['4set'].'></a>
 28+ <a class="addthis_button_'.$wgAddThisHServ[5].'" '.$wgAddThisHServ['5set'].'></a>
 29+ <a class="addthis_button_'.$wgAddThisHServ[6].'" '.$wgAddThisHServ['6set'].'></a>
 30+ <a class="addthis_button_'.$wgAddThisHServ[7].'" '.$wgAddThisHServ['7set'].'></a>
 31+ <a class="addthis_button_'.$wgAddThisHServ[8].'" '.$wgAddThisHServ['8set'].'></a>
3332 </div>
3433 <script type="text/javascript" src="http://s7.addthis.com/js/250/addthis_widget.js#pubid='.$wgAddThispubid.'"></script>
3534 <!-- AddThis Button END -->';
——@@ -41,8 +40,7 @@
4241 *
4342 */
4443 static function AddThisSidebar( $skin, &$bar ) {
45 - global $wgOut, $wgAddThispubid, $wgAddThisSidebar, $wgAddThisSBServ1, $wgAddThisSBServ2, $wgAddThisSBServ3, $wgAddThisSBServ4, $wgAddThisSBServ5, $wgAddThisSBServ1set, $wgAddThisSBServ2set,
46 - $wgAddThisSBServ3set, $wgAddThisSBServ4set, $wgScriptPath, $wgAddThisSBServ5set;
 44+ global $wgOut, $wgAddThispubid, $wgAddThisSidebar, $wgAddThisSBServ;
4745 # Load css stylesheet
4846 $wgOut->addModuleStyles( 'ext.addThis' );
4947 # Check setting to enable/disable sidebar portlet
——@@ -50,11 +48,11 @@
5149 # Output AddThis widget
5250 $bar['addthis'] = '<!-- AddThis Button BEGIN -->
5351 <div class="addthis_toolbox addthis_default_style" id="addthissidebar">
54 - <a class="addthis_button_'.$wgAddThisSBServ1.'" '.$wgAddThisSBServ1set.'></a>
55 - <a class="addthis_button_'.$wgAddThisSBServ2.'" '.$wgAddThisSBServ2set.'></a>
56 - <a class="addthis_button_'.$wgAddThisSBServ3.'" '.$wgAddThisSBServ3set.'></a>
57 - <a class="addthis_button_'.$wgAddThisSBServ4.'" '.$wgAddThisSBServ4set.'></a>
58 - <a class="addthis_button_'.$wgAddThisSBServ5.'" '.$wgAddThisSBServ5set.'></a>
 52+ <a class="addthis_button_'.$wgAddThisSBServ[1].'" '.$wgAddThisSBServ['1set'].'></a>
 53+ <a class="addthis_button_'.$wgAddThisSBServ[2].'" '.$wgAddThisSBServ['2set'].'></a>
 54+ <a class="addthis_button_'.$wgAddThisSBServ[3].'" '.$wgAddThisSBServ['3set'].'></a>
 55+ <a class="addthis_button_'.$wgAddThisSBServ[4].'" '.$wgAddThisSBServ['4set'].'></a>
 56+ <a class="addthis_button_'.$wgAddThisSBServ[5].'" '.$wgAddThisSBServ['5set'].'></a>
5957 </div>
6058 <script type="text/javascript" src="http://s7.addthis.com/js/250/addthis_widget.js#pubid='.$wgAddThispubid.'"></script>
6159 <!-- AddThis Button END -->';
——@@ -69,9 +67,7 @@
7068 *
7169 */
7270 public static function AddThisHeader( &$article, &$outputDone, &$pcache ) {
73 - global $wgOut, $wgAddThispubid, $wgAddThisHeader, $wgAddThisMain, $wgAddThisMainpage, $wgRequest, $wgAddThisHServ1, $wgAddThisHServ1set, $wgAddThisHServ2, $wgAddThisHServ2set, $wgAddThisHServ3,
74 - $wgAddThisHServ3set, $wgAddThisHServ4, $wgAddThisHServ4set, $wgAddThisHServ5, $wgAddThisHServ5set, $wgAddThisHServ6, $wgAddThisHServ6set, $wgAddThisHServ7, $wgAddThisHServ7set, $wgAddThisHServ8,
75 - $wgAddThisHServ8set, $wgAddThisBackground, $wgAddThisBorder;
 71+ global $wgOut, $wgAddThispubid, $wgAddThisHeader, $wgAddThisMain, $wgAddThisMainpage, $wgRequest, $wgAddThisHServ, $wgAddThisBackground, $wgAddThisBorder;
7672 # Localisation for "Share"
7773 $share = wfMsg( 'addthis' );
7874 # Check if page is in main namespace
——@@ -91,14 +87,14 @@
9288 $wgOut->addHTML('<!-- AddThis Button BEGIN -->
9389 <div class="addthis_toolbox addthis_default_style" id="addthisheader" style="background:'.$wgAddThisBackground.'; border-color:'.$wgAddThisBorder.';">
9490 <a href="http://www.addthis.com/bookmark.php?v=250&amp;pubid=ra-4eb75def4ec6488b" class="addthis_button_compact">&nbsp;' . $share . '</a><span class="addthis_separator">&nbsp;</span>
95 - <a class="addthis_button_'.$wgAddThisHServ1.'" '.$wgAddThisHServ1set.'></a>
96 - <a class="addthis_button_'.$wgAddThisHServ2.'" '.$wgAddThisHServ2set.'></a>
97 - <a class="addthis_button_'.$wgAddThisHServ3.'" '.$wgAddThisHServ3set.'></a>
98 - <a class="addthis_button_'.$wgAddThisHServ4.'" '.$wgAddThisHServ4set.'></a>
99 - <a class="addthis_button_'.$wgAddThisHServ5.'" '.$wgAddThisHServ5set.'></a>
100 - <a class="addthis_button_'.$wgAddThisHServ6.'" '.$wgAddThisHServ6set.'></a>
101 - <a class="addthis_button_'.$wgAddThisHServ7.'" '.$wgAddThisHServ7set.'></a>
102 - <a class="addthis_button_'.$wgAddThisHServ8.'" '.$wgAddThisHServ8set.'></a>
 91+ <a class="addthis_button_'.$wgAddThisHServ[1].'" '.$wgAddThisHServ['1set'].'></a>
 92+ <a class="addthis_button_'.$wgAddThisHServ[2].'" '.$wgAddThisHServ['2set'].'></a>
 93+ <a class="addthis_button_'.$wgAddThisHServ[3].'" '.$wgAddThisHServ['3set'].'></a>
 94+ <a class="addthis_button_'.$wgAddThisHServ[4].'" '.$wgAddThisHServ['4set'].'></a>
 95+ <a class="addthis_button_'.$wgAddThisHServ[5].'" '.$wgAddThisHServ['5set'].'></a>
 96+ <a class="addthis_button_'.$wgAddThisHServ[6].'" '.$wgAddThisHServ['6set'].'></a>
 97+ <a class="addthis_button_'.$wgAddThisHServ[7].'" '.$wgAddThisHServ['7set'].'></a>
 98+ <a class="addthis_button_'.$wgAddThisHServ[8].'" '.$wgAddThisHServ['8set'].'></a>
10399 </div>
104100 <script type="text/javascript" src="http://s7.addthis.com/js/250/addthis_widget.js#pubid='.$wgAddThispubid.'"></script>
105101 <!-- AddThis Button END -->');
Index: trunk/extensions/AddThis/AddThis.php
——@@ -9,6 +9,7 @@
1010 * @license GPL
1111 *
1212 * Loosely based on the Google Translator extension by Joachim De Schrijver
 13+ * Thank you to Johnduhart for feedback
1314 */
1415
1516 /**
——@@ -42,72 +43,72 @@
4344 * $wgAddThisMain
4445 * - Display AddThis widget on main page
4546 * Default is true
46 - * $wgAddThisSBServ1
 47+ * $wgAddThisSBServ[1]
4748 * - Service code for 1st button in sidebar - service codes: http://www.addthis.com/services/list
4849 * Default is compact - AddThis icon used to access full AddThis popup menu
49 - * $wgAddThisSBServ1set
 50+ * $wgAddThisSBServ['1set']
5051 * - Settings for 1st button in sidebar - more info: http://www.addthis.com/help/client-api#attribute-config
51 - * $wgAddThisSBServ2
 52+ * $wgAddThisSBServ[2]
5253 * - Service code for 2nd button in sidebar
5354 * Default is facebook
54 - * $wgAddThisSBServ2set
 55+ * $wgAddThisSBServ['2set']
5556 * - Settings for 2nd button in sidebar
56 - * $wgAddThisSBServ3
 57+ * $wgAddThisSBServ[3]
5758 * - Service code for 3rd button in sidebar
5859 * Default is twitter
59 - * $wgAddThisSBServ3set
 60+ * $wgAddThisSBServ['3set']
6061 * - Settings for 3rd button in sidebar
61 - * $wgAddThisSBServ4
 62+ * $wgAddThisSBServ[4]
6263 * - Service code for 4th button in sidebar
6364 * Default is google_plusone
64 - * $wgAddThisSBServ4set
 65+ * $wgAddThisSBServ['4set']
6566 * - Settings for 4th button in sidebar
6667 * Default is g:plusone:count="false" style="margin-top:1px;"
67 - * $wgAddThisSBServ5
 68+ * $wgAddThisSBServ[5]
6869 * - Service code for 5th button in sidebar
6970 * Default is email
70 - * $wgAddThisSBServ5set
 71+ * $wgAddThisSBServ['5set']
7172 * - Settings for 5th button in sidebar
72 - * $wgAddThisHServ1
 73+ * $wgAddThisHServ[1]
7374 * - Service code for 1st button in article header after AddThis icon (which cannot be moved in the header)
7475 * Default is facebook
75 - * $wgAddThisHServ1set
 76+ * $wgAddThisHServ['1set']
7677 * - Settings for 1st button in article header
77 - * $wgAddThisHServ2
 78+ * $wgAddThisHServ[2]
7879 * - Service code for 2nd button in article header
7980 * Default is twitter
80 - * $wgAddThisHServ2set
 81+ * $wgAddThisHServ['2set']
8182 * - Settings for 2nd button in article header
82 - * $wgAddThisHServ3
 83+ * $wgAddThisHServ[3]
8384 * - Service code for 3rd button in article header
8485 * Default is google_plusone
85 - * $wgAddThisHServ3set
 86+ * $wgAddThisHServ['3set']
8687 * - Settings for 3rd button in article header
8788 * Default is g:plusone:count="false" style="margin-top:1px;"
88 - * $wgAddThisHServ4
 89+ * $wgAddThisHServ[4]
8990 * - Service code for 4th button in article header
9091 * Default is linkedin
91 - * $wgAddThisHServ4set
 92+ * $wgAddThisHServ['4set']
9293 * - Settings for 4th button in article header
93 - * $wgAddThisHServ5
 94+ * $wgAddThisHServ[5]
9495 * - Service code for 5th button in article header
9596 * Default is tumblr
96 - * $wgAddThisHServ5set
 97+ * $wgAddThisHServ['5set']
9798 * - Settings for 5th button in article header
98 - * $wgAddThisHServ6
 99+ * $wgAddThisHServ[6]
99100 * - Service code for 6th button in article header
100101 * Default is stumbleupon
101 - * $wgAddThisHServ6set
 102+ * $wgAddThisHServ['6set']
102103 * - Settings for 6th button in article header
103 - * $wgAddThisHServ7
 104+ * $wgAddThisHServ[7]
104105 * - Service code for 7th button in article header
105106 * Default is reddit
106 - * $wgAddThisHServ7set
 107+ * $wgAddThisHServ['7set']
107108 * - Settings for 7th button in article header
108 - * $wgAddThisHServ8
 109+ * $wgAddThisHServ[8]
109110 * - Service code for 8th button in article header
110111 * Default is email
111 - * $wgAddThisHServ8set
 112+ * $wgAddThisHServ['8set']
112113 * - Settings for 8th button in article header
113114 */
114115
——@@ -121,34 +122,38 @@
122123 $wgAddThisMain = 'true';
123124
124125 # Sidebar settings
125 -$wgAddThisSBServ1 = 'compact';
126 -$wgAddThisSBServ1set = '';
127 -$wgAddThisSBServ2 = 'facebook';
128 -$wgAddThisSBServ2set = '';
129 -$wgAddThisSBServ3 = 'twitter';
130 -$wgAddThisSBServ3set = '';
131 -$wgAddThisSBServ4 = 'google_plusone';
132 -$wgAddThisSBServ4set = 'g:plusone:count="false" style="margin-top:1px;"';
133 -$wgAddThisSBServ5 = 'email';
134 -$wgAddThisSBServ5set = '';
 126+$wgAddThisSBServ = array(
 127+ 1 => 'compact',
 128+ '1set' => '',
 129+ 2 => 'facebook',
 130+ '2set' => '',
 131+ 3 => 'twitter',
 132+ '3set' => '',
 133+ 4 => 'google_plusone',
 134+ '4set' => 'g:plusone:count="false" style="margin-top:1px;"',
 135+ 5 => 'email',
 136+ '5set' => '',
 137+);
135138
136139 # Toolbar settings
137 -$wgAddThisHServ1 = 'facebook';
138 -$wgAddThisHServ1set = '';
139 -$wgAddThisHServ2 = 'twitter';
140 -$wgAddThisHServ2set = '';
141 -$wgAddThisHServ3 = 'google_plusone';
142 -$wgAddThisHServ3set = 'g:plusone:count="false" style="margin-top:1px;"';
143 -$wgAddThisHServ4 = 'linkedin';
144 -$wgAddThisHServ4set = '';
145 -$wgAddThisHServ5 = 'tumblr';
146 -$wgAddThisHServ5set = '';
147 -$wgAddThisHServ6 = 'stumbleupon';
148 -$wgAddThisHServ6set = '';
149 -$wgAddThisHServ7 = 'reddit';
150 -$wgAddThisHServ7set = '';
151 -$wgAddThisHServ8 = 'email';
152 -$wgAddThisHServ8set = '';
 140+$wgAddThisHServ = array(
 141+ 1 => 'facebook',
 142+ '1set' => '',
 143+ 2 => 'twitter',
 144+ '2set' => '',
 145+ 3 => 'google_plusone',
 146+ '3set' => 'g:plusone:count="false" style="margin-top:1px;"',
 147+ 4 => 'linkedin',
 148+ '4set' => '',
 149+ 5 => 'tumblr',
 150+ '5set' => '',
 151+ 6 => 'stumbleupon',
 152+ '6set' => '',
 153+ 7 => 'reddit',
 154+ '7set' => '',
 155+ 8 => 'email',
 156+ '8set' => '',
 157+);
153158
154159
155160 /**

Follow-up revisions

RevisionCommit summaryAuthorDate
r104388improvement in use of arrays per feedback to r104384 - ty Johnduhartvarnent05:00, 28 November 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r104381initial extension commitvarnent23:37, 27 November 2011

Comments

#Comment by Johnduhart (talk | contribs)   00:55, 28 November 2011

Much better, however still room for improvement. How about instead of hardcoding the links you have a foreach loop that goes over an array of arrays. Let's say I give you this as a configuration: 

$wgAddThisSBServ = array(
	array(
		'type' => 'facebook',
	),
	array(
		'type' => 'google_plusone',
		'attribs' => 'g:plusone:count="false" style="margin-top:1px;"',
	),
	// Etc...
);

In my opinion this is much more cleaner than now.

Status & tagging log