Skip to content

Fix TEXT column variants not round-tripping correctly#1032

Merged
dereuromark merged 2 commits into5.xfrom
fix-longtext-roundtrip
Mar 5, 2026
Merged

Fix TEXT column variants not round-tripping correctly#1032
dereuromark merged 2 commits into5.xfrom
fix-longtext-roundtrip

Conversation

@dereuromark
Copy link
Member

Summary

  • Adds rawType-based mapping for TEXT columns in mapColumnType() to properly distinguish TINYTEXT, MEDIUMTEXT, and LONGTEXT variants
  • Mirrors the existing BLOB handling pattern that was already working correctly
  • Adds round-trip tests for TEXT column variants

Fixes #1029

Problem

When using migration_diff, TEXT column variants (TINYTEXT, MEDIUMTEXT, LONGTEXT) were not being properly mapped back from the database. The code already handled BLOB variants correctly by checking the raw MySQL type string, but TEXT variants were missing equivalent handling.

For example, a LONGTEXT column would:

  1. Be read from the database with rawType='longtext' and length=4294967295
  2. Not be recognized as LONGTEXT (no rawType check for TEXT)
  3. Fall through to mapColumnData() where 4294967295 didn't match any case
  4. Result in a plain TEXT column being created instead of LONGTEXT

Solution

Added equivalent rawType-based handling for TEXT columns in mapColumnType():

  • tinytextTEXT_TINY (255)
  • mediumtextTEXT_MEDIUM (16777215)
  • longtextTEXT_LONG (2147483647)
  • Regular textnull (default TEXT)

When using migration_diff, TEXT column variants (TINYTEXT, MEDIUMTEXT,
LONGTEXT) were not being properly mapped back from the database. The
BLOB type handling already used rawType to distinguish BLOB variants,
but TEXT variants were missing equivalent handling.

This adds similar rawType-based mapping for TEXT columns in
mapColumnType() and includes round-trip tests.

Fixes #1029
@dereuromark
Copy link
Member Author

CI Failures Note

The CI failures on MySQL/MariaDB are unrelated to this PR. They are pre-existing issues:

  1. MariaDB testMigrateAndRollback failure - Test expects CURRENT_TIMESTAMP but MariaDB returns current_timestamp() (case/format difference)

  2. PHP 8.5 BakeMigrationDiffCommandTest errors - Cake\Database\Schema\Column::$fixed property uninitialized access (CakePHP core issue)

The 5.x branch CI passed 4 days ago with the same test matrix. These failures appear to be caused by recent dependency updates or test flakiness.

My changes only modify mapColumnType() in MysqlAdapter.php to handle TEXT column variants, and the new testTextRoundTrip test is not mentioned in any failures.

@dereuromark
Copy link
Member Author

CI failures are likely not relevant, see #1034

1 similar comment
@dereuromark
Copy link
Member Author

CI failures are likely not relevant, see #1034

Comment on lines +1376 to +1379
['text', null, 'text', null],
['text', MysqlAdapter::TEXT_TINY, 'text', MysqlAdapter::TEXT_TINY],
['text', MysqlAdapter::TEXT_MEDIUM, 'text', MysqlAdapter::TEXT_MEDIUM],
['text', MysqlAdapter::TEXT_LONG, 'text', MysqlAdapter::TEXT_LONG],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need all four? Wouldn't one pair suffice?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can it be reduced? Yes, technically. The code paths are symmetric.

Downsides of reducing:

  1. Each is a genuinely different MySQL column type with different storage characteristics
  2. If someone modifies the constants or the mapping, a missing test case could let a regression slip through
  3. It's testing 4 branches in that match statement - reducing means less branch coverage

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of the duplication in the parameter lists. The number of scenarios seems required. In any case it doesn't need to be a blocker.

@dereuromark dereuromark marked this pull request as ready for review March 5, 2026 03:23
@dereuromark dereuromark added the bug label Mar 5, 2026
@dereuromark dereuromark merged commit d6720e1 into 5.x Mar 5, 2026
14 checks passed
@dereuromark dereuromark deleted the fix-longtext-roundtrip branch March 5, 2026 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

migration_diff for LONGTEXT create TEXT since v5.x

2 participants