More refactoring to Updates.php base & Columns\Updater, make Updates.php methods instance methods, create Update instances via DI, make Columns\Updater use instance methods instead of static, and add integration test for Columns\Updater.

1 files changed, 59 insertions, 61 deletions

diff --git a/core/Columns/Updater.php b/core/Columns/Updater.php
index 093910a0f2..e23b7f4375 100644
--- a/core/Columns/Updater.php
+++ b/core/Columns/Updater.php
@@ -7,6 +7,7 @@
  *
  */
 namespace Piwik\Columns;
+
 use Piwik\Common;
 use Piwik\DbHelper;
 use Piwik\Plugin\Dimension\ActionDimension;
@@ -25,65 +26,78 @@ class Updater extends \Piwik\Updates
     private static $cacheId = 'AllDimensionModifyTime';
 
     /**
-     * Return SQL to be executed in this update
-     *
-     * @return array(
-     *              'ALTER .... ' => '1234', // if the query fails, it will be ignored if the error code is 1234
-     *              'ALTER .... ' => false,  // if an error occurs, the update will stop and fail
-     *                                       // and user will have to manually run the query
-     *         )
+     * @var VisitDimension[]
+     */
+    public $visitDimensions;
+
+    /**
+     * @var ActionDimension[]
      */
-    public static function getMigrationQueries(PiwikUpdater $updater)
+    private $actionDimensions;
+
+    /**
+     * @var ConversionDimension[]
+     */
+    private $conversionDimensions;
+
+    /**
+     * @param VisitDimension[]|null $visitDimensions
+     * @param ActionDimension[]|null $actionDimensions
+     * @param ConversionDimension[]|null $conversionDimensions
+     */
+    public function __construct(array $visitDimensions = null, array $actionDimensions = null, array $conversionDimensions = null)
+    {
+        $this->visitDimensions = $visitDimensions === null ? VisitDimension::getAllDimensions() : $visitDimensions;
+        $this->actionDimensions = $actionDimensions === null ? ActionDimension::getAllDimensions() : $actionDimensions;
+        $this->conversionDimensions = $conversionDimensions === null ? ConversionDimension::getAllDimensions() : $conversionDimensions;
+    }
+
+    public function getMigrationQueries(PiwikUpdater $updater)
     {
         $sqls = array();
 
-        $changingColumns = self::getUpdates($updater);
+        $changingColumns = $this->getUpdates($updater);
 
         foreach ($changingColumns as $table => $columns) {
             if (empty($columns) || !is_array($columns)) {
                 continue;
             }
 
-            $sqls["ALTER TABLE `" . Common::prefixTable($table) . "` " . implode(', ', $columns)] = false;
+            $sqls["ALTER TABLE `" . Common::prefixTable($table) . "` " . implode(', ', $columns)] = array('1091', '1060');
         }
 
         return $sqls;
     }
 
-    /**
-     * Incremental version update
-     */
-    public static function doUpdate(PiwikUpdater $updater)
+    public function doUpdate(PiwikUpdater $updater)
     {
-        foreach (self::getMigrationQueries($updater) as $sql => $errorCode) {
-            $updater->executeMigrationQuery($sql, $errorsToIgnore = array('1091', '1060'), __FILE__);
-        }
+        $updater->executeMigrationQueries(__FILE__, $this->getMigrationQueries($updater));
     }
 
-    private static function getUpdates(PiwikUpdater $updater)
+    private function getUpdates(PiwikUpdater $updater)
     {
         $visitColumns      = DbHelper::getTableColumns(Common::prefixTable('log_visit'));
         $actionColumns     = DbHelper::getTableColumns(Common::prefixTable('log_link_visit_action'));
         $conversionColumns = DbHelper::getTableColumns(Common::prefixTable('log_conversion'));
 
-        $changingColumns = array();
+        $allUpdatesToRun = array();
 
-        foreach (self::getVisitDimensions() as $dimension) {
-            $updates         = self::getUpdatesForDimension($updater, $dimension, 'log_visit.', $visitColumns, $conversionColumns);
-            $changingColumns = self::mixinUpdates($changingColumns, $updates);
+        foreach ($this->visitDimensions as $dimension) {
+            $updates         = $this->getUpdatesForDimension($updater, $dimension, 'log_visit.', $visitColumns, $conversionColumns);
+            $allUpdatesToRun = $this->mixinUpdates($allUpdatesToRun, $updates);
         }
 
-        foreach (self::getActionDimensions() as $dimension) {
-            $updates         = self::getUpdatesForDimension($updater, $dimension, 'log_link_visit_action.', $actionColumns);
-            $changingColumns = self::mixinUpdates($changingColumns, $updates);
+        foreach ($this->actionDimensions as $dimension) {
+            $updates         = $this->getUpdatesForDimension($updater, $dimension, 'log_link_visit_action.', $actionColumns);
+            $allUpdatesToRun = $this->mixinUpdates($allUpdatesToRun, $updates);
         }
 
-        foreach (self::getConversionDimensions() as $dimension) {
-            $updates         = self::getUpdatesForDimension($updater, $dimension, 'log_conversion.', $conversionColumns);
-            $changingColumns = self::mixinUpdates($changingColumns, $updates);
+        foreach ($this->conversionDimensions as $dimension) {
+            $updates         = $this->getUpdatesForDimension($updater, $dimension, 'log_conversion.', $conversionColumns);
+            $allUpdatesToRun = $this->mixinUpdates($allUpdatesToRun, $updates);
         }
 
-        return $changingColumns;
+        return $allUpdatesToRun;
     }
 
     /**
@@ -93,7 +107,7 @@ class Updater extends \Piwik\Updates
      * @param array $conversionColumns
      * @return array
      */
-    private static function getUpdatesForDimension(PiwikUpdater $updater, $dimension, $componentPrefix, $existingColumnsInDb, $conversionColumns = array())
+    private function getUpdatesForDimension(PiwikUpdater $updater, $dimension, $componentPrefix, $existingColumnsInDb, $conversionColumns = array())
     {
         $column = $dimension->getColumnName();
         $componentName = $componentPrefix . $column;
@@ -115,19 +129,19 @@ class Updater extends \Piwik\Updates
         return $sqlUpdates;
     }
 
-    private static function mixinUpdates($changingColumns, $updatesFromDimension)
+    private function mixinUpdates($allUpdatesToRun, $updatesFromDimension)
     {
         if (!empty($updatesFromDimension)) {
             foreach ($updatesFromDimension as $table => $col) {
-                if (empty($changingColumns[$table])) {
-                    $changingColumns[$table] = $col;
+                if (empty($allUpdatesToRun[$table])) {
+                    $allUpdatesToRun[$table] = $col;
                 } else {
-                    $changingColumns[$table] = array_merge($changingColumns[$table], $col);
+                    $allUpdatesToRun[$table] = array_merge($allUpdatesToRun[$table], $col);
                 }
             }
         }
 
-        return $changingColumns;
+        return $allUpdatesToRun;
     }
 
     public function getAllVersions(PiwikUpdater $updater)
@@ -148,15 +162,15 @@ class Updater extends \Piwik\Updates
         $actionColumns     = DbHelper::getTableColumns(Common::prefixTable('log_link_visit_action'));
         $conversionColumns = DbHelper::getTableColumns(Common::prefixTable('log_conversion'));
 
-        foreach (self::getVisitDimensions() as $dimension) {
+        foreach ($this->visitDimensions as $dimension) {
             $versions = $this->mixinVersions($updater, $dimension, 'log_visit.', $visitColumns, $versions);
         }
 
-        foreach (self::getActionDimensions() as $dimension) {
+        foreach ($this->actionDimensions as $dimension) {
             $versions = $this->mixinVersions($updater, $dimension, 'log_link_visit_action.', $actionColumns, $versions);
         }
 
-        foreach (self::getConversionDimensions() as $dimension) {
+        foreach ($this->conversionDimensions as $dimension) {
             $versions = $this->mixinVersions($updater, $dimension, 'log_conversion.', $conversionColumns, $versions);
         }
 
@@ -174,6 +188,7 @@ class Updater extends \Piwik\Updates
     {
         $columnName = $dimension->getColumnName();
 
+        // dimensions w/o columns do not need DB updates
         if (!$columnName || !$dimension->hasColumnType()) {
             return $versions;
         }
@@ -181,6 +196,10 @@ class Updater extends \Piwik\Updates
         $component = $componentPrefix . $columnName;
         $version   = $dimension->getVersion();
 
+        // if the column exists in the table, but has no associated version, and was one of the core columns
+        // that was moved when the dimension refactor took place, then:
+        // - set the installed version in the DB to the current code version
+        // - and do not check for updates since we just set the version to the latest
         if (array_key_exists($columnName, $columns)
             && false === $updater->getCurrentComponentVersion($component)
             && self::wasDimensionMovedFromCoreToPlugin($component, $version)
@@ -217,7 +236,7 @@ class Updater extends \Piwik\Updates
             'log_visit.config_realplayer' => 'TINYINT(1) NOT NULL',
             'log_visit.config_device_type' => 'TINYINT( 100 ) NULL DEFAULT NULL',
             'log_visit.visitor_localtime' => 'TIME NOT NULL',
-            'log_visit.location_region' => 'char(2) DEFAULT NULL1',
+            'log_visit.location_region' => 'char(2) DEFAULT NULL1', // TODO: some of these have "NULL1", is this a bug?
             'log_visit.visitor_days_since_last' => 'SMALLINT(5) UNSIGNED NOT NULL',
             'log_visit.location_longitude' => 'float(10, 6) DEFAULT NULL1',
             'log_visit.visit_total_events' => 'SMALLINT(5) UNSIGNED NOT NULL',
@@ -317,25 +336,4 @@ class Updater extends \Piwik\Updates
 
         return array();
     }
-
-    private static function getVisitDimensions()
-    {
-        return VisitDimension::getAllDimensions();
-    }
-
-    /**
-     * @return mixed|Dimension[]
-     */
-    private static function getActionDimensions()
-    {
-        return ActionDimension::getAllDimensions();
-    }
-
-    /**
-     * @return mixed|Dimension[]
-     */
-    private static function getConversionDimensions()
-    {
-        return ConversionDimension::getAllDimensions();
-    }
-}
+}
\ No newline at end of file