Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unset the default value if the given column definition is an integer … #5391

Closed
wants to merge 3 commits into from

Conversation

berrugo
Copy link

@berrugo berrugo commented May 12, 2022

Unset the default value if the given column definition is an integer and the default value is a string

Q A
Type bug
Fixed issues N/A

Summary

This is kinda of an edge case that I found when working with Laravel migrations. I wanted to change a column type from VARCHAR(32) DEFAULT '' to INT and got the following malformed SQL expression from Doctrine\DBAL\Platforms\AbstractMySQLPlatform\getAlterTableSQL()

"ALTER TABLE location_details CHANGE location_id location_id INT DEFAULT NOT NULL"

Since the column's original default value was a string, in this case an empty one '', it got carried over to the ALTER statement. The fix I propose is to check if the new column type is an PhpIntegerMappingType and if its default value a string then unset the default, similar to what we already do with TextType and BlobType. After this fix, the resulting SQL statement should be the following:

"ALTER TABLE location_details CHANGE location_id location_id INT NOT NULL"

@morozov
Copy link
Member

morozov commented May 14, 2022

This looks similar to #3714 and #3813 (which are also related to Laravel).

I don't believe that the DBAL should be in the business of silently dropping the default. If it's not important, it shouldn't have been set in the original column to an empty string in the first place.

@morozov morozov closed this May 14, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants