Unmark SchemaDiff/TableDiff/ColumnDiff as final or provide an interface

[VincentLanglet]

Feature Request

What

The PR #6923 mark Diff classes as final but do not provide/use Interface for these which blocks the ability to extends those classes. (cc @greg0ire @morozov)

Why

I wrote my own custom Comparator which allows to generate PgSql constraint check like

ALTER TABLE foo ADD CONSTRAINT foo_check CHECK (tone IN (\'foo\', \'bar\', \'baz\'))

This need

• to extends the Diff classes in order to add more data in them
• to override the isEmpty method of those Diff classes.

How

One solution would be to

• Remove @final on those classes
• Introduce an interface for these
• Changing the param/return type in 5.0
• Re-add @final.

Another solution would be to

• Remove @internal to the constructor
• Add a new param like extraChanges
• Updating the isEmpty method to check extraChanges too

[morozov]

These diff classes should be thought of as value objects. Could you provide an example of a use case where it needs to be extended?

[VincentLanglet]

Could you provide an example of a use case where it needs to be extended?

Sure,
As quickly explained in the first message the purpose is to have doctrine:migration:diff generating automatically the sql to add/drop/update constraint_checks on postgreSQL for enum field ; like

ALTER TABLE foo ADD CONSTRAINT foo_tone_check CHECK (tone IN (\'foo\', \'bar\', \'baz\'))

I have

• My own Driver extending AbstractDriverMiddleware in order to override getDatabasePlatform.
• A Middleware implementing MiddlewareInterface to wrap the Driver
• My own platform extending PostgreSQLPlatform with
  • Some extra logic in _getCreateTableSQL
  • Some extra logic in getAlterTableSQL
  • An override of createSchemaManager
• My own SchemaManager extending PostgreSQLSchemaManager to override createComparator
• My own Comparator extending the one from doctrine to override compareTables

Then, when comparingTables I found the following solution.
I used an extended TableDiff which has all the existing property

        private readonly Table $oldTable,
        private readonly array $addedColumns = [],
        private readonly array $changedColumns = [],
        private readonly array $droppedColumns = [],
        private array $addedIndexes = [],
        private readonly array $modifiedIndexes = [],
        private array $droppedIndexes = [],
        private readonly array $renamedIndexes = [],
        private readonly array $addedForeignKeys = [],
        private readonly array $modifiedForeignKeys = [],
        private readonly array $droppedForeignKeys = [],

but also an addedConstraintChecks, changedConstraintChecks and droppedConstraintChecks.

With some code like

$diff = parent::compareTables($oldTable, $newTable);

// someLogic

return new MyTableDiff(
     $diff->getOldTable(),
     $diff->getAddedColumns(),
     // ...
     $addedConstraintChecks,
     $changedConstraintChecks,
     $droppedConstraintChecks,
);

Which would requires

• TableDiff to not be final in order to extends it
• Removing the @internal on the constructor to instantiate such class in my Comparator (the comment say "The diff can be only instantiated by a {@see Comparator}." but it shouldn't be forbidden to have a custom comparator)

[morozov]

It looks like this logic belongs to the library, not the application.

If a class is not final, we cannot touch its protected API between major releases because it would be considered a breaking change. That's why I want to be careful and clear about which APIs we do and do not want to be extended. In this specific case, the diffs are the means of communication between the comparator and platform. Unless you are building the support for a new platform, the diff classes are not supposed to be modified.

[VincentLanglet]

If a class is not final, we cannot touch its protected API between major releases because it would be considered a breaking change.

I do understand this point ; I maintain lib too.
It's often useful to work with interface then in such situation.

It looks like this logic belongs to the library, not the application.

Doctrine/libs are doing there best to provide abstraction from the database and I don't think it's the purpose of doctrine/dbal to provide a more complex Platform

It's the same idea than libs like https://github.com/beberlei/DoctrineExtensions which are imho too database-specific to be into the doctrine core libs.

I'm not doing this platform/comparator/diff logic in my application ; I wrote a specific bundle for this.
When mature enough it could be an open-source lib.

In this specific case, the diffs are the means of communication between the comparator and platform. Unless you are building the support for a new platform, the diff classes are not supposed to be modified.

This is basically saying "Sorry but you cannot build support a new platform in your own lib because the constructor is internal" and "Sorry but you cannot add extra feature to the Comparator since diff are finals". :/

A nice easy way to solve my issue would be to add something like

TableDiff::setExtraMetadata(array $metadata);
TableDiff::getExtraMetadata(): array;

and changing the method isEmpty to

public function isEmpty(): bool
    {
        return count($this->addedColumns) === 0
            && count($this->changedColumns) === 0
            && count($this->droppedColumns) === 0
            && count($this->addedIndexes) === 0
            && count($this->modifiedIndexes) === 0
            && count($this->droppedIndexes) === 0
            && count($this->renamedIndexes) === 0
            && count($this->addedForeignKeys) === 0
            && count($this->modifiedForeignKeys) === 0
            && count($this->droppedForeignKeys) === 0;
            && count($this->extraMetadata) === 0;
    }

Would it be something acceptable @morozov ?

[VincentLanglet]

If both #7034 and #7035 are accepted, I found another implementation which does not require to extends Diff classes @morozov @greg0ire.

Dunno if it could be ok for you :)