Make column and index renaming configurable

[ausi]

Q	A
Type	improvement
Fixed issues	#6299

Summary

Makes column and index rename detection of the Comparator configurable, as suggested in #6299 (comment)

[github-actions]

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.
If you want to continue working on it, please leave a comment.

[ausi]

I adressed all review comments.

Adressing this issue is quite important for us in order to get compatible with Doctrine 4.

[greg0ire]

I think this lacks documentation, and I'm curious to see what a real-world usage would look like, since "The comparator can be only instantiated by a schema manager" (so, at runtime, rather than at DIC compilation time).

[ausi]

I think this lacks documentation,

Should I add a section for createComparator() to docs/en/reference/schema-manager.rst ?
Or should it be a new page about the Comparator itself?

and I'm curious to see what a real-world usage would look like, since "The comparator can be only instantiated by a schema manager" (so, at runtime, rather than at DIC compilation time).

We would use it here: https://github.com/contao/contao/blob/aa54c3df007e4b01b73e08036e35c054a7d1fd84/core-bundle/src/Migration/CommandCompiler.php#L61-L66 like this:

$comparator = $schemaManager->createComparator();
$config = new ComparatorConfig();
$config->setDetectRenamedColumns(false);
$config->setDetectRenamedIndexes(false);
$comparator->setConfig($config);
$diffCommands = $comparator
    ->compareSchemas($fromSchema, $toSchema)
    ->toSql($this->connection->getDatabasePlatform())
;

Are you happy with the setConfig() approach? I think I’d prefer a third parameter to the methods compareTables and compareSchemas instead, see #6300 (comment)

[greg0ire]

I would drop setConfig, and add a new argument to createComparator, so that the config can be readonly. Having it mutable doesn't feel great.

[ausi]

Should I make the Config object itself also immutable?

[greg0ire]

I think everything should be made immutable by default, yes. If people need to mutate anything, they will send a PR explaining their use case, making it mutable won't be a breaking change.

[greg0ire]

Should I add a section for createComparator() to docs/en/reference/schema-manager.rst ?

A section feels enough

[ausi]

I think this lacks documentation

Done in e117bbb

[ausi]

Failing CI seems to be unrelated.

[ausi]

As this didn’t make it into the 4.1.0 release, should I change the base branch to 4.2.x?

[uncaught]

This looks quite similar to what I've just done in #6521 - I've injected the Configuration where you inject this new config class of yours.

However, I'm curious as to how I would configure this at the end-user-level? The doctrine-bundle configures the Configuration, so it would be easy enough to add new options there. But how will this work on your class?

In my symfony project I see several places outside the dbal that call createComparator(): doctrine/migrations, symfony/doctrine-messenger, doctrine/orm. Do we have to configure all those places individually, add PRs there? The config should surely be the same across all of them.

I'm fine with having a dedicated config sub-class for the comparator, but would it not make sense to have it also as property on the Configuration, and then inject it from there, rather via an argument of createComparator?

[ausi]

@derrabus should I update this PR to also use Doctrine\DBAL\Configuration instead of introducing the new Doctrine\DBAL\Schema\ComparatorConfig?
Or should I nest the ComparatorConfig inside the Configuration object and pass that to the comparator?
Or should this PR be kept as it is now?

[derrabus]

I think that it makes sense to configure this behavior when I actually want to compare stuff. Relying on global configuration for that matter sounds brittle to me. So, your new object is fine. The only thing that I would change is that passing a configuration to the comparator constructors should remain optional. I know that all the constructors are flagged as internal technically, but I don't believe that we have to break existing constructor calls for this change.

The base branch was changed.

[ausi]

…passing a configuration to the comparator constructors should remain optional…

Done in 4751bc9

It would be great if we could get this merged into the next version as this issue currently makes it impossible for us to be compatible with dbal version 4.

[uncaught]

I think that it makes sense to configure this behavior when I actually want to compare stuff.

So we'll need more PRs then for all the rest to implement support for this, right?

That means we need at least 3 new configurations like:

doctrine:
    dbal:
        compare_index_names: true # for ***SchemaManager::createComparator
    orm:
        compare_index_names: true # for SchemaTool::getUpdateSchemaSql

doctrine_migrations:
    compare_index_names: true # for DiffGenerator::generate

And fingers crossed they stay in sync.

[derrabus]

That means we need at least 3 new configurations like:

Not sure what you're getting at an how this is relevant for this PR.

[uncaught]

If the comparator config needs to be provided to the createComparator method, then all these external libraries need to be updated, too, because they all call createComparator and you probably want them all to use the same config.

If this dbal provided a way to set this config somehow, before one would call createComparator, integration would be easier.

[derrabus]

If the comparator config needs to be provided to the createComparator method

Which is not the case. You can do that, you don't need to if you're happy with the defaults.

you probably want them all to use the same config.

No, why?

[uncaught]

Ok, sorry, I thought this would be a fix for the indexes breaking because the migrations don't track index names at the moment.

A migration diff generated on db1 is not guaranteed to run on db2 if db2 was created later than db1 with the schema tools and the generated index names don't match anymore.

So I thought you would always want to have index name comparison enabled across the entire db suite (dbal/orm/migrations) or not at all.

I'll just stop complaining now - seems I cannot convince you ;)

[ausi]

I thought this would be a fix for the indexes breaking because the migrations don't track index names at the moment.

This pull request is about making the current behavior of detecting renames of columns and indexes optional by making it configurable. The fix you are talking about sounds like your own pull request here: #6521 ?

[greg0ire]

Another small nit and I think I'm OK. Please squash commits that should not appear in the history such as the one to fix this nit.

[ausi]

Please squash commits that should not appear in the history such as the one to fix this nit.

Done.

[greg0ire]

Thanks @ausi !