diff --git a/src/Command/BakeMigrationDiffCommand.php b/src/Command/BakeMigrationDiffCommand.php index e24d6d3b0..e835dd063 100644 --- 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 index 42b3dbfac..f6dd991c7 100644 --- 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 index 070c65ec7..576d0756a 100644 --- 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 new file mode 100644 index 000000000..984829c9c --- /dev/null +++ b/tests/comparisons/Diff/decimalChange/initial_decimal_change_mysql.php @@ -0,0 +1,40 @@ +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 new file mode 100644 index 000000000..a64755d93 --- /dev/null +++ b/tests/comparisons/Diff/decimalChange/mysql/the_diff_decimal_change_mysql.php @@ -0,0 +1,62 @@ +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 new file mode 100644 index 000000000..9c497e7b2 Binary files /dev/null and b/tests/comparisons/Diff/decimalChange/schema-dump-test_comparisons_mysql.lock differ diff --git a/tests/test_app/config/MigrationsDiffDecimalChange/.gitkeep b/tests/test_app/config/MigrationsDiffDecimalChange/.gitkeep new file mode 100644 index 000000000..e69de29bb