Refactoring tests & LocationFetcher (renamed to VisitorGeolocator and documented as service).

6 files changed, 182 insertions, 144 deletions

diff --git a/plugins/UserCountry/Columns/Base.php b/plugins/UserCountry/Columns/Base.php
index 880279dc1b..d28db0e426 100644
--- a/plugins/UserCountry/Columns/Base.php
+++ b/plugins/UserCountry/Columns/Base.php
@@ -11,7 +11,7 @@ namespace Piwik\Plugins\UserCountry\Columns;
 use Piwik\Common;
 use Piwik\Network\IPUtils;
 use Piwik\Plugin\Dimension\VisitDimension;
-use Piwik\Plugins\UserCountry\LocationFetcher;
+use Piwik\Plugins\UserCountry\VisitorGeolocator;
 use Piwik\Plugins\UserCountry\LocationProvider\GeoIp;
 use Piwik\Plugins\UserCountry\LocationProvider;
 use Piwik\Plugins\PrivacyManager\Config as PrivacyManagerConfig;
@@ -21,7 +21,7 @@ use Piwik\Tracker\Request;
 abstract class Base extends VisitDimension
 {
     /**
-     * @var LocationFetcher
+     * @var VisitorGeolocator
      */
     private $locationFetcher;
 
@@ -61,7 +61,7 @@ abstract class Base extends VisitDimension
     protected function getLocationFetcher()
     {
         if ($this->locationFetcher === null) {
-            $this->locationFetcher = new LocationFetcher();
+            $this->locationFetcher = new VisitorGeolocator();
         }
 
         return $this->locationFetcher;
diff --git a/plugins/UserCountry/Commands/AttributeHistoricalDataWithLocations.php b/plugins/UserCountry/Commands/AttributeHistoricalDataWithLocations.php
index b00a5062b4..43fb8042cf 100644
--- a/plugins/UserCountry/Commands/AttributeHistoricalDataWithLocations.php
+++ b/plugins/UserCountry/Commands/AttributeHistoricalDataWithLocations.php
@@ -11,7 +11,7 @@ namespace Piwik\Plugins\UserCountry\Commands;
 use Piwik\DataAccess\RawLogUpdater;
 use Piwik\Network\IPUtils;
 use Piwik\Plugin\ConsoleCommand;
-use Piwik\Plugins\UserCountry\LocationFetcher;
+use Piwik\Plugins\UserCountry\VisitorGeolocator;
 use Piwik\Plugins\UserCountry\LocationProvider;
 use Piwik\DataAccess\RawLogFetcher;
 use Symfony\Component\Console\Input\InputArgument;
@@ -44,7 +44,7 @@ class AttributeHistoricalDataWithLocations extends ConsoleCommand
     protected $updater;
 
     /**
-     * @var LocationFetcher
+     * @var VisitorGeolocator
      */
     protected $locationFetcher;
 
@@ -143,7 +143,8 @@ class AttributeHistoricalDataWithLocations extends ConsoleCommand
             return 1;
         }
 
-        $this->locationFetcher = new LocationFetcher($input->getArgument(self::PROVIDER_ARGUMENT));
+        $providerId = $input->getArgument(self::PROVIDER_ARGUMENT);
+        $this->locationFetcher = new VisitorGeolocator(LocationProvider::getProviderById($providerId) ?: null);
 
         $output->writeln(
             sprintf('Re-attribution for date range: %s to %s. %d visits to process with provider "%s".',
diff --git a/plugins/UserCountry/LocationFetcher.php b/plugins/UserCountry/LocationFetcher.php
deleted file mode 100644
index 82e36e62ca..0000000000
--- a/plugins/UserCountry/LocationFetcher.php
+++ /dev/null
@@ -1,128 +0,0 @@
-<?php
-/**
- * Piwik - free/libre analytics platform
- *
- * @link http://piwik.org
- * @license http://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later
- *
- */
-namespace Piwik\Plugins\UserCountry;
-
-use Piwik\Common;
-use Piwik\Plugins\UserCountry\LocationProvider\DefaultProvider;
-use Piwik\Tracker\Visit;
-
-require_once PIWIK_INCLUDE_PATH . "/plugins/UserCountry/LocationProvider.php";
-
-class LocationFetcher
-{
-    /**
-     * @var array
-     */
-    protected static $cachedLocations = array();
-
-    /**
-     * @var string
-     */
-    protected $currentLocationProviderId;
-
-    /**
-     * @var callable|string
-     */
-    protected $getProviderByIdCallback;
-
-    /**
-     * @param null|string $currentLocationProviderId
-     * @param null $getProviderByIdCallback
-     */
-    public function __construct($currentLocationProviderId = null, $getProviderByIdCallback = null)
-    {
-        if ($currentLocationProviderId === null) {
-            $currentLocationProviderId = Common::getCurrentLocationProviderId();
-        }
-
-        $this->currentLocationProviderId = $currentLocationProviderId;
-
-        if (!is_callable($getProviderByIdCallback)) {
-            $getProviderByIdCallback = '\Piwik\Plugins\UserCountry\LocationProvider::getProviderById';
-        }
-
-        $this->getProviderByIdCallback = $getProviderByIdCallback;
-    }
-
-    public function getLocation($userInfo, $useClassCache = true, $defaultProviderId = null)
-    {
-        if ($defaultProviderId === null) {
-            $defaultProviderId = DefaultProvider::ID;
-        }
-
-        $userInfoKey = md5(implode(',', $userInfo));
-
-        if (array_key_exists($userInfoKey, self::$cachedLocations) && $useClassCache) {
-            return self::$cachedLocations[$userInfoKey];
-        }
-
-        $provider = $this->getProvider();
-        $location = $this->getLocationObject($provider, $userInfo);
-
-        if (empty($location)) {
-            $providerId = $provider->getId();
-            Common::printDebug("GEO: couldn't find a location with Geo Module '$providerId'");
-
-            if (empty($provider) && $defaultProviderId == $provider->getId()) {
-                Common::printDebug("Using default provider as fallback...");
-                $provider = call_user_func($this->getProviderByIdCallback, $defaultProviderId);
-                $location = $this->getLocationObject($provider, $userInfo);
-            }
-        }
-
-        if (empty($location)) {
-            $location = array();
-        }
-
-        if (empty($location['country_code'])) {
-            $location['country_code'] = Visit::UNKNOWN_CODE;
-        }
-
-        self::$cachedLocations[$userInfoKey] = $location;
-
-        return $location;
-    }
-
-    public function getProvider($defaultProviderId = null)
-    {
-        if ($defaultProviderId === null) {
-            $defaultProviderId = DefaultProvider::ID;
-        }
-
-        $id = $this->currentLocationProviderId;
-        $provider = call_user_func($this->getProviderByIdCallback, $id);
-
-        if ($provider === false) {
-            $provider = call_user_func($this->getProviderByIdCallback, $defaultProviderId);
-            Common::printDebug("GEO: no current location provider sent, falling back to default '$id' one.");
-        }
-
-        return $provider;
-    }
-
-    /**
-     * @param LocationProvider $provider
-     * @param array $userInfo
-     * @return array|false
-     */
-    private function getLocationObject(LocationProvider $provider, $userInfo)
-    {
-        $location   = $provider->getLocation($userInfo);
-        $providerId = $provider->getId();
-        $ipAddress  = $userInfo['ip'];
-
-        if ($location === false) {
-            return false;
-        }
-
-        Common::printDebug("GEO: Found IP $ipAddress location (provider '" . $providerId . "'): " . var_export($location, true));
-
-        return $location;
-    }
-} 
diff --git a/plugins/UserCountry/VisitorGeolocator.php b/plugins/UserCountry/VisitorGeolocator.php
new file mode 100644
index 0000000000..6111aeafa7
--- /dev/null
+++ b/plugins/UserCountry/VisitorGeolocator.php
@@ -0,0 +1,161 @@
+<?php
+/**
+ * Piwik - free/libre analytics platform
+ *
+ * @link http://piwik.org
+ * @license http://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later
+ *
+ */
+namespace Piwik\Plugins\UserCountry;
+
+use Piwik\Cache\Cache;
+use Piwik\Cache\Transient;
+use Piwik\Common;
+use Piwik\Plugins\UserCountry\LocationProvider\DefaultProvider;
+use Piwik\Tracker\Visit;
+
+require_once PIWIK_INCLUDE_PATH . "/plugins/UserCountry/LocationProvider.php";
+
+/**
+ * Service that determines a visitor's location using visitor information.
+ *
+ * Individual locations are provided by a LocationProvider instance. By default,
+ * the configured LocationProvider (as determined by
+ * `Common::getCurrentLocationProviderId()` is used.
+ *
+ * If the configured location provider cannot provide a location for the visitor,
+ * the default location provider (`DefaultProvider`) is used.
+ *
+ * A cache is used internally to speed up location retrieval. By default, an
+ * in-memory cache is used, but another type of cache can be supplied during
+ * construction.
+ *
+ * This service can be used from within the tracker.
+ */
+class VisitorGeolocator
+{
+    /**
+     * @var Cache
+     */
+    protected static $defaultLocationCache = null;
+
+    /**
+     * @var LocationProvider
+     */
+    private $provider;
+
+    /**
+     * @var LocationProvider
+     */
+    private $backupProvider;
+
+    /**
+     * @var Cache
+     */
+    private $locationCache;
+
+    /**
+     * @param LocationProvider $provider
+     * @param LocationProvider $backupProvider
+     * @param Cache $locationCache
+     */
+    public function __construct(LocationProvider $provider = null, LocationProvider $backupProvider = null, Cache $locationCache = null)
+    {
+        if ($provider === null) {
+            // note: Common::getCurrentLocationProviderId() uses the tracker cache, so this should be used here instead
+            // of accessing the option table
+            $provider = LocationProvider::getProviderById(Common::getCurrentLocationProviderId());
+
+            if (empty($provider)) {
+                Common::printDebug("GEO: no current location provider sent, falling back to default '" . DefaultProvider::ID . "' one.");
+
+                $provider = $this->getDefaultProvider();
+            }
+        }
+        $this->provider = $provider;
+
+        $this->backupProvider = $backupProvider ?: $this->getDefaultProvider();
+        $this->locationCache = $locationCache ?: self::getDefaultLocationCache();
+    }
+
+    public function getLocation($userInfo, $useClassCache = true)
+    {
+        $userInfoKey = md5(implode(',', $userInfo));
+        if ($useClassCache
+            && $this->locationCache->contains($userInfoKey)
+        ) {
+            return $this->locationCache->fetch($userInfoKey);
+        }
+
+        $location = $this->getLocationObject($this->provider, $userInfo);
+
+        if (empty($location)) {
+            $providerId = $this->provider->getId();
+            Common::printDebug("GEO: couldn't find a location with Geo Module '$providerId'");
+
+            if ($providerId != $this->backupProvider->getId()) {
+                Common::printDebug("Using default provider as fallback...");
+
+                $location = $this->getLocationObject($this->backupProvider, $userInfo);
+            }
+        }
+
+        $location = $location ?: array();
+        if (empty($location['country_code'])) {
+            $location['country_code'] = Visit::UNKNOWN_CODE;
+        }
+
+        $this->locationCache->save($userInfoKey, $location);
+
+        return $location;
+    }
+
+    /**
+     * @param LocationProvider $provider
+     * @param array $userInfo
+     * @return array|false
+     */
+    private function getLocationObject(LocationProvider $provider, $userInfo)
+    {
+        $location   = $provider->getLocation($userInfo);
+        $providerId = $provider->getId();
+        $ipAddress  = $userInfo['ip'];
+
+        if ($location === false) {
+            return false;
+        }
+
+        Common::printDebug("GEO: Found IP $ipAddress location (provider '" . $providerId . "'): " . var_export($location, true));
+
+        return $location;
+    }
+
+    /**
+     * @return LocationProvider
+     */
+    public function getProvider()
+    {
+        return $this->provider;
+    }
+
+    /**
+     * @return LocationProvider
+     */
+    public function getBackupProvider()
+    {
+        return $this->backupProvider;
+    }
+
+    private function getDefaultProvider()
+    {
+        return LocationProvider::getProviderById(DefaultProvider::ID);
+    }
+
+    public static function getDefaultLocationCache()
+    {
+        if (self::$defaultLocationCache === null) {
+            self::$defaultLocationCache = new Transient();
+        }
+        return self::$defaultLocationCache;
+    }
+}
\ No newline at end of file
diff --git a/plugins/UserCountry/tests/Integration/AttributeHistoricalDataWithLocationsTest.php b/plugins/UserCountry/tests/Integration/AttributeHistoricalDataWithLocationsTest.php
index ef29c1a131..5fdf8febbf 100644
--- a/plugins/UserCountry/tests/Integration/AttributeHistoricalDataWithLocationsTest.php
+++ b/plugins/UserCountry/tests/Integration/AttributeHistoricalDataWithLocationsTest.php
@@ -10,7 +10,7 @@ namespace Piwik\Plugins\UserCountry\Test\Integration;
 
 use Piwik\Plugins\UserCountry\Commands\AttributeHistoricalDataWithLocations;
 use Piwik\Tests\Fixtures\ManyVisitsWithGeoIP;
-use Piwik\Tests\Framework\TestCase\SystemTestCase;
+use Piwik\Tests\Framework\TestCase\IntegrationTestCase;
 use Symfony\Component\Console\Application;
 use Symfony\Component\Console\Tester\CommandTester;
 
@@ -20,9 +20,14 @@ use Symfony\Component\Console\Tester\CommandTester;
  *
  * @group UserCountry
  */
-class AttributeHistoricalDataWithLocationsTest extends SystemTestCase
+class AttributeHistoricalDataWithLocationsTest extends IntegrationTestCase
 {
     /**
+     * @var ManyVisitsWithGeoIP
+     */
+    public static $fixture = null;
+
+    /**
      * @expectedException \RuntimeException
      * @expectedExceptionMessage  Not enough arguments
      */
@@ -41,16 +46,13 @@ class AttributeHistoricalDataWithLocationsTest extends SystemTestCase
     public function testExecute_ShouldReturnEmptyWorkingProcessLogs_IfThereIsNoData()
     {
         $this->assertRegExp(
-            '/Re-attribution for date range: 2014-06-01 to 2014-06-06. 0 visits to process with provider "default"./',
+            '/Re-attribution for date range: 2014-06-01 to 2014-06-06. 0 visits to process with provider "geoip_php"./',
             $this->executeCommand('2014-06-01,2014-06-06')
         );
     }
 
     public function testExecute_ShouldReturnLogAfterWorkingWithSomeData_IfThereAreData()
     {
-        $fixture = new ManyVisitsWithGeoIP();
-        $fixture->setUp();
-
         $result = $this->executeCommand('2010-01-03,2010-06-03');
 
         $this->assertContains(
@@ -97,3 +99,5 @@ class AttributeHistoricalDataWithLocationsTest extends SystemTestCase
         return $stream;
     }
 } 
+
+AttributeHistoricalDataWithLocationsTest::$fixture = new ManyVisitsWithGeoIP();
\ No newline at end of file
diff --git a/plugins/UserCountry/tests/Unit/LocationFetcherTest.php b/plugins/UserCountry/tests/Unit/LocationFetcherTest.php
index ae0f3dd9b7..2337309202 100644
--- a/plugins/UserCountry/tests/Unit/LocationFetcherTest.php
+++ b/plugins/UserCountry/tests/Unit/LocationFetcherTest.php
@@ -9,7 +9,7 @@
 namespace Piwik\Plugins\UserCountry\tests\Unit;
 
 use PHPUnit_Framework_MockObject_MockObject;
-use Piwik\Plugins\UserCountry\LocationFetcher;
+use Piwik\Plugins\UserCountry\VisitorGeolocator;
 use Piwik\Plugins\UserCountry\LocationProvider;
 use Piwik\Tracker\Visit;
 
@@ -130,7 +130,7 @@ class LocationFetcherTest extends ProviderTest
                 }
             ));
 
-        $locationFetcher = new LocationFetcher(
+        $locationFetcher = new VisitorGeolocator(
             'CurrentProviderId', array($providerGetterMock, 'getProviderById'), 'DefaultProviderId'
         );
 
@@ -160,7 +160,7 @@ class LocationFetcherTest extends ProviderTest
                 }
             ));
 
-        $locationFetcher = new LocationFetcher(
+        $locationFetcher = new VisitorGeolocator(
             'CurrentProviderId', array($providerGetterMock, 'getProviderById')
         );
 
@@ -170,7 +170,7 @@ class LocationFetcherTest extends ProviderTest
     /**
      * @param $provider
      * @param string $currentLocationProviderId
-     * @return PHPUnit_Framework_MockObject_MockObject|LocationFetcher
+     * @return PHPUnit_Framework_MockObject_MockObject|VisitorGeolocator
      */
     protected function getLocationFetcherWithProviderMock($provider, $currentLocationProviderId = null)
     {