From 2ac9233920b9c568cea786ba70beb5513128c5b0 Mon Sep 17 00:00:00 2001 From: mscherer Date: Mon, 20 Oct 2025 02:29:46 +0200 Subject: [PATCH 1/3] Fix up decimal migration_diff --- src/Command/BakeMigrationDiffCommand.php | 47 +++- .../Command/BakeMigrationDiffCommandTest.php | 16 +- .../BakeMigrationDiffCommandUnitTest.php | 258 ++++++++++++++++++ .../mysql/the_diff_decimal_column_mysql.php | 49 ++++ .../pgsql/the_diff_decimal_column_pgsql.php | 49 ++++ .../schema-dump-test_comparisons_mysql.lock | 1 + .../schema-dump-test_comparisons_pgsql.lock | 1 + .../the_diff_decimal_column_mysql.php | 45 +++ .../the_diff_decimal_column_pgsql.php | 45 +++ 9 files changed, 501 insertions(+), 10 deletions(-) create mode 100644 tests/TestCase/Command/BakeMigrationDiffCommandUnitTest.php create mode 100644 tests/comparisons/Diff/decimalColumn/mysql/the_diff_decimal_column_mysql.php create mode 100644 tests/comparisons/Diff/decimalColumn/pgsql/the_diff_decimal_column_pgsql.php create mode 100644 tests/comparisons/Diff/decimalColumn/schema-dump-test_comparisons_mysql.lock create mode 100644 tests/comparisons/Diff/decimalColumn/schema-dump-test_comparisons_pgsql.lock create mode 100644 tests/comparisons/Diff/decimalColumn/the_diff_decimal_column_mysql.php create mode 100644 tests/comparisons/Diff/decimalColumn/the_diff_decimal_column_pgsql.php diff --git a/src/Command/BakeMigrationDiffCommand.php b/src/Command/BakeMigrationDiffCommand.php index e24d6d3b0..f5e7b38a5 100644 --- a/src/Command/BakeMigrationDiffCommand.php +++ b/src/Command/BakeMigrationDiffCommand.php @@ -280,13 +280,15 @@ protected function getColumns(): void ) { $changedAttributes = array_diff_assoc($column, $oldColumn); + $isDecimal = isset($column['type']) && $column['type'] === 'decimal'; + foreach (['type', 'length', 'null', 'default'] as $attribute) { - $phinxAttributeName = $attribute; - if ($attribute === 'length') { - $phinxAttributeName = 'limit'; + $migrationAttributeName = $attribute; + if ($attribute === 'length' && !$isDecimal) { + $migrationAttributeName = 'limit'; } - if (!isset($changedAttributes[$phinxAttributeName])) { - $changedAttributes[$phinxAttributeName] = $column[$attribute]; + if (!isset($changedAttributes[$migrationAttributeName])) { + $changedAttributes[$migrationAttributeName] = $column[$attribute]; } } @@ -300,12 +302,39 @@ protected function getColumns(): void } } - if (isset($changedAttributes['length'])) { - if (!isset($changedAttributes['limit'])) { - $changedAttributes['limit'] = $changedAttributes['length']; + // For decimal columns, CakePHP schema uses (length, precision) but migrations use (precision, scale) + // where CakePHP schema's length = migration's precision and CakePHP schema's precision = migration's scale + if ($isDecimal) { + // Track if precision was changed in the original diff (before we rename length) + $precisionChanged = isset($changedAttributes['precision']); + + // Convert CakePHP schema's length to migration's precision + if (isset($changedAttributes['length'])) { + $changedAttributes['precision'] = $changedAttributes['length']; + unset($changedAttributes['length']); } - unset($changedAttributes['length']); + // Convert CakePHP schema's precision to migration's scale + if ($precisionChanged) { + $changedAttributes['scale'] = $column['precision']; + unset($changedAttributes['precision']); + } + + // Ensure both precision and scale are set for decimal columns + if (isset($column['length']) && !isset($changedAttributes['precision'])) { + $changedAttributes['precision'] = $column['length']; + } + if (isset($column['precision']) && !isset($changedAttributes['scale'])) { + $changedAttributes['scale'] = $column['precision']; + } + } else { + if (isset($changedAttributes['length'])) { + if (!isset($changedAttributes['limit'])) { + $changedAttributes['limit'] = $changedAttributes['length']; + } + + unset($changedAttributes['length']); + } } $this->templateData[$table]['columns']['changed'][$columnName] = $changedAttributes; diff --git a/tests/TestCase/Command/BakeMigrationDiffCommandTest.php b/tests/TestCase/Command/BakeMigrationDiffCommandTest.php index 070c65ec7..91fd8d02b 100644 --- a/tests/TestCase/Command/BakeMigrationDiffCommandTest.php +++ b/tests/TestCase/Command/BakeMigrationDiffCommandTest.php @@ -60,7 +60,7 @@ public function tearDown(): void 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', 'products']; foreach ($tables as $table) { $connection->execute("DROP TABLE IF EXISTS $table"); } @@ -240,6 +240,20 @@ public function testBakingDiffWithAutoIdIncompatibleUnsignedPrimaryKeys(): void $this->runDiffBakingTest('WithAutoIdIncompatibleUnsignedPrimaryKeys'); } + /** + * Tests that baking a diff with decimal column changes uses precision and scale + * + * Regression test for https://github.com/cakephp/migrations/issues/659 + * + * @return void + */ + public function testBakingDiffDecimalColumn(): void + { + $this->skipIf(!env('DB_URL_COMPARE')); + + $this->runDiffBakingTest('DecimalColumn'); + } + /** * Tests that baking a diff with --plugin option only includes tables with Table classes */ diff --git a/tests/TestCase/Command/BakeMigrationDiffCommandUnitTest.php b/tests/TestCase/Command/BakeMigrationDiffCommandUnitTest.php new file mode 100644 index 000000000..8df9cc74a --- /dev/null +++ b/tests/TestCase/Command/BakeMigrationDiffCommandUnitTest.php @@ -0,0 +1,258 @@ +addColumn('id', ['type' => 'integer', 'autoIncrement' => true]); + $oldSchema->addColumn('price', [ + 'type' => 'decimal', + 'length' => 4, + 'precision' => 2, + 'null' => false, + 'default' => null, + ]); + $oldSchema->addConstraint('primary', ['type' => 'primary', 'columns' => ['id']]); + + $currentSchema = new TableSchema('products'); + $currentSchema->addColumn('id', ['type' => 'integer', 'autoIncrement' => true]); + $currentSchema->addColumn('price', [ + 'type' => 'decimal', + 'length' => 6, // Changed from 4 to 6 + 'precision' => 2, + 'null' => false, + 'default' => null, + ]); + $currentSchema->addConstraint('primary', ['type' => 'primary', 'columns' => ['id']]); + + // Set up the command + $command = new BakeMigrationDiffCommand(); + + // Use reflection to set protected properties + $reflection = new ReflectionClass($command); + + $dumpSchemaProperty = $reflection->getProperty('dumpSchema'); + $dumpSchemaProperty->setAccessible(true); + $dumpSchemaProperty->setValue($command, ['products' => $oldSchema]); + + $currentSchemaProperty = $reflection->getProperty('currentSchema'); + $currentSchemaProperty->setAccessible(true); + $currentSchemaProperty->setValue($command, ['products' => $currentSchema]); + + $commonTablesProperty = $reflection->getProperty('commonTables'); + $commonTablesProperty->setAccessible(true); + $commonTablesProperty->setValue($command, ['products' => $currentSchema]); + + $templateDataProperty = $reflection->getProperty('templateData'); + $templateDataProperty->setAccessible(true); + $templateDataProperty->setValue($command, []); + + // Call the protected getColumns method + $getColumnsMethod = $reflection->getMethod('getColumns'); + $getColumnsMethod->setAccessible(true); + $getColumnsMethod->invoke($command); + + // Get the template data + $templateData = $templateDataProperty->getValue($command); + + // Assert that the decimal column change has precision and scale, not limit + $this->assertArrayHasKey('products', $templateData); + $this->assertArrayHasKey('columns', $templateData['products']); + $this->assertArrayHasKey('changed', $templateData['products']['columns']); + $this->assertArrayHasKey('price', $templateData['products']['columns']['changed']); + + $priceChanges = $templateData['products']['columns']['changed']['price']; + + // Should have precision and scale + $this->assertArrayHasKey('precision', $priceChanges, 'Decimal column should have precision'); + $this->assertArrayHasKey('scale', $priceChanges, 'Decimal column should have scale'); + $this->assertEquals(6, $priceChanges['precision'], 'Precision should be 6 (the total digits)'); + $this->assertEquals(2, $priceChanges['scale'], 'Scale should be 2 (the decimal places)'); + + // Should NOT have length or limit + $this->assertArrayNotHasKey('length', $priceChanges, 'Decimal column should not have length'); + $this->assertArrayNotHasKey('limit', $priceChanges, 'Decimal column should not have limit'); + } + + /** + * Test that non-decimal column changes use limit (not precision/scale) + * + * @return void + */ + public function testNonDecimalColumnChangesUseLimit(): void + { + // Create mock schemas + $oldSchema = new TableSchema('products'); + $oldSchema->addColumn('id', ['type' => 'integer', 'autoIncrement' => true]); + $oldSchema->addColumn('name', [ + 'type' => 'string', + 'length' => 100, + 'null' => false, + 'default' => null, + ]); + $oldSchema->addConstraint('primary', ['type' => 'primary', 'columns' => ['id']]); + + $currentSchema = new TableSchema('products'); + $currentSchema->addColumn('id', ['type' => 'integer', 'autoIncrement' => true]); + $currentSchema->addColumn('name', [ + 'type' => 'string', + 'length' => 255, // Changed from 100 to 255 + 'null' => false, + 'default' => null, + ]); + $currentSchema->addConstraint('primary', ['type' => 'primary', 'columns' => ['id']]); + + // Set up the command + $command = new BakeMigrationDiffCommand(); + + // Use reflection to set protected properties + $reflection = new ReflectionClass($command); + + $dumpSchemaProperty = $reflection->getProperty('dumpSchema'); + $dumpSchemaProperty->setAccessible(true); + $dumpSchemaProperty->setValue($command, ['products' => $oldSchema]); + + $currentSchemaProperty = $reflection->getProperty('currentSchema'); + $currentSchemaProperty->setAccessible(true); + $currentSchemaProperty->setValue($command, ['products' => $currentSchema]); + + $commonTablesProperty = $reflection->getProperty('commonTables'); + $commonTablesProperty->setAccessible(true); + $commonTablesProperty->setValue($command, ['products' => $currentSchema]); + + $templateDataProperty = $reflection->getProperty('templateData'); + $templateDataProperty->setAccessible(true); + $templateDataProperty->setValue($command, []); + + // Call the protected getColumns method + $getColumnsMethod = $reflection->getMethod('getColumns'); + $getColumnsMethod->setAccessible(true); + $getColumnsMethod->invoke($command); + + // Get the template data + $templateData = $templateDataProperty->getValue($command); + + // Assert that the string column change has limit, not precision/scale + $this->assertArrayHasKey('products', $templateData); + $this->assertArrayHasKey('columns', $templateData['products']); + $this->assertArrayHasKey('changed', $templateData['products']['columns']); + $this->assertArrayHasKey('name', $templateData['products']['columns']['changed']); + + $nameChanges = $templateData['products']['columns']['changed']['name']; + + // Should have limit + $this->assertArrayHasKey('limit', $nameChanges, 'String column should have limit'); + $this->assertEquals(255, $nameChanges['limit'], 'Limit should be 255'); + + // Should NOT have length, precision, or scale + $this->assertArrayNotHasKey('length', $nameChanges, 'String column should not have length'); + $this->assertArrayNotHasKey('precision', $nameChanges, 'String column should not have precision'); + $this->assertArrayNotHasKey('scale', $nameChanges, 'String column should not have scale'); + } + + /** + * Test that decimal columns with only scale change are handled correctly + * + * @return void + */ + public function testDecimalColumnScaleChangeOnly(): void + { + // Create mock schemas + $oldSchema = new TableSchema('products'); + $oldSchema->addColumn('id', ['type' => 'integer', 'autoIncrement' => true]); + $oldSchema->addColumn('price', [ + 'type' => 'decimal', + 'length' => 6, + 'precision' => 2, + 'null' => false, + 'default' => null, + ]); + $oldSchema->addConstraint('primary', ['type' => 'primary', 'columns' => ['id']]); + + $currentSchema = new TableSchema('products'); + $currentSchema->addColumn('id', ['type' => 'integer', 'autoIncrement' => true]); + $currentSchema->addColumn('price', [ + 'type' => 'decimal', + 'length' => 6, // Same + 'precision' => 3, // Changed from 2 to 3 + 'null' => false, + 'default' => null, + ]); + $currentSchema->addConstraint('primary', ['type' => 'primary', 'columns' => ['id']]); + + // Set up the command + $command = new BakeMigrationDiffCommand(); + + // Use reflection to set protected properties + $reflection = new ReflectionClass($command); + + $dumpSchemaProperty = $reflection->getProperty('dumpSchema'); + $dumpSchemaProperty->setAccessible(true); + $dumpSchemaProperty->setValue($command, ['products' => $oldSchema]); + + $currentSchemaProperty = $reflection->getProperty('currentSchema'); + $currentSchemaProperty->setAccessible(true); + $currentSchemaProperty->setValue($command, ['products' => $currentSchema]); + + $commonTablesProperty = $reflection->getProperty('commonTables'); + $commonTablesProperty->setAccessible(true); + $commonTablesProperty->setValue($command, ['products' => $currentSchema]); + + $templateDataProperty = $reflection->getProperty('templateData'); + $templateDataProperty->setAccessible(true); + $templateDataProperty->setValue($command, []); + + // Call the protected getColumns method + $getColumnsMethod = $reflection->getMethod('getColumns'); + $getColumnsMethod->setAccessible(true); + $getColumnsMethod->invoke($command); + + // Get the template data + $templateData = $templateDataProperty->getValue($command); + + // Assert that the decimal column change has precision and scale + $this->assertArrayHasKey('products', $templateData); + $this->assertArrayHasKey('columns', $templateData['products']); + $this->assertArrayHasKey('changed', $templateData['products']['columns']); + $this->assertArrayHasKey('price', $templateData['products']['columns']['changed']); + + $priceChanges = $templateData['products']['columns']['changed']['price']; + + // Should have precision (same as before) and scale (new value) + $this->assertArrayHasKey('precision', $priceChanges, 'Decimal column should have precision'); + $this->assertArrayHasKey('scale', $priceChanges, 'Decimal column should have scale'); + $this->assertEquals(6, $priceChanges['precision'], 'Precision should be 6'); + $this->assertEquals(3, $priceChanges['scale'], 'Scale should be 3 (changed)'); + } +} diff --git a/tests/comparisons/Diff/decimalColumn/mysql/the_diff_decimal_column_mysql.php b/tests/comparisons/Diff/decimalColumn/mysql/the_diff_decimal_column_mysql.php new file mode 100644 index 000000000..4542ab52a --- /dev/null +++ b/tests/comparisons/Diff/decimalColumn/mysql/the_diff_decimal_column_mysql.php @@ -0,0 +1,49 @@ +table('products') + ->changeColumn('price', 'decimal', [ + 'default' => null, + 'null' => false, + 'precision' => 6, + 'scale' => 2, + 'type' => 'decimal', + ]) + ->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('products') + ->changeColumn('price', 'decimal', [ + 'default' => null, + 'null' => false, + 'precision' => 4, + 'scale' => 2, + 'type' => 'decimal', + ]) + ->update(); + } +} diff --git a/tests/comparisons/Diff/decimalColumn/pgsql/the_diff_decimal_column_pgsql.php b/tests/comparisons/Diff/decimalColumn/pgsql/the_diff_decimal_column_pgsql.php new file mode 100644 index 000000000..cbb8deb81 --- /dev/null +++ b/tests/comparisons/Diff/decimalColumn/pgsql/the_diff_decimal_column_pgsql.php @@ -0,0 +1,49 @@ +table('products') + ->changeColumn('price', 'decimal', [ + 'default' => null, + 'null' => false, + 'precision' => 6, + 'scale' => 2, + 'type' => 'decimal', + ]) + ->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('products') + ->changeColumn('price', 'decimal', [ + 'default' => null, + 'null' => false, + 'precision' => 4, + 'scale' => 2, + 'type' => 'decimal', + ]) + ->update(); + } +} diff --git a/tests/comparisons/Diff/decimalColumn/schema-dump-test_comparisons_mysql.lock b/tests/comparisons/Diff/decimalColumn/schema-dump-test_comparisons_mysql.lock new file mode 100644 index 000000000..d47c0a6e9 --- /dev/null +++ b/tests/comparisons/Diff/decimalColumn/schema-dump-test_comparisons_mysql.lock @@ -0,0 +1 @@ +a:1:{s:8:"products";O:32:"Cake\Database\Schema\TableSchema":7:{s:9:" * _table";s:8:"products";s:11:" * _columns";a:3:{s:2:"id";a:9:{s:4:"type";s:7:"integer";s:6:"length";i:11;s:8:"unsigned";b:1;s:4:"null";b:0;s:7:"default";N;s:7:"comment";s:0:"";s:13:"autoIncrement";b:1;s:8:"baseType";N;s:9:"precision";N;}s:4:"name";a:9:{s:4:"type";s:6:"string";s:6:"length";i:255;s:4:"null";b:0;s:7:"default";N;s:7:"collate";s:17:"utf8mb4_general_ci";s:7:"comment";s:0:"";s:8:"baseType";N;s:9:"precision";N;s:5:"fixed";N;}s:5:"price";a:9:{s:4:"type";s:7:"decimal";s:6:"length";i:4;s:9:"precision";i:2;s:4:"null";b:0;s:7:"default";N;s:7:"comment";s:0:"";s:8:"baseType";N;s:13:"autoIncrement";N;s:8:"unsigned";b:0;}}s:11:" * _typeMap";a:3:{s:2:"id";s:7:"integer";s:4:"name";s:6:"string";s:5:"price";s:7:"decimal";}s:11:" * _indexes";a:0:{}s:15:" * _constraints";a:1:{s:7:"primary";a:3:{s:4:"type";s:7:"primary";s:7:"columns";a:1:{i:0;s:2:"id";}s:6:"length";a:0:{}}}s:11:" * _options";a:2:{s:6:"engine";s:6:"InnoDB";s:9:"collation";s:17:"utf8mb4_general_ci";}s:13:" * _temporary";b:0;}} diff --git a/tests/comparisons/Diff/decimalColumn/schema-dump-test_comparisons_pgsql.lock b/tests/comparisons/Diff/decimalColumn/schema-dump-test_comparisons_pgsql.lock new file mode 100644 index 000000000..f2cac51e7 --- /dev/null +++ b/tests/comparisons/Diff/decimalColumn/schema-dump-test_comparisons_pgsql.lock @@ -0,0 +1 @@ +a:1:{s:8:"products";O:32:"Cake\Database\Schema\TableSchema":7:{s:9:" * _table";s:8:"products";s:11:" * _columns";a:3:{s:2:"id";a:9:{s:4:"type";s:7:"integer";s:6:"length";N;s:8:"unsigned";b:0;s:4:"null";b:0;s:7:"default";N;s:7:"comment";N;s:13:"autoIncrement";b:1;s:8:"baseType";N;s:9:"precision";N;}s:4:"name";a:8:{s:4:"type";s:6:"string";s:6:"length";i:255;s:4:"null";b:0;s:7:"default";N;s:7:"collate";N;s:7:"comment";N;s:8:"baseType";N;s:9:"precision";N;}s:5:"price";a:9:{s:4:"type";s:7:"decimal";s:6:"length";i:4;s:9:"precision";i:2;s:4:"null";b:0;s:7:"default";N;s:7:"comment";N;s:8:"baseType";N;s:13:"autoIncrement";N;s:8:"unsigned";b:0;}}s:11:" * _typeMap";a:3:{s:2:"id";s:7:"integer";s:4:"name";s:6:"string";s:5:"price";s:7:"decimal";}s:11:" * _indexes";a:0:{}s:15:" * _constraints";a:1:{s:7:"primary";a:3:{s:4:"type";s:7:"primary";s:7:"columns";a:1:{i:0;s:2:"id";}s:6:"length";a:0:{}}}s:11:" * _options";a:0:{}s:13:" * _temporary";b:0;}} diff --git a/tests/comparisons/Diff/decimalColumn/the_diff_decimal_column_mysql.php b/tests/comparisons/Diff/decimalColumn/the_diff_decimal_column_mysql.php new file mode 100644 index 000000000..ea8457046 --- /dev/null +++ b/tests/comparisons/Diff/decimalColumn/the_diff_decimal_column_mysql.php @@ -0,0 +1,45 @@ +table('products') + ->addColumn('name', 'string', [ + 'default' => null, + 'limit' => 255, + 'null' => false, + ]) + ->addColumn('price', 'decimal', [ + 'default' => null, + 'null' => false, + 'precision' => 4, + '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('products')->drop()->save(); + } +} diff --git a/tests/comparisons/Diff/decimalColumn/the_diff_decimal_column_pgsql.php b/tests/comparisons/Diff/decimalColumn/the_diff_decimal_column_pgsql.php new file mode 100644 index 000000000..85de65090 --- /dev/null +++ b/tests/comparisons/Diff/decimalColumn/the_diff_decimal_column_pgsql.php @@ -0,0 +1,45 @@ +table('products') + ->addColumn('name', 'string', [ + 'default' => null, + 'limit' => 255, + 'null' => false, + ]) + ->addColumn('price', 'decimal', [ + 'default' => null, + 'null' => false, + 'precision' => 4, + '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('products')->drop()->save(); + } +} From 3a47b16f541b90b2795c3d572e2dcd92bca22264 Mon Sep 17 00:00:00 2001 From: mscherer Date: Wed, 22 Oct 2025 03:12:26 +0200 Subject: [PATCH 2/3] Fix up tests. --- src/Db/Adapter/MysqlAdapter.php | 24 +++++++++++++++++-- .../Db/Adapter/AbstractAdapterTest.php | 1 + 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/Db/Adapter/MysqlAdapter.php b/src/Db/Adapter/MysqlAdapter.php index 148e4c4b8..67ac01f02 100644 --- a/src/Db/Adapter/MysqlAdapter.php +++ b/src/Db/Adapter/MysqlAdapter.php @@ -607,6 +607,24 @@ protected function afterClause(Column $column): string */ protected function getRenameColumnInstructions(string $tableName, string $columnName, string $newColumnName): AlterInstructions { + $columns = $this->getColumns($tableName); + $targetColumn = null; + + foreach ($columns as $column) { + if (strcasecmp($column->getName(), $columnName) === 0) { + $targetColumn = $column; + break; + } + } + + if ($targetColumn === null) { + throw new InvalidArgumentException(sprintf( + "The specified column doesn't exist: %s", + $columnName, + )); + } + + // Fetch raw MySQL column info for the full definition string $rows = $this->fetchAll(sprintf('SHOW FULL COLUMNS FROM %s', $this->quoteTableName($tableName))); foreach ($rows as $row) { @@ -624,8 +642,10 @@ static function ($value) { $extra = ' ' . implode(' ', $extras); if (($row['Default'] !== null)) { - $phinxTypeInfo = $this->getPhinxType($row['Type']); - $extra .= $this->getDefaultValueDefinition($row['Default'], $phinxTypeInfo['name']); + $columnType = $targetColumn->getType(); + // Column::getType() can return string|Literal, but getDefaultValueDefinition expects string|null + $columnTypeName = is_string($columnType) ? $columnType : null; + $extra .= $this->getDefaultValueDefinition($row['Default'], $columnTypeName); } $definition = $row['Type'] . ' ' . $null . $extra . $comment; diff --git a/tests/TestCase/Db/Adapter/AbstractAdapterTest.php b/tests/TestCase/Db/Adapter/AbstractAdapterTest.php index 1e101935c..ae44931a8 100644 --- a/tests/TestCase/Db/Adapter/AbstractAdapterTest.php +++ b/tests/TestCase/Db/Adapter/AbstractAdapterTest.php @@ -10,6 +10,7 @@ use Migrations\Db\Literal; use Migrations\Test\TestCase\Db\Adapter\DefaultAdapterTrait; use PDOException; +use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; use RuntimeException; From c4fd8fa4731045a530b2ce49c60d565598576d9a Mon Sep 17 00:00:00 2001 From: mscherer Date: Wed, 22 Oct 2025 15:24:44 +0200 Subject: [PATCH 3/3] Adding more tests. --- src/Command/BakeMigrationDiffCommand.php | 33 +++---- .../BakeMigrationDiffCommandUnitTest.php | 90 +++++++++++++++++-- 2 files changed, 99 insertions(+), 24 deletions(-) diff --git a/src/Command/BakeMigrationDiffCommand.php b/src/Command/BakeMigrationDiffCommand.php index f5e7b38a5..aa29bea84 100644 --- a/src/Command/BakeMigrationDiffCommand.php +++ b/src/Command/BakeMigrationDiffCommand.php @@ -303,30 +303,25 @@ protected function getColumns(): void } // For decimal columns, CakePHP schema uses (length, precision) but migrations use (precision, scale) - // where CakePHP schema's length = migration's precision and CakePHP schema's precision = migration's scale + // Mapping: CakePHP 'length' → migration 'precision' (total digits) + // CakePHP 'precision' → migration 'scale' (decimal places) + // Example: DECIMAL(6,2) in DB = CakePHP ['length' => 6, 'precision' => 2] + // = Migration ['precision' => 6, 'scale' => 2] if ($isDecimal) { - // Track if precision was changed in the original diff (before we rename length) - $precisionChanged = isset($changedAttributes['precision']); + $decimalAttributes = []; - // Convert CakePHP schema's length to migration's precision - if (isset($changedAttributes['length'])) { - $changedAttributes['precision'] = $changedAttributes['length']; - unset($changedAttributes['length']); + // Copy all non-decimal-specific attributes (type, null, default, etc.) + foreach ($changedAttributes as $key => $value) { + if ($key !== 'length' && $key !== 'precision') { + $decimalAttributes[$key] = $value; + } } - // Convert CakePHP schema's precision to migration's scale - if ($precisionChanged) { - $changedAttributes['scale'] = $column['precision']; - unset($changedAttributes['precision']); - } + // Always set both precision and scale for decimal columns + $decimalAttributes['precision'] = $column['length']; + $decimalAttributes['scale'] = $column['precision']; - // Ensure both precision and scale are set for decimal columns - if (isset($column['length']) && !isset($changedAttributes['precision'])) { - $changedAttributes['precision'] = $column['length']; - } - if (isset($column['precision']) && !isset($changedAttributes['scale'])) { - $changedAttributes['scale'] = $column['precision']; - } + $changedAttributes = $decimalAttributes; } else { if (isset($changedAttributes['length'])) { if (!isset($changedAttributes['limit'])) { diff --git a/tests/TestCase/Command/BakeMigrationDiffCommandUnitTest.php b/tests/TestCase/Command/BakeMigrationDiffCommandUnitTest.php index 8df9cc74a..ece896581 100644 --- a/tests/TestCase/Command/BakeMigrationDiffCommandUnitTest.php +++ b/tests/TestCase/Command/BakeMigrationDiffCommandUnitTest.php @@ -17,7 +17,6 @@ use Migrations\Command\BakeMigrationDiffCommand; use Migrations\Test\TestCase\TestCase; use ReflectionClass; -use ReflectionMethod; /** * Unit tests for BakeMigrationDiffCommand @@ -49,7 +48,7 @@ public function testDecimalColumnChangesUsePrecisionAndScale(): void $currentSchema->addColumn('id', ['type' => 'integer', 'autoIncrement' => true]); $currentSchema->addColumn('price', [ 'type' => 'decimal', - 'length' => 6, // Changed from 4 to 6 + 'length' => 6, // Changed from 4 to 6 'precision' => 2, 'null' => false, 'default' => null, @@ -127,7 +126,7 @@ public function testNonDecimalColumnChangesUseLimit(): void $currentSchema->addColumn('id', ['type' => 'integer', 'autoIncrement' => true]); $currentSchema->addColumn('name', [ 'type' => 'string', - 'length' => 255, // Changed from 100 to 255 + 'length' => 255, // Changed from 100 to 255 'null' => false, 'default' => null, ]); @@ -204,8 +203,8 @@ public function testDecimalColumnScaleChangeOnly(): void $currentSchema->addColumn('id', ['type' => 'integer', 'autoIncrement' => true]); $currentSchema->addColumn('price', [ 'type' => 'decimal', - 'length' => 6, // Same - 'precision' => 3, // Changed from 2 to 3 + 'length' => 6, // Same + 'precision' => 3, // Changed from 2 to 3 'null' => false, 'default' => null, ]); @@ -255,4 +254,85 @@ public function testDecimalColumnScaleChangeOnly(): void $this->assertEquals(6, $priceChanges['precision'], 'Precision should be 6'); $this->assertEquals(3, $priceChanges['scale'], 'Scale should be 3 (changed)'); } + + /** + * Test that decimal columns with both precision and scale changing are handled correctly + * + * This tests the edge case where both values change together + * + * @return void + */ + public function testDecimalColumnBothPrecisionAndScaleChange(): void + { + // Create mock schemas + $oldSchema = new TableSchema('products'); + $oldSchema->addColumn('id', ['type' => 'integer', 'autoIncrement' => true]); + $oldSchema->addColumn('price', [ + 'type' => 'decimal', + 'length' => 4, + 'precision' => 2, + 'null' => false, + 'default' => null, + ]); + $oldSchema->addConstraint('primary', ['type' => 'primary', 'columns' => ['id']]); + + $currentSchema = new TableSchema('products'); + $currentSchema->addColumn('id', ['type' => 'integer', 'autoIncrement' => true]); + $currentSchema->addColumn('price', [ + 'type' => 'decimal', + 'length' => 6, // Changed from 4 to 6 + 'precision' => 3, // Changed from 2 to 3 + 'null' => false, + 'default' => null, + ]); + $currentSchema->addConstraint('primary', ['type' => 'primary', 'columns' => ['id']]); + + // Set up the command + $command = new BakeMigrationDiffCommand(); + + // Use reflection to set protected properties + $reflection = new ReflectionClass($command); + + $dumpSchemaProperty = $reflection->getProperty('dumpSchema'); + $dumpSchemaProperty->setAccessible(true); + $dumpSchemaProperty->setValue($command, ['products' => $oldSchema]); + + $currentSchemaProperty = $reflection->getProperty('currentSchema'); + $currentSchemaProperty->setAccessible(true); + $currentSchemaProperty->setValue($command, ['products' => $currentSchema]); + + $commonTablesProperty = $reflection->getProperty('commonTables'); + $commonTablesProperty->setAccessible(true); + $commonTablesProperty->setValue($command, ['products' => $currentSchema]); + + $templateDataProperty = $reflection->getProperty('templateData'); + $templateDataProperty->setAccessible(true); + $templateDataProperty->setValue($command, []); + + // Call the protected getColumns method + $getColumnsMethod = $reflection->getMethod('getColumns'); + $getColumnsMethod->setAccessible(true); + $getColumnsMethod->invoke($command); + + // Get the template data + $templateData = $templateDataProperty->getValue($command); + + // Assert that the decimal column change has both precision and scale updated + $this->assertArrayHasKey('products', $templateData); + $this->assertArrayHasKey('columns', $templateData['products']); + $this->assertArrayHasKey('changed', $templateData['products']['columns']); + $this->assertArrayHasKey('price', $templateData['products']['columns']['changed']); + + $priceChanges = $templateData['products']['columns']['changed']['price']; + + // Should have both precision and scale with new values + $this->assertArrayHasKey('precision', $priceChanges, 'Decimal column should have precision'); + $this->assertArrayHasKey('scale', $priceChanges, 'Decimal column should have scale'); + $this->assertEquals(6, $priceChanges['precision'], 'Precision should be 6 (changed from 4)'); + $this->assertEquals(3, $priceChanges['scale'], 'Scale should be 3 (changed from 2)'); + + // Should NOT have length or limit + $this->assertArrayNotHasKey('length', $priceChanges, 'Decimal column should not have length'); + $this->assertArrayNotHasKey('limit', $priceChanges, 'Decimal column should not have limit'); + } }