Ejegg has submitted this change and it was merged. Change subject: Various fixes to CNBannerChoiceDataResourceLoaderModule ......................................................................
Various fixes to CNBannerChoiceDataResourceLoaderModule Still missing fix for getModifiedTimestamp Change-Id: I1e9651439966127a150cdcfe816e3a51f3924a94 --- M CentralNotice.php M includes/CNBannerChoiceDataResourceLoaderModule.php M tests/CNBannerChoicesResourceLoaderModuleTest.php 3 files changed, 14 insertions(+), 54 deletions(-) Approvals: Ejegg: Looks good to me, approved diff --git a/CentralNotice.php b/CentralNotice.php index 550e139..262dde8 100644 --- a/CentralNotice.php +++ b/CentralNotice.php @@ -90,9 +90,6 @@ // $wgCentralDBname). $wgCentralNoticeApiUrl = false; -// How long to cache the banner choice data in memcached, in seconds -$wgCentralNoticeBannerChoiceDataCacheExpiry = 300; - // Enable the new mechanism for making the banner selection on the client $wgCentralNoticeChooseBannerOnClient = false; diff --git a/includes/CNBannerChoiceDataResourceLoaderModule.php b/includes/CNBannerChoiceDataResourceLoaderModule.php index 3e7837f..ad547c7 100644 --- a/includes/CNBannerChoiceDataResourceLoaderModule.php +++ b/includes/CNBannerChoiceDataResourceLoaderModule.php @@ -5,9 +5,6 @@ * * Note: this module does nothing if $wgCentralNoticeChooseBannerOnClient * is false. - * - * HTTP query and caching modeled after EventLogging's RemoteSchema class. - * See https://github.com/wikimedia/mediawiki-extensions-EventLogging/blob/master/includes/RemoteSchema.php */ class CNBannerChoiceDataResourceLoaderModule extends ResourceLoaderModule { @@ -18,18 +15,10 @@ const API_REQUEST_TIMEOUT = 20; - protected $choices; - protected $key; - - public function __construct() { - $this->http = new Http(); - } - protected function getChoices( ResourceLoaderContext $context ) { global $wgNoticeProject, $wgUser, $wgCentralNoticeApiUrl, - $wgCentralNoticeBannerChoiceDataCacheExpiry, $wgCentralDBname; $project = $wgNoticeProject; @@ -37,29 +26,8 @@ // TODO Find out what's up with $context->getUser() $status = ( $wgUser->isAnon() ) ? 'anonymous' : 'loggedin'; - $key = $wgNoticeProject . '|' . $language . '|' . $status; - // Get via state variable if it's there and the key is the same - if ( ( $this->key === $key ) && $this->choices ) { - return $this->choices; - } - - $this->key = $key; - - // Hmmm, try the cache (if configured to) - $useCache = ( $wgCentralNoticeBannerChoiceDataCacheExpiry !== 0 ); - - if ( $useCache ) { - - $cache = wfGetCache( CACHE_ANYTHING ); - $this->choices = $cache->get( $key ); - - if ( $this->choices ) { - return $this->choices; - } - } - - // OK, fetch the data via the DB or the API. Decide which to use based + // Fetch the data via the DB or the API. Decide which to use based // on whether the appropriate global variables are set. // If something's amiss, we warn and return an empty array, but don't // bring everything to a standstill. @@ -84,12 +52,6 @@ return array(); } - if ( $useCache ) { - $cache->set( $key, $choices, - $wgCentralNoticeBannerChoiceDataCacheExpiry ); - } - - $this->choices = $choices; return $choices; } @@ -137,14 +99,19 @@ ); $url = wfAppendQuery( $wgCentralNoticeApiUrl, $q ); - $http = new Http(); - $apiResult = $http->get( $url, self::API_REQUEST_TIMEOUT * 0.8 ); + $apiResult = Http::get( $url, self::API_REQUEST_TIMEOUT * 0.8 ); if ( !$apiResult ) { + wfLogWarning( 'Couldn\'t get banner choice data via API.'); return false; } $parsedApiResult = FormatJson::parse( $apiResult ) ?: false; + + if ( !$parsedApiResult ) { + wfLogWarning( 'Couldn\'t parse banner choice data from API.'); + return false; + } if ( !isset( $parsedApiResult->value ) ) { wfLogWarning( 'Error fetching banner choice data via API: ' . @@ -162,7 +129,7 @@ return false; } - return $result; + return $result; } /** @@ -173,16 +140,13 @@ public function getScript( ResourceLoaderContext $context ) { global $wgCentralNoticeChooseBannerOnClient; - // Only send in data if we'll choose banners on the client - if ( $wgCentralNoticeChooseBannerOnClient ) { - - return Xml::encodeJsCall( 'mw.cnBannerControllerLib.setChoiceData', - array( $this->getChoices( $context ) ) ); - - } else { - // Otherwise, make this a no-op + // If we don't choose banners on the client, this is a no-op + if ( !$wgCentralNoticeChooseBannerOnClient ) { return ''; } + + return Xml::encodeJsCall( 'mw.cnBannerControllerLib.setChoiceData', + array( $this->getChoices( $context ) ) ); } /** diff --git a/tests/CNBannerChoicesResourceLoaderModuleTest.php b/tests/CNBannerChoicesResourceLoaderModuleTest.php index d65a393..d4a78c7 100644 --- a/tests/CNBannerChoicesResourceLoaderModuleTest.php +++ b/tests/CNBannerChoicesResourceLoaderModuleTest.php @@ -14,7 +14,6 @@ $this->setMwGlobals( array( 'wgCentralNoticeChooseBannerOnClient' => true, - 'wgCentralNoticeBannerChoiceDataCacheExpiry' => 0, 'wgNoticeProject' => 'wikipedia', ) ); $this->cnFixtures = new CentralNoticeTestFixtures(); -- To view, visit https://gerrit.wikimedia.org/r/173980 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: I1e9651439966127a150cdcfe816e3a51f3924a94 Gerrit-PatchSet: 5 Gerrit-Project: mediawiki/extensions/CentralNotice Gerrit-Branch: master Gerrit-Owner: AndyRussG <andrew.green...@gmail.com> Gerrit-Reviewer: AndyRussG <andrew.green...@gmail.com> Gerrit-Reviewer: Awight <awi...@wikimedia.org> Gerrit-Reviewer: Ejegg <eeggles...@wikimedia.org> Gerrit-Reviewer: Mwalker <mwal...@khaosdev.com> Gerrit-Reviewer: Ssmith <ssm...@wikimedia.org> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
