r83715 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r83714‎ | r83715 | r83716 >
Date:20:29, 11 March 2011
Author:saper
Status:resolved (Comments)
Tags:
Comment:
Improve fetching of the donation amount, remove unecessary globals.

* Add User-Agent header when accessing wikimediafoundation.org to prevent 403s
* If remote information is for some reason unavailable, don't display the banner at all
* Remove globals that are no longer used
* Use $wgDBName as the default $wgCentralDBname for small wikis. Schema update
will create tables in $wgDBName anyway.
Modified paths:
  • /trunk/extensions/CentralNotice/CentralNotice.php (modified) (history)
  • /trunk/extensions/CentralNotice/SpecialBannerLoader.php (modified) (history)

Diff [purge]

Index: trunk/extensions/CentralNotice/CentralNotice.php
@@ -25,11 +25,10 @@
2626
2727 // Enable the notice-hosting infrastructure on this wiki...
2828 // Leave at false for wikis that only use a sister site for the control.
29 -// All remaining options apply only to the infrastructure wiki.
3029 $wgNoticeInfrastructure = true;
3130
3231 // The name of the database which hosts the centralized campaign data
33 -$wgCentralDBname = '';
 32+$wgCentralDBname = $wgDBname;
3433
3534 // The script path on the wiki that hosts the CentralNotice infrastructure
3635 // For example 'http://meta.wikimedia.org/w/index.php'
@@ -40,24 +39,8 @@
4140 // for cached content
4241 $wgCentralNoticeLoader = true;
4342
44 -// If true, notice only displays if 'sitenotice=yes' is in the query string
45 -$wgNoticeTestMode = false;
46 -
47 -// Array of '$lang.$project' for exceptions to the test mode rule
48 -$wgNoticeEnabledSites = array();
49 -
50 -// Client-side cache timeout for the loader JS stub.
51 -// If 0, clients will (probably) rechceck it on every hit,
52 -// which is good for testing.
53 -$wgNoticeTimeout = 0;
54 -
55 -// Server-side cache timeout for the loader JS stub.
56 -// Should be big if you won't include the counter info in the text,
57 -// smallish if you will. :)
58 -$wgNoticeServerTimeout = 0;
59 -
6043 // Source for live counter information
61 -$wgNoticeCounterSource = "http://donate.wikimedia.org/counter.php";
 44+$wgNoticeCounterSource = 'http://wikimediafoundation.org/wiki/Special:ContributionTotal?action=raw';
6245
6346 // Domain to set global cookies for.
6447 // Example: '.wikipedia.org'
Index: trunk/extensions/CentralNotice/SpecialBannerLoader.php
@@ -28,14 +28,18 @@
2929
3030 if ( $wgRequest->getText( 'banner' ) ) {
3131 $bannerName = $wgRequest->getText( 'banner' );
32 - $content = $this->getJsNotice( $bannerName );
33 - if ( preg_match( "/<centralnotice-template-\w+>\z/", $content ) ) {
34 - echo "/* Failed cache lookup */";
35 - } elseif ( strlen( $content ) == 0 ) {
36 - // Hack for IE/Mac 0-length keepalive problem, see RawPage.php
37 - echo "/* Empty */";
38 - } else {
39 - echo $content;
 32+ try {
 33+ $content = $this->getJsNotice( $bannerName );
 34+ if ( preg_match( "/<centralnotice-template-\w+>\z/", $content ) ) {
 35+ echo "/* Failed cache lookup */";
 36+ } elseif ( strlen( $content ) == 0 ) {
 37+ // Hack for IE/Mac 0-length keepalive problem, see RawPage.php
 38+ echo "/* Empty */";
 39+ } else {
 40+ echo $content;
 41+ }
 42+ } catch (SpecialBannerLoaderException $e) {
 43+ echo "/* Banner could not be generated */";
4044 }
4145 } else {
4246 echo "/* No banner specified */";
@@ -106,7 +110,7 @@
107111 $field = $match[1];
108112 $params = array();
109113 if ( $field == 'amount' ) {
110 - $params = array( $this->formatNum( $this->getDonationAmount() ) );
 114+ $params = array( $this->toMillions( $this->getDonationAmount() ) );
111115 }
112116 $message = "centralnotice-{$this->bannerName}-$field";
113117 $source = $this->getMessage( $message, $params );
@@ -116,7 +120,7 @@
117121 /**
118122 * Convert number of dollars to millions of dollars
119123 */
120 - private function formatNum( $num ) {
 124+ private function toMillions( $num ) {
121125 $num = sprintf( "%.1f", $num / 1e6 );
122126 if ( substr( $num, - 2 ) == '.0' ) {
123127 $num = substr( $num, 0, - 2 );
@@ -151,6 +155,20 @@
152156
153157 return $out;
154158 }
 159+
 160+ private function fetchUrl($url) {
 161+ $ctx = stream_context_create('http' => array(
 162+ 'method' => "GET",
 163+ 'header' => "User-Agent: CentralNotice/1.0 (+http://www.mediawiki.org/wiki/Extension:CentralNotice)\r\n")
 164+ );
 165+ wfSuppressWarnings();
 166+ $content = file_get_contents( $url, false, $ctx);
 167+ wfRestoreWarnings();
 168+ if( !$content ) {
 169+ throw new RemoteServerProblemException();
 170+ }
 171+ return $content;
 172+ }
155173
156174 /**
157175 * Pull the current amount raised during a fundraiser
@@ -161,16 +179,13 @@
162180 $count = intval( $wgMemc->get( wfMemcKey( 'centralnotice', 'counter' ) ) );
163181 if ( !$count ) {
164182 // Pull from dynamic counter
165 - wfSuppressWarnings();
166 - $count = intval( file_get_contents( $wgNoticeCounterSource ) );
167 - wfRestoreWarnings();
 183+ $count = intval( $this->fetchUrl( $wgNoticeCounterSource ));
168184 if ( !$count ) {
169185 // Pull long-cached amount
170186 $count = intval( $wgMemc->get(
171187 wfMemcKey( 'centralnotice', 'counter', 'fallback' ) ) );
172188 if ( !$count ) {
173 - // Return hard-coded amount if all else fails
174 - return 1100000; // Update as needed during fundraiser
 189+ throw new DonationAmountUnknownException();
175190 }
176191 }
177192 // Expire in 60 seconds
@@ -181,3 +196,25 @@
182197 return $count;
183198 }
184199 }
 200+/**
 201+ * @defgroup Exception Exception
 202+ */
 203+
 204+/**
 205+ * SpecialBannerLoaderException exception
 206+ *
 207+ * This exception is being thrown whenever
 208+ * some fatal error occurs that may affect
 209+ * how the banner is presented.
 210+ *
 211+ * @ingroup Exception
 212+ */
 213+
 214+class SpecialBannerLoaderException extends Exception {
 215+}
 216+
 217+class RemoteServerProblemException extends SpecialBannerLoaderException {
 218+}
 219+
 220+class DonationAmountUnknownException extends SpecialBannerLoaderException {
 221+}

Follow-up revisions

RevisionCommit summaryAuthorDate
r83718Fix PHP Parse error from r83715, use $wgExtensionAssetsPath...saper20:59, 11 March 2011
r83744Followup to r83715: use internal HTTP client, handle exception when previewing...saper02:26, 12 March 2011

Comments

#Comment by Catrope (talk | contribs)   23:31, 11 March 2011
+		$ctx = stream_context_create('http' => array(
+			'method' => "GET",
+			'header' => "User-Agent: CentralNotice/1.0 (+[http://www.mediawiki.org/wiki/Extension:CentralNotice)\r\n http://www.mediawiki.org/wiki/Extension:CentralNotice)\r\n]")
+		);
+		wfSuppressWarnings();
+		$content = file_get_contents( $url, false, $ctx);
+		wfRestoreWarnings();
+		if( !$content ) {
+			throw new RemoteServerProblemException();
+		}
+		return $content;

We have Http::get() don't reinvent the wheel.

#Comment by Saper (talk | contribs)   10:58, 12 March 2011

Excellent! This was an old piece there, fixed in r83744.

#Comment by Brion VIBBER (talk | contribs)   18:07, 27 June 2011

Looks ok, though there is a lot of additional code duplication added in r83744 which should be undone.

Status & tagging log