smarter install and update of columns which will automatically update the column in case the type changes making it super easy for developers, not sure if everything works already and need to xhprof it

6 files changed, 141 insertions, 65 deletions

diff --git a/core/Columns/Dimension.php b/core/Columns/Dimension.php
index 26ede26f72..9f897b968e 100644
--- a/core/Columns/Dimension.php
+++ b/core/Columns/Dimension.php
@@ -27,14 +27,6 @@ abstract class Dimension
 
     }
 
-    public function getVersion()
-    {
-        $reflectionClass = new \ReflectionObject($this);
-        $file = $reflectionClass->getFileName();
-
-        return filemtime($file);
-    }
-
     public function hasImplementedEvent($method)
     {
         $reflectionObject = new \ReflectionObject($this);
diff --git a/core/Columns/Updates.php b/core/Columns/Updates.php
index 30cfa017cd..85b27ee0ff 100644
--- a/core/Columns/Updates.php
+++ b/core/Columns/Updates.php
@@ -20,6 +20,11 @@ use Piwik\Updater;
 class Updates extends \Piwik\Updates
 {
     /**
+     * @var Updater
+     */
+    private static $updater;
+
+    /**
      * Return SQL to be executed in this update
      *
      * @return array(
@@ -35,7 +40,7 @@ class Updates extends \Piwik\Updates
         $changingColumns = self::getUpdates();
 
         foreach ($changingColumns as $table => $columns) {
-            $sqls["ALTER TABLE `" . $table . "` " . implode(', ', $columns)] = false;
+            $sqls["ALTER TABLE `" . Common::prefixTable($table) . "` " . implode(', ', $columns)] = false;
         }
 
         return $sqls;
@@ -57,6 +62,16 @@ class Updates extends \Piwik\Updates
         }
     }
 
+    public static function setUpdater($updater)
+    {
+        self::$updater = $updater;
+    }
+
+    private static function hasComponentNewVersion($component)
+    {
+        return empty(self::$updater) || self::$updater->hasNewVersion($component);
+    }
+
     private static function getUpdates()
     {
         $visitColumns      = DbHelper::getTableColumns(Common::prefixTable('log_visit'));
@@ -66,7 +81,17 @@ class Updates extends \Piwik\Updates
         $changingColumns = array();
 
         foreach (VisitDimension::getAllDimensions() as $dimension) {
-            $columns = $dimension->install($visitColumns, $conversionColumns);
+            $column = $dimension->getColumnName();
+
+            if (!self::hasComponentNewVersion('log_visit.' . $column)) {
+                continue;
+            }
+
+            if (array_key_exists($column, $visitColumns)) {
+                $columns = $dimension->update($visitColumns, $conversionColumns);
+            } else {
+                $columns = $dimension->install();
+            }
             if (!empty($columns)) {
                 foreach ($columns as $table => $col) {
                     if (empty($changingColumns[$table])) {
@@ -79,7 +104,18 @@ class Updates extends \Piwik\Updates
         }
 
         foreach (ActionDimension::getAllDimensions() as $dimension) {
-            $columns = $dimension->install($actionColumns);
+            $column = $dimension->getColumnName();
+
+            if (!self::hasComponentNewVersion('log_link_visit_action.' . $column)) {
+                continue;
+            }
+
+            if (array_key_exists($column, $actionColumns)) {
+                $columns = $dimension->update($actionColumns);
+            } else {
+                $columns = $dimension->install();
+            }
+
             if (!empty($columns)) {
                 foreach ($columns as $table => $col) {
                     if (empty($changingColumns[$table])) {
diff --git a/core/Plugin/ActionDimension.php b/core/Plugin/ActionDimension.php
index ca0e7bfc3e..f662155144 100644
--- a/core/Plugin/ActionDimension.php
+++ b/core/Plugin/ActionDimension.php
@@ -10,6 +10,7 @@ namespace Piwik\Plugin;
 
 use Piwik\Cache\PluginAwareStaticCache;
 use Piwik\Columns\Dimension;
+use Piwik\DbHelper;
 use Piwik\Plugin\Manager as PluginManager;
 use Piwik\Common;
 use Piwik\Db;
@@ -26,35 +27,47 @@ abstract class ActionDimension extends Dimension
 {
     private $tableName = 'log_link_visit_action';
 
-    public function install($actionColumns)
+    public function install()
     {
         if (empty($this->columnName) || empty($this->columnType)) {
             return array();
         }
 
-        $columnExists = array_key_exists($this->columnName, $actionColumns);
+        return array(
+            $this->tableName => array("ADD COLUMN `$this->columnName` $this->columnType")
+        );
+    }
 
-        if (!$columnExists) {
-            return array(
-                Common::prefixTable($this->tableName) => array("ADD COLUMN `$this->columnName` $this->columnType")
-            );
+    public function update()
+    {
+        if (empty($this->columnName) || empty($this->columnType)) {
+            return array();
         }
 
-        return array();
+        return array(
+            $this->tableName => array("MODIFY COLUMN `$this->columnName` $this->columnType")
+        );
     }
 
-    public function uninstall($actionColumns)
+    public function uninstall()
     {
-        if (!empty($this->columnName)
-            && !empty($this->columnType)
-            && array_key_exists($this->columnName, $actionColumns)) {
+        if (empty($this->columnName) || empty($this->columnType)) {
+            return;
+        }
 
-            return array(
-                Common::prefixTable($this->tableName) => array("DROP COLUMN `$this->columnName`")
-            );
+        try {
+            $sql = "ALTER TABLE `" . Common::prefixTable($this->tableName) . "` DROP COLUMN `$this->columnName`";
+            Db::exec($sql);
+        } catch (\Exception $e) {
+            if (!Db::get()->isErrNo($e, '1091')) {
+                throw $e;
+            }
         }
+    }
 
-        return array();
+    public function getVersion()
+    {
+        return $this->columnType;
     }
 
     /**
diff --git a/core/Plugin/Manager.php b/core/Plugin/Manager.php
index 3bbbf0084f..d77d6d07a3 100644
--- a/core/Plugin/Manager.php
+++ b/core/Plugin/Manager.php
@@ -1278,13 +1278,31 @@ class Manager extends Singleton
     /**
      * @param $pluginName
      */
-    public function executePluginUninstall($pluginName)
+    private function executePluginUninstall($pluginName)
     {
         try {
             $plugin = $this->getLoadedPlugin($pluginName);
             $plugin->uninstall();
         } catch (\Exception $e) {
         }
+
+        if (empty($plugin)) {
+            return;
+        }
+
+        try {
+            foreach (VisitDimension::getDimensions($plugin) as $dimension) {
+                $dimension->uninstall();
+            }
+        } catch (\Exception $e) {
+        }
+
+        try {
+            foreach (ActionDimension::getDimensions($plugin) as $dimension) {
+                $dimension->uninstall();
+            }
+        } catch (\Exception $e) {
+        }
     }
 
     /**
diff --git a/core/Plugin/VisitDimension.php b/core/Plugin/VisitDimension.php
index 33899b211b..49fa40ad52 100644
--- a/core/Plugin/VisitDimension.php
+++ b/core/Plugin/VisitDimension.php
@@ -26,43 +26,50 @@ abstract class VisitDimension extends Dimension
 {
     private $tableName = 'log_visit';
 
-    public function install($visitColumns, $conversionColumns)
+    public function install()
     {
         if (!$this->columnType) {
             return array();
         }
 
-        $changes = array();
+        $changes = array(
+            $this->tableName => array("ADD COLUMN `$this->columnName` $this->columnType")
+        );
+
+        if ($this->isHandlingLogConversion()) {
+            $changes['log_conversion'] = array("ADD COLUMN `$this->columnName` $this->columnType");
+        }
 
-        $hasVisitColumn = array_key_exists($this->columnName, $visitColumns);
+        return $changes;
+    }
 
-        if (!$hasVisitColumn) {
-            $tableVisit           = Common::prefixTable($this->tableName);
-            $changes[$tableVisit] = array("ADD COLUMN `$this->columnName` $this->columnType");
+    public function update($visitColumns, $conversionColumns)
+    {
+        if (!$this->columnType) {
+            return array();
         }
 
-        $handlingLogConversion = $this->isHandlingLogConversion();
-        $hasConversionColumn   = array_key_exists($this->columnName, $conversionColumns);
-        $tableConversion       = Common::prefixTable("log_conversion");
+        $changes = array();
+
+        $changes[$this->tableName] = array("MODIFY COLUMN `$this->columnName` $this->columnType");
 
-        if (!$hasConversionColumn && $handlingLogConversion) {
-            $changes[$tableConversion] = array("ADD COLUMN `$this->columnName` $this->columnType");
-        } elseif ($hasConversionColumn && !$handlingLogConversion) {
-            $changes[$tableConversion] = array("DROP COLUMN `$this->columnName`");
+        $handlingConversion  = $this->isHandlingLogConversion();
+        $hasConversionColumn = array_key_exists($this->columnName, $conversionColumns);
+
+        if ($hasConversionColumn && $handlingConversion) {
+            $changes['log_conversion'] = array("MODIFY COLUMN `$this->columnName` $this->columnType");
+        } elseif (!$hasConversionColumn && $handlingConversion) {
+            $changes['log_conversion'] = array("ADD COLUMN `$this->columnName` $this->columnType");
+        } elseif ($hasConversionColumn && !$handlingConversion) {
+            $changes['log_conversion'] = array("DROP COLUMN `$this->columnName`");
         }
 
         return $changes;
     }
 
-    private function isHandlingLogVisit()
+    public function getVersion()
     {
-        if (empty($this->columnName) || empty($this->columnType)) {
-            return false;
-        }
-
-        return $this->hasImplementedEvent('onNewVisit')
-            || $this->hasImplementedEvent('onExistingVisit')
-            || $this->hasImplementedEvent('onConvertedVisit');
+        return $this->columnType . $this->isHandlingLogConversion();
     }
 
     private function isHandlingLogConversion()
@@ -74,25 +81,29 @@ abstract class VisitDimension extends Dimension
         return $this->hasImplementedEvent('onRecordGoal');
     }
 
-    public function uninstall($visitColumns, $conversionColumns)
+    public function uninstall()
     {
         if (empty($this->columnName) || empty($this->columnType)) {
-            return array();
+            return;
         }
 
-        $columnsToDrop = array();
-
-        if (array_key_exists($this->columnName, $visitColumns) && $this->isHandlingLogVisit()) {
-            $tableVisit                 = Common::prefixTable($this->tableName);
-            $columnsToDrop[$tableVisit] = array("DROP COLUMN `$this->columnName`");
+        try {
+            $sql = "ALTER TABLE `" . Common::prefixTable($this->tableName) . "` DROP COLUMN `$this->columnName`";
+            Db::exec($sql);
+        } catch (\Exception $e) {
+            if (!Db::get()->isErrNo($e, '1091')) {
+                throw $e;
+            }
         }
 
-        if (array_key_exists($this->columnName, $conversionColumns) && $this->isHandlingLogConversion()) {
-            $tableConversion                 = Common::prefixTable("log_conversion");
-            $columnsToDrop[$tableConversion] = array("DROP COLUMN `$this->columnName`");
+        try {
+            $sql = "ALTER TABLE `" . Common::prefixTable('log_conversion') . "` DROP COLUMN `$this->columnName`";
+            Db::exec($sql);
+        } catch (\Exception $e) {
+            if (!Db::get()->isErrNo($e, '1091')) {
+                throw $e;
+            }
         }
-
-        return $columnsToDrop;
     }
 
     protected function addSegment(Segment $segment)
diff --git a/core/Updater.php b/core/Updater.php
index d0b2fd9ad3..9e58572900 100644
--- a/core/Updater.php
+++ b/core/Updater.php
@@ -210,6 +210,8 @@ class Updater
         $componentsWithUpdateFile = array();
         $hasDimensionUpdate = null;
 
+        ColumnUpdates::setUpdater($this);
+
         foreach ($this->componentsWithNewVersion as $name => $versions) {
             $currentVersion = $versions[self::INDEX_CURRENT_VERSION];
             $newVersion = $versions[self::INDEX_NEW_VERSION];
@@ -217,7 +219,7 @@ class Updater
             if ($name == 'core') {
                 $pathToUpdates = $this->pathUpdateFileCore . '*.php';
             } elseif ($this->isDimensionComponent($name)) {
-                if (is_null($hasDimensionUpdate)) {
+                if (!$hasDimensionUpdate) {
                     $hasDimensionUpdate = ColumnUpdates::hasUpdates();
                 }
                 if ($hasDimensionUpdate) {
@@ -295,10 +297,14 @@ class Updater
                 self::recordComponentSuccessfullyUpdated($name, $currentVersion);
             }
 
-            // note: when versionCompare == 1, the version in the DB is newer, we choose to ignore
-            $currentVersionIsOutdated = version_compare($currentVersion, $version) == -1;
-            $isComponentOutdated = $currentVersion === false || $currentVersionIsOutdated;
-            if ($isComponentOutdated) {
+            if ($this->isDimensionComponent($name)) {
+                $isComponentOutdated = $currentVersion !== $version;
+            } else {
+                // note: when versionCompare == 1, the version in the DB is newer, we choose to ignore
+                $isComponentOutdated = version_compare($currentVersion, $version) == -1;
+            }
+
+            if ($isComponentOutdated || $currentVersion === false) {
                 $componentsToUpdate[$name] = array(
                     self::INDEX_CURRENT_VERSION => $currentVersion,
                     self::INDEX_NEW_VERSION     => $version