r75370 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r75369‎ | r75370 | r75371 >
Date:17:53, 25 October 2010
Author:kaldari
Status:ok (Comments)
Tags:
Comment:
fixes for r75222 and r75181
Modified paths:
  • /trunk/extensions/CentralNotice/CentralNotice.php (modified) (history)
  • /trunk/extensions/CentralNotice/SpecialBannerController.php (modified) (history)

Diff [purge]

Index: trunk/extensions/CentralNotice/CentralNotice.php
@@ -145,7 +145,7 @@
146146 global $wgUser, $wgOut, $wgCentralDBname, $wgScript;
147147
148148 // Include '.js' to exempt script from squid cache override
149 - $centralLoader = $wgScript . '?title=' . SpecialPage::getTitleFor( 'BannerController' ) . '&cache=cn.js';
 149+ $centralLoader = SpecialPage::getTitleFor( 'BannerController' )->getLocalUrl( 'cache=cn.js' );
150150
151151 // Insert the banner controller Javascript into the <head>
152152 $wgOut->addScriptFile( $centralLoader );
Index: trunk/extensions/CentralNotice/SpecialBannerController.php
@@ -69,8 +69,8 @@
7070 } else {
7171 var geoLocation = Geo.country; // pull the geo info
7272 }
73 - var bannerListQuery = $.param( { 'language': wgContentLanguage, 'project': wgNoticeProject, 'country': geoLocation, 'cache': 'cn.js' } );
74 - bannerListURL = wgScript + '?title=' + wgFormattedNamespaces[-1] + ':BannerListLoader&' + bannerListQuery;
 73+ var bannerListQuery = $.param( { 'title': wgFormattedNamespaces[-1] + ':BannerListLoader', 'language': wgContentLanguage, 'project': wgNoticeProject, 'country': geoLocation, 'cache': 'cn.js' } );
 74+ bannerListURL = wgScript + '?' + bannerListQuery;
7575 var request = $.ajax( {
7676 url: bannerListURL,
7777 dataType: 'json',

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r75181potential fix for stats issuekaldari01:55, 22 October 2010
r75222fix for Bug 25564 (won't fix stats for meta), to test on Mondaykaldari23:51, 22 October 2010

Comments

#Comment by Trevor Parscal (WMF) (talk | contribs)   20:24, 25 October 2010

I don't see any issue with this code - I heard that there was possibly a concern about the way the URL was being built (see bannerListURL) but this seems sane to me.

#Comment by Catrope (talk | contribs)   19:28, 26 October 2010

There was a concern, yes, but this rev actually fixed it.

Status & tagging log