Foreign key name change not picked up by schema comperator

[achterin]

Bug Report

Q	A
Version	3.8.4

Summary

I had created a migration via the symfony command doc:mig:diff early in development that created a new table and a new foreign key constraint to an already existing table. Later I must have changed something that influenced the foreign key name generation because if i delete the constraint now and create a new migration, it has a different name.
Old FK name: FK_E19D9AD24A7A78F6
New FK name: FK_E19D9AD2491C2540

Current behaviour

I would expect doctrine to pick up on that change and drop the old foreign key and create a new one but that isn't the case - it just gets ignored. This is due to Schema/Comperator.php:diffForeignKey() not checking for that name change. This function just checks if the foreign key columns, tables or actions changed.

How to reproduce

Rename an existing foreign key constraint and create a table diff.

Expected behaviour

The comperator should notice the foreign change and therefore drop the "orphaned" key and create a new one with the correct name.

Proposed change

Adding the following lines to the diffForeignKey() function will fix the issue:
if (strtolower($key1->getName()) !== strtolower($key2->getName())) { return true; }

[greg0ire]

@achterin I think it sounds good, would you like to send a PR with tests showing the issue as well as your change?

[achterin]

@greg0ire sure, I'll give it a shot. The thing is, there is already a test that will fail with my changes in place but I don't understand why doctrine should behave that way.

The test I'm talking about is this one: https://github.com/doctrine/dbal/blob/4.0.x/tests/Schema/AbstractComparatorTestCase.php#L366

To me it seems this test was introduced way back as part of this issue: doctrine/orm#2336

In my opinion a foreign-key name change should always be reflected in a tablediff as it will result in a different schema.
The question is: should i just remove the test or am I missing something entirely and this test tries to test for something else and is just coded in a way so the assertion gets triggered with my changes in place?

Thanks for you help in advance!

Edit: After a little more digging I found that testCompareIndexBasedOnPropertiesNotName once behaved exactly the same but was later changed due to the introduction of renaming indexes. So I guess it will be fine to change the existing test to also reflect my change and expect one fk being dropped and one being added.

Edit2: In my opinion it would also be good to rename the tests as they now test for the complete opposite.