Add explicit renameColumn method for Table

[Tofandel]

Q	A
Type	feature
Fixed issues	doctrine/migrations#57

Summary

Reopening #6078 on 3.7.x

Add a renameColumn in the Table class

This allow to create migration with schemas that rename columns explicitely, which is a missing feature

Right now only implicit detection is done (it looks for added columns with the same definition as a dropped column), and that means if you change an option or the type of the column, it will not consider it as a rename anymore and instead will drop and recreate, by being explicit, this is not a problem anymore

This PR allows the following migration, without losing the column data

    public function up(Schema $schema): void
    {
        $schema->getTable('test')->renameColumn('foo', 'bar')->setType(Type::getType('text'));
    }
    
    public function down(Schema $schema): void
    {
        $schema->getTable('test')->renameColumn('bar', 'foo')->setType(Type::getType('string'));
    }

Which before would have to drop the column and recreate it, as well as needing a full redefinition of the column, and losing the data in the process

    public function up(Schema $schema): void
    {
        $schema->getTable('test')->dropColumn('foo')->addColumn('bar', 'text');
    }
    
    public function down(Schema $schema): void
    {
        $schema->getTable('test')->dropColumn('bar')->addColumn('foo', 'string');
    }

PS: I'm building a feature for the doctrine/migration package to generate diffs of schema, directly using Schema instead of platform specific sql and without this renameColumn this would be impossible, it would also allow other frameworks which use schemas (Yes I'm thinking laravel) to rename columns without using internals

[Tofandel]

See

dbal/tests/Platforms/AbstractPlatformTestCase.php

Lines 432 to 440 in 0febc80

	$tableDiff->changedColumns['bar'] = new ColumnDiff(
	'bar',
	new Column(
	'baz',
	Type::getType(Types::STRING),
	['comment' => 'B comment'],
	),
	['comment'],
	);

To understand why those tests were incorrect

The column is renamed in the test, but not in the test case output

[derrabus]

Sorry, I simply haven't had time to take another look at this PR. I'm adding it to the milestone so I hopefully won't forget it.

[Tofandel]

I've been cleaning up the PR, and in the end decided to do some deprecations directly in this PR so it's more clear

I deprecated the getRenamedColums and for backwards compatibility I changed getModifiedColumns to getChangedColumns so that getRenamedColum and getModifiedColumns will still output exactly the same as before, but getChangedColumns will be a mix of the renamed and modified columns

I also digged a bit on the case sensitivty issue, none of them are case sensitive (unless the identifier is quoted) and the Comparator is already case insensitive (strtolower in Table::normalizeIdentifier), so we are already assuming everyone is using lower case column names, so for now I don't think it's necessary to handle case sensitive renames at all, so I'm just throwing an exception if we use renameColumn('foo', 'Foo')

Unrelated: Does anyone have the issue where phpunit just exits when it encounters a skipped test? I've been trying to run them but it stops in the middle because of this I can't run all the tests locally

[Tofandel]

This statement was not needed, if you look at the column diff

[
    'mycolumn' => new ColumnDiff(
        'mycolumn',
        new Column('mycolumn', Type::getType(Types::STRING), ['default' => 'bar']),
        ['default'],
        new Column('mycolumn', Type::getType(Types::STRING), ['default' => 'foo']),
    ),
],

Because only the default value changed, the type didn't

[stof]

The automatic detection of renames should be kept IMO, as not all Schema diffing happens for a Schema that has been manipulating based on the introspected one.
The common case for doctrine/migrations is that the introspected schema is compared to an ideal schema built based on the ORM mapping.

PS: I'm building a feature for the doctrine/migration package to generate diffs of schema, directly using Schema instead of platform specific sql and without this renameColumn this would be impossible, it would also allow other frameworks which use schemas (Yes I'm thinking laravel) to rename columns without using internals

Note that Schema-based migrations have 2 big drawbacks compared to generating the SQL at generation time:

• they are much slower as they require introspecting the database between each executed migration (they cannot rely on using the schema altered in the previous migration as extra SQL queries might also have been executed to do more changes)
• they are unable to migrate data, making them safe to run only on an empty database (when generating SQL, you can add extra queries in the middle to perform the necessary data updates).

[greg0ire]

@Tofandel can you please try merging this up into 4.0.x? Or alternatively, send an equivalent PR to 4.0.x? @derrabus and I both tried the merge up, and it's a nightmare, mostly because of #5788, which also touches a lot of the files you touched.

[Tofandel]

@greg0ire When I have some time sure, I was looking at submitting it at 4.x originally because I saw how much code I was touching was already modified, but I was told to do it on 3.7. So yes I can imagine the nightmare, I think the main problem is that a big chunk of the tests that this rellies on where also modifed or removed

[Tofandel]

@greg0ire I'm having a look, is it okay if I replace all arguments of TableDiff with named arguments? It's a class with a ton of them and it's hard to keep track of them, it will make this kind of changes easier to work with

[greg0ire]

@Tofandel on 4.0.x, yes, on 3.8.x no, because named arguments were introduced in PHP 8.0

[derrabus]

I've reverted the PR for now, in order to not block the new releases any further, see #6276. This buys us some time. Once we have a version of this feature for 4.x, we can restore your changes and do another 3.x feature release.

[morozov]

@Tofandel where in this method is SchemaException thrown?

[Tofandel]

It's not, i think it was a leftover because I was not sure which exception to chose and I started with SchemaException

[morozov]

@Tofandel do you mind filing a PR that removes it? PhpStorm treats it as check exception meaning that the caller of this method should either catch it or have a @throws SchemaException annotation in its declaration. This is false-positive and should be fixed.