DBAL-1096: schema-tool:update does not understand columnDefinition correctly

[doctrinebot]

Jira issue originally created by user benjamin:

This issue is similar to the old DDC-625 and to DDC-1657, but its scope is limited to columns declared with columnDefinition.

Take this entity:

class Faq
{
    /****
     * @ORM\Column(type="integer", columnDefinition="TINYINT(3) UNSIGNED NOT NULL")
     *
     * @var integer
     */
    private $position;
}

The command doctrine orm:schema-tool:update --dump-sql generates the following output:

ALTER TABLE Faq CHANGE position position TINYINT(3) UNSIGNED NOT NULL;

Even though the column is already declared this way. If I manually execute this query, and run the tool again, I get the same output.

I'm using Doctrine 2.4.7 with PHP 5.5.12 and MySQL 5.6.17.

[doctrinebot]

• duplicates DBAL-167: Schema comparator doesn't work properly with columnDefinition's
• relates to DDC-1657: The Doctrine cli tool does not handle schema correctly.
• relates to DDC-625: orm:schema-tool:update --dump-sql showing SQL when DB is up-to-date

[doctrinebot]

Comment created by @Ocramius:

The schema tool can't and won't handle diffs containing custom columnDefinition.

[doctrinebot]

Issue was closed with resolution "Can't Fix"

[doctrinebot]

Comment created by benjamin:

@Ocramius Can you tell me why? Can't it at the very least compare them as strings, to avoid repeating endlessly the same queries?

[doctrinebot]

Comment created by @Ocramius:

That's exactly the problem: string comparison won't work in these cases, because of minor mismatches and because the column definition may include special markers, comments, definitions and so on in scrambled order.

This limitation is also documented in http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/annotations-reference.html#column

Handling it goes beyond the scope of the schema comparator, as the complexity to deal with it is not worth the effort.

[doctrinebot]

Comment created by benjamin:

While I fully understand these concerns, it's really disappointing to have to either stick to default, unoptimized data types such as INT when you only need a 8- or 16-bit integer, or to have to give up the command-line schema validation tool that will always go red.

The same issue applies when using custom data types such as Geometry or LocalDate, even when using supported SQL declarations.
This restricts a lot what you can do with the ORM while keeping the convenience of the schema tools!

I may have a few ideas to try out to make this work; if the core team is not 100% opposed to the concept, I could open a PR with a proof of concept at some point.

[doctrinebot]

Comment created by @Ocramius:

Fine tuning is the job of a DBA, not of the schema-tool. The schema-tool is by far a silver bullet.

[doctrinebot]

Comment created by @deeky666:

[~benjamin] if you find a way to make custom columnDefinitions comparable in a reasonable way, go ahead, make a contribution. It's not that we are totally against it, we just didn't find a good solution so far to make it work in a usable way. By the way, this is actually a DBAL issue, moving.

[doctrinebot]

Comment created by stof:

Note that for custom data types, the SchemaTool supports comparing them if you use custom DBAL types to map them instead of using an existing type and adding a customDefinition property in it (the result might even be a better integration)

[doctrinebot]

Comment created by benjamin:

@Christophe Hmm I'm going to play a bit with this, but the LocalDate custom type I mentioned above, even though it uses $platform->getDateTypeDeclarationSQL(), is not properly handled by the SchemaTool either (it also endlessly suggests the same DDL)!

[doctrinebot]

Comment created by @deeky666:

[~benjamin] try making your custom type a commented type. This should make the schema manager be able to distinguish your custom type from the default DateType.
Override the method requiresSQLCommentHint() in your custom type class and let it return true.

[doctrinebot]

Comment created by benjamin:

[~deeky666] It works indeed with requiresSQLCommentHint(), I didn't know about that, thank you.

As far as I can see, the commented types are not checked by the SchemaTool: if I manually change a commented column type in the database, schema-tool:update won't offer me to fix it.

Could we do the same thing for columns with a columnDefinition? i.e. comment them with DC2Type:integer or DC2Type:string or whatever the native type is, and have the SchemaTool skip them?

[doctrinebot]

Comment created by @deeky666:

[~benjamin] actually it is the DBAL schema manager that is extracting the custom type name from the column's comment and then mapping it accordingly.
For instance if your custom type is called "money" (which is based on the native "decimal" type for example), DBAL will add the string "(DC2Type:money)" to the column's comment on creation/update. On reverse engineering the column from the database the schema manager will first detect the column as "decimal" (native type), then check for the custom type string in the comment and convert it into the "money" type.
This should work out of the box but is processed by DBAL, not ORM's schema tool. The schema tool just utilizes DBAL's schema manager here.
Speaking of custom columnDefinition your idea sounds reasonable to me and I think to remember that something similar has been proposed in the past. However to make matching and extraction easier we should hash the columnDefinition string somehow.

[doctrinebot]

Comment created by benjamin:

[~deeky666] Including a hash of the column definition in the DC2Type comment is a good idea, however it would only detect when you update the definition in the entity, not when you update the column type in the database manually.

I would be happy to work on this suggestion anyway, but I'm not sure where to start as I'm not comfortable with the internals of the schema manager.

[doctrinebot]

Comment created by @deeky666:

[~benjamin] of course we would have to make a compromise here. Comparing the column declaration SQL coming from the database with an arbitrary column declaration string from the mapping is not safe and also impossible considering that you usually use the columnDefinition to define vendor specific semantics for the column that DBAL does not/cannot support.
Implementing this feature would be similar to the commented type one.

Places to start:

https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Schema/AbstractSchemaManager.php#L1085-L1112
https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Schema/MySqlSchemaManager.php#L129-L132 (needs to be adopted for all schema managers)
https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Platforms/AbstractPlatform.php#L497-L525

Those should be the important parts to adopt. Feel free to provide a PR :)

[doctrinebot]

Comment created by @deeky666:

Related: DBAL-167

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