search

doctrine / dbal

Doctrine Database Abstraction Layer
https://www.doctrine-project.org/projects/dbal.html
MIT License
9.4k stars 1.33k forks source link

Fix foreign key name change detection #6413

Closed achterin closed 1 month ago

achterin commented 1 month ago
Q A
Type bug
Fixed issues #6390

Summary

This changes how the schema comparator treats foreign key name changes. The index renaming had the same behavior in the past, expecting a index name change to result in an empty table diff. This was changed some time ago but foreign key changes were still not picked up by the comparator. This pr will change that. While at it, I've also changed the names of both tests to reflect the changes.

greg0ire commented 1 month ago

The index renaming had the same behavior in the past, expecting a index name change to result in an empty table diff. This was changed some time ago but foreign key changes were still not picked up by the comparator.

Please add links showing that.

achterin commented 1 month ago

The index renaming had the same behavior in the past, expecting a index name change to result in an empty table diff. This was changed some time ago but foreign key changes were still not picked up by the comparator.

Please add links showing that.

Sure, no problem. This changeset introduced the index-rename support and changed the corresponding test. As you can see, before this patch the test expected an empty table-diff - same as for the fk-name test below.

https://github.com/doctrine/dbal/commit/a996bea800ee76d950f8a415aabf7d1499e7b506#diff-1e412c2c0134199408e114ba49a15f2019f3ccbac5472c3815f59c815f810e45

greg0ire commented 1 month ago

Since this is a bugfix, and it affects 3.x, you should retarget this PR on 3.8.x (and rebase on that).

achterin commented 1 month ago

Okay, I thought it would be the other way around, so fix it first in the default branch and then backport it. Will do.

achterin commented 1 month ago

@greg0ire sadly i was unable to adapt my exact changes to the 3.8.x branch, thats why i did reduce the test down to just checking if one foreign key was added and one removed. i also used the deprecated diffTable function to be in line with the other tests. i guess this should be reworked in the way i had initially committed it when merging this into the newer version branches.

let me know what you think. thanks!

greg0ire commented 1 month ago

I think you should open another PR after we merge this up to adapt the test.

greg0ire commented 1 month ago

Thanks @achterin !