r98296 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r98295‎ | r98296 | r98297 >
Date:01:05, 28 September 2011
Author:kaldari
Status:ok (Comments)
Tags:fundraising 
Comment:
allow people to set expiration date for Special:HideBanners
Modified paths:
  • /trunk/extensions/CentralNotice/CentralNotice.php (modified) (history)
  • /trunk/extensions/CentralNotice/special/SpecialHideBanners.php (modified) (history)

Diff [purge]

Index: trunk/extensions/CentralNotice/special/SpecialHideBanners.php
——@@ -28,9 +28,16 @@
2929 readfile( dirname( __FILE__ ) . '/../1x1.png' );
3030 }
3131
 32+ /**
 33+ * Set the cookie for hiding fundraising banners.
 34+ */
3235 function setHideCookie() {
33 - global $wgNoticeCookieDomain, $wgCookieSecure;
34 - $exp = time() + 86400 * 14; // Cookie expires after 2 weeks
 36+ global $wgNoticeCookieDomain, $wgCookieSecure, $wgNoticeHideBannersExpiration;
 37+ if ( is_numeric( $wgNoticeHideBannersExpiration ) ) {
 38+ $exp = $wgNoticeHideBannersExpiration;
 39+ } else {
 40+ $exp = time() + 86400 * 14; // Cookie expires after 2 weeks
 41+ }
3542 if ( is_callable( array( 'CentralAuthUser', 'getCookieDomain' ) ) ) {
3643 $cookieDomain = CentralAuthUser::getCookieDomain();
3744 } else {
Index: trunk/extensions/CentralNotice/CentralNotice.php
——@@ -52,6 +52,11 @@
5353 // Example: '.wikipedia.org'
5454 $wgNoticeCookieDomain = '';
5555
 56+// When the cookie set in SpecialHideBanners.php should expire
 57+// This would typically be the end date for a fundraiser
 58+// NOTE: This must be in UNIX timestamp format, for example, '1325462400'
 59+$wgNoticeHideBannersExpiration = '';
 60+
5661 $wgExtensionFunctions[] = 'efCentralNoticeSetup';
5762
5863 $wgExtensionCredits['other'][] = array(

Follow-up revisions

RevisionCommit summaryAuthorDate
r98906MFT r98296awjrichards20:45, 4 October 2011
r98916MFT r92701, r95822, r95914, r98296awjrichards21:00, 4 October 2011

Comments

#Comment by Awjrichards (talk | contribs)   20:21, 4 October 2011

Looks good, but as a stylistic note, I'd recommend setting the default expiry time of 2 weeks when you create $wgNoticeHideBannersExpiration in CentralNotice.php. That way the default behavior is a lot clearer to someone configuring this stuff. Also, the comment above the definition of $wgNoticeHIdeBannersExpiration is a bit confusing - the way the processing code is written, you would actually need to configure $wgNoticeHideBannersExpiration to be the result of some kind of expression to do anything meaningful (or at least dynamic, like 'two weeks from now'). If you were to change your handling of determining the expiration time to using strtotime, you would greatly improve the userfriendly-ness/clarity of this feature. For instance, in SpecialHideBanners.php: 

		$exp = strtotime( $wgNoticeHideBannersExpiration );
                if ( !$exp ) {
                    // throw an error, retry a default, etc
                }

And in CentralNotice.php: // a string parsable by strtotime defining when the // banner cookie should expire $wgNoticeHideBannersExpiration = '+2 weeks';

Just a thought!

Status & tagging log