doctrine / dbal

Doctrine Database Abstraction Layer
https://www.doctrine-project.org/projects/dbal.html
MIT License
9.44k stars 1.33k forks source link

RENAME TABLE instead of re-creation #2395

Open ghost opened 8 years ago

ghost commented 8 years ago

This issue is related to the schema comparator. We use it to run migrations.

If I change the table name it will result into DROP TABLE and CREATE TABLE sql queries. This will result into a data-loss. It would be better to use the RENAME TABLE feature.

See https://github.com/doctrine/migrations/issues/461

deeky666 commented 8 years ago

Which DBAL version are you using and which platform does this affect? Also some sort of information/code how to replicate this is necessary otherwise we cannot do much about it.

Grundik commented 3 years ago

Doctrine\DBAL\Schema\Schema code for that are not platform-specific and about 12 years old now:

    public function renameTable($oldName, $newName)
    {
        $table = $this->getTable($oldName);
        $table->_setName($newName);

        $this->dropTable($oldName);
        $this->_addTable($table);

        return $this;
    }

See also: https://github.com/doctrine/migrations/issues/17

This method should not exist with such name, its a trap.

Killerkiwi2005 commented 1 year ago

At the very least this method should be removed...

NiklasBr commented 1 week ago

Maybe add this to Milestone 5.0.0, I would love that!

derrabus commented 1 week ago

Do you want to work on this topic?

NiklasBr commented 1 week ago

Which solution would you most like? Change the method name to something that suggests that it will delete content, or change the internal actions to use the RENAME TABLE grammar?

derrabus commented 1 week ago

I think, we should use RENAME TABLE on platforms that support it.

Grundik commented 1 week ago

...and leave trap armed for other ones.;)

derrabus commented 1 week ago

First of all, it would be interesting to know, how many "other ones" there are. Haven't researched it, tbh.

How would you deal with platforms that don't support renaming tables?

Grundik commented 1 week ago

I think, this trap must be disarmed. Best time for that was several years ago, second best time is now. So, if database dont support RENAME, then this migration should throw an exception, but not discard user data.

Grundik commented 1 week ago

And another option could be to remove this method entirely (or at least rename it), to not cause the situation, when developer intended RENAME, but production dependencies was not updated yet, so data will be lost.

NiklasBr commented 1 week ago

Looking at the platforms at https://www.doctrine-project.org/projects/doctrine-dbal/en/latest/reference/platforms.html, I think this could work. although I have not looked at which versions need to be supported for each platform…

How would you deal with platforms that don't support renaming tables?

I would probably throw an Unsupported-style exception.

Grundik commented 1 week ago

Its a very dangerous method now, and making it usable is also dangerous. This method should not be called ever, if user cares about his data. So real rename should be new method, to not allow any confusion or mistake, since cost of calling that method could be devastating.

And current one should just fail loudly (or dont even exist), instead of discarding data.

derrabus commented 1 week ago

Best time for that was several years ago

Let's switch to solution mode, shall we.

derrabus commented 1 week ago

Looking at the platforms at https://www.doctrine-project.org/projects/doctrine-dbal/en/latest/reference/platforms.html, I think this could work. although I have not looked at which versions need to be supported for each platform…

Thank you, that looks very promising indeed. The versions that support renaming would be interesting indeed. Right now, it looks like we don't even need to discuss the case that a platform does not support renaming.