Dbal: Allow to disable renaming detections in Comparator

[Toflar]

Feature Request

For my purpose, index and column renames are never the desired option.
I would always like to disable renaming indexes and columns and have drop and create queries instead.

| Q | A
|------------ | ------
| New Feature | yes
| RFC | yes
| BC Break | no

Summary

I would like to create a PR where the Comparator would accept flags in its constructor (I'd go for bitwise operator flags so it's easily extensible) where one can optionally disable renaming columns and or indexes. Those flags would also be accepted in Schema::getMigrateToSql() and Schema::getMigrateFromSql().

I just wanted to ask if you'd be open for such a feature before I put in all the work in a PR? 😊

[SenseException]

What does "index and column renames are never the desired option" mean? What is your usecase?

Can you please extend your summary what the resulting queries are if you implement your proposal compared to what DBAL does now?

[Toflar]

In that case the use case is Contao, a CMS where we have a database schema migration tool. It allows the users to select which statements to execute. If you update from an older version to the latest version, the Comparator gives us something like ALTER TABLE tl_newsletter_channel CHANGE smtpuser sender VARCHAR(128) DEFAULT '' NOT NULL which is complete nonsense because they are absolutely not related and and sender will then contain nonsense data.
In our case we would like to have two statements (ALTER TABLE tl_newsletter_channel ADD COLUMN sender ... and an ALTER TABLE tl_newsletter_channel DROP smtpuser) so the users get a fresh and empty column and can decide when to drop the old column.

[SenseException]

As far as I understand there is a column smtpuser that is totally different than the new additional column sender. smtpuser got dropped out in the new version and sender got added as a new column. smtpuser is unrelated to sender and is not the same (but renamed) column with the same information?

[Toflar]

Exactly. smtpuser is just a column that's not needed anymore so it can be dropped. However, we don't want to do this automatically so people can review etc. That's why I would like to be able to disable the renaming in the Comparator.

[SenseException]

Is this the only occurrence where you observed this behavior?

Your proposal has also unwanted properties like when someone is renaming a column that this would also result in deleting a column and creating a new one, making all the data of the column disappear. So I guess your mentioned flag is for preventing this. Can you please describe your plan what you are going to change to introduce this flag?

[Toflar]

In fact, it's the other way around now :) The Comparator just dictates that renaming is better which is not always the case as my use case illustrates. Of course I wouldn't change the behaviour, just make it configurable.

So it'll be as easy as having:

class Comparator
{
    public function __construct($detectRenames = true)

And I'll just set that to false in my case - done.

[morozov]

And I'll just set that to false in my case - done.

Define "my case". Is it supposed to be set on the application level or for a specific migration? What if in a specific migration, one column is renamed and another is and created?

Wouldn't it make sense to just add ALTER TABLE tl_newsletter_channel UPDATE sender = '' at the end of migration?

[aschempp]

It's not about migration, it's about the schema diff. There's no ALTER TABLE on schema diffs. For our specific case, if a field is renamed, a custom migration should probably take care of it before the schema diff. Then the schema diff should never rename, as all field changes are "unrelated".

[RobertZenz]

From an use-case perspective, this behavior makes sense. However, from an API perspective this is quite bad, as one would expect a dropColumn to do what it says on the tin and instead having a dedicated renameColumn function. Also matching the columns only based on properties must yield false-positives (f.e. removal of one flag column and adding another). Additionally, I'm not seeing the documentation mentioning this at all.

[Toflar]

I agree but I think we cannot just disable that feature for BC reasons. Somebody might rely on the fact that it's actually renaming and not dropping. Hence the proposal for a new flag.
Any feedback if this would be accepted @morozov? :)

[RobertZenz]

A good way to handle this would be to introduce a new renameColumn method which does exactly what it says on the tin and allow to disable the implicit renaming on a global scale.

What if in a specific migration, one column is renamed and another is and created?

Introducing the renameColumn method would be an explicit way of handling this.

Wouldn't it make sense to just add ALTER TABLE tl_newsletter_channel UPDATE sender = '' at the end of migration?

That's a workaround. If your official suggestion to using your API includes workarounds for default behavior, you need to reconsider your API design and implementation.

[aschempp]

I don't see how a renameColumn method can be implemented. The Doctrine\DBAL\Schema\Comparator compares two table schemas (in our case, the file definition and the database). It calculates that fields have been added or removed, and it runs detectColumnRenamings to magically rename instead of add/drop columns.

As @Toflar initially said, the Comparator would need an argument to bypass detectColumnRenamings.

[Toflar]

@morozov are there still any open questions we might answer to move this forward?

[morozov]

I'm not that well familiar with schema management in DBAL but at a glance, it looks like comparator is only responsible for producing the diff while the job of the schema manager is to process the diff and generate SQL statements to migrate the schema from one state to the other. This logic is extensible via the eventing system.

The logic of migrating a column from one definition to another is platform-specific. E.g. PostgreSQL allows using a USING clause.

With all the above said, I do not believe that adding this argument is the right approach. Unfortunately, I don't have enough time to work with you on this.

[Toflar]

Thanks for your reply @morozov.

I'm not that well familiar with schema management in DBAL but at a glance, it looks like comparator is only responsible for producing the diff while the job of the schema manager is to process the diff and generate SQL statements to migrate the schema from one state to the other. This logic is extensible via the eventing system.

That is correct. The problem we face (and thus thousands of users) is that the comparator produces an incorrect diff for us. It always tries to rename columns instead of dropping the old and creating a new one. However, this logic is currently not extensible and there's no way you can properly change that behaviour which is why I wanted to introduce a new flag to be able to fix that.

With all the above said, I do not believe that adding this argument is the right approach. Unfortunately, I don't have enough time to work with you on this.

I understand you do not have enough time to work on it. We're all in the same boat here. This is an issue affecting us so we'll happily put in the work required to get this done.
I think I'm just going to put in the work anyway so you guys can actually see that the changes are minimal.

[Toflar]

Here we go, I quickly outlined the changes in https://github.com/doctrine/dbal/pull/3917.

[dmolineus]

I have a look at this, doctrine already provides an event to customize the behaviour by adding an event listener.

final class DoctrineListener
{
    public function onSchemaAlterTableRenameColumn(\Doctrine\DBAL\Event\SchemaAlterTableRenameColumnEventArgs $args) : void
    {
        $args->preventDefault();

        $diff        = $args->getTableDiff();
        $platform    = $args->getPlatform();
        $column      = $args->getColumn();
        $columnArray = $column->toArray();
        $alterTable  = 'ALTER TABLE ' . $diff->getName($platform)->getQuotedName($platform);

        $args->addSql($alterTable . ' DROP ' . $platform->quoteIdentifier($args->getOldColumnName()));
        $args->addSql($alterTable . ' ADD ' . $platform->getColumnDeclarationSQL($column->getQuotedName($platform), $columnArray));
    }
}

The general issue is that a schema diff can't decide when it's reasonable to rename a column and when not. It depends. A flag disabling it wouldn't solve the isse that the decision has to be done on each rename statement.