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?

[derrabus]

Done. And please rebase your PR and fix the red CI checks.

[ausi]

Done.

[ausi]

…and fix the red CI checks.

Only the “Upload coverage to Codecov” job fails now. I’m not sure how to fix that.

[derrabus]

Only the “Upload coverage to Codecov” job fails now.

You can ignore that one.

[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?