r47044 MediaWiki - Code Review archive

Repository:	MediaWiki
Revision:	< r47043‎ \| r47044 \| r47045 >
Date:	16:24, 9 February 2009
Author:	siebrand
Status:	deferred
Tags:
Comment:	Remove hard coding of pipe separators in skins. Use Language::pipeList as much as possible. Should also make code a bit more readable here and there.
Modified paths:	/trunk/phase3/includes/Skin.php (modified) (history) /trunk/phase3/skins/CologneBlue.php (modified) (history) /trunk/phase3/skins/Standard.php (modified) (history)

Diff [purge]

Index: trunk/phase3/skins/CologneBlue.php
—	—	@@ -66,7 +66,7 @@
67	67
68	68	function doAfterContent()
69	69	{
70		~~- global $wgOut;~~
	70	+ global $wgOut, $wgLang;
71	71
72	72	$s = "\n</div><br clear='all' />\n";
73	73
—	—	@@ -80,9 +80,11 @@
81	81	$s .= "<td class='bottom' align='center' valign='top'>";
82	82
83	83	$s .= $this->bottomLinks();
84		~~- $s .= "\n<br />" . $this->makeKnownLinkObj( Title::newMainPage() ) . " \| "~~
85		~~- . $this->aboutLink() . " \| "~~
86		~~- . $this->searchForm( wfMsg( "qbfind" ) );~~
	84	+ $s .= $wgLang->pipeList( array(
	85	+ "\n<br />" . $this->makeKnownLinkObj( Title::newMainPage() ),
	86	+ $this->aboutLink(),
	87	+ $this->searchForm( wfMsg( "qbfind" ) )
	88	+ ) );
87	89
88	90	$s .= "\n<br />" . $this->pageStats();
89	91
—	—	@@ -121,7 +123,7 @@
122	124	}
123	125
124	126	function sysLinks() {
125		~~- global $wgUser, $wgContLang, $wgTitle;~~
	127	+ global $wgUser, $wgLang, $wgContLang, $wgTitle;
126	128	$li = $wgContLang->specialPage("Userlogin");
127	129	$lo = $wgContLang->specialPage("Userlogout");
128	130
—	—	@@ -132,29 +134,28 @@
133	135	$q = "returnto={$rt}";
134	136	}
135	137
136		~~- $s = "" .~~
137		~~- $this->mainPageLink()~~
138		~~- . " \| " .~~
139		~~- $this->makeKnownLink( wfMsgForContent( "aboutpage" ), wfMsg( "about" ) )~~
140		~~- . " \| " .~~
141		~~- $this->makeKnownLink( wfMsgForContent( "helppage" ), wfMsg( "help" ) )~~
142		~~- . " \| " .~~
143		~~- $this->makeKnownLink( wfMsgForContent( "faqpage" ), wfMsg("faq") )~~
144		~~- . " \| " .~~
145		~~- $this->specialLink( "specialpages" );~~
	138	+ $s = array(
	139	+ $this->mainPageLink(),
	140	+ $this->makeKnownLink( wfMsgForContent( "aboutpage" ), wfMsg( "about" ) ),
	141	+ $this->makeKnownLink( wfMsgForContent( "helppage" ), wfMsg( "help" ) ),
	142	+ $this->makeKnownLink( wfMsgForContent( "faqpage" ), wfMsg("faq") ),
	143	+ $this->specialLink( "specialpages" )
	144	+ );
146	145
147	146	/* show links to different language variants */
148		~~- $s .= $this->variantLinks();~~
149		~~- $s .= $this->extensionTabLinks();~~
150		-
151		~~- $s .= " \| ";~~
	147	+ if( $this->variantLinks() ) {
	148	+ $s[] = $this->variantLinks();
	149	+ }
	150	+ if( $this->extensionTabLinks() ) {
	151	+ $s[] = $this->extensionTabLinks();
	152	+ }
152	153	if ( $wgUser->isLoggedIn() ) {
153		~~- $s .= $this->makeKnownLink( $lo, wfMsg( "logout" ), $q );~~
	154	+ $s[] = $this->makeKnownLink( $lo, wfMsg( "logout" ), $q );
154	155	} else {
155		~~- $s .= $this->makeKnownLink( $li, wfMsg( "login" ), $q );~~
	156	+ $s[] = $this->makeKnownLink( $li, wfMsg( "login" ), $q );
156	157	}
157	158
158		~~- return $s;~~
	159	+ return $wgLang->pipeList( $s );
159	160	}
160	161
161	162	/**
Index: trunk/phase3/skins/Standard.php
—	—	@@ -82,7 +82,7 @@
83	83	}
84	84
85	85	function doAfterContent() {
86		~~- global $wgContLang;~~
	86	+ global $wgContLang, $wgLang;
87	87	$fname = 'SkinStandard::doAfterContent';
88	88	wfProfileIn( $fname );
89	89	wfProfileIn( $fname.'-1' );
—	—	@@ -108,10 +108,11 @@
109	109	$s .= "<td class='bottom' align='$l' valign='top'>";
110	110
111	111	$s .= $this->bottomLinks();
112		~~- $s .= "\n<br />" . $this->mainPageLink()~~
113		~~- . ' \| ' . $this->aboutLink()~~
114		~~- . ' \| ' . $this->specialLink( 'recentchanges' )~~
115		~~- . ' \| ' . $this->searchForm()~~
	112	+ $s .= "\n<br />" . $wgLang->pipeList( array(
	113	+ $this->mainPageLink(),
	114	+ $this->aboutLink(),
	115	+ $this->specialLink( 'recentchanges' ),
	116	+ $this->searchForm() ) )
116	117	. '<br /><span id="pagestats">' . $this->pageStats() . '</span>';
117	118
118	119	$s .= "</td>";
Index: trunk/phase3/includes/Skin.php
—	—	@@ -940,20 +940,20 @@
941	941	function doAfterContent() { return "</div></div>"; }
942	942
943	943	function pageTitleLinks() {
944		~~- global $wgOut, $wgTitle, $wgUser, $wgRequest;~~
	944	+ global $wgOut, $wgTitle, $wgUser, $wgRequest, $wgLang;
945	945
946	946	$oldid = $wgRequest->getVal( 'oldid' );
947	947	$diff = $wgRequest->getVal( 'diff' );
948	948	$action = $wgRequest->getText( 'action' );
949	949
950		~~- $s = $this->printableLink();~~
	950	+ $s[] = $this->printableLink();
951	951	$disclaimer = $this->disclaimerLink(); # may be empty
952	952	if( $disclaimer ) {
953		~~- $s .= ' \| ' . $disclaimer;~~
	953	+ $s[] = $disclaimer;
954	954	}
955	955	$privacy = $this->privacyLink(); # may be empty too
956	956	if( $privacy ) {
957		~~- $s .= ' \| ' . $privacy;~~
	957	+ $s[] = $privacy;
958	958	}
959	959
960	960	if ( $wgOut->isArticleRelated() ) {
—	—	@@ -963,12 +963,12 @@
964	964	if( $image ) {
965	965	$link = htmlspecialchars( $image->getURL() );
966	966	$style = $this->getInternalLinkAttributes( $link, $name );
967		~~- $s .= " \| <a href=\"{$link}\"{$style}>{$name}</a>";~~
	967	+ $s[] = "<a href=\"{$link}\"{$style}>{$name}</a>";
968	968	}
969	969	}
970	970	}
971	971	if ( 'history' == $action \|\| isset( $diff ) \|\| isset( $oldid ) ) {
972		~~- $s .= ' \| ' . $this->makeKnownLinkObj( $wgTitle,~~
	972	+ $s[] .= $this->makeKnownLinkObj( $wgTitle,
973	973	wfMsg( 'currentrev' ) );
974	974	}
975	975
—	—	@@ -978,7 +978,7 @@
979	979	if( !$wgTitle->equals( $wgUser->getTalkPage() ) ) {
980	980	$tl = $this->makeKnownLinkObj( $wgUser->getTalkPage(), wfMsgHtml( 'newmessageslink' ), 'redirect=no' );
981	981	$dl = $this->makeKnownLinkObj( $wgUser->getTalkPage(), wfMsgHtml( 'newmessagesdifflink' ), 'diff=cur' );
982		~~- $s.= ' \| <strong>'. wfMsg( 'youhavenewmessages', $tl, $dl ) . '</strong>';~~
	982	+ $s[] = '<strong>'. wfMsg( 'youhavenewmessages', $tl, $dl ) . '</strong>';
983	983	# disable caching
984	984	$wgOut->setSquidMaxage(0);
985	985	$wgOut->enableClientCache(false);
—	—	@@ -987,9 +987,9 @@
988	988
989	989	$undelete = $this->getUndeleteLink();
990	990	if( !empty( $undelete ) ) {
991		~~- $s .= ' \| '.$undelete;~~
	991	+ $s[] = $undelete;
992	992	}
993		~~- return $s;~~
	993	+ return $wgLang->pipeList( $s );
994	994	}
995	995
996	996	function getUndeleteLink() {
—	—	@@ -1012,18 +1012,18 @@
1013	1013	}
1014	1014
1015	1015	function printableLink() {
1016		~~- global $wgOut, $wgFeedClasses, $wgRequest;~~
	1016	+ global $wgOut, $wgFeedClasses, $wgRequest, $wgLang;
1017	1017
1018	1018	$printurl = $wgRequest->escapeAppendQuery( 'printable=yes' );
1019	1019
1020		~~- $s = "<a href=\"$printurl\" rel=\"alternate\">" . wfMsg( 'printableversion' ) . '</a>';~~
	1020	+ $s[] = "<a href=\"$printurl\" rel=\"alternate\">" . wfMsg( 'printableversion' ) . '</a>';
1021	1021	if( $wgOut->isSyndicated() ) {
1022	1022	foreach( $wgFeedClasses as $format => $class ) {
1023	1023	$feedurl = $wgRequest->escapeAppendQuery( "feed=$format" );
1024		~~- $s .= " \| <a href=\"$feedurl\" rel=\"alternate\" type=\"application/{$format}+xml\" class=\"feedlink\">{$format}</a>";~~
	1024	+ $s[] = "<a href=\"$feedurl\" rel=\"alternate\" type=\"application/{$format}+xml\" class=\"feedlink\">{$format}</a>";
1025	1025	}
1026	1026	}
1027		~~- return $s;~~
	1027	+ return $wgLang->pipeList( $s );
1028	1028	}
1029	1029
1030	1030	function pageTitle() {
—	—	@@ -1068,7 +1068,7 @@
1069	1069	$getlink = $this->makeKnownLinkObj( $linkObj, htmlspecialchars( $display ) );
1070	1070	$c++;
1071	1071	if ($c>1) {
1072		~~- $subpages .= ' \| ';~~
	1072	+ $subpages .= wfMsgExt( 'pipe-separator' , 'escapenoentities' );
1073	1073	} else {
1074	1074	$subpages .= '< ';
1075	1075	}
—	—	@@ -1131,16 +1131,21 @@
1132	1132	$ret .= $this->link( $wgUser->getUserPage(),
1133	1133	htmlspecialchars( $wgUser->getName() ) );
1134	1134	$ret .= " ($talkLink)<br />";
1135		~~- $ret .= $this->link(~~
1136		~~- SpecialPage::getTitleFor( 'Userlogout' ), wfMsg( 'logout' ),~~
1137		~~- array(), array( 'returnto' => $returnTo )~~
1138		~~- );~~
1139		~~- $ret .= ' \| ' . $this->specialLink( 'preferences' );~~
	1135	+ $ret .= $wgLang->pipeList( array(
	1136	+ $this->link(
	1137	+ SpecialPage::getTitleFor( 'Userlogout' ), wfMsg( 'logout' ),
	1138	+ array(), array( 'returnto' => $returnTo )
	1139	+ ),
	1140	+ $this->specialLink( 'preferences' ),
	1141	+ ) );
1140	1142	}
1141		~~- $ret .= ' \| ' . $this->link(~~
1142		~~- Title::newFromText( wfMsgForContent( 'helppage' ) ),~~
1143		~~- wfMsg( 'help' )~~
1144		~~- );~~
	1143	+ $ret = $wgLang->pipeList( array(
	1144	+ $ret,
	1145	+ $this->link(
	1146	+ Title::newFromText( wfMsgForContent( 'helppage' ) ),
	1147	+ wfMsg( 'help' )
	1148	+ ),
	1149	+ ) );
1145	1150
1146	1151	return $ret;
1147	1152	}
—	—	@@ -1179,23 +1184,29 @@
1180	1185
1181	1186	function topLinks() {
1182	1187	global $wgOut;
1183		~~- $sep = " \|\n";~~
1184	1188
1185		~~- $s = $this->mainPageLink() . $sep~~
1186		~~- . $this->specialLink( 'recentchanges' );~~
	1189	+ $s = array(
	1190	+ $this->mainPageLink(),
	1191	+ $this->specialLink( 'recentchanges' )
	1192	+ );
1187	1193
1188	1194	if ( $wgOut->isArticleRelated() ) {
1189		~~- $s .= $sep . $this->editThisPage()~~
1190		~~- . $sep . $this->historyLink();~~
	1195	+ $s[] = $this->editThisPage();
	1196	+ $s[] = $this->historyLink();
1191	1197	}
1192	1198	# Many people don't like this dropdown box
1193		~~- #$s .= $sep . $this->specialPagesList();~~
	1199	+ #$s[] = $this->specialPagesList();
1194	1200
1195		~~- $s .= $this->variantLinks();~~
	1201	+ if( $this->variantLinks() ) {
	1202	+ $s = $this->variantLinks();
	1203	+ }
1196	1204
1197		~~- $s .= $this->extensionTabLinks();~~
	1205	+ if( $this->extensionTabLinks() ) {
	1206	+ $s[] = $this->extensionTabLinks();
	1207	+ }
1198	1208
1199		~~- return $s;~~
	1209	+ // FIXME: Is using Language::pipeList impossible here? Do not quite understand the use of the newline
	1210	+ return implode( $s, wfMsgExt( 'pipe-separator' , 'escapenoentities' ) . "\n" );
1200	1211	}
1201	1212
1202	1213	/**
—	—	@@ -1206,14 +1217,23 @@
1207	1218	*/
1208	1219	function extensionTabLinks() {
1209	1220	$tabs = array();
1210		~~- $s = '';~~
	1221	+ $out = '';
	1222	+ $s = array();
1211	1223	wfRunHooks( 'SkinTemplateTabs', array( $this, &$tabs ) );
1212	1224	foreach( $tabs as $tab ) {
1213		~~- $s .= ' \| ' . Xml::element( 'a',~~
	1225	+ $s[] = Xml::element( 'a',
1214	1226	array( 'href' => $tab['href'] ),
1215	1227	$tab['text'] );
1216	1228	}
1217		~~- return $s;~~
	1229	+
	1230	+ if( count( $s ) ) {
	1231	+ global $wgLang;
	1232	+
	1233	+ $out = wfMsgExt( 'pipe-separator' , 'escapenoentities' );
	1234	+ $out .= $wgLang->pipeList( $s );
	1235	+ }
	1236	+
	1237	+ return $out;
1218	1238	}
1219	1239
1220	1240	/**
—	—	@@ -1223,14 +1243,14 @@
1224	1244	function variantLinks() {
1225	1245	$s = '';
1226	1246	/* show links to different language variants */
1227		~~- global $wgDisableLangConversion, $wgContLang, $wgTitle;~~
	1247	+ global $wgDisableLangConversion, $wgLang, $wgContLang, $wgTitle;
1228	1248	$variants = $wgContLang->getVariants();
1229	1249	if( !$wgDisableLangConversion && sizeof( $variants ) > 1 ) {
1230	1250	foreach( $variants as $code ) {
1231	1251	$varname = $wgContLang->getVariantname( $code );
1232	1252	if( $varname == 'disable' )
1233	1253	continue;
1234		~~- $s .= ' \| <a href="' . $wgTitle->escapeLocalUrl( 'variant=' . $code ) . '">' . htmlspecialchars( $varname ) . '</a>';~~
	1254	+ $s = $wgLang->pipeList( array( $s, '<a href="' . $wgTitle->escapeLocalUrl( 'variant=' . $code ) . '">' . htmlspecialchars( $varname ) . '</a>' ) );
1235	1255	}
1236	1256	}
1237	1257	return $s;
—	—	@@ -1238,21 +1258,21 @@
1239	1259
1240	1260	function bottomLinks() {
1241	1261	global $wgOut, $wgUser, $wgTitle, $wgUseTrackbacks;
1242		~~- $sep = " \|\n";~~
	1262	+ $sep = wfMsgExt( 'pipe-separator' , 'escapenoentities' ) . "\n";
1243	1263
1244	1264	$s = '';
1245	1265	if ( $wgOut->isArticleRelated() ) {
1246		~~- $s .= '<strong>' . $this->editThisPage() . '</strong>';~~
	1266	+ $element[] = '<strong>' . $this->editThisPage() . '</strong>';
1247	1267	if ( $wgUser->isLoggedIn() ) {
1248		~~- $s .= $sep . $this->watchThisPage();~~
	1268	+ $element[] = $this->watchThisPage();
1249	1269	}
1250		~~- $s .= $sep . $this->talkLink()~~
1251		~~- . $sep . $this->historyLink()~~
1252		~~- . $sep . $this->whatLinksHere()~~
1253		~~- . $sep . $this->watchPageLinksLink();~~
	1270	+ $element[] = $this->talkLink();
	1271	+ $element[] = $this->historyLink();
	1272	+ $element[] = $this->whatLinksHere();
	1273	+ $element[] = $this->watchPageLinksLink();
1254	1274
1255	1275	if ($wgUseTrackbacks)
1256		~~- $s .= $sep . $this->trackbackLink();~~
	1276	+ $element[] = $this->trackbackLink();
1257	1277
1258	1278	if ( $wgTitle->getNamespace() == NS_USER
1259	1279	\|\| $wgTitle->getNamespace() == NS_USER_TALK )
—	—	@@ -1262,12 +1282,15 @@
1263	1283	$ip=User::isIP($wgTitle->getText());
1264	1284
1265	1285	if($id \|\| $ip) { # both anons and non-anons have contri list
1266		~~- $s .= $sep . $this->userContribsLink();~~
	1286	+ $element[] = $this->userContribsLink();
1267	1287	}
1268	1288	if( $this->showEmailUser( $id ) ) {
1269		~~- $s .= $sep . $this->emailUserLink();~~
	1289	+ $element[] = $this->emailUserLink();
1270	1290	}
1271	1291	}
	1292	+
	1293	+ $s = implode( $element, $sep );
	1294	+
1272	1295	if ( $wgTitle->getArticleId() ) {
1273	1296	$s .= "\n<br />";
1274	1297	if($wgUser->isAllowed('delete')) { $s .= $this->deleteThisPage(); }
—	—	@@ -1276,6 +1299,7 @@
1277	1300	}
1278	1301	$s .= "<br />\n" . $this->otherLanguages();
1279	1302	}
	1303	+
1280	1304	return $s;
1281	1305	}
1282	1306
—	—	@@ -1657,7 +1681,7 @@
1658	1682	$first = true;
1659	1683	if($wgContLang->isRTL()) $s .= '<span dir="LTR">';
1660	1684	foreach( $a as $l ) {
1661		~~- if ( ! $first ) { $s .= ' \| '; }~~
	1685	+ if ( ! $first ) { $s .= wfMsgExt( 'pipe-separator' , 'escapenoentities' ); }
1662	1686	$first = false;
1663	1687
1664	1688	$nt = Title::newFromText( $l );

Follow-up revisions

Revision	Commit summary	Author	Date
r47046	(bug 7509) Separation strings should be configurable...	siebrand	17:48, 9 February 2009

Status & tagging log

00:20, 14 September 2011 Meno25 (talk | contribs) changed the status of r47044 [removed: old added: deferred]