Refactor Hooks code and move it to separate services

This cleans up the hook file a lot and movesthe logic to seperate services
provided by MediaWikiServices. This also removes some setters and passed-
around variables and stuff. Also fixes the unit tests by not querying
external services anymore.

Change-Id: I0c3c0b7f2f5bd68aaad624e8e2dcad9bdcf97770

extension.json

@@ -128,11 +128,15 @@
 	},
 	"AutoloadClasses": {
 		"CookieWarningHooks": "includes/CookieWarningHooks.php",
+		"CookieWarningDecisions": "includes/CookieWarningDecisions.php",
 		"CookieWarningTestTemplate": "tests/phpunit/includes/CookieWarningTestTemplate.php",
 		"GeoLocation": "includes/GeoLocation.php"
 	},
 	"ConfigRegistry": {
 		"cookiewarning": "GlobalVarConfig::newInstance"
 	},
+	"ServiceWiringFiles": [
+		"includes/ServiceWiring.php"
+	],
 	"manifest_version": 1
 }

includes/CookieWarningDecisions.php

@@ -0,0 +1,76 @@
+<?php
+
+class CookieWarningDecisions {
+	private $config;
+	private $geoLocation;
+
+	/**
+	 * @param Config $config
+	 * @param GeoLocation $geoLocation
+	 */
+	public function __construct( Config $config, GeoLocation $geoLocation ) {
+		$this->config = $config;
+		$this->geoLocation = $geoLocation;
+	}
+
+	/**
+	 * Checks, if the CookieWarning information bar should be visible to this user on
+	 * this page.
+	 *
+	 * @param IContextSource $context
+	 * @return bool Returns true, if the cookie warning should be visible, false otherwise.
+	 * @throws ConfigException
+	 * @throws MWException
+	 */
+	public function shouldShowCookieWarning( IContextSource $context ) {
+		$user = $context->getUser();
+
+		return $this->config->get( 'CookieWarningEnabled' ) &&
+			!$user->getBoolOption( 'cookiewarning_dismissed', false ) &&
+			!$context->getRequest()->getCookie( 'cookiewarning_dismissed' ) &&
+			( $this->config->get( 'CookieWarningGeoIPLookup' ) === 'js' ||
+				$this->isInConfiguredRegion( $context ) );
+	}
+
+	/**
+	 * Checks, if the user is in one of the configured regions.
+	 *
+	 * @TODO: This function or the function users should set the cookie or user option, if this
+	 * function returns false to avoid a location lookup on each request.
+	 * @param IContextSource $context
+	 * @return bool
+	 * @throws ConfigException
+	 * @throws MWException
+	 */
+	private function isInConfiguredRegion( IContextSource $context ) {
+		if ( !$this->config->get( 'CookieWarningForCountryCodes' ) ||
+			$this->config->get( 'CookieWarningGeoIPLookup' ) === 'none' ) {
+			wfDebugLog( 'CookieWarning', 'IP geolocation not configured, skipping.' );
+
+			return true;
+		}
+
+		wfDebugLog( 'CookieWarning', 'Try to locate the user\'s IP address.' );
+		$located = $this->geoLocation->locate( $context->getRequest()->getIP() );
+		if ( !$located ) {
+			wfDebugLog( 'CookieWarning',
+				'Locating the user\'s IP address failed or is' . ' configured false.' );
+
+			return true;
+		}
+		wfDebugLog( 'CookieWarning', 'Locating the user was successful, located' . ' region: ' .
+			$this->geoLocation->getCountryCode() );
+
+		return array_key_exists( $this->geoLocation->getCountryCode(),
+			$this->config->get( 'CookieWarningForCountryCodes' ) );
+	}
+
+	/**
+	 * @return bool
+	 * @throws ConfigException
+	 */
+	public function shouldAddResourceLoaderComponents() {
+		return $this->config->get( 'CookieWarningGeoIPLookup' ) === 'js' &&
+			is_array( $this->config->get( 'CookieWarningForCountryCodes' ) );
+	}
+}

includes/CookieWarningHooks.php

@@ -1,8 +1,8 @@
 <?php
 
-class CookieWarningHooks {
-	private static $inConfiguredRegion;
+use MediaWiki\MediaWikiServices;
 
+class CookieWarningHooks {
 	/**
 	 * BeforeInitialize hook handler.
 	 *
@@ -15,6 +15,7 @@ class CookieWarningHooks {
 	 * @param User &$user
 	 * @param WebRequest $request
 	 * @param MediaWiki $mediawiki
+	 * @throws MWException
 	 */
 	public static function onBeforeInitialize( Title &$title, &$unused, OutputPage &$output,
 		User &$user, WebRequest $request, MediaWiki $mediawiki
@@ -31,6 +32,7 @@ class CookieWarningHooks {
 		}
 		$output->redirect( $request->getRequestURL() );
 	}
+
 	/**
 	 * SkinTemplateOutputPageBeforeExec hook handler.
 	 *
@@ -38,17 +40,21 @@ class CookieWarningHooks {
 	 *
 	 * @param SkinTemplate &$sk
 	 * @param QuickTemplate &$tpl
+	 * @throws ConfigException
+	 * @throws MWException
 	 */
 	public static function onSkinTemplateOutputPageBeforeExec(
 		SkinTemplate &$sk, QuickTemplate &$tpl
 	) {
-		// if the cookiewarning should not be visible to the user, exit.
-		if ( !self::showWarning( $sk->getContext() ) ) {
+		/** @var CookieWarningDecisions $cookieWarningDecisions */
+		$cookieWarningDecisions = MediaWikiServices::getInstance()
+			->getService( 'CookieWarning.Decisions' );
+
+		if ( !$cookieWarningDecisions->shouldShowCookieWarning( $sk->getContext() ) ) {
 			return;
 		}
 		$moreLink = self::getMoreLink();
-		// if a "more information" URL was configured, add a link to it in the cookiewarning
-		// information bar
+
 		if ( $moreLink ) {
 			$moreLink = '&#160;' . Html::element(
 				'a',
@@ -108,21 +114,24 @@ class CookieWarningHooks {
 	 *  - the interface message MediaWiki:Cookie-policy-link (bc T145781)
 	 *
 	 * @return string|null The url or null if none set
+	 * @throws ConfigException
 	 */
 	private static function getMoreLink() {
-		// Config instance of CookieWarning
-		$conf = ConfigFactory::getDefaultInstance()->makeConfig( 'cookiewarning' );
+		$conf = self::getConfig();
 		if ( $conf->get( 'CookieWarningMoreUrl' ) ) {
 			return $conf->get( 'CookieWarningMoreUrl' );
 		}
+
 		$cookieWarningMessage = wfMessage( 'cookiewarning-more-link' );
 		if ( $cookieWarningMessage->exists() && !$cookieWarningMessage->isDisabled() ) {
 			return $cookieWarningMessage->escaped();
 		}
+
 		$cookiePolicyMessage = wfMessage( 'cookie-policy-link' );
 		if ( $cookiePolicyMessage->exists() && !$cookiePolicyMessage->isDisabled() ) {
 			return $cookiePolicyMessage->escaped();
 		}
+
 		return null;
 	}
 
@@ -132,42 +141,49 @@ class CookieWarningHooks {
 	 * Adds the required style and JS module, if cookiewarning is enabled.
 	 *
 	 * @param OutputPage $out
+	 * @throws ConfigException
+	 * @throws MWException
 	 */
 	public static function onBeforePageDisplay( OutputPage $out ) {
-		if ( self::showWarning( $out->getContext() ) ) {
-			$conf = self::getConfig();
-			if (
-				ExtensionRegistry::getInstance()->isLoaded( 'MobileFrontend' ) &&
-				MobileContext::singleton()->shouldDisplayMobileView()
-			) {
-				$moduleStyles = [ 'ext.CookieWarning.mobile.styles' ];
-			} else {
-				$moduleStyles = [ 'ext.CookieWarning.styles' ];
-			}
-			$modules = [ 'ext.CookieWarning' ];
-			if (
-				$conf->get( 'CookieWarningGeoIPLookup' ) === 'js' &&
-				is_array( $conf->get( 'CookieWarningForCountryCodes' ) )
-			) {
-				$modules[] = 'ext.CookieWarning.geolocation';
-				$moduleStyles[] = 'ext.CookieWarning.geolocation.styles';
-			}
-			$out->addModules( $modules );
-			$out->addModuleStyles( $moduleStyles );
+		/** @var CookieWarningDecisions $cookieWarningDecisions */
+		$cookieWarningDecisions = MediaWikiServices::getInstance()
+			->getService( 'CookieWarning.Decisions' );
+
+		if ( !$cookieWarningDecisions->shouldShowCookieWarning( $out->getContext() ) ) {
+			return;
 		}
+
+		if (
+			ExtensionRegistry::getInstance()->isLoaded( 'MobileFrontend' ) &&
+			MobileContext::singleton()->shouldDisplayMobileView()
+		) {
+			$moduleStyles = [ 'ext.CookieWarning.mobile.styles' ];
+		} else {
+			$moduleStyles = [ 'ext.CookieWarning.styles' ];
+		}
+		$modules = [ 'ext.CookieWarning' ];
+
+		if ( $cookieWarningDecisions->shouldAddResourceLoaderComponents() ) {
+			$modules[] = 'ext.CookieWarning.geolocation';
+			$moduleStyles[] = 'ext.CookieWarning.geolocation.styles';
+		}
+		$out->addModules( $modules );
+		$out->addModuleStyles( $moduleStyles );
 	}
 
 	/**
 	 * ResourceLoaderGetConfigVars hook handler.
 	 *
 	 * @param array &$vars
+	 * @throws ConfigException
 	 */
 	public static function onResourceLoaderGetConfigVars( array &$vars ) {
+		/** @var CookieWarningDecisions $cookieWarningDecisions */
+		$cookieWarningDecisions = MediaWikiServices::getInstance()
+			->getService( 'CookieWarning.Decisions' );
 		$conf = self::getConfig();
-		if (
-			$conf->get( 'CookieWarningGeoIPLookup' ) === 'js' &&
-			is_array( $conf->get( 'CookieWarningForCountryCodes' ) )
-		) {
+
+		if ( $cookieWarningDecisions->shouldAddResourceLoaderComponents() ) {
 			$vars += [
 				'wgCookieWarningGeoIPServiceURL' => $conf->get( 'CookieWarningGeoIPServiceURL' ),
 				'wgCookieWarningForCountryCodes' => $conf->get( 'CookieWarningForCountryCodes' ),
@@ -181,73 +197,7 @@ class CookieWarningHooks {
 	 * @return Config
 	 */
 	private static function getConfig() {
-		return ConfigFactory::getDefaultInstance()->makeConfig( 'cookiewarning' );
-	}
-
-	/**
-	 * Checks, if the CookieWarning information bar should be visible to this user on
-	 * this page.
-	 *
-	 * @param IContextSource $context
-	 * @return bool Returns true, if the cookie warning should be visible, false otherwise.
-	 */
-	private static function showWarning( IContextSource $context ) {
-		$user = $context->getUser();
-		$conf = self::getConfig();
-		if (
-			// if enabled in LocalSettings.php
-			$conf->get( 'CookieWarningEnabled' ) &&
-			// if not already dismissed by this user (and saved in the user prefs)
-			!$user->getBoolOption( 'cookiewarning_dismissed', false ) &&
-			// if not already dismissed by this user (and saved in the browser cookies)
-			!$context->getRequest()->getCookie( 'cookiewarning_dismissed' ) &&
-			(
-				$conf->get( 'CookieWarningGeoIPLookup' ) === 'js' ||
-				self::inConfiguredRegion( $context, $conf )
-			)
-		) {
-			return true;
-		}
-		return false;
-	}
-
-	/**
-	 * Checks, if the user is in one of the configured regions.
-	 *
-	 * @TODO: This function or the function users should set the cookie or user option, if this
-	 * function returns false to avoid a location lookup on each request.
-	 * @param IContextSource $context
-	 * @param Config $conf
-	 * @return bool
-	 */
-	private static function inConfiguredRegion( IContextSource $context, Config $conf ) {
-		if ( self::$inConfiguredRegion === null ) {
-			if (
-				!$conf->get( 'CookieWarningForCountryCodes' ) ||
-				$conf->get( 'CookieWarningGeoIPLookup' ) === 'none'
-			) {
-				wfDebugLog( 'CookieWarning', 'IP geolocation not configured, skipping.' );
-				self::$inConfiguredRegion = true;
-			} else {
-				wfDebugLog( 'CookieWarning', 'Try to locate the user\'s IP address.' );
-				$geoLocation = new GeoLocation;
-				$located = $geoLocation
-					->setConfig( $conf )
-					->setIP( $context->getRequest()->getIP() )
-					->locate();
-				if ( !$located ) {
-					wfDebugLog( 'CookieWarning', 'Locating the user\'s IP address failed or is' .
-						' configured false.' );
-					self::$inConfiguredRegion = true;
-				} else {
-					wfDebugLog( 'CookieWarning', 'Locating the user was successful, located' .
-						' region: ' . $geoLocation->getCountryCode() );
-					self::$inConfiguredRegion = array_key_exists( $geoLocation->getCountryCode(),
-						$conf->get( 'CookieWarningForCountryCodes' ) );
-				}
-			}
-		}
-		return self::$inConfiguredRegion;
+		return MediaWikiServices::getInstance()->getService( 'CookieWarning.Config' );
 	}
 
 	/**

includes/GeoLocation.php

@@ -7,44 +7,14 @@
  * Implements the GeoLocation class, which allows to locate the user based on the IP address.
  */
 class GeoLocation {
-	private $ip;
 	private $config;
 	private $countryCode;
 
 	/**
-	 * Set the IP address you want to locate.
-	 *
-	 * @param string $ip A valid IP address
-	 * @return $this
-	 */
-	public function setIP( $ip ) {
-		if ( !IP::isValid( $ip ) ) {
-			throw new InvalidArgumentException( "$ip is not a valid IP address." );
-		}
-		$this->ip = $ip;
-
-		return $this;
-	}
-
-	/**
-	 * Returns the IP address.
-	 *
-	 * @return null|string NULL if the address is not set so far, string otherwise.
-	 */
-	public function getIP() {
-		return $this->ip;
-	}
-
-	/**
-	 * Sets the Config object used by this class.
-	 *
 	 * @param Config $config
-	 * @return $this
 	 */
-	public function setConfig( Config $config ) {
+	public function __construct( Config $config ) {
 		$this->config = $config;
-
-		return $this;
 	}
 
 	/**
@@ -63,17 +33,14 @@ class GeoLocation {
 	 * other problem occures which resulted in a failed locating process, this function returns
 	 * false, otherwise it returns true.
 	 *
+	 * @param string $ip The IP address to lookup
 	 * @return bool|null NULL if no geolocation service configured, false on error, true otherwise.
+	 * @throws ConfigException
 	 */
-	public function locate() {
+	public function locate( $ip ) {
 		$this->countryCode = null;
-		if ( $this->ip === null ) {
-			throw new RuntimeException(
-				'No IP address set, locating now would return the servers location.' );
-		}
-		if ( $this->config === null ) {
-			throw new RuntimeException(
-				'You need to set the Config object first, before you can locate an IP address.' );
+		if ( !IP::isValid( $ip ) ) {
+			throw new InvalidArgumentException( "$ip is not a valid IP address." );
 		}
 		if ( !$this->config->get( 'CookieWarningGeoIPServiceURL' ) ) {
 			return null;
@@ -82,7 +49,7 @@ class GeoLocation {
 		if ( substr( $requestUrl, -1 ) !== '/' ) {
 			$requestUrl .= '/';
 		}
-		$json = Http::get( $requestUrl . $this->getIP(), [
+		$json = Http::get( $requestUrl . $ip, [
 			'timeout' => '2'
 		] );
 		if ( !$json ) {

includes/ServiceWiring.php

@@ -0,0 +1,17 @@
+<?php
+
+use MediaWiki\MediaWikiServices;
+
+return [
+	'CookieWarning.Config' => function ( MediaWikiServices $services ) {
+		return $services->getService( 'ConfigFactory' )
+			->makeConfig( 'cookiewarning' );
+	},
+	'GeoLocation' => function ( MediaWikiServices $services ) {
+		return new GeoLocation( $services->getService( 'CookieWarning.Config' ) );
+	},
+	'CookieWarning.Decisions' => function ( MediaWikiServices $services ) {
+		return new CookieWarningDecisions( $services->getService( 'CookieWarning.Config' ),
+			$services->getService( 'GeoLocation' ) );
+	},
+];

tests/phpunit/includes/CookieWarningHooksTest.php

@@ -124,12 +124,12 @@ class CookieWarningHooksTest extends MediaWikiLangTestCase {
 	public function testOnSkinTemplateOutputPageBeforeExecGeoLocation( $ipAddress, $countryCodes,
 		$expected
 	) {
-		$this->resetCookieWarningHooks();
 		$this->setMwGlobals( [
 			'wgCookieWarningEnabled' => true,
 			'wgCookieWarningGeoIPLookup' => is_array( $countryCodes ) ? 'php' : 'none',
 			'wgCookieWarningForCountryCodes' => $countryCodes,
 		] );
+		$this->mockGeoLocationService();
 
 		$request = new FauxRequest();
 		$request->setIP( $ipAddress );
@@ -166,13 +166,12 @@ class CookieWarningHooksTest extends MediaWikiLangTestCase {
 		];
 	}
 
-	private function resetCookieWarningHooks() {
-		// reset the inConfiguredRegion value to retrigger a location lookup, if called again
-		$singleton = CookieWarningHooks::class;
-		$reflection = new ReflectionClass( $singleton );
-		$instance = $reflection->getProperty( 'inConfiguredRegion' );
-		$instance->setAccessible( true );
-		$instance->setValue( null, null );
-		$instance->setAccessible( false );
+	private function mockGeoLocationService() {
+		$geoLocation = $this->getMockBuilder( GeoLocation::class )
+			->disableOriginalConstructor()
+			->getMock();
+		$geoLocation->method( 'locate' )->willReturn( true );
+		$geoLocation->method( 'getCountryCode' )->willReturn( 'US' );
+		$this->setService( 'GeoLocation', $geoLocation );
 	}
 }