Emit CHARACTER SET for columns with an explicit encoding - #1112
Open
maurobrandoni wants to merge 1 commit into
Open
Emit CHARACTER SET for columns with an explicit encoding#1112maurobrandoni wants to merge 1 commit into
maurobrandoni wants to merge 1 commit into
Conversation
The per-column `encoding` option has been silently dropped since 5.0, when column SQL generation moved from Phinx to cakephp/database. `Column` accepts the option and `Table::getChangedColumnOptions()` even preserves it across a `changeColumn`, but `Column::toArray()` exports no charset key and `MysqlSchemaDialect::columnDefinitionSql()` renders no per-column character set, so `CHARACTER SET` only ever reached the SQL for enum and set columns. All three paths that render a column definition are affected, since they all go through `MysqlAdapter::columnDefinitionSql()`: `CREATE TABLE`, `ALTER TABLE ... ADD` and `ALTER TABLE ... CHANGE`. Migrations that pair `encoding` with a `collation` were unaffected, since MySQL infers the character set from the collation. Migrations that set only `encoding` produced a column in the table's default character set, with no error to indicate the option had been ignored. MySQL requires `CHARACTER SET` to precede `COLLATE`, so the fragment cannot be appended to the generated definition. Render it with a placeholder collation instead and substitute the pair back in at that position. Types that carry no collation never render the placeholder, so their definition is untouched and the encoding is ignored, as MySQL would. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Author
|
Happy to be told this is the wrong fix or not worth fixing. The part that cost me time was that encoding is accepted, preserved, and documented, but silently does nothing — so I'd just like it to end up either working or documented as unsupported. |
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Emit CHARACTER SET for columns with an explicit encoding
Fixes #1113.
The problem
The per-column
encodingoption is a no-op for every type exceptenum/set, and has been since 5.0. All three paths that render a column definition are affected —CREATE TABLE,ALTER TABLE ... ADDandALTER TABLE ... CHANGE— since each one goes throughMysqlAdapter::columnDefinitionSql().The column keeps the table's default character set. No error, no warning — the option is accepted and discarded.
Where it is lost
Column::getValidOptions()acceptsencoding, andTable::getChangedColumnOptions()goes as far as preserving it across achangeColumnwhen it is not passed explicitly. From there:Column::toArray()exports'collate'and no character-set key.MysqlSchemaDialect::columnDefinitionSql()in cakephp/database rendersCOLLATE, but has no per-columnCHARACTER SET— only the table-levelDEFAULT CHARSET.MysqlAdapter::columnDefinitionSql()emitsCHARACTER SETonly inside theenum/setbackwards-compatibility branch; every other type returns the dialect's output verbatim.That branch is the surviving fragment of Phinx's
MysqlAdapter::getColumnSqlDefinition(), which appendedCHARACTER SETfor all column types (phinx 0.16.9, line 1374). The general case was dropped in 5.0 along with the move to cakephp/database, and the option has been documented as supported throughout:Why it went unnoticed
encodingcombined withcollationstill produces the right column, because MySQL infers the character set from the collation. Onlyencodingon its own silently does nothing — which is also why the existing test suite never caught it.The fix
MySQL requires
CHARACTER SETto precedeCOLLATE, so the fragment cannot simply be appended to the definition the dialect returns. It is rendered with a placeholder collation instead, and theCHARACTER SET/COLLATEpair replaces the placeholder in that slot.Types that render no collation never produce the placeholder, so their definition comes back unchanged and the encoding is ignored — which matches MySQL, where an integer has no character set. The set of affected types therefore stays in sync with the dialect's own list rather than being duplicated here.
Generated SQL,
5.xvs. this PR:5.x['encoding' => 'ascii']`data` VARCHAR(255) NOT NULL`data` VARCHAR(255) CHARACTER SET ascii NOT NULL['encoding' => 'ascii', 'collation' => 'ascii_bin']`data` VARCHAR(255) COLLATE ascii_bin NOT NULL`data` VARCHAR(255) CHARACTER SET ascii COLLATE ascii_bin NOT NULL['collation' => 'ascii_bin']`data` VARCHAR(255) COLLATE ascii_bin NOT NULL[]`data` VARCHAR(255) NOT NULLinteger+['encoding' => 'ascii']`counter` INTEGER NOT NULLTests
Four tests in
MysqlAdapterTest, one per affected path. They assertinformation_schema.COLUMNS.CHARACTER_SET_NAMErather thanSHOW FULL COLUMNS, which exposes only the collation and so cannot distinguish an explicit character set from an inherited one:testCreateTableWithCustomEncoding— theCREATE TABLEpath. Fails on5.x(utf8mb4).testAddStringColumnWithCustomEncoding— theALTER ... ADDpath,stringandtext. Fails on5.x(utf8mb4).testChangeColumnWithCustomEncoding— theALTER ... CHANGEpath. Fails on5.x(utf8mb4).testAddStringColumnWithCustomEncodingAndCollation— pins theCHARACTER SET/COLLATEpairing and ordering. Passes on5.x, guards the fix.Full suite green against MySQL: 1785 tests (MariaDB 10.11).
phpcsclean. No change required in cakephp/database.The other backends are untouched.
columnDefinitionSql()is defined only inMysqlAdapter, andPostgresAdapter,SqliteAdapterandSqlserverAdapterextendAbstractAdapterrather than it, so none of them can reach this code. None of them referenceencodingat all, which is correct: PostgreSQL sets its character set per database, and SQL Server and SQLite have no per-column character set either — all three offer only per-columnCOLLATE, which this PR does not touch. The new tests live inMysqlAdapterTestand skip on other drivers.If you would rather see per-column charset support added to
MysqlSchemaDialectin cakephp/database and consumed from here, I am happy to redo it that way — this version was kept self-contained so it can ship on5.xwithout a core release.