-
Notifications
You must be signed in to change notification settings - Fork 121
Fix up decimal migration_diff #938
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 5.x
Are you sure you want to change the base?
Conversation
ccd0d48 to
7359c99
Compare
3d34231 to
17c402f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes decimal column handling in migration diffs by correctly mapping CakePHP schema attributes to migration attributes. The main issue (referenced as #659) is that CakePHP's schema representation uses 'length' for precision and 'precision' for scale, while migrations use 'precision' and 'scale' respectively.
- Added logic to correctly map decimal column attributes between CakePHP schema and migration formats
- Added comprehensive test coverage for decimal column changes
- Enhanced test setup/teardown to handle decimal change test scenarios
Reviewed Changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_app/config/MigrationsDiffDecimalChange/.gitkeep | Empty directory marker for decimal change test migrations |
| tests/comparisons/Diff/decimalChange/schema-dump-test_comparisons_mysql.lock | Schema dump for decimal change test baseline |
| tests/comparisons/Diff/decimalChange/mysql/the_diff_decimal_change_mysql.php | Expected migration output for decimal column changes |
| tests/comparisons/Diff/decimalChange/initial_decimal_change_mysql.php | Initial migration creating test table with decimal column |
| tests/TestCase/Command/BakeMigrationDiffCommandTest.php | Test case for decimal change scenario with cleanup logic |
| src/Command/BakeMigrationDiffCommand.php | Core fix mapping CakePHP schema attributes to migration attributes for decimal columns |
Comments suppressed due to low confidence (1)
tests/TestCase/Command/BakeMigrationDiffCommandTest.php:420
- The pattern 'TheDiff$scenario' will not match DecimalChange migrations which use the 'Initial' prefix instead of 'TheDiff'. This will cause the test to fail when trying to find the generated migration file. The pattern should use the $classPrefix variable defined earlier in the method: "{$classPrefix}{$scenario}".
$generatedMigration = $this->getGeneratedMigrationName($destinationConfigDir, "*TheDiff$scenario*");
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
| ->changeColumn('amount', 'decimal', [ | ||
| 'default' => null, | ||
| 'null' => false, | ||
| 'precision' => 10, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this wrong? The precision was 5 before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be a quirk in the 5.next core layer?
- Initial migration creates: DECIMAL(5,2) with precision => 5, scale => 2
- After running the initial migration, CakePHP's schema reflection reads the column back from MySQL
- When CakePHP reads a DECIMAL(5,2) column, it appears to be returning the total length/precision as 10 instead of 5
According to claude possible reasons "Why this happens":
This is likely due to how CakePHP's database schema layer interprets decimal columns from MySQL. MySQL stores DECIMAL(5,2) as having:
- M = 5 (total digits including scale)
- D = 2 (scale/decimal places)
But when CakePHP reads this back, it may be applying a default precision value of 10, or calculating the total column width differently (possibly including sign, decimal point, etc.).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The weird thing:
Core has existing tests here
[
'DECIMAL(11,2)',
['type' => 'decimal', 'length' => 11, 'precision' => 2, 'unsigned' => false],
],
They seem to pass fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am out of ideas whats going on. once its green locally, its red on CI :(
Co-authored-by: Mark Story <[email protected]>
995ecbf to
22061a3
Compare
Fixes #659
targeting 5.x if we cannot get it shimmed in 4.x for both builtin and phinx.
I wonder if we want to change this at some point
maybe we will also want to look into cakephp/phinx#2383 changes and whats relevant for us.