Skip to content

Conversation

cristi-contiu
Copy link
Contributor

@cristi-contiu cristi-contiu commented Feb 9, 2025

Q A
Type improvement
BC Break no/minor
Fixed issues #1487

Summary

Ports doctrine/orm#7875 into Migrations to improve consistency between queries generated by Migrations diff and the ones generated by the ORM schema update/validate commands. In most cases, the queries where already identical, but in some rare cases (like #1487), the migrations generated from diff did not contain the same queries as the ORM schema update and validate commands.

It also removes filtering the schema generated from metadata ($toSchema) using DBAL's schemaAssetsFilter and the doctrine.dbal.schema_filter config, which should only be used to exclude tables in the schema generated from the existing database ($fromSchema - DBAL and DoctrineBundle already cover this). This might be considered a minor BC-break, but if the metadata contains unwanted tables, they shouldn't be included in the mappings in the first place.

The decoration of the existing schemaAssetsFilter from the DBAL configuration is done in doctrine/orm#7875 to whitelist assets present in the $toSchema so that they don't get removed from the $fromSchema by the default schemaAssetsFilter (usually based on the doctrine.dbal.schema_filter config) - a difference which would lead to always generating CREATE TABLE queries.

I think this last part is best tested in a functional/integration test, but I would need some guidance on how to set it up.

If it's a wanted change, I can also add a note to UPGRADE.md, depending on the branch it would be included in and if it's considered a minor BC break.

@cristi-contiu cristi-contiu marked this pull request as ready for review February 9, 2025 20:52
@cristi-contiu cristi-contiu deleted the align-diff-with-orm branch February 14, 2025 10:57
@cristi-contiu cristi-contiu restored the align-diff-with-orm branch February 14, 2025 10:57
@cristi-contiu
Copy link
Contributor Author

cristi-contiu commented Feb 14, 2025

Accidentaly closed this PR, opened a new one here: #1490

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