What to use when Doctrine\DBAL\Types\Type::requiresSQLCommentHint() was removed in DBAL 4

[michnovka]

Q	A
Version	4.0.0

Support Question

In DBAL 4 the requiresSQLCommentHint() function was removed. How do I let Doctrine know that any special DB column is of that specific custom type?

[arokettu]

That also breaks the Solution 2 for the MySQL enums
https://www.doctrine-project.org/projects/doctrine-orm/en/3.1/cookbook/mysql-enums.html#solution-2-defining-a-type

[jifer]

In version 3 it has deprication

Deprecation::triggerIfCalledFromOutside(
    'doctrine/dbal',
    'https://github.com/doctrine/dbal/pull/5509',
    '%s is deprecated.',
    __METHOD__,
);

But NOT using it on custom types equals to SchemaTool freak outs.

For example lets create such simple custom type without requiresSQLCommentHint:

final class CustomerOrderIdType extends Type
{
    public function getSQLDeclaration(array $column, AbstractPlatform $platform): string
    {
        $column['length'] = 9;
        return $platform->getStringTypeDeclarationSQL($column);
    }

    public function getName(): string
    {
        return 'customer_order_id';
    }
}

This is result for doctrine:migration:diff command:

final class Version20240514183655 extends AbstractMigration
{
    public function getDescription(): string
    {
        return '';
    }

    public function up(Schema $schema): void
    {
        $this->addSql('ALTER TABLE customer_order ALTER id TYPE VARCHAR(9)');
        $this->addSql('ALTER TABLE customer_order ALTER id TYPE VARCHAR(9)');
    }

    public function down(Schema $schema): void
    {
        $this->addSql('ALTER TABLE customer_order ALTER id TYPE VARCHAR(9)');
    }
}

1. Schema Tool try to change what is already corect.
2. There are duplicated ALTERS in up method.
3. down & up queries try to do exact same thing.

Now imagine whole database having like 50 references to table customer_order. This means diff always showing 150 queries to execute even if nothing should be changed.

So question is: how we should resolve this situation?

[greg0ire]

@jifer what you're experiencing sounds like a DBAL bug. Can you please try to reproduce it as a DBAL test case, using the DBAL schema comparison API? Note that SchemaTool and doctrine/migrations are 2 different things (SchemaTool is part of the ORM package). I would expect you to get a migration that removes the SQL comment. Here is where said API is used, in case you want to debug it yourself: https://github.com/doctrine/migrations/blob/704de5e6e470a8031aadee7a242913a3222db6d5/lib/Doctrine/Migrations/Provider/DBALSchemaDiffProvider.php#L44

[jifer]

@greg0ire Thank you for your reply.

I feel like this issue is not new at all and consequences of removing this method were known.
#5107

---

Going into some details.
requiresSQLCommentHint was deprecated without clear message how to deal with it.

Referenced github topic in deprecation message is more than confusing as there is no mention how to deal with it and whats the replacement for it.
We try to deal with it by searching how to make it work without using requiresSQLCommentHint. While proper solutions is "do nothing"! Leave it as is, override method, return true and everything will work as intended. When you will upgrade to 4 just remember to remove this unused methods in your custom types and create migration to remove comments not used anymore.

In DBAL 3:

// \Doctrine\DBAL\Platforms\AbstractPlatform::columnsEqual
public function columnsEqual(Column $column1, Column $column2): bool
{
    // [..]

    // false - declarations are same.
    if (
        $this->getColumnDeclarationSQL('', $column1Array)
        !== $this->getColumnDeclarationSQL('', $column2Array)
    ) {
        return false;
    }

    // false - platform (postgresql) does not support that.
    if ($this->supportsInlineColumnComments()) {
        return true;
    }

    // false -- comment are equal (null & null)
    if ($column1->getComment() !== $column2->getComment()) {
        return false;
    }

    // false - types differ
    // $column1 is recognized as \Doctrine\DBAL\Types\StringType [without comment with type info on column reverse mapping can only guess base mapping types ]
    // $column2 is recognized as \App\CustomType because it is taken from ORM definition.
    // So even if column declaration equals and comment equals this indicates that column should be updated.
    return $column1->getType() === $column2->getType();
}

In 4.0+ Type comparison was removed.

public function columnsEqual(Column $column1, Column $column2): bool
{
    $column1Array = $this->columnToArray($column1);
    $column2Array = $this->columnToArray($column2);

    // ignore explicit columnDefinition since it's not set on the Column generated by the SchemaManager
    unset($column1Array['columnDefinition']);
    unset($column2Array['columnDefinition']);

    if (
        $this->getColumnDeclarationSQL('', $column1Array)
        !== $this->getColumnDeclarationSQL('', $column2Array)
    ) {
        return false;
    }

    // If the platform supports inline comments, all comparison is already done above
    if ($this->supportsInlineColumnComments()) {
        return true;
    }

    return $column1->getComment() === $column2->getComment();
}

It doesn't feel like bug but rather miscommunication or lack of proper information that we should just ignore that requiresSQLCommentHint is deprecated and we should not do anything about it till we upgrade to DBAL 4.

[greg0ire]

Can you try using the setting implemented in doctrine/DoctrineBundle#1714 ?

[jifer]

@greg0ire Good you mentioned this. I already did and this does not affect result (if excluding comment removal from columns) because dbal assumes there is no comment on both sides:

// false -- comment are equal (null & null)
if ($column1->getComment() !== $column2->getComment()) {
    return false;
}

and gos straight to return $column1->getType() === $column2->getType(); which differ.

So everything would work as intended (probably) if we add this temporary flag in columnsEqual method like this:

return $this->disableTypeComments || $column1->getType() === $column2->getType();

Ignore type checking (like in DBAL 4.0) if disableTypeComments is true.

[greg0ire]

What do you mean, it's not supported? I literally pointed you to the MR that implements it, didn't I? 🤔

and gos straight to return $column1->getComment() === $column2->getComment(); which differ.

You just said they were equal. Are you in fact referring to $column1->getType() === $column2->getType()

Anyway, I think your idea might not work: what if the type differ not because of the comment? Like the basic type int, and the basic type string?

[jifer]

My apologies.
I went straight to doctine/dbal PR implementing that flag instead of DoctrineBundle, so yeah this is implemented.
I also copied wrong code fragment.
Really sorry for that mess. I've updated previous post.

---

You probably are also right about type comparison. But if so, how 4.0 recognize type changes if method columnsEqual does not check if Type class is different - this was removed in 4.0 entirely?
There is already check if type was changed few lines earlier:

if (
        $this->getColumnDeclarationSQL('', $column1Array)
        !== $this->getColumnDeclarationSQL('', $column2Array)
    ) {
        return false;
    }

If SQL declaration is exactly the same nothing should be changed in schema, right?
From schema update perspective it doesn't matter if column sql declaration is dictated by TypaA class or TypeB class.
So it goes like this:
"Columns equals if column sql declaration and comment are the same."
Not like this:
"Columns equals if column sql declaration, comment and Type::class are the same".

Here again whole method form 4.0:

public function columnsEqual(Column $column1, Column $column2): bool
{
    $column1Array = $this->columnToArray($column1);
    $column2Array = $this->columnToArray($column2);

    // ignore explicit columnDefinition since it's not set on the Column generated by the SchemaManager
    unset($column1Array['columnDefinition']);
    unset($column2Array['columnDefinition']);

    if (
        $this->getColumnDeclarationSQL('', $column1Array)
        !== $this->getColumnDeclarationSQL('', $column2Array)
    ) {
        return false;
    }

    // If the platform supports inline comments, all comparison is already done above
    if ($this->supportsInlineColumnComments()) {
        return true;
    }

    return $column1->getComment() === $column2->getComment();
    
    //
    // There is no $column1->getType() === $column2->getType() check in 4.0 which is present in dbal 3.0
    //
}

Still, rookie like me can miss something ;)

[greg0ire]

But if so, how 4.0 recognize type changes if method columnsEqual does not check if Type class is different - this was removed in 4.0 entirely?

🤔 I think you're the one who's right now. Please send a PR with return $this->disableTypeComments || $column1->getType() === $column2->getType();.

[jifer]

Holy... I completely forgot about that!
@SenseException Thank you for this fix & your contribution.
@greg0ire Thank you for support and conversation.

With regards.

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