Fix up decimal migration_diff (#938)

dereuromark · markstory · web-flow · commit 926194860f89 · 2025-11-19T23:48:10.000-05:00
* Fix up decimal migration_diff
* Add .gitkeep for MigrationsDiffDecimalChange directory
* Update tests/TestCase/Command/BakeMigrationDiffCommandTest.php
* Update tests/TestCase/Command/BakeMigrationDiffCommandTest.php
* Fixes.

---------

Co-authored-by: Mark Story &lt;mark@mark-story.com&gt;
diff --git a/src/Command/BakeMigrationDiffCommand.php b/src/Command/BakeMigrationDiffCommand.php
@@ -300,11 +300,38 @@ protected function getColumns(): void
                         }
                     }
 
-                    if (isset($changedAttributes['length'])) {
+                    // For decimal columns, handle CakePHP schema -> migration attribute mapping
+                    if ($column['type'] === 'decimal') {
+                        // In CakePHP schema: 'length' = precision, 'precision' = scale
+                        // In migrations: 'precision' = precision, 'scale' = scale
+
+                        // Convert CakePHP schema's 'precision' (which is scale) to migration's 'scale'
+                        if (isset($changedAttributes['precision'])) {
+                            $changedAttributes['scale'] = $changedAttributes['precision'];
+                            unset($changedAttributes['precision']);
+                        }
+
+                        // Convert CakePHP schema's 'length' (which is precision) to migration's 'precision'
+                        if (isset($changedAttributes['length'])) {
+                            $changedAttributes['precision'] = $changedAttributes['length'];
+                            unset($changedAttributes['length']);
+                        }
+
+                        // Ensure both precision and scale are always set for decimal columns
+                        if (!isset($changedAttributes['precision']) && isset($column['length'])) {
+                            $changedAttributes['precision'] = $column['length'];
+                        }
+                        if (!isset($changedAttributes['scale']) && isset($column['precision'])) {
+                            $changedAttributes['scale'] = $column['precision'];
+                        }
+
+                        // Remove 'limit' for decimal columns as they use precision/scale instead
+                        unset($changedAttributes['limit']);
+                    } elseif (isset($changedAttributes['length'])) {
+                        // For non-decimal columns, convert 'length' to 'limit'
                         if (!isset($changedAttributes['limit'])) {
                             $changedAttributes['limit'] = $changedAttributes['length'];
                         }
-
                         unset($changedAttributes['length']);
                     }
 
diff --git a/src/View/Helper/MigrationHelper.php b/src/View/Helper/MigrationHelper.php
@@ -387,6 +387,7 @@ public function getColumnOption(array $options): array
             'comment',
             'autoIncrement',
             'precision',
+            'scale',
             'after',
             'collate',
         ]);
@@ -420,7 +421,10 @@ public function getColumnOption(array $options): array
             unset($columnOptions['precision']);
         } else {
             // due to Phinx using different naming for the precision and scale to CakePHP
-            $columnOptions['scale'] = $columnOptions['precision'];
+            // Only convert precision to scale if scale is not already set (for decimal columns from diff)
+            if (!isset($columnOptions['scale'])) {
+                $columnOptions['scale'] = $columnOptions['precision'];
+            }
 
             if (isset($columnOptions['limit'])) {
                 $columnOptions['precision'] = $columnOptions['limit'];
diff --git a/tests/TestCase/Command/BakeMigrationDiffCommandTest.php b/tests/TestCase/Command/BakeMigrationDiffCommandTest.php
@@ -18,9 +18,13 @@
 use Cake\Core\Configure;
 use Cake\Core\Plugin;
 use Cake\Database\Driver\Mysql;
+use Cake\Database\Driver\Postgres;
+use Cake\Database\Driver\Sqlite;
+use Cake\Database\Driver\Sqlserver;
 use Cake\Datasource\ConnectionManager;
 use Cake\TestSuite\StringCompareTrait;
 use Cake\Utility\Inflector;
+use Exception;
 use Migrations\Migrations;
 use Migrations\Test\TestCase\TestCase;
 use function Cake\Core\env;
@@ -47,6 +51,36 @@ public function setUp(): void
         parent::setUp();
 
         $this->generatedFiles = [];
+
+        // Clean up any TheDiff migration files from all directories before test starts
+        $configPath = ROOT . DS . 'config' . DS;
+        $directories = glob($configPath . '*', GLOB_ONLYDIR) ?: [];
+        foreach ($directories as $dir) {
+            // Clean up TheDiff migration files
+            $migrationFiles = glob($dir . DS . '*TheDiff*.php') ?: [];
+            foreach ($migrationFiles as $file) {
+                if (file_exists($file)) {
+                    unlink($file);
+                }
+            }
+            // Clean up Initial migration files
+            $initialMigrationFiles = glob($dir . DS . '*Initial*.php') ?: [];
+            foreach ($initialMigrationFiles as $file) {
+                if (file_exists($file)) {
+                    unlink($file);
+                }
+            }
+        }
+
+        // Clean up test_decimal_types table if it exists
+        if (env('DB_URL_COMPARE')) {
+            try {
+                $connection = ConnectionManager::get('test_comparisons');
+                $connection->execute('DROP TABLE IF EXISTS test_decimal_types');
+            } catch (Exception $e) {
+                // Ignore errors if connection doesn't exist yet
+            }
+        }
     }
 
     public function tearDown(): void
@@ -57,10 +91,29 @@ public function tearDown(): void
                 unlink($file);
             }
         }
+
+        // Clean up any TheDiff migration files from all directories
+        $configPath = ROOT . DS . 'config' . DS;
+        $directories = glob($configPath . '*', GLOB_ONLYDIR) ?: [];
+        foreach ($directories as $dir) {
+            $migrationFiles = glob($dir . DS . '*TheDiff*.php') ?: [];
+            foreach ($migrationFiles as $file) {
+                if (file_exists($file)) {
+                    unlink($file);
+                }
+            }
+            $initialMigrationFiles = glob($dir . DS . '*Initial*.php') ?: [];
+            foreach ($initialMigrationFiles as $file) {
+                if (file_exists($file)) {
+                    unlink($file);
+                }
+            }
+        }
+
         if (env('DB_URL_COMPARE')) {
             // Clean up the comparison database each time. Table order is important.
             $connection = ConnectionManager::get('test_comparisons');
-            $tables = ['articles', 'categories', 'comments', 'users', 'orphan_table', 'phinxlog', 'tags', 'test_blog_phinxlog'];
+            $tables = ['articles', 'categories', 'comments', 'users', 'orphan_table', 'phinxlog', 'tags', 'test_blog_phinxlog', 'test_decimal_types'];
             foreach ($tables as $table) {
                 $connection->execute("DROP TABLE IF EXISTS $table");
             }
@@ -240,6 +293,17 @@ public function testBakingDiffWithAutoIdIncompatibleUnsignedPrimaryKeys(): void
         $this->runDiffBakingTest('WithAutoIdIncompatibleUnsignedPrimaryKeys');
     }
 
+    /**
+     * Tests baking a diff with decimal column changes
+     * Regression test for issue #659
+     */
+    public function testBakingDiffDecimalChange(): void
+    {
+        $this->skipIf(!env('DB_URL_COMPARE'));
+
+        $this->runDiffBakingTest('DecimalChange');
+    }
+
     /**
      * Tests that baking a diff with --plugin option only includes tables with Table classes
      */
@@ -330,13 +394,21 @@ protected function runDiffBakingTest(string $scenario): void
     {
         $this->skipIf(!env('DB_URL_COMPARE'));
 
+        // Detect database type from connection if DB env is not set
+        $db = env('DB') ?: $this->getDbType();
+
         $diffConfigFolder = Plugin::path('Migrations') . 'tests' . DS . 'comparisons' . DS . 'Diff' . DS . lcfirst($scenario) . DS;
-        $diffMigrationsPath = $diffConfigFolder . 'the_diff_' . Inflector::underscore($scenario) . '_' . env('DB') . '.php';
-        $diffDumpPath = $diffConfigFolder . 'schema-dump-test_comparisons_' . env('DB') . '.lock';
+
+        // DecimalChange uses 'initial_' prefix to avoid class name conflicts
+        $prefix = $scenario === 'DecimalChange' ? 'initial_' : 'the_diff_';
+        $classPrefix = $scenario === 'DecimalChange' ? 'Initial' : 'TheDiff';
+
+        $diffMigrationsPath = $diffConfigFolder . $prefix . Inflector::underscore($scenario) . '_' . $db . '.php';
+        $diffDumpPath = $diffConfigFolder . 'schema-dump-test_comparisons_' . $db . '.lock';
 
         $destinationConfigDir = ROOT . DS . 'config' . DS . "MigrationsDiff{$scenario}" . DS;
-        $destination = $destinationConfigDir . "20160415220805_TheDiff{$scenario}" . ucfirst(env('DB')) . '.php';
-        $destinationDumpPath = $destinationConfigDir . 'schema-dump-test_comparisons_' . env('DB') . '.lock';
+        $destination = $destinationConfigDir . "20160415220805_{$classPrefix}{$scenario}" . ucfirst($db) . '.php';
+        $destinationDumpPath = $destinationConfigDir . 'schema-dump-test_comparisons_' . $db . '.lock';
         copy($diffMigrationsPath, $destination);
 
         $this->generatedFiles = [
@@ -387,6 +459,29 @@ protected function runDiffBakingTest(string $scenario): void
         $this->getMigrations("MigrationsDiff{$scenario}")->rollback(['target' => 'all']);
     }
 
+    /**
+     * Detect database type from connection
+     *
+     * @return string Database type (mysql, pgsql, sqlite, sqlserver)
+     */
+    protected function getDbType(): string
+    {
+        $connection = ConnectionManager::get('test_comparisons');
+        $driver = $connection->getDriver();
+
+        if ($driver instanceof Mysql) {
+            return 'mysql';
+        } elseif ($driver instanceof Postgres) {
+            return 'pgsql';
+        } elseif ($driver instanceof Sqlite) {
+            return 'sqlite';
+        } elseif ($driver instanceof Sqlserver) {
+            return 'sqlserver';
+        }
+
+        return 'mysql'; // Default fallback
+    }
+
     /**
      * Get the baked filename based on the current db environment
      *
@@ -395,7 +490,11 @@ protected function runDiffBakingTest(string $scenario): void
      */
     public function getBakeName($name)
     {
-        $name .= ucfirst(getenv('DB'));
+        $db = getenv('DB');
+        if (!$db) {
+            $db = $this->getDbType();
+        }
+        $name .= ucfirst($db);
 
         return $name;
     }
@@ -428,6 +527,9 @@ protected function getMigrations($source = 'MigrationsDiff')
     public function assertCorrectSnapshot($bakeName, $result)
     {
         $dbenv = getenv('DB');
+        if (!$dbenv) {
+            $dbenv = $this->getDbType();
+        }
         $bakeName = Inflector::underscore($bakeName);
         if (file_exists($this->_compareBasePath . $dbenv . DS . $bakeName . '.php')) {
             $this->assertSameAsFile($dbenv . DS . $bakeName . '.php', $result);
diff --git a/tests/comparisons/Diff/decimalChange/initial_decimal_change_mysql.php b/tests/comparisons/Diff/decimalChange/initial_decimal_change_mysql.php
@@ -0,0 +1,40 @@
+<?php
+declare(strict_types=1);
+
+use Migrations\BaseMigration;
+
+class InitialDecimalChangeMysql extends BaseMigration
+{
+    /**
+     * Up Method.
+     *
+     * More information on this method is available here:
+     * https://book.cakephp.org/migrations/5/en/migrations.html#the-up-method
+     *
+     * @return void
+     */
+    public function up(): void
+    {
+        $this->table('test_decimal_types')
+            ->addColumn('amount', 'decimal', [
+                'default' => null,
+                'null' => false,
+                'precision' => 5,
+                'scale' => 2,
+            ])
+            ->create();
+    }
+
+    /**
+     * Down Method.
+     *
+     * More information on this method is available here:
+     * https://book.cakephp.org/migrations/5/en/migrations.html#the-down-method
+     *
+     * @return void
+     */
+    public function down(): void
+    {
+        $this->table('test_decimal_types')->drop()->save();
+    }
+}
diff --git a/tests/comparisons/Diff/decimalChange/mysql/the_diff_decimal_change_mysql.php b/tests/comparisons/Diff/decimalChange/mysql/the_diff_decimal_change_mysql.php
@@ -0,0 +1,62 @@
+<?php
+declare(strict_types=1);
+
+use Migrations\BaseMigration;
+
+class TheDiffDecimalChangeMysql extends BaseMigration
+{
+    /**
+     * Up Method.
+     *
+     * More information on this method is available here:
+     * https://book.cakephp.org/migrations/5/en/migrations.html#the-up-method
+     *
+     * @return void
+     */
+    public function up(): void
+    {
+
+        $this->table('test_decimal_types')
+            ->changeColumn('id', 'integer', [
+                'default' => null,
+                'length' => null,
+                'limit' => null,
+                'null' => false,
+                'signed' => false,
+            ])
+            ->changeColumn('amount', 'decimal', [
+                'default' => null,
+                'null' => false,
+                'precision' => 5,
+                'scale' => 2,
+            ])
+            ->update();
+    }
+
+    /**
+     * Down Method.
+     *
+     * More information on this method is available here:
+     * https://book.cakephp.org/migrations/5/en/migrations.html#the-down-method
+     *
+     * @return void
+     */
+    public function down(): void
+    {
+
+        $this->table('test_decimal_types')
+            ->changeColumn('id', 'integer', [
+                'autoIncrement' => true,
+                'default' => null,
+                'length' => 11,
+                'null' => false,
+            ])
+            ->changeColumn('amount', 'decimal', [
+                'default' => null,
+                'null' => false,
+                'precision' => 10,
+                'scale' => 2,
+            ])
+            ->update();
+    }
+}
diff --git a/tests/comparisons/Diff/decimalChange/schema-dump-test_comparisons_mysql.lock b/tests/comparisons/Diff/decimalChange/schema-dump-test_comparisons_mysql.lock
diff --git a/tests/test_app/config/MigrationsDiffDecimalChange/.gitkeep b/tests/test_app/config/MigrationsDiffDecimalChange/.gitkeep
