doctrine
Preventing incorrect column renames no longer possible in 4.0
Bug Report
| Q | A |
|---|---|
| Version | 4.0.0 |
Summary
With doctrine 3.x we use the onSchemaAlterTableRenameColumn event in order to disable automatic column renaming.
The code looks like this (basically reversing what Comparator::detectRenamedColumns() does):
public function onSchemaAlterTableRenameColumn(SchemaAlterTableRenameColumnEventArgs $args): void
{
$args->preventDefault();
$platform = $args->getPlatform();
$table = $args->getTableDiff()->getName($platform);
$oldColumn = $args->getTableDiff()->fromTable->getColumn($args->getOldColumnName());
$column = $args->getColumn();
$tableDiff = new TableDiff($table->getName());
$tableDiff->removedColumns[$args->getOldColumnName()] = $oldColumn;
$tableDiff->addedColumns[$column->getName()] = $column;
$args->addSql($platform->getAlterTableSQL($tableDiff));
}
Now the event is gone in version 4.0 and it looks like there is no other way to influence this behavior.
I thought of decorating the SchemaManagerFactory so that it returns a decorated SchemaManager that returns a decorated Comparator. But even then, there would be no way to decorate Comparator::compareTables() as it returns an immutable TableDiff that cannot be instantiated as the constructor is marked @internal. So I’d have to create a decorated TableDiff as well.
This approach would mean, if in a future version any of the classes SchemaManager, Comparator or TableDiff get a new method, it will fail. (Because there are no interfaces for these 3 classes)
Is there a better way to do this?
I basically just want to disable Comparator::detectRenamedColumns() so that it never detects a rename of a column.
Is there a better way to do this? I basically just want to disable
Comparator::detectRenamedColumns()so that it never detects a rename of a column.
We could make that configurable. Detecting renames is fuzzy, so the requirement to disable that feature seems reasonable to me. Up for a PR?
Yes. However, we could think about backporting the API to 3.x if that makes the upgrade from 3.x to 4.x easier. But let's target 4.1.x first.
@ausi why do you want to avoid detecting column renames? And would the problem you have disappear with https://github.com/doctrine/dbal/pull/6280 ?
My PR does not remove the rename detection even if an explicit renaming has been provided (maybe it should?) but if a column is explicitly renamed then those 2 particular columns shouldn't be taken into account during the detection
why do you want to avoid detecting column renames?
We (Contao) use the doctrine schema manager to update the database schema after updates or after installing an extension. In this case the user can not alter the result of the comparator (like it would be possible using doctrine migrations) so if an incorrect rename was detected there would be no way to intervene. (Most users would probably also not recognise the wrong rename). We had many issues with this in the past (see also #3661) but we since used the workaround as outlined in https://github.com/doctrine/dbal/issues/6299#issue-2121075690 and this worked flawlessly ever since.
And would the problem you have disappear with #6280 ?
No, as the rename detection is still in place and there is no option to disable it. But I think #6280 is a very good feature as it makes intentional renames possible 👍
Since the detection is made from dropped columns and added columns of the same type, I'm thinking of an alternative, which would be to tag dropped columns, when doing dropColumn, as excluded from detection with a flag on Column (which already has a list of options available)
… I'm thinking of an alternative, which would be to tag dropped columns, when doing
dropColumn, as excluded from detection with a flag on Column …
The problem is we never do a manual dropColumn. We simply compare the live schema with the desired one like $comparator->compareSchemas($schemaManager->introspectSchema(), $schemaTool->getSchemaFromMetadata(…)) and need to make sure this never results in any renames.
(in our case, intentional renames would be done “manually” before the schema update)
You could still iterate over the tables and columns in the old schema to set the flag
Or an in between, we add a configuration in the SchemaConfig of Table so the iteration is only on the tables
Otherwise I don't have anything against the comparator config but I do think it should indeed be as a state to limit the signature changes, the comparator class is not final and only the constructor is internal, so it's still allowed to be extended it seems which means a signature change of public or protected methods would indeed be a breaking change
Up for a PR?
Done: #6300
Addressed all review comments there, so this should be ready. 🎉