Skip to content

Commit a666f58

Browse files
dereuromarkdereuromark
committed
Fix up decimal migration_diff
1 parent 23bf8ca commit a666f58

9 files changed

+501
-10
lines changed

‎src/Command/BakeMigrationDiffCommand.php‎

Lines changed: 38 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -280,13 +280,15 @@ protected function getColumns(): void
280280
) {
281281
$changedAttributes = array_diff_assoc($column, $oldColumn);
282282

283+
$isDecimal = isset($column['type']) && $column['type'] === 'decimal';
284+
283285
foreach (['type', 'length', 'null', 'default'] as $attribute) {
284-
$phinxAttributeName = $attribute;
285-
if ($attribute === 'length') {
286-
$phinxAttributeName = 'limit';
286+
$migrationAttributeName = $attribute;
287+
if ($attribute === 'length' && !$isDecimal) {
288+
$migrationAttributeName = 'limit';
287289
}
288-
if (!isset($changedAttributes[$phinxAttributeName])) {
289-
$changedAttributes[$phinxAttributeName] = $column[$attribute];
290+
if (!isset($changedAttributes[$migrationAttributeName])) {
291+
$changedAttributes[$migrationAttributeName] = $column[$attribute];
290292
}
291293
}
292294

@@ -300,12 +302,39 @@ protected function getColumns(): void
300302
}
301303
}
302304

303-
if (isset($changedAttributes['length'])) {
304-
if (!isset($changedAttributes['limit'])) {
305-
$changedAttributes['limit'] = $changedAttributes['length'];
305+
// For decimal columns, CakePHP schema uses (length, precision) but migrations use (precision, scale)
306+
// where CakePHP schema's length = migration's precision and CakePHP schema's precision = migration's scale
307+
if ($isDecimal) {
308+
// Track if precision was changed in the original diff (before we rename length)
309+
$precisionChanged = isset($changedAttributes['precision']);
310+
311+
// Convert CakePHP schema's length to migration's precision
312+
if (isset($changedAttributes['length'])) {
313+
$changedAttributes['precision'] = $changedAttributes['length'];
314+
unset($changedAttributes['length']);
306315
}
307316

308-
unset($changedAttributes['length']);
317+
// Convert CakePHP schema's precision to migration's scale
318+
if ($precisionChanged) {
319+
$changedAttributes['scale'] = $column['precision'];
320+
unset($changedAttributes['precision']);
321+
}
322+
323+
// Ensure both precision and scale are set for decimal columns
324+
if (isset($column['length']) && !isset($changedAttributes['precision'])) {
325+
$changedAttributes['precision'] = $column['length'];
326+
}
327+
if (isset($column['precision']) && !isset($changedAttributes['scale'])) {
328+
$changedAttributes['scale'] = $column['precision'];
329+
}
330+
} else {
331+
if (isset($changedAttributes['length'])) {
332+
if (!isset($changedAttributes['limit'])) {
333+
$changedAttributes['limit'] = $changedAttributes['length'];
334+
}
335+
336+
unset($changedAttributes['length']);
337+
}
309338
}
310339

311340
$this->templateData[$table]['columns']['changed'][$columnName] = $changedAttributes;

‎tests/TestCase/Command/BakeMigrationDiffCommandTest.php‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ public function tearDown(): void
6060
if (env('DB_URL_COMPARE')) {
6161
// Clean up the comparison database each time. Table order is important.
6262
$connection = ConnectionManager::get('test_comparisons');
63-
$tables = ['articles', 'categories', 'comments', 'users', 'orphan_table', 'phinxlog', 'tags', 'test_blog_phinxlog'];
63+
$tables = ['articles', 'categories', 'comments', 'users', 'orphan_table', 'phinxlog', 'tags', 'test_blog_phinxlog', 'products'];
6464
foreach ($tables as $table) {
6565
$connection->execute("DROP TABLE IF EXISTS $table");
6666
}
@@ -240,6 +240,20 @@ public function testBakingDiffWithAutoIdIncompatibleUnsignedPrimaryKeys(): void
240240
$this->runDiffBakingTest('WithAutoIdIncompatibleUnsignedPrimaryKeys');
241241
}
242242

243+
/**
244+
* Tests that baking a diff with decimal column changes uses precision and scale
245+
*
246+
* Regression test for https://github.com/cakephp/migrations/issues/659
247+
*
248+
* @return void
249+
*/
250+
public function testBakingDiffDecimalColumn(): void
251+
{
252+
$this->skipIf(!env('DB_URL_COMPARE'));
253+
254+
$this->runDiffBakingTest('DecimalColumn');
255+
}
256+
243257
/**
244258
* Tests that baking a diff with --plugin option only includes tables with Table classes
245259
*/

‎tests/TestCase/Command/BakeMigrationDiffCommandUnitTest.php‎

Lines changed: 258 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,258 @@
1+
<?php
2+
declare(strict_types=1);
3+
4+
/**
5+
* Copyright (c) Cake Software Foundation, Inc. (https://cakefoundation.org)
6+
*
7+
* Licensed under The MIT License
8+
* Redistributions of files must retain the above copyright notice.
9+
*
10+
* @copyright Copyright (c) Cake Software Foundation, Inc. (https://cakefoundation.org)
11+
* @link https://cakephp.org CakePHP(tm) Project
12+
* @license https://www.opensource.org/licenses/mit-license.php MIT License
13+
*/
14+
namespace Migrations\Test\TestCase\Command;
15+
16+
use Cake\Database\Schema\TableSchema;
17+
use Migrations\Command\BakeMigrationDiffCommand;
18+
use Migrations\Test\TestCase\TestCase;
19+
use ReflectionClass;
20+
use ReflectionMethod;
21+
22+
/**
23+
* Unit tests for BakeMigrationDiffCommand
24+
*/
25+
class BakeMigrationDiffCommandUnitTest extends TestCase
26+
{
27+
/**
28+
* Test that decimal column changes generate correct precision and scale
29+
*
30+
* This tests the fix for https://github.com/cakephp/migrations/issues/659
31+
*
32+
* @return void
33+
*/
34+
public function testDecimalColumnChangesUsePrecisionAndScale(): void
35+
{
36+
// Create mock schemas
37+
$oldSchema = new TableSchema('products');
38+
$oldSchema->addColumn('id', ['type' => 'integer', 'autoIncrement' => true]);
39+
$oldSchema->addColumn('price', [
40+
'type' => 'decimal',
41+
'length' => 4,
42+
'precision' => 2,
43+
'null' => false,
44+
'default' => null,
45+
]);
46+
$oldSchema->addConstraint('primary', ['type' => 'primary', 'columns' => ['id']]);
47+
48+
$currentSchema = new TableSchema('products');
49+
$currentSchema->addColumn('id', ['type' => 'integer', 'autoIncrement' => true]);
50+
$currentSchema->addColumn('price', [
51+
'type' => 'decimal',
52+
'length' => 6, // Changed from 4 to 6
53+
'precision' => 2,
54+
'null' => false,
55+
'default' => null,
56+
]);
57+
$currentSchema->addConstraint('primary', ['type' => 'primary', 'columns' => ['id']]);
58+
59+
// Set up the command
60+
$command = new BakeMigrationDiffCommand();
61+
62+
// Use reflection to set protected properties
63+
$reflection = new ReflectionClass($command);
64+
65+
$dumpSchemaProperty = $reflection->getProperty('dumpSchema');
66+
$dumpSchemaProperty->setAccessible(true);
67+
$dumpSchemaProperty->setValue($command, ['products' => $oldSchema]);
68+
69+
$currentSchemaProperty = $reflection->getProperty('currentSchema');
70+
$currentSchemaProperty->setAccessible(true);
71+
$currentSchemaProperty->setValue($command, ['products' => $currentSchema]);
72+
73+
$commonTablesProperty = $reflection->getProperty('commonTables');
74+
$commonTablesProperty->setAccessible(true);
75+
$commonTablesProperty->setValue($command, ['products' => $currentSchema]);
76+
77+
$templateDataProperty = $reflection->getProperty('templateData');
78+
$templateDataProperty->setAccessible(true);
79+
$templateDataProperty->setValue($command, []);
80+
81+
// Call the protected getColumns method
82+
$getColumnsMethod = $reflection->getMethod('getColumns');
83+
$getColumnsMethod->setAccessible(true);
84+
$getColumnsMethod->invoke($command);
85+
86+
// Get the template data
87+
$templateData = $templateDataProperty->getValue($command);
88+
89+
// Assert that the decimal column change has precision and scale, not limit
90+
$this->assertArrayHasKey('products', $templateData);
91+
$this->assertArrayHasKey('columns', $templateData['products']);
92+
$this->assertArrayHasKey('changed', $templateData['products']['columns']);
93+
$this->assertArrayHasKey('price', $templateData['products']['columns']['changed']);
94+
95+
$priceChanges = $templateData['products']['columns']['changed']['price'];
96+
97+
// Should have precision and scale
98+
$this->assertArrayHasKey('precision', $priceChanges, 'Decimal column should have precision');
99+
$this->assertArrayHasKey('scale', $priceChanges, 'Decimal column should have scale');
100+
$this->assertEquals(6, $priceChanges['precision'], 'Precision should be 6 (the total digits)');
101+
$this->assertEquals(2, $priceChanges['scale'], 'Scale should be 2 (the decimal places)');
102+
103+
// Should NOT have length or limit
104+
$this->assertArrayNotHasKey('length', $priceChanges, 'Decimal column should not have length');
105+
$this->assertArrayNotHasKey('limit', $priceChanges, 'Decimal column should not have limit');
106+
}
107+
108+
/**
109+
* Test that non-decimal column changes use limit (not precision/scale)
110+
*
111+
* @return void
112+
*/
113+
public function testNonDecimalColumnChangesUseLimit(): void
114+
{
115+
// Create mock schemas
116+
$oldSchema = new TableSchema('products');
117+
$oldSchema->addColumn('id', ['type' => 'integer', 'autoIncrement' => true]);
118+
$oldSchema->addColumn('name', [
119+
'type' => 'string',
120+
'length' => 100,
121+
'null' => false,
122+
'default' => null,
123+
]);
124+
$oldSchema->addConstraint('primary', ['type' => 'primary', 'columns' => ['id']]);
125+
126+
$currentSchema = new TableSchema('products');
127+
$currentSchema->addColumn('id', ['type' => 'integer', 'autoIncrement' => true]);
128+
$currentSchema->addColumn('name', [
129+
'type' => 'string',
130+
'length' => 255, // Changed from 100 to 255
131+
'null' => false,
132+
'default' => null,
133+
]);
134+
$currentSchema->addConstraint('primary', ['type' => 'primary', 'columns' => ['id']]);
135+
136+
// Set up the command
137+
$command = new BakeMigrationDiffCommand();
138+
139+
// Use reflection to set protected properties
140+
$reflection = new ReflectionClass($command);
141+
142+
$dumpSchemaProperty = $reflection->getProperty('dumpSchema');
143+
$dumpSchemaProperty->setAccessible(true);
144+
$dumpSchemaProperty->setValue($command, ['products' => $oldSchema]);
145+
146+
$currentSchemaProperty = $reflection->getProperty('currentSchema');
147+
$currentSchemaProperty->setAccessible(true);
148+
$currentSchemaProperty->setValue($command, ['products' => $currentSchema]);
149+
150+
$commonTablesProperty = $reflection->getProperty('commonTables');
151+
$commonTablesProperty->setAccessible(true);
152+
$commonTablesProperty->setValue($command, ['products' => $currentSchema]);
153+
154+
$templateDataProperty = $reflection->getProperty('templateData');
155+
$templateDataProperty->setAccessible(true);
156+
$templateDataProperty->setValue($command, []);
157+
158+
// Call the protected getColumns method
159+
$getColumnsMethod = $reflection->getMethod('getColumns');
160+
$getColumnsMethod->setAccessible(true);
161+
$getColumnsMethod->invoke($command);
162+
163+
// Get the template data
164+
$templateData = $templateDataProperty->getValue($command);
165+
166+
// Assert that the string column change has limit, not precision/scale
167+
$this->assertArrayHasKey('products', $templateData);
168+
$this->assertArrayHasKey('columns', $templateData['products']);
169+
$this->assertArrayHasKey('changed', $templateData['products']['columns']);
170+
$this->assertArrayHasKey('name', $templateData['products']['columns']['changed']);
171+
172+
$nameChanges = $templateData['products']['columns']['changed']['name'];
173+
174+
// Should have limit
175+
$this->assertArrayHasKey('limit', $nameChanges, 'String column should have limit');
176+
$this->assertEquals(255, $nameChanges['limit'], 'Limit should be 255');
177+
178+
// Should NOT have length, precision, or scale
179+
$this->assertArrayNotHasKey('length', $nameChanges, 'String column should not have length');
180+
$this->assertArrayNotHasKey('precision', $nameChanges, 'String column should not have precision');
181+
$this->assertArrayNotHasKey('scale', $nameChanges, 'String column should not have scale');
182+
}
183+
184+
/**
185+
* Test that decimal columns with only scale change are handled correctly
186+
*
187+
* @return void
188+
*/
189+
public function testDecimalColumnScaleChangeOnly(): void
190+
{
191+
// Create mock schemas
192+
$oldSchema = new TableSchema('products');
193+
$oldSchema->addColumn('id', ['type' => 'integer', 'autoIncrement' => true]);
194+
$oldSchema->addColumn('price', [
195+
'type' => 'decimal',
196+
'length' => 6,
197+
'precision' => 2,
198+
'null' => false,
199+
'default' => null,
200+
]);
201+
$oldSchema->addConstraint('primary', ['type' => 'primary', 'columns' => ['id']]);
202+
203+
$currentSchema = new TableSchema('products');
204+
$currentSchema->addColumn('id', ['type' => 'integer', 'autoIncrement' => true]);
205+
$currentSchema->addColumn('price', [
206+
'type' => 'decimal',
207+
'length' => 6, // Same
208+
'precision' => 3, // Changed from 2 to 3
209+
'null' => false,
210+
'default' => null,
211+
]);
212+
$currentSchema->addConstraint('primary', ['type' => 'primary', 'columns' => ['id']]);
213+
214+
// Set up the command
215+
$command = new BakeMigrationDiffCommand();
216+
217+
// Use reflection to set protected properties
218+
$reflection = new ReflectionClass($command);
219+
220+
$dumpSchemaProperty = $reflection->getProperty('dumpSchema');
221+
$dumpSchemaProperty->setAccessible(true);
222+
$dumpSchemaProperty->setValue($command, ['products' => $oldSchema]);
223+
224+
$currentSchemaProperty = $reflection->getProperty('currentSchema');
225+
$currentSchemaProperty->setAccessible(true);
226+
$currentSchemaProperty->setValue($command, ['products' => $currentSchema]);
227+
228+
$commonTablesProperty = $reflection->getProperty('commonTables');
229+
$commonTablesProperty->setAccessible(true);
230+
$commonTablesProperty->setValue($command, ['products' => $currentSchema]);
231+
232+
$templateDataProperty = $reflection->getProperty('templateData');
233+
$templateDataProperty->setAccessible(true);
234+
$templateDataProperty->setValue($command, []);
235+
236+
// Call the protected getColumns method
237+
$getColumnsMethod = $reflection->getMethod('getColumns');
238+
$getColumnsMethod->setAccessible(true);
239+
$getColumnsMethod->invoke($command);
240+
241+
// Get the template data
242+
$templateData = $templateDataProperty->getValue($command);
243+
244+
// Assert that the decimal column change has precision and scale
245+
$this->assertArrayHasKey('products', $templateData);
246+
$this->assertArrayHasKey('columns', $templateData['products']);
247+
$this->assertArrayHasKey('changed', $templateData['products']['columns']);
248+
$this->assertArrayHasKey('price', $templateData['products']['columns']['changed']);
249+
250+
$priceChanges = $templateData['products']['columns']['changed']['price'];
251+
252+
// Should have precision (same as before) and scale (new value)
253+
$this->assertArrayHasKey('precision', $priceChanges, 'Decimal column should have precision');
254+
$this->assertArrayHasKey('scale', $priceChanges, 'Decimal column should have scale');
255+
$this->assertEquals(6, $priceChanges['precision'], 'Precision should be 6');
256+
$this->assertEquals(3, $priceChanges['scale'], 'Scale should be 3 (changed)');
257+
}
258+
}

0 commit comments

Comments
 (0)