Skip to content

Conversation

@PaulCombal
Copy link

Q A
Type bug/improvement
Fixed issues doctrine/migrations#1542

Summary

For MySQL databases, the default value is always reported as being a string, which causes doctrine/migrations to be confused sometimes.

It's my first time contributing to Doctrine please be free to request edits.

@derrabus
Copy link
Member

derrabus commented Dec 4, 2025

Thank you for the PR. Please always add a test that reproduces the issue you're attempting to solve.

@morozov
Copy link
Member

morozov commented Dec 5, 2025

For MySQL databases, the default value is always reported as being a string, which causes doctrine/migrations to be confused sometimes.

I would attempt to fix that instead. Database drivers represent some numbers as strings for a reason.

Using floats for any kind of logic that involves comparison may just exacerbate the problem. Also, if we want to make such a change, it should be implemented and tested consistently across all supported platforms, not only MySQL.

@PaulCombal
Copy link
Author

PaulCombal commented Dec 5, 2025

What you're saying is that I should be handling this in doctrine/migrations instead of dbal, right?

@morozov
Copy link
Member

morozov commented Dec 5, 2025

Not necessarily. The problem may be in the DBAL itself but not in the schema manager but let's say in the comparator.

The best way to facilitate reasoning about the proper fix is to implement a failing scenario in code using the minimum of dependencies (e.g. w/o the ORM).

@PaulCombal
Copy link
Author

PaulCombal commented Dec 5, 2025

Thanks for the guidance. I moved the logic to AbstractPlatform::columnsEqual. Is this appropriate?

I will of course write tests for the change once a maintainer confirms the code is at the right place.

@morozov
Copy link
Member

morozov commented Dec 5, 2025

I will of course write tests

Please start with an integration test.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants