Comparator miss-detects custom columns as changed

[DASPRiD]

While working a little with custom types in Doctrine lately, I recently noticed a bug in the Comparator, although its roots are deeper down. Consider having a custom type like this:

class CustomType extends Type
{
    public function getSQLDeclaration(array $fieldDeclaration, AbstractPlatform $platform)
    {
        return $platform->getBinaryTypeDeclarationSQL([
            'length' => '16',
            'fixed' => true,
        ]);
    }

    public function convertToDatabaseValue($value, AbstractPlatform $platform)
    {
        // …
    }

    public function convertToPHPValue($value, AbstractPlatform $platform)
    {
        // …
    }

    public function requiresSQLCommentHint(AbstractPlatform $platform)
    {
        return true;
    }

    public function getName()
    {
        return 'CustomType';
    }
}

Now, when using that type in an entity, at first when generating the database SQL and inserting it, everything is fine. The column has the correct type (binary, fixed 16) and a comment denoting the "CustomType" type.

The problem now is when asking the schema-tool for a diff. Even though the column using that type wasn't changed, the schema-tool suggests an ALTER TABLE statement, changing the column to exactly what it already is.

I looked a bit into this, and it seems that the problem is that the Schema generated from the mapping files does not include the columns comment, length, fixed status and also that the type in the Schema coming from the DB has Binary as type, instead of CustomType. This lets the Comparator think it needs to update the column.

This problem looks rather complex to me, and I have no real idea on how to solve this, maybe someone else who has a better insight can analyse this further.

[deeky666]

Which database / platform are you using?

[DASPRiD]

This does not depend on any specific platform, it applies to all (since the bug is coming from the XML/annotation schema side).

[deeky666]

Can you please provide your exact ORM mapping and custom type implementation? Then I can have a closer look. Thanks.

[DASPRiD]

Sure, there you go:

<?xml version="1.0" encoding="UTF-8"?>
<doctrine-mapping xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns="http://doctrine-project.org/schemas/orm/doctrine-mapping" xsi:schemaLocation="http://doctrine-project.org/schemas/orm/doctrine-mapping http://doctrine-project.org/schemas/orm/doctrine-mapping.xsd">
    <entity name="SomeEntity">
        <id name="id" type="integer"><generator strategy="AUTO"/></id>
        <field name="locale" type="localeType"/>
    </entity>
</doctrine-mapping>

And here's the type:

<?php
use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Types\StringType;

class LocaleType extends StringType
{
    public function getSQLDeclaration(array $fieldDeclaration, AbstractPlatform $platform)
    {
        return $platform->getVarcharTypeDeclarationSQL([
            'length' => '5',
            'fixed' => true,
        ]);
    }

    // This type would also override covnertToDatabaseValue() and convertToPHPValue(), to handle
    //  a specific "Locale" class, but that is not relevant to this issue.
}

[DASPRiD]

By the way, I've got a PR in to work around this issue:

doctrine/orm#5896

[deeky666]

Yeah I think the issue here is that you cannot hard code properties of the custom type into the type SQL declaration because then the comparator cannot use them for comparison and thus detects changes. The comparator is not comparing SQL but Column objects. You will need to define the length and fixed attribute in your mapping I suppose. I see no other way to avoid unnecessary diffs.

Hope that helps.

[deeky666]

Another option you could try is to not derive your type from StringType but from Type instead because then the comparator should not compare length and fixed attributes. This way "hard coding" those attributes in the SQL declaration could work.

[DASPRiD]

Nope, it will always compare all the mapping options. For a solution, see my PR I linked.

[deeky666]

I doubt what you say is true because the comparator clearly shows it:
https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Schema/Comparator.php#L447-L467

To get any further here I would suggest you to PR a failing test case which reveals the issue, then things might get clearer.

[DASPRiD]

Oh, you are right about that, dunno how I could overlook that. Although that leaves a problem that it wouldn't detect a change when you extend Type directly and then later change the definition in the custom type.

[deeky666]

That is true, but custom types unfortunately have some limitations atm. So you will need to go either way... We cannot do much about it without completly rewriting the whole type system :( Every change is rather fragile and cause additional issues so we tend to touch it as little as possible.

[deeky666]

Also note that while Doctrine migrations are rather popular, it still is not intended to be a full feature complete production migration tool (IMO) and should only be used for development purposes. There will always be some compromises.

[morozov]

This will be fixed by #4746:

$desiredTable = new Table('test', [
    new Column('id', new class extends Type
    {
        public function getSQLDeclaration(array $fieldDeclaration, AbstractPlatform $platform)
        {
            return $platform->getBinaryTypeDeclarationSQL([
                'length' => 16,
                'fixed' => true,
            ]);
        }

        public function requiresSQLCommentHint(AbstractPlatform $platform)
        {
            return true;
        }

        public function getName()
        {
            return 'CustomType';
        }
    })
]);

$sm = $connection->createSchemaManager();
$sm->dropAndCreateTable($desiredTable);
echo $connection->fetchNumeric('SHOW CREATE TABLE test')[1], PHP_EOL;

// CREATE TABLE `test` (
//   `id` binary(16) NOT NULL
// ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb3 COLLATE=utf8_unicode_ci

$actualTable = $sm->listTableDetails('test');

$comparator = $sm->createComparator();
$diff = $comparator->diffTable($actualTable, $desiredTable);
var_dump($diff);
// bool(false)

[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.