Skip to content

Fix changing a column on MySQL, MariaDB, PostgreSQL and SQL Server - #258

Open
JonasPardon wants to merge 6 commits into
wintercms:wip/1.3from
JonasPardon:fix/change-mysql-mariadb-attributes
Open

JonasPardon wants to merge 6 commits into
wintercms:wip/1.3from
JonasPardon:fix/change-mysql-mariadb-attributes

Conversation

@JonasPardon

@JonasPardon JonasPardon commented Oct 8, 2026 •

Copy link
Copy Markdown

The ->change() shim loses or corrupts column attributes on MySQL and MariaDB on wip/1.3, and generates invalid SQL on PostgreSQL and SQL Server. SQLite is fine, apart from ->default(null), which #254 fixes.

I ran the same migrations on real servers against Winter 1.2 (Laravel 9 + DBAL), current wip/1.3, and this branch: MySQL 8.4.11, MariaDB 11.4.13 (with both the mysql and mariadb drivers), PostgreSQL 16.15 and SQL Server 2022. Winter 1.2 wasn't run on SQL Server.

MySQL and MariaDB

->change() without restating the attribute Winter 1.2 wip/1.3 this PR
Nullable column on MariaDB stays NULL default becomes the string 'NULL' stays NULL
utf8mb4_bin collation kept dropped kept
useCurrent() timestamp, made nullable fails (DBAL) fails: 1067 Invalid default value works, keeps CURRENT_TIMESTAMP
O'Brien default on MariaDB can't be created (Laravel 9 doesn't escape the ') becomes O''Brien kept
Two columns changed in one blueprint 1 statement 2 identical statements, each altering both columns 1 statement per column

PostgreSQL and SQL Server

->change() without restating the attribute Winter 1.2 wip/1.3 this PR
PostgreSQL: any change that doesn't restate nullable() works syntax error: alter column "a" not null works
PostgreSQL: column with a default ('foo', 0.00) kept syntax error: alter column "a" default … kept
PostgreSQL: O'Brien default can't be created (Laravel 9 doesn't escape the ') syntax error kept
PostgreSQL: useCurrent() timestamp, made nullable fails (DBAL) syntax error works, keeps CURRENT_TIMESTAMP
PostgreSQL: comment kept syntax error kept
PostgreSQL: C collation dropped syntax error kept
SQL Server: column with a default not tested Incorrect syntax near the keyword 'default' default added back
SQL Server: Latin1_General_BIN collation not tested dropped kept
SQL Server: two columns changed in one blueprint not tested 6 statements, each column altered twice 2 statements per column

Causes:

  • The old column is built from getColumns(), but its attributes were read by modifier name (strtolower('Collate') is collate, the key is collation), so the collation was never kept. PostgreSQL only used the new definition's collation.
  • On PostgreSQL and SQL Server, that old column has no change flag, so Laravel's PostgreSQL modifiers compile it as a new column (null, default 'foo') instead of drop not null and set default. On SQL Server, modifyDefault() adds an inline default, which alter column doesn't accept.
  • The reported default was passed through as a string. MariaDB, PostgreSQL and SQL Server report defaults as SQL expressions ('text', 'text'::character varying, ('text')), so they were quoted again. MySQL reports CURRENT_TIMESTAMP as is, which got quoted too.
  • compileChange() compiled every changed column for each change command, instead of the one column in $command->column.

Changes:

  • Copy the attributes the new definition doesn't set onto the changed column, and let Laravel's own compileChange() compile it. That compiles one column per command and keeps instant/lock on MySQL. The kept attributes are nullable, default and the collation while the column stays a text type (like Laravel 9 on MySQL), plus comment and unsigned on MySQL and MariaDB, and comment on PostgreSQL. They are set on the column itself, because on PostgreSQL and SQL Server the comment and the default are compiled by separate commands that run after the change.
  • Turn the reported default back into a definition per server. On MariaDB, NULL means no default and anything else is an Expression, which also covers the escaped quote case from this comment. On MySQL, CURRENT_TIMESTAMP… becomes an Expression, as in Laravel's compileLegacyRenameColumn(), and anything else is the raw value. On PostgreSQL and SQL Server, every default is an Expression.
  • Add tests with columns exactly as each server reports them (a GrammarTestCase::existingColumns() helper and a new MariaDbSchemaGrammarTest). They fail without the change. The PostgreSQL test that asserted alter column "name" null now expects drop not null.
  • Remove the six phpstan baseline entries for the old ColumnDefinition::$name/$renameTo access.

What it doesn't change:

  • integer('x')->nullable()->change() still makes an unsigned column signed, because integer() sets unsigned to false explicitly. Winter 1.2 did the same.
  • A MySQL 8 expression default such as (uuid()) is reported without a marker, so it's still treated as text, the same limitation as Laravel's heuristic.
  • Laravel's own behavior on PostgreSQL stays: a change also emits drop identity if exists for a column that isn't auto-incrementing, and comment on column … is NULL when the column has no comment.
  • On SQL Server a primary key column still can't be changed, so bigIncrements('id')->change() fails on the primary key constraint, as it does without the shim.
  • Winter 1.2 dropped the collation on PostgreSQL. This keeps it, as on MySQL.

This builds on #254 (same method), so it includes #254's commits. The diff gets smaller once that's merged.

🤖 Generated with Claude Code

JonasPardon and others added 2 commits October 8, 2026 11:42
The ->change() shims used isset() to decide whether the migration set a modifier, which is false for null, so ->default(null) kept the old default instead of removing it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…MariaDB

The change shim read the existing column with the modifier names, so the collation was never kept, and passed reported defaults through as strings: MariaDB's NULL became the string 'NULL' and CURRENT_TIMESTAMP was quoted. It also compiled every changed column for each change command. Keep the unset attributes on a copy of the column, normalise the reported default per server, and let Laravel compile the one column of each command.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 552df7ad-b977-4601-bfd2-f5d1d755d99d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@JonasPardon

Copy link
Copy Markdown
Author

This one was fully done by Claude, so please give this one the necessary scrutiny.

@JonasPardon JonasPardon mentioned this pull request Oct 8, 2026
4 of 16 tasks
@mjauvin

mjauvin commented Oct 8, 2026

Copy link
Copy Markdown
Member

@JonasPardon can you take the $column->getAttributes()) out of the for loop in this PR ?

JonasPardon and others added 2 commits October 8, 2026 15:54
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@JonasPardon

JonasPardon commented Oct 8, 2026 •

Copy link
Copy Markdown
Author

@JonasPardon can you take the $column->getAttributes()) out of the for loop in this PR ?

@mjauvin Done in f8e7293. I did the same for the loops in #254 (b330ecd) and picked that into this branch too.

@mjauvin

mjauvin commented Oct 8, 2026

Copy link
Copy Markdown
Member

@JonasPardon have you made the same tests on all other DB servers? Maybe they have the same issues?

The ->change() shims compiled the kept attributes as a new column definition, which PostgreSQL and SQL Server reject. Copy them onto the changed column instead and let Laravel compile it, as for MySQL and MariaDB.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@JonasPardon JonasPardon changed the title Keep column attributes correctly when changing a column on MySQL and MariaDB Fix changing a column on MySQL, MariaDB, PostgreSQL and SQL Server Oct 8, 2026
@JonasPardon

JonasPardon commented Oct 8, 2026 •

Copy link
Copy Markdown
Author

have you made the same tests on all other DB servers? Maybe they have the same issues?

@mjauvin Yes, and they do. PostgreSQL and SQL Server were worse: most changes fail with a syntax error on PostgreSQL, and on SQL Server a column with a default fails unless the migration restates it. I've added the fix to this PR in e87c9fb and updated the description with the results per server. SQLite is fine, apart from ->default(null), which #254 fixes.

@mjauvin

mjauvin commented Oct 8, 2026

Copy link
Copy Markdown
Member

@JonasPardon Why is the MySQLGrammar calling parent::compileChange() in a different way than the other DB servers?

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@JonasPardon

Copy link
Copy Markdown
Author

Why is the MySQLGrammar calling parent::compileChange() in a different way than the other DB servers?

@mjauvin No good reason. I wrote the MySQL version first and changed a copy of the column. PostgreSQL and SQL Server have to change the column itself, because their comment and default are compiled by separate commands that run after the change and read the same column. On MySQL the only such command is the auto-increment starting value, which doesn't read any of the kept attributes, so both ways give the same SQL. I've made MySQL change the column itself too in 3c493b8. The statements are identical on MySQL 8.4 and MariaDB 11.4.

@mjauvin

mjauvin commented Oct 9, 2026

Copy link
Copy Markdown
Member

@JonasPardon any idea what's causing the tests to fail now?

@JonasPardon

JonasPardon commented Oct 9, 2026 •

Copy link
Copy Markdown
Author

@JonasPardon any idea what's causing the tests to fail now?

@mjauvin A packagist security advisory, unrelated to the changes here

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants