this is a very complicated one. Problem was when updating from 2.4.0 to 2.5.0-b1 the updater wanted to update all dimensions (meaning alter all columns in log_visit, link_action and conversion) to the same column type. This was happening because the system did not know those dimensions were already installed from a previous Piwik version. There was an update script that was supposed to tell Piwik those components are actually already installed since we only moved them from core to plugins but it cannot work as it is an update as well and therefore not executed before the actual update check. I tried many solutions to overcome this issue including reverting all the columns to the initial MySql Schema but even then there are problems as some plugins like DevicesDetection are not defined in MySql Schema but in the plugin. It is especially complicated since users might update from 2.4 to a future version where the column type of one of those dimension changes and we need to make sure to actually execute an alter update if one of those dimension changes. In such a case we cannot directly mark the component as successfully recorded. The conclusion was for me it is only possible to solve this problem by listing all dimensions that were moved from core to plugins including their version at that time hard coded...

5 files changed, 154 insertions, 91 deletions

diff --git a/core/Columns/Dimension.php b/core/Columns/Dimension.php
index d58284ce7c..4455480cdf 100644
--- a/core/Columns/Dimension.php
+++ b/core/Columns/Dimension.php
@@ -66,6 +66,11 @@ abstract class Dimension
         return $this->columnName;
     }
 
+    public function hasColumnType()
+    {
+        return !empty($this->columnType);
+    }
+
     abstract public function getName();
 
     /**
diff --git a/core/Columns/Updater.php b/core/Columns/Updater.php
index ed67b81702..c04779a81c 100644
--- a/core/Columns/Updater.php
+++ b/core/Columns/Updater.php
@@ -88,18 +88,7 @@ class Updater extends \Piwik\Updates
         $changingColumns = array();
 
         foreach (VisitDimension::getAllDimensions() as $dimension) {
-            $column = $dimension->getColumnName();
-
-            if (!self::hasComponentNewVersion('log_visit.' . $column)) {
-                continue;
-            }
-
-            if (array_key_exists($column, $visitColumns)) {
-                $updates = $dimension->update($visitColumns, $conversionColumns);
-            } else {
-                $updates = $dimension->install();
-            }
-
+            $updates         = self::getUpdatesForDimension($dimension, 'log_visit.', $visitColumns, $conversionColumns);
             $changingColumns = self::mixinUpdates($changingColumns, $updates);
         }
 
@@ -116,37 +105,28 @@ class Updater extends \Piwik\Updates
         return $changingColumns;
     }
 
-    private static function mixinUpdates($changingColumns, $updatesFromDimension)
-    {
-        if (!empty($updatesFromDimension)) {
-            foreach ($updatesFromDimension as $table => $col) {
-                if (empty($changingColumns[$table])) {
-                    $changingColumns[$table] = $col;
-                } else {
-                    $changingColumns[$table] = array_merge($changingColumns[$table], $col);
-                }
-            }
-        }
-
-        return $changingColumns;
-    }
-
     /**
-     * @param ActionDimension|ConversionDimension $dimension
+     * @param ActionDimension|ConversionDimension|VisitDimension $dimension
      * @param string $componentPrefix
      * @param array $existingColumnsInDb
+     * @param array $conversionColumns
      * @return array
      */
-    private static function getUpdatesForDimension($dimension, $componentPrefix, $existingColumnsInDb)
+    private static function getUpdatesForDimension($dimension, $componentPrefix, $existingColumnsInDb, $conversionColumns = array())
     {
         $column = $dimension->getColumnName();
+        $componentName = $componentPrefix . $column;
 
-        if (!self::hasComponentNewVersion($componentPrefix . $column)) {
+        if (!self::hasComponentNewVersion($componentName)) {
             return array();
         }
 
         if (array_key_exists($column, $existingColumnsInDb)) {
-            $sqlUpdates = $dimension->update($existingColumnsInDb);
+            if ($dimension instanceof VisitDimension) {
+                $sqlUpdates = $dimension->update($conversionColumns);
+            } else {
+                $sqlUpdates = $dimension->update();
+            }
         } else {
             $sqlUpdates = $dimension->install();
         }
@@ -154,6 +134,21 @@ class Updater extends \Piwik\Updates
         return $sqlUpdates;
     }
 
+    private static function mixinUpdates($changingColumns, $updatesFromDimension)
+    {
+        if (!empty($updatesFromDimension)) {
+            foreach ($updatesFromDimension as $table => $col) {
+                if (empty($changingColumns[$table])) {
+                    $changingColumns[$table] = $col;
+                } else {
+                    $changingColumns[$table] = array_merge($changingColumns[$table], $col);
+                }
+            }
+        }
+
+        return $changingColumns;
+    }
+
     public static function getAllVersions()
     {
         // to avoid having to load all dimensions on each request we check if there were any changes on the file system
@@ -170,21 +165,21 @@ class Updater extends \Piwik\Updates
 
         foreach (VisitDimension::getAllDimensions() as $dimension) {
             $columnName = $dimension->getColumnName();
-            if ($columnName) {
+            if ($columnName && $dimension->hasColumnType()) {
                 $versions['log_visit.' . $columnName] = $dimension->getVersion();
             }
         }
 
         foreach (ActionDimension::getAllDimensions() as $dimension) {
             $columnName = $dimension->getColumnName();
-            if ($columnName) {
+            if ($columnName && $dimension->hasColumnType()) {
                 $versions['log_link_visit_action.' . $columnName] = $dimension->getVersion();
             }
         }
 
         foreach (ConversionDimension::getAllDimensions() as $dimension) {
             $columnName = $dimension->getColumnName();
-            if ($columnName) {
+            if ($columnName && $dimension->hasColumnType()) {
                 $versions['log_conversion.' . $columnName] = $dimension->getVersion();
             }
         }
@@ -192,6 +187,86 @@ class Updater extends \Piwik\Updates
         return $versions;
     }
 
+    public static function isDimensionComponent($name)
+    {
+        return 0 === strpos($name, 'log_visit.')
+            || 0 === strpos($name, 'log_conversion.')
+            || 0 === strpos($name, 'log_conversion_item.')
+            || 0 === strpos($name, 'log_link_visit_action.');
+    }
+
+    public static function wasDimensionMovedFromCoreToPlugin($name, $version)
+    {
+        $dimensions = array (
+            'log_visit.config_resolution' => 'VARCHAR(9) NOT NULL',
+            'log_visit.config_device_brand' => 'VARCHAR( 100 ) CHARACTER SET utf8 COLLATE utf8_general_ci NULL DEFAULT NULL',
+            'log_visit.config_device_model' => 'VARCHAR( 100 ) CHARACTER SET utf8 COLLATE utf8_general_ci NULL DEFAULT NULL',
+            'log_visit.config_windowsmedia' => 'TINYINT(1) NOT NULL',
+            'log_visit.config_silverlight' => 'TINYINT(1) NOT NULL',
+            'log_visit.config_java' => 'TINYINT(1) NOT NULL',
+            'log_visit.config_gears' => 'TINYINT(1) NOT NULL',
+            'log_visit.config_pdf' => 'TINYINT(1) NOT NULL',
+            'log_visit.config_quicktime' => 'TINYINT(1) NOT NULL',
+            '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.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',
+            'log_visit.config_os_version' => 'VARCHAR( 100 ) CHARACTER SET utf8 COLLATE utf8_general_ci NULL DEFAULT NULL',
+            'log_visit.location_city' => 'varchar(255) DEFAULT NULL1',
+            'log_visit.location_country' => 'CHAR(3) NOT NULL1',
+            'log_visit.location_latitude' => 'float(10, 6) DEFAULT NULL1',
+            'log_visit.config_flash' => 'TINYINT(1) NOT NULL',
+            'log_visit.config_director' => 'TINYINT(1) NOT NULL',
+            'log_visit.visit_total_time' => 'SMALLINT(5) UNSIGNED NOT NULL',
+            'log_visit.visitor_count_visits' => 'SMALLINT(5) UNSIGNED NOT NULL1',
+            'log_visit.example_visit_dimension' => 'INTEGER(11) DEFAULT 0 NOT NULL',
+            'log_visit.visit_entry_idaction_name' => 'INTEGER(11) UNSIGNED NOT NULL',
+            'log_visit.visit_entry_idaction_url' => 'INTEGER(11) UNSIGNED NOT NULL',
+            'log_visit.visitor_returning' => 'TINYINT(1) NOT NULL1',
+            'log_visit.visitor_days_since_order' => 'SMALLINT(5) UNSIGNED NOT NULL1',
+            'log_visit.visit_goal_buyer' => 'TINYINT(1) NOT NULL',
+            'log_visit.visit_first_action_time' => 'DATETIME NOT NULL',
+            'log_visit.visit_goal_converted' => 'TINYINT(1) NOT NULL',
+            'log_visit.visitor_days_since_first' => 'SMALLINT(5) UNSIGNED NOT NULL1',
+            'log_visit.visit_exit_idaction_name' => 'INTEGER(11) UNSIGNED NOT NULL',
+            'log_visit.visit_exit_idaction_url' => 'INTEGER(11) UNSIGNED NULL DEFAULT 0',
+            'log_visit.config_browser_version' => 'VARCHAR(20) NOT NULL',
+            'log_visit.config_browser_name' => 'VARCHAR(10) NOT NULL',
+            'log_visit.location_browser_lang' => 'VARCHAR(20) NOT NULL',
+            'log_visit.config_os' => 'CHAR(3) NOT NULL',
+            'log_visit.config_cookie' => 'TINYINT(1) NOT NULL',
+            'log_visit.referer_visit_server_date' => 'date default NULL1',
+            'log_visit.referer_url' => 'TEXT NOT NULL',
+            'log_visit.visit_total_searches' => 'SMALLINT(5) UNSIGNED NOT NULL',
+            'log_visit.visit_total_actions' => 'SMALLINT(5) UNSIGNED NOT NULL',
+            'log_visit.referer_keyword' => 'VARCHAR(255) NULL1',
+            'log_visit.referer_name' => 'VARCHAR(70) NULL1',
+            'log_visit.referer_type' => 'TINYINT(1) UNSIGNED NULL1',
+            'log_link_visit_action.example_action_dimension' => 'VARCHAR(255) DEFAULT NULL',
+            'log_link_visit_action.idaction_name' => 'INTEGER(10) UNSIGNED',
+            'log_link_visit_action.idaction_url' => 'INTEGER(10) UNSIGNED DEFAULT NULL',
+            'log_link_visit_action.server_time' => 'DATETIME NOT NULL',
+            'log_link_visit_action.time_spent_ref_action' => 'INTEGER(10) UNSIGNED NOT NULL',
+            'log_link_visit_action.idaction_event_action' => 'INTEGER(10) UNSIGNED DEFAULT NULL',
+            'log_link_visit_action.idaction_event_category' => 'INTEGER(10) UNSIGNED DEFAULT NULL',
+            'log_conversion.example_conversion_dimension' => 'INTEGER(11) DEFAULT 0 NOT NULL',
+            'log_conversion.revenue_discount' => 'float default NULL',
+            'log_conversion.revenue' => 'float default NULL',
+            'log_conversion.revenue_shipping' => 'float default NULL',
+            'log_conversion.revenue_subtotal' => 'float default NULL',
+            'log_conversion.revenue_tax' => 'float default NULL',
+        );
+
+        if (!array_key_exists($name, $dimensions)) {
+            return false;
+        }
+
+        return strtolower($dimensions[$name]) === strtolower($version);
+    }
+
     public static function onNoUpdateAvailable($versionsThatWereChecked)
     {
         if (!empty($versionsThatWereChecked)) {
diff --git a/core/Plugin/Dimension/VisitDimension.php b/core/Plugin/Dimension/VisitDimension.php
index f87f09acc8..cf7204bdd5 100644
--- a/core/Plugin/Dimension/VisitDimension.php
+++ b/core/Plugin/Dimension/VisitDimension.php
@@ -45,7 +45,7 @@ abstract class VisitDimension extends Dimension
         return $changes;
     }
 
-    public function update($visitColumns, $conversionColumns)
+    public function update($conversionColumns)
     {
         if (!$this->columnType) {
             return array();
diff --git a/core/Updater.php b/core/Updater.php
index 747163dfad..79754172d9 100644
--- a/core/Updater.php
+++ b/core/Updater.php
@@ -62,6 +62,30 @@ class Updater
     }
 
     /**
+     * Retrieve the current version of a recorded component
+     * @param string $name
+     * @return false|string
+     * @throws \Exception
+     */
+    private static function getCurrentRecordedComponentVersion($name)
+    {
+        try {
+            $currentVersion = Option::get(self::getNameInOptionTable($name));
+        } catch (\Exception $e) {
+            // mysql error 1146: table doesn't exist
+            if (Db::get()->isErrNo($e, '1146')) {
+                // case when the option table is not yet created (before 0.2.10)
+                $currentVersion = false;
+            } else {
+                // failed for some other reason
+                throw $e;
+            }
+        }
+
+        return $currentVersion;
+    }
+
+    /**
      * Returns the flag name to use in the option table to record current schema version
      * @param string $name
      * @return string
@@ -155,7 +179,7 @@ class Updater
             return '\\Piwik\\Updates\\' . $className;
         }
 
-        if ($this->isDimensionComponent($componentName)) {
+        if (ColumnUpdater::isDimensionComponent($componentName)) {
             return '\\Piwik\\Columns\\Updater';
         }
 
@@ -199,14 +223,6 @@ class Updater
         return $warningMessages;
     }
 
-    private function isDimensionComponent($name)
-    {
-        return 0 === strpos($name, 'log_visit.')
-            || 0 === strpos($name, 'log_conversion.')
-            || 0 === strpos($name, 'log_conversion_item.')
-            || 0 === strpos($name, 'log_link_visit_action.');
-    }
-
     /**
      * Construct list of update files for the outdated components
      *
@@ -223,7 +239,7 @@ class Updater
 
             if ($name == 'core') {
                 $pathToUpdates = $this->pathUpdateFileCore . '*.php';
-            } elseif ($this->isDimensionComponent($name)) {
+            } elseif (ColumnUpdater::isDimensionComponent($name)) {
                 $componentsWithUpdateFile[$name][PIWIK_INCLUDE_PATH . '/core/Columns/Updater.php'] = $newVersion;
             } else {
                 $pathToUpdates = sprintf($this->pathUpdateFilePlugins, $name) . '*.php';
@@ -276,27 +292,22 @@ class Updater
             $this->componentsToCheck = array_merge(array('core' => $coreVersions), $this->componentsToCheck);
         }
 
+        $recordedCoreVersion = self::getCurrentRecordedComponentVersion('core');
+        if ($recordedCoreVersion === false) {
+            // This should not happen
+            $recordedCoreVersion = Version::VERSION;
+            self::recordComponentSuccessfullyUpdated('core', $recordedCoreVersion);
+        }
+
         foreach ($this->componentsToCheck as $name => $version) {
-            try {
-                $currentVersion = Option::get(self::getNameInOptionTable($name));
-            } catch (\Exception $e) {
-                // mysql error 1146: table doesn't exist
-                if (Db::get()->isErrNo($e, '1146')) {
-                    // case when the option table is not yet created (before 0.2.10)
-                    $currentVersion = false;
-                } else {
-                    // failed for some other reason
-                    throw $e;
-                }
-            }
+            $currentVersion = self::getCurrentRecordedComponentVersion($name);
 
-            if ($name === 'core' && $currentVersion === false) {
-                // This should not happen
-                $currentVersion = Version::VERSION;
-                self::recordComponentSuccessfullyUpdated($name, $currentVersion);
-            }
+            if (ColumnUpdater::isDimensionComponent($name)) {
+                if ($currentVersion === false && ColumnUpdater::wasDimensionMovedFromCoreToPlugin($name, $version)) {
+                    self::recordComponentSuccessfullyUpdated($name, $version);
+                    continue;
+                }
 
-            if ($this->isDimensionComponent($name)) {
                 $isComponentOutdated = $currentVersion !== $version;
             } else {
                 // note: when versionCompare == 1, the version in the DB is newer, we choose to ignore
diff --git a/core/Updates/2.5.0-b1.php b/core/Updates/2.5.0-b1.php
index 4e5fc056c4..54c0e6e026 100644
--- a/core/Updates/2.5.0-b1.php
+++ b/core/Updates/2.5.0-b1.php
@@ -9,10 +9,6 @@
 namespace Piwik\Updates;
 
 use Piwik\Config;
-use Piwik\Plugin\Dimension\ActionDimension;
-use Piwik\Plugin\Dimension\ConversionDimension;
-use Piwik\Plugin\Dimension\VisitDimension;
-use Piwik\Updater;
 use Piwik\Updates;
 
 class Updates_2_5_0_b1 extends Updates
@@ -20,7 +16,6 @@ class Updates_2_5_0_b1 extends Updates
     public static function update()
     {
         self::updateConfig();
-        self::markDimensionsAsInstalled();
     }
 
     private static function updateConfig()
@@ -39,27 +34,4 @@ class Updates_2_5_0_b1 extends Updates
         }
     }
 
-    private static function markDimensionsAsInstalled()
-    {
-        foreach (VisitDimension::getAllDimensions() as $dimension) {
-            if ($dimension->getColumnName()) {
-                $component = 'log_visit.' . $dimension->getColumnName();
-                Updater::recordComponentSuccessfullyUpdated($component, $dimension->getVersion());
-            }
-        }
-
-        foreach (ActionDimension::getAllDimensions() as $dimension) {
-            if ($dimension->getColumnName()) {
-                $component = 'log_link_visit_action.' . $dimension->getColumnName();
-                Updater::recordComponentSuccessfullyUpdated($component, $dimension->getVersion());
-            }
-        }
-
-        foreach (ConversionDimension::getAllDimensions() as $dimension) {
-            if ($dimension->getColumnName()) {
-                $component = 'log_conversion.' . $dimension->getColumnName();
-                Updater::recordComponentSuccessfullyUpdated($component, $dimension->getVersion());
-            }
-        }
-    }
 }