Skip to content

Conversation

jochen-jung
Copy link

Q A
Type bug
Fixed issues (doctrine/migrations#1460)

Summary

The schema was missing on the table name for MySQL.

Because of that "bin/console doctrine:migrations:diff" did not generate correct diffs and always detected the whole table as changed.

See:
doctrine/migrations#1460

Jochen Jung added 2 commits September 2, 2025 11:02
…d the whole table as changed instead of the true changes.
…d the whole table as changed instead of the true changes.
@derrabus
Copy link
Member

derrabus commented Sep 4, 2025

Looks like your change broke a lot of existing tests and you didn't add any new tests that would reproduce the issue that you're trying to solve. Are you planning to continue your work?

@jochen-jung
Copy link
Author

Hi, I was hoping for some feedback whether the change is fine this way in general, as I do not know the doctrine code well enough.

But I can try to resolve the unit tests first next Monday, if you prefer.

Jochen Jung added 4 commits September 8, 2025 08:35
…d the whole table as changed instead of the true changes.
…d the whole table as changed instead of the true changes.
…d the whole table as changed instead of the true changes.
…d the whole table as changed instead of the true changes.
@jochen-jung
Copy link
Author

I got the tests fixed now.

As I prepend the schema to the table name only for MySQL (I don't know if that makes sense for other engines as well), I added some ifs to the tests, so in case of MySQL the schema is prepended in the test as well.

For the tests/Functional/Schema/SchemaManagerFunctionalTestCase.php I overwrite the problematic tests now inside tests/Functional/Schema/MySQLSchemaManagerTest.php with a version that prepends the schema.

As for the change in src, another option would be instead of doing the concat on the database it could be done in the _getPortableTableDefinition() method instead, if that is considered cleaner. But I decided against that, as the method is marked as deprecated.

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.

2 participants