Schema diff changes since version 3.3.1

[dmaicher]

Bug Report

Q	A
BC Break	yes?
Version	3.3.1

Summary

SInce 3.3.1 I have schema changes that I did not have before.

Current behaviour

app/console doctrine:schema:validate

Database
--------

                                                                                                                        
 [ERROR] The database schema is not in sync with the current mapping file.                                              

Running app/console doctrine:schema:update --dump-sql mostly shows changes related to ENUM column definitions like

ALTER TABLE my_entity CHANGE foo foo ENUM('foo', 'bar') NOT NULL;

However: the generated SQL is identical with the actual column definition in MySQL. So I cannot get rid of the diff.

Might be related to #5220 ?

How to reproduce

Add an ORM entity with a ENUM columnDefinition like

class MyEntity
{
     /**
     * @ORM\Column(
     *     name="foo",
     *     type="string",
     *     length=0,
     *     columnDefinition="ENUM('foo', 'bar') NOT NULL"
     * )
     */
    private $foo;
}

And map enum to string like

doctrine:
    dbal:
        connections:
            default:
                driver:   pdo_mysql
                charset:  UTF8
                mapping_types:
                    enum: string

See https://www.doctrine-project.org/projects/doctrine-orm/en/2.11/cookbook/mysql-enums.html#solution-1-mapping-to-varchars

Expected behaviour

There should not be any schema diff.

[greg0ire]

Might be related to #5220

Yes, you are now using platform-aware schema comparison, which was meant to be introduced in 3.2.0 and probably has its own bugs (so #5220 did not really introduce a new bug, but exposed latent bugs instead).

[bcremer]

It's related to the new platform-aware schema comparison.

Currently it seems to be impossible to have a custom columnDefinition without having a false positive in the schema diff.

Repro

<?php

$connection->executeStatement('DROP TABLE IF EXISTS `repro`');
$connection->executeStatement(<<<'SQL'
    CREATE TABLE `repro` (
    `product` enum('hotel','homes') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL DEFAULT 'hotel' COMMENT 'Embeddable: Product'
    ) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci
    SQL);

$desiredTable = new Table('repro', [
    new Column(
        'product',
        new StringType(),
        [
            'length' => 0,
            'notnull' => true,
            'default' => 'hotel',
            'comment' => 'Embeddable: Product',
            'columnDefinition' => <<<'DEF'
                enum('hotel','homes') CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL DEFAULT 'hotel' COMMENT 'Embeddable: Product'
                DEF,
        ]
    ),
]);

$sm = $connection->createSchemaManager();
$actualTable = $sm->listTableDetails('repro');

if (method_exists($sm, 'createComparator')) {
    $comparator = $sm->createComparator();
} else {
    $comparator = new Comparator();
}

$diff = $comparator->diffTable($actualTable, $desiredTable);


if ($diff !== false) {
    $schemaDiff = new SchemaDiff([], [$diff]);
    var_dump(
        $schemaDiff->toSql($connection->getDatabasePlatform())
    );
} else {
    var_dump("no diff");
}

I was able to trace it down to \Doctrine\DBAL\Platforms\AbstractPlatform::columnsEqual

        if (
            $this->getColumnDeclarationSQL('', $this->columnToArray($column1))
            !== $this->getColumnDeclarationSQL('', $this->columnToArray($column2))
        ) {
            return false;
        }

The column from the fromSchema or $actualTable in this example does not contain a value for getColumnDeclarationSQL and thus there is always a diff present when columnDefinition was used in a schema definition.

[greg0ire]

If I understood correctly, you're saying that the buggy part is $sm->listTableDetails('repro'). If yes, let's remove the rest from the equation and simplify the reproducer , we can even have it rely on $sm->listTableColumns('repro'), which is called by listTableDetails. It seems weird that column_definition would be needed at all by schema introspection, I'd expect it to be useful for desiredTable only.

The SQL executed to retrieve the list of columns is here I think:

dbal/src/Platforms/AbstractMySQLPlatform.php

Lines 338 to 346 in 5b6eb6c

	public function getListTableColumnsSQL($table, $database = null)
	{
	return 'SELECT COLUMN_NAME AS Field, COLUMN_TYPE AS Type, IS_NULLABLE AS `Null`, ' .
	'COLUMN_KEY AS `Key`, COLUMN_DEFAULT AS `Default`, EXTRA AS Extra, COLUMN_COMMENT AS Comment, ' .
	'CHARACTER_SET_NAME AS CharacterSet, COLLATION_NAME AS Collation ' .
	'FROM information_schema.COLUMNS WHERE TABLE_SCHEMA = ' . $this->getDatabaseNameSQL($database) .
	' AND TABLE_NAME = ' . $this->quoteStringLiteral($table) .
	' ORDER BY ORDINAL_POSITION ASC';
	}

A great next step my be to open a PR with a failing test, and tests/Functional/Schema/MySQLSchemaManagerTest.php might be the right place to write it, since this bug has only be observed with ENUM() for now.

[bcremer]

@greg0ire I put together a quick PR to verify the high level solution here:
#5224

[greg0ire]

@bcremer I must have understood wrong then, because you don't seem to be fixing SchemaManager::listTableColumns()… I'm not saying that it is broken, but so far that is my understanding. I'd expect the column from $actualTable to not be empty.

[bcremer]

\Doctrine\DBAL\Schema\AbstractSchemaManager::listTableColumns calls \Doctrine\DBAL\Schema\AbstractSchemaManager::_getPortableTableColumnList which calls the abstract method \Doctrine\DBAL\Schema\AbstractSchemaManager::_getPortableTableColumnDefinition which is implemeted by \Doctrine\DBAL\Schema\MySQLSchemaManager::_getPortableTableColumnDefinition

I'm not yet positive if that's really a road to go for the fix. columnDefinition is also used in \Doctrine\DBAL\Platforms\AbstractPlatform::getColumnDeclarationSQL.

We might need an extra options field like platformColumnDefinition which can be distinguished by the manually provided columnDefinition.

[greg0ire]

@morozov hi! Maybe you have some insight to provide about this? I'm afraid I'm too unfamiliar with how this works to give a good piece of advice on this.

[morozov]

I don't believe schema migrations could ever work correctly when the column definition is specified manually. See #3647 (comment).

When I run the above example on top of bare DBAL, I get the following error:

Uncaught Doctrine\DBAL\Exception: Unknown database type enum requested, Doctrine\DBAL\Platforms\MySQL80Platform may not support it

I assume it's the ORM that defines the ENUM type.

If, for the sake of testing, I replace enum('hotel','homes') with varchar(32) both in the table DDL and the column definition, I get the following diff:

- VARCHAR(32) CHARACTER SET utf8mb4 DEFAULT 'hotel' NOT NULL COLLATE `utf8mb4_unicode_ci` COMMENT 'Embeddable: Product'
+ varchar(32) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL DEFAULT 'hotel' COMMENT 'Embeddable: Product'

While both definitions are equal semantically, there are differences in their syntax:

1. The order or constraints in the declaration.
2. Casing of the keywords.
3. Escaping of identifiers.

If I take the DDL that the comparator expects and put it into the column definition, the diff disappears.

If I understand the issue correctly, I don't believe there will be a better solution than to tweak the column definition to the comparator's expectations.

[bcremer]

When I run the above example on top of bare DBAL, I get the following error:

The enum type was mapped to string like it's described in https://www.doctrine-project.org/projects/doctrine-orm/en/2.11/cookbook/mysql-enums.html#solution-1-mapping-to-varchars

$conn->getDatabasePlatform()->registerDoctrineTypeMapping('enum', 'string');

If I understand the issue correctly, I don't believe there will be a better solution than to tweak the column definition to the comparator's expectations.

That's what is not possible anymore right now due to the following condition in \Doctrine\DBAL\Platforms\AbstractPlatform::columnsEqual:

        if (
            $this->getColumnDeclarationSQL('', $this->columnToArray($column1))
            !== $this->getColumnDeclarationSQL('', $this->columnToArray($column2))
        ) {
            return false;
        }

$column1 is the column generated from the real database schema by the SchemaManager

$sm = $connection->createSchemaManager();
$actualTable = $sm->listTableDetails('repro');
$actualTable->getColumn('product')->getColumnDefinition(); // null

$column2 is the column generated by the entities metadata/annotation

$desiredTable->getColumn('product')->getColumnDefinition(); // string(126) "enum('hotel','homes')[...]

So the comparison will always fail when a columnDefinition is used on an entity.

[aprat84]

Since I updated to 3.3.1 I get some weird migrations when using diff command. The up method is always empty, and the down method looks like following:

final class Version20220201154458 extends AbstractMigration
{
    public function up(Schema $schema): void
    {
        // this up() migration is auto-generated, please modify it to your needs
    }

    public function down(Schema $schema): void
    {
        // this down() migration is auto-generated, please modify it to your needs
        $this->addSql('
            ALTER TABLE Account 
                CHANGE name name VARCHAR(100) DEFAULT NULL COLLATE `utf8mb4_0900_ai_ci`, 
                CHANGE email email VARCHAR(100) DEFAULT NULL COLLATE `utf8mb4_0900_ai_ci`
        ');
}

This is just an example, but I get the ALTER TABLE for every table that has a column with collation.

My database is already using utf8mb4_0900_ai_ci collation everywhere. This is some of the output for SHOW FULL COLUMNS FROM Account:

Field	Type	Collation	Null	Key	Default	Extra
name	varchar(100)	utf8mb4_0900_ai_ci	YES		NULL
email	varchar(100)	utf8mb4_0900_ai_ci	YES	UNI	NULL

Using mysql 8.0.26

Edit: found what was the problem!

There's a config name mismatch, for the table default collation, I think.

Documentation for DoctrineBundle says that config name is collate. DoctrineBundle

And AbstractMySqlPlatform also uses this name. src/Platforms/AbstractMySQLPlatform.php:456

But, MySQL comparator uses collation as config name. src/Platforms/MySQL/Comparator.php:36

So, which is the correct one?
If I change my config files to this, it works as expected. So it seems collation it is!

default_table_options:
    charset: utf8mb4
    collation: utf8mb4_0900_ai_ci
    engine: InnoDB

PS: I'm using these options until DBAL v4, where if I'm not mistaken, charset and collation will not forced. (#4644)

[greg0ire]

@aprat84 are you maybe experiencing #5214 ?

[aprat84]

Exactly! That's a better explanation of what I exposed. Sorry, should have searched a bit more...

But, this only happened to my when updated to 3.3.1. When at 3.2.1 it worked 🤷‍♂️

[bcremer]

I'm currently using #5224 to workaround this.

In my opinion there are two possible solutions to handle a columnDefinition during schema comparison:

• Obtain the "real" type definition from the schema (CREATE SHOW TABLE ...) via SchemaManager and compare that against the columnDefinition if defined. In that case skip comparison of other portable options like nullable, lengths etc..
  For that we might need to introduce a dedicated field like schemaColumnDefinition to distinguish a colum generated by the schemamanager from a "desired" column definition.

• Ignore columnDefinition for the column comparison but validate the other portable options (Ignore columnDefinition in schema comparison #5224). The columnDefinition will still be used to generate the diff-statements if a column is missing etc.

[Ocramius]

Potential fix @ #5234

In practice, killing platform-aware schema diffing works wonders there :D

[greg0ire]

I think we should go with #5224, and then if somebody feels like it, they can work on your first solution @bcremer . It seems a lot more involving but if I understood correctly, columnDefinition was not well supported already so…

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.