Skip to content

Conversation

@dereuromark
Copy link
Member

@dereuromark dereuromark commented Nov 3, 2025

Fixes #659

targeting 5.x if we cannot get it shimmed in 4.x for both builtin and phinx.

In CakePHP schema: 'length' = precision, 'precision' = scale
In migrations: 'precision' = precision, 'scale' = scale

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.

Copy link

Copilot AI left a 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.

->changeColumn('amount', 'decimal', [
'default' => null,
'null' => false,
'precision' => 10,
Copy link
Member

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.

Copy link
Member Author

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?

  1. Initial migration creates: DECIMAL(5,2) with precision => 5, scale => 2
  2. After running the initial migration, CakePHP's schema reflection reads the column back from MySQL
  3. 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.).

Copy link
Member Author

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

Copy link
Member Author

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 :(

@dereuromark dereuromark marked this pull request as draft November 5, 2025 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants