Add comparison of foreign key names with option to opt-out of this

[uncaught]

Q	A
Type	improvement
Fixed issues	#6518

Summary

• Re-adds changes of Fix foreign key name change detection #6413
• Additionally add configuration option to opt-out of foreign key name comparison

This PR is targeted at 4.1. I initially tried to target 3.9 (#6520), but that was pretty much not mergeable into 4.1. Using the platform as a go-between for configuration settings seemed wrong, too.

I hope it is okay to inject the Configuration into the Comparator now!? I didn't know how else to configure this behavior. And I'm considering to add another opt-in option for the comparator to enable comparing the implicitly added indexes for FKs, too, in a later PR because these have the same problems in migrations (generated once but then forgotten and untracked).

I'm also not sure what this classifies under. This should be a bug fix, but the conditions under which this hurts are quite rare. So I went with "improvement" because this improves the schema diff.

I've decided to make the behavior opt-out instead of opt-in because it is more dangerous to have wrong FK names than to have extra migration diffs. (You will notice the latter during development, but not the other way around.)

[derrabus]

I hope it is okay to inject the Configuration into the Comparator now!?

I don't think this is the right way. Also, after your change, the schema managers don't inject the configuration into the comparators. They inject an empty configuration object with all default settings.

That means, I can change that new settings of yours for my connection, but it will have absolutely no effect.

I've decided to make the behavior opt-out instead of opt-in because it is more dangerous to have wrong FK names than to have extra migration diffs. (You will notice the latter during development, but not the other way around.)

That's not how we decide such things. If the new behavior you're introducing has the potential of breaking existing apps, the new behavior has to be opt-in. I have to be able to upgrade my apps to a new minor release of DBAL without changing either my code or configuration.

[uncaught]

I don't think this is the right way. Also, after your change, the schema managers don't inject the configuration into the comparators. They inject an empty configuration object with all default settings.

Ah, I've only missed it in the MySQLSchemaManager, the base method uses $this->connection->getConfiguration(), which should be fine, yes?

That's not how we decide such things. If the new behavior you're introducing has the potential of breaking existing apps, the new behavior has to be opt-in. I have to be able to upgrade to a new minor release without changing either my code or configuration.

Ok, I'll change it to opt-in then.

[uncaught]

I don't think this is the right way.

I'm happy to change it if you could tell me how to configure this otherwise.

[derrabus]

#6300 also introduces configurable behavior to the schema comparison. Maybe you find some inspiration there.

[uncaught]

#6300 also introduces configurable behavior to the schema comparison. Maybe you find some inspiration there.

I've replied there - looks like quite similar, but I don't see how his solution can be configured with one configuration (e.g. symfony), yet

[berkut1]

@uncaught If you want Symfony to support it through configs, you also need to create a PR here - https://github.com/doctrine/DoctrineBundle

[uncaught]

@uncaught If you want Symfony to support it through configs, you also need to create a PR here - https://github.com/doctrine/DoctrineBundle

yes, will do that. I just thought I wait until I know this here is ok to merge.

[uncaught]

I'm gonna give up here. With #6300 introducing a configuration class that currently cannot be configured in the dbal/orm/migrations ecosystem this isn't worth persuing.

I'll keep relying on an phpunit test for now, which compares the index/FK names of the existing DB with those of a freshly created schema.