Merge pull request #23 from phalcon/fix/#22-field-type-date-datetime

#22 - Fix unnecessary table alter due DATE and DATETIME columns
phalcon · Dec 14, 2019 · 3d2db67 · 3d2db67
2 parents cdab8a7 + 2123242
commit 3d2db67
Show file tree

Hide file tree

Showing 7 changed files with 168 additions and 10 deletions.
diff --git a/composer.json b/composer.json
@@ -11,7 +11,8 @@
         "phpunit/phpunit": "8.4.1",
         "phalcon/ide-stubs": "^4.0.0-rc.3",
         "vimeo/psalm": "3.6.2",
-        "squizlabs/php_codesniffer": "3.5.1"
+        "squizlabs/php_codesniffer": "3.5.1",
+        "fzaninotto/faker": "^1.9"
     },
     "autoload": {
         "psr-4": {

diff --git a/composer.lock b/composer.lock
diff --git a/src/Migrations.php b/src/Migrations.php
@@ -186,6 +186,8 @@ public static function generate(array $options)
         } elseif (self::isConsole() && !$optionStack->getOption('verbose')) {
             print Color::info('Nothing to generate. You should create tables first.') . PHP_EOL;
         }
+
+        return true;
     }
 
     /**
@@ -278,12 +280,13 @@ public static function run(array $options)
         // Everything is up to date
         if ($initialVersion->getStamp() === $finalVersion->getStamp()) {
             print Color::info('Everything is up to date');
-            exit(0);
+            return;
         }
 
-        $direction = ModelMigration::DIRECTION_FORWARD;
         if ($finalVersion->getStamp() < $initialVersion->getStamp()) {
             $direction = ModelMigration::DIRECTION_BACK;
+        } else {
+            $direction = ModelMigration::DIRECTION_FORWARD;
         }
 
         if (ModelMigration::DIRECTION_FORWARD === $direction) {

diff --git a/src/Mvc/Model/Migration.php b/src/Mvc/Model/Migration.php
@@ -303,7 +303,8 @@ public static function generate(
             } else {
                 if ($field->getSize()) {
                     $fieldDefinition[] = "'size' => " . $field->getSize();
-                } else {
+                } elseif (!in_array($field->getType(), [Column::TYPE_DATE, Column::TYPE_DATETIME])) {
+                    // TODO: probably there are more types. Any way need global refactor of it
                     $fieldDefinition[] = "'size' => 1";
                 }
             }
@@ -625,12 +626,10 @@ public function morphTable(string $tableName, array $definition): void
 
             if ($tableExists) {
                 $localFields = [];
-                /**
-                 * @var ColumnInterface[] $description
-                 * @var ColumnInterface[] $localFields
-                 */
+                /** @var ColumnInterface[] $description */
                 $description = self::$connection->describeColumns($tableName, $defaultSchema);
                 foreach ($description as $field) {
+                    /** @var ColumnInterface[] $localFields */
                     $localFields[$field->getName()] = $field;
                 }
 

diff --git a/tests/Helpers.php b/tests/Helpers.php
@@ -3,6 +3,8 @@
 
 namespace Phalcon\Migrations\Tests;
 
+use Phalcon\Db\Adapter\Pdo\AbstractPdo;
+
 /**
  * @param string $prefix
  * @return string
@@ -28,3 +30,42 @@ function remove_dir(string $path): void
         $file->isDir() ? rmdir($realPath) : unlink($realPath);
     }
 }
+
+/**
+ * @see https://gist.github.com/afischoff/9608738
+ * @see https://github.com/phalcon/cphalcon/issues/14620
+ *
+ * @param AbstractPdo $db
+ * @param string $table
+ * @param array $columns
+ * @param array $rows
+ */
+function db_batch_insert(AbstractPdo $db, string $table, array $columns, array $rows): void
+{
+    $str = '';
+    foreach ($rows as $values) {
+        foreach ($values as &$val) {
+            if (is_null($val)) {
+                $val = 'NULL';
+                continue;
+            }
+
+            if (is_string($val)) {
+                $val = $db->escapeString($val);
+            }
+        }
+
+        $str .= sprintf('(%s),', implode(',', $values));
+    }
+
+    $str = rtrim($str, ',');
+    $str .= ';';
+    $query = sprintf(
+        "INSERT INTO `%s` (%s) VALUES %s",
+        $table,
+        sprintf('`%s`', implode('`,`', $columns)),
+        $str
+    );
+
+    $db->execute($query);
+}
diff --git a/tests/Integration/IntegrationTestCase.php b/tests/Integration/IntegrationTestCase.php
@@ -54,7 +54,10 @@ protected function initializeDatabase(): AbstractPdo
             'password' => getenv('TEST_DB_PASSWORD'),
         ]);
 
-        $db->query('CREATE DATABASE IF NOT EXISTS `' . getenv('TEST_DB_DATABASE') . '`;');
+        $databaseName = getenv('TEST_DB_DATABASE');
+        $db->query('DROP DATABASE IF EXISTS `' . $databaseName . '`;');
+        $db->query('CREATE DATABASE `' . $databaseName . '`;');
+        $db->query('USE `' . $databaseName . '`');
 
         return $db;
     }

diff --git a/tests/Integration/MigrationsTest.php b/tests/Integration/MigrationsTest.php
@@ -4,9 +4,11 @@
 namespace Phalcon\Migrations\Tests\Integration;
 
 use Exception;
+use Faker\Factory as FakerFactory;
 use Phalcon\Config;
 use Phalcon\Db\Column;
 use Phalcon\Migrations\Migrations;
+use function Phalcon\Migrations\Tests\db_batch_insert;
 use function Phalcon\Migrations\Tests\root_path;
 
 final class MigrationsTest extends IntegrationTestCase
@@ -167,4 +169,63 @@ public function testGenerateByMigrationsDirAsString(): void
         $this->assertDirectoryExists($migrationsDir);
         $this->assertSame(3, count(scandir($migrationsDir . '/1.0.0')));
     }
+
+    /**
+     * @throws Exception
+     */
+    public function testTypeDateWithManyRows(): void
+    {
+        $faker = FakerFactory::create();
+        $tableName = 'test_date_with_many_rows';
+        $migrationsDir = root_path('tests/var/output/' . __FUNCTION__);
+
+        $this->db->createTable($tableName, getenv('TEST_DB_DATABASE'), [
+            'columns' => [
+                new Column('id', [
+                    'type' => Column::TYPE_INTEGER,
+                    'size' => 10,
+                    'unsigned' => true,
+                    'notNull' => true,
+                    'first' => true,
+                ]),
+                new Column('name', [
+                    'type' => Column::TYPE_VARCHAR,
+                    'size' => 255,
+                    'notNull' => true,
+                ]),
+                new Column('create_date', [
+                    'type' => Column::TYPE_DATE,
+                    'notNull' => true,
+                ]),
+            ],
+        ]);
+
+        $data = [];
+        for ($id = 1; $id <= 10000; $id++) {
+            $data[] = [
+                'id' => $id,
+                'name' => $faker->name,
+                'create_date' => $faker->date(),
+            ];
+        }
+
+        db_batch_insert($this->db, $tableName, ['id', 'name', 'create_date'], $data);
+
+        Migrations::generate([
+            'migrationsDir' => $migrationsDir,
+            'config' => self::$generateConfig,
+            'tableName' => '@',
+        ]);
+
+        for ($i = 0; $i < 3; $i++) {
+            Migrations::run([
+                'migrationsDir' => $migrationsDir,
+                'config' => self::$generateConfig,
+                'migrationsInDb' => true,
+            ]);
+        }
+
+        $this->assertDirectoryExists($migrationsDir);
+        $this->assertSame(3, count(scandir($migrationsDir)));
+    }
 }