Skip to content

Fix changing a column on PostgreSQL and SQL Server - #259

Closed
JonasPardon wants to merge 5 commits into
wintercms:wip/1.3from
JonasPardon:fix/change-postgres-sqlserver
Closed

JonasPardon wants to merge 5 commits into
wintercms:wip/1.3from
JonasPardon:fix/change-postgres-sqlserver

Conversation

@JonasPardon

Copy link
Copy Markdown

Following up on this question on #258: the ->change() shim generates invalid SQL on PostgreSQL and SQL Server on wip/1.3. SQLite is fine: it keeps every attribute, apart from ->default(null), which #254 fixes.

I ran the same migrations on PostgreSQL 16.15 against Winter 1.2 (Laravel 9 + DBAL), current wip/1.3, and this branch, and on SQL Server 2022 against wip/1.3 and this branch:

->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() without the 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, but both servers report defaults as SQL expressions ('foo'::character varying, ('foo'), (getdate())).
  • The collation was never copied from the old column: PostgreSQL only used the new definition's, and SQL Server looked it up as collate while getColumns() reports collation.
  • compileChange() compiled every changed column for each change command.

Changes:

  • The same approach as Fix changing a column on MySQL, MariaDB, PostgreSQL and SQL Server #258: copy the attributes the new definition doesn't set (nullable, default, the comment on PostgreSQL, and the collation while the column stays a text type) onto the changed column, and let Laravel's own compileChange() compile it. Unlike Fix changing a column on MySQL, MariaDB, PostgreSQL and SQL Server #258, the attributes are set on the column itself and not on a copy, because PostgreSQL compiles the comment and SQL Server the default in separate commands that run after the change.
  • Keep the reported default as an Expression.
  • Add tests with columns exactly as PostgreSQL and SQL Server report them. They fail without the change. The PostgreSQL test that asserted alter column "name" null now expects drop not null.
  • Remove the two phpstan baseline entries for the old ColumnDefinition::$name access.

What it doesn't change: Laravel's own behavior stays. On PostgreSQL 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 #258 (and #254), and uses the existingColumns() test helper from #258, so it includes their commits. Only the last commit is new. The diff gets smaller once those are merged.

🤖 Generated with Claude Code

JonasPardon and others added 5 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>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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>
@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: 60564da8-dbad-4d5a-95d4-d305a5aa1e76

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
  • Autopilot · 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

Closing in favour of #258, which now includes this change (e87c9fb).

@JonasPardon JonasPardon closed this Oct 8, 2026
@JonasPardon
JonasPardon deleted the fix/change-postgres-sqlserver branch October 8, 2026 15:02
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.

1 participant