Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrations always generated for custom type with DBAL 4 #1441

Open
michnovka opened this issue Jul 3, 2024 · 76 comments
Open

Migrations always generated for custom type with DBAL 4 #1441

michnovka opened this issue Jul 3, 2024 · 76 comments

Comments

@michnovka
Copy link

michnovka commented Jul 3, 2024

Bug Report

Q A
BC Break no
Migrations version 3.8.0
Migrations bundle 3.3.1
ORM 3.2.1
DBAL 4.0.4
Symfony 7.1.2

Summary

I have issue with DBAL 4 and custom type, the migrations keep getting generated again and again. Furthermore, the down migration looks totally bogus. This is possibly related to #1435

I know that DBAL 4 dropped requiresSqlHint (in doctrine/dbal#5107 , afterwards some issues were found and fixed - doctrine/dbal#6257 )

So when I am using custom type, I expect the first migration diff to drop the DC2Type comments. However my tables have these fields already dropped and yet the migration is being generated.

enum ActionType: string
{
    case SUBMIT = 'submit';
    case CANCEL = 'cancel';
}

class ActionTypeType extends Type
{
    public function getSQLDeclaration(array $column, AbstractPlatform $platform): string
    {
        $class = ActionType::class;

        $values = [];

        foreach ($class::cases() as $val) {
            $values[] = "'{$val->value}'";
        }

        return "enum(" . implode(", ", $values) . ")";
    }

    public function convertToDatabaseValue($value, AbstractPlatform $platform): mixed
    {
        $class = ActionType::class;
        if ($value !== null && !($value instanceof BackedEnum)) {
            $value = $class::tryFrom($value);
        }else{
            return null;
        }

        return $value->value;
    }

    public function convertToPHPValue($value, AbstractPlatform $platform): ?BackedEnum
    {
        if ((!is_int($value)) && !is_string($value)) {
            return null;
        }

        return $class::tryFrom($value);
    }
}

I then have my entity as

#[ORM\Entity(repositoryClass: ActionRepository::class)]
#[ORM\Table(name: 'actions')]
class Action
{
    #[ORM\Id]
    private string $name;

    #[ORM\Column(type: ActionType::class, nullable: false)]
    private ActionType $type;
}

and in Kernel.php I set up type mapping inside Kernel::process():

$typesDefinition[ActionType::class] = ['class' => ActionTypeType::class];
$container->setParameter('doctrine.dbal.connection_factory.types', $typesDefinition);

Now I know that the types are assigned correctly, as migrations generate up like this:

    public function up(Schema $schema): void
    {
        $this->addSql('ALTER TABLE actions CHANGE type type enum(\'submit\', \'cancel\') NOT NULL');
    }

but it is generated ALWAYS.

and the down() migration looks even weirder:

    public function down(Schema $schema): void
    {
        $this->addSql('ALTER TABLE actions CHANGE type type VARCHAR(0) NOT NULL');
    }

Everything works fine with DBAL 3, which uses SQL comments

@michnovka
Copy link
Author

@greg0ire I am tagging you as you authored some of the relevant changes and I know you have deep insight into removing comments in DBAL 4.

I was digging into the code, the issue lies here:

So Doctrine\Migrations\Generator\DiffGenerator::generate() calls createFromSchema to do comparison of old vs new schema. This calls Doctrine\DBAL\Schema\MySQLSchemaManager (which inherits AbstractSchemaManager) and specifically it fails on function _getPortableTableColumnDefinition(). Where the array $tableColumn which this function gets is

 array:11 [
  "table_name" => "actions"
  "field" => "type"
  "type" => "enum('submit','cancel')"
  "null" => "NO"
  "key" => ""
  "default" => null
  "extra" => ""
  "comment" => ""
  "characterset" => "utf8mb4"
  "collation" => "utf8mb4_general_ci"
  "name" => ""
]

This function then calls $type = $this->platform->getDoctrineTypeMapping($dbType) and since enum, or whatever custom type is not recognized, it defaults to StringType.

Now check DBAL 3 code:
https://github.com/doctrine/dbal/blob/893417fee2bc5a94a10a2010ae83cab927e21df3/src/Schema/MySQLSchemaManager.php#L189-L192

This uses the comment to deduce the proper type and override whatever type is in DB. Without this now, DBAL considers any unknown DB type to be StringType and I have no way how to tell it its not a string type. Hence the migration will always consider old schema column to be string.

Now the new schema uses the DiffGenerator::createToSchema() which does not use Doctrine\DBAL\Schema\AbstractSchemaManager but instead Doctrine\Migrations\Provider\OrmSchemaProvider and this uses under the hood EntityManager which of course identifies the column type properly as ActionTypeType or whatever custom type you use.

So, since we removed comments in DBAL 4, how can the fromSchema be generated properly? This uses only DB values, it does not care about type definition in Entity code (thats relevant for toSchema only).

@greg0ire
Copy link
Member

greg0ire commented Jul 3, 2024

Please upgrade the ORM to 3.8.5: doctrine/dbal#6423

EDIT: I shouldn't answer so hastily

@michnovka
Copy link
Author

michnovka commented Jul 3, 2024

@greg0ire I am using DBAL 4.0.4 which is based on 3.8.6 that includes the linked fix. ORM is at 3.2.1 and there is no newer version. And the issue is ONLY with DBAL 4 not DBAL 3

@berkut1
Copy link

berkut1 commented Jul 3, 2024

@michnovka It seems that when Doctrine reads the schema of your database, the getSQLDeclaration function is not being called, which is why it cannot recognize the type. There might be a bug in https://github.com/doctrine/dbal/blob/4.0.x/src/Platforms/AbstractMySQLPlatform.php

Try debugging this function to determine at which stage the type is being lost:
https://github.com/doctrine/dbal/blob/90424473eb144659a89fb1b5c9bca37297085351/src/Platforms/AbstractPlatform.php#L2190

Probably, this is a mistaken assumption. Here, two schemas columns are being compared. If initially it was read as StringType, then the error is somewhere earlier.

@michnovka
Copy link
Author

@berkut1 this is indeed a mistaken assumption imo, as getSQLDeclaration not being called is a consequence of not having the proper type. The type has to be known in order for that function to be called. And it is called, just for the new schema.

The issue is that oldSchema is created based on database ONLY. It ignores PHP code. And thats good, as the php code defines the new schema, if it took the PHP code as a hint of the type, then migration has no point, as it would be the same type always.

I honestly see no other way than to somehow store in DB the name of the custom type which was used. Which is exactly what the DB comment was used for. WDYT?

@berkut1
Copy link

berkut1 commented Jul 3, 2024

@michnovka
No, DC2Type comments have been useless since around DBAL 3.x. Doctrine can determine custom types on its own if they are properly registered. You need to find out why, in your case, this is not happening for Enum and where exactly the error is in the code.

@michnovka
Copy link
Author

michnovka commented Jul 3, 2024

@berkut1 how are they useless when even in 3.9.x there is this code:

https://github.com/doctrine/dbal/blob/893417fee2bc5a94a10a2010ae83cab927e21df3/src/Schema/MySQLSchemaManager.php#L189-L192 ?

And this is where it fails, this is where in 3.8.x version it assigned properly custom type, but in 4.0.x it assigns StringType (as the code assigning type based on comment is missing)

And again, think about what I observed above - the oldSchema is created based on DB data ONLY. not PHP. Only DB. So if DB has no indication about the type, how can it be properly determined for nonstandard types?
newSchema uses PHP code to determine types. And thats good.

Doctrine can determine custom types on its own if they are properly registered.

This has no sense for migrations. If I had customType1 field in table, and I changed it to customType2 field, then did migration, how would doctrine deduce it was before customType1? It is no longer defined and registered. PHP code always reflects only the newSchema.

@berkut1
Copy link

berkut1 commented Jul 3, 2024

DBAL 3.x should have backward compatibility, that why it can works with DC2Types and without. there was added this https://github.com/doctrine/dbal/blob/893417fee2bc5a94a10a2010ae83cab927e21df3/src/Platforms/AbstractPlatform.php#L108
that people can smoothly migrate from DBAL3 to DBAL4

@michnovka
Copy link
Author

With DBAL 3.8.X and doctrine.dbal.disable_type_comments: true the issue is also present.

And of course it is. Read again where and why it happens, I explained exactly where the issue lies. oldSchema is created based on DB details only. If we dont use comments (either as they are removed in DBAL 4, or if we disable them manually in DBAL 3) then oldSchema will have no idea what type was used before.

Thanks for your interest and will to help.

@berkut1
Copy link

berkut1 commented Jul 3, 2024

Here,
https://github.com/doctrine/dbal/blob/90424473eb144659a89fb1b5c9bca37297085351/src/Schema/MySQLSchemaManager.php#L135
and
https://github.com/doctrine/dbal/blob/90424473eb144659a89fb1b5c9bca37297085351/src/Schema/MySQLSchemaManager.php#L218
Doctrine is trying to map your registered custom types to your database. What happens when it tries to map the Enum type in your case?

UPD:
You also can check if your type registered, here:
image

@michnovka
Copy link
Author

michnovka commented Jul 3, 2024

https://github.com/doctrine/dbal/blob/90424473eb144659a89fb1b5c9bca37297085351/src/Schema/MySQLSchemaManager.php#L135

dump($type, $dbType);

gives

1 "string"
2 "enum"

and

https://github.com/doctrine/dbal/blob/90424473eb144659a89fb1b5c9bca37297085351/src/Schema/MySQLSchemaManager.php#L218

dump($column);

gives

Doctrine\DBAL\Schema\Column {#926
  #_name: "type"
  #_namespace: null
  #_quoted: false
  #_type: Doctrine\DBAL\Types\StringType {#636}
  #_length: 0
  #_precision: null
  #_scale: 0
  #_unsigned: false
  #_fixed: false
  #_notnull: true
  #_default: null
  #_autoincrement: false
  #_platformOptions: []
  #_columnDefinition: null
  #_comment: ""
}

UPD:
I checked and the typeRegistry does contain my custom type ActionTypeType

@greg0ire
Copy link
Member

greg0ire commented Jul 3, 2024

@michnovka it's unclear to me whether you know what platform-aware comparison is. In case you don't, since DBAL 3.2.0, platform-aware comparison allows to compare the generated SQL instead of comparing schemas. This means that 2 different schemas could result in an empty diff, if they result in the same SQL being generated. More on this here: https://www.doctrine-project.org/2021/11/26/dbal-3.2.0.html

Sorry if you already know this, it's genuinely hard to tell for me.

@michnovka
Copy link
Author

michnovka commented Jul 3, 2024

@greg0ire yes, I get this.

The problem is that when the comparator tries to compare the schemas, it is working with oldSchema and newSchema. oldSchema is Schema instance which is created and populated with data using Doctrine\DBAL\Schema\MySQLSchemaManager and it considers my column to be StringType (cos DB column definition is enum(...) which it doesnt understand, so it says, its StringType - this can be seen in my debug output in comment #1441 (comment)).

This means that when it tries to compare SQL generated between old and new schemas, it calls StringType::getSQLDeclaration output instead of the ActionTypeType::getSQLDeclaration for the old schema.

And this is expected. As if in DB there is no info about which custom type the column is, how can the MySQLSchemaManager assume its ActionTypeType and then gets its SQL from ActionTypeType::getSQLDeclaration to compare with new schema.

Now the new schema is another Schema instance, but this one is created using Doctrine\Migrations\Provider\OrmSchemaProvider and this calls internally EntityManager and that uses PHP code and properly considers the column ActionTypeType. Then new schema's SQL is the proper one, while the old ones is nonsense, as it thinks its a StringType.

@greg0ire
Copy link
Member

greg0ire commented Jul 3, 2024

Ah I get it now, thanks for explaining it again. I think this means the issue is that the DBAL does not understand enum(…), and cannot find a type that will produce the same DDL. I don't see a way around that. I now better get the point of doctrine/dbal#6444, which I now realize was about the same thing.

@michnovka
Copy link
Author

yes, it all comes to a simple problem - the oldSchema is generated using ONLY DB introspection. And if DB contains no info about which type was used, then it cannot call proper getSQLDeclaration function.

And no, the problem does not simply limit to enum. I can make an example with another custom type which will have the same issue without enums. ALL custom types have this issue in fact, i.e. all custom types with custom getSQLDeclaration which differs from those provided by DBAL package.

@berkut1
Copy link

berkut1 commented Jul 3, 2024

@greg0ire
Copy link
Member

greg0ire commented Jul 3, 2024

This looks related: doctrine/dbal#5308 (comment)

@michnovka
Copy link
Author

Here, Doctrine is reading your database. Does it also read the type as a string here? https://github.com/doctrine/dbal/blob/90424473eb144659a89fb1b5c9bca37297085351/src/Schema/AbstractSchemaManager.php#L195

No, here it fetches it from DB like this:

   1 => array:10 [
      "TABLE_NAME" => "actions"
      "field" => "type"
      "type" => "enum('submit','cancel')"
      "null" => "NO"
      "key" => ""
      "default" => null
      "EXTRA" => ""
      "comment" => ""
      "characterset" => "utf8mb4"
      "collation" => "utf8mb4_general_ci"
    ]

But this is just one step before _getPortableTableColumnDefinition is called which is where the $this->platform->getDoctrineTypeMapping($dbType); is called, and which we already addressed and debugged in #1441 (comment)

And @greg0ire this is where the issue lies:
https://github.com/doctrine/dbal/blob/90424473eb144659a89fb1b5c9bca37297085351/src/Schema/MySQLSchemaManager.php#L218

        $column = new Column($tableColumn['field'], Type::getType($type), $options);

specifically the Type::getType($type). Without comments, there is no way at all for the old type to be "guessed" correctly. If I use custom type which has SQL declaration always as int(3), (lets call it CustomInt3Type), then the Column instance will always be deduced as IntegerType and for comparison IntegerType::getSQLDeclaration will be called to be compared against newSchema's CustomInt3Type::getSQLDeclaration. There is just no way for it to know that this int(3) is actually CustomInt3Type and not IntegerType

@berkut1
Copy link

berkut1 commented Jul 3, 2024

Ah, I understand now. It calls Type::getType with enum and cannot find such a type because it doesn't exist.
In Symfony, you can work around this by registering another type to let Doctrine know about it: https://symfony.com/doc/current/doctrine/dbal.html#registering-custom-mapping-types-in-the-schematool (this instruction correct for DBAL3)
In your case, for DBAL 4 with Symfony, you need to do it like this:

        mapping_types:
            enum: ancestor_enum 
        types:
            ancestor_enum : 'App\Model\AncestorEnumType' 

Unfortunately, I can't help with how to do this without Symfony.

UPD:
In the example, I specified the type as ancestor, assuming that you can register it in the mapping (mapping_types) only once. For all your other Enums, it will be enough to inherit from it and register only as a custom type (types). Otherwise, you would have to duplicate entries for all Enums in mapping_types.

@michnovka
Copy link
Author

@greg0ire as a temporary workaround, how can I tell migration to ignore certain columns? Thanks!

@greg0ire
Copy link
Member

greg0ire commented Jul 4, 2024

Maybe you can use asset filtering, but I don't think it works at the column level.

@michnovka
Copy link
Author

So the issue is this:

  1. we get table columns in raw format from DB using DBAL\Schema\AbstractSchemaManager::fetchTableColumnsByTable() This is an array like
   1 => array:10 [
      "TABLE_NAME" => "actions"
      "field" => "type"
      "type" => "enum('submit','cancel')"
      "null" => "NO"
      "key" => ""
      "default" => null
      "EXTRA" => ""
      "comment" => ""
      "characterset" => "utf8mb4"
      "collation" => "utf8mb4_general_ci"
    ]
  1. This is converted to DBAL\Schema\Column instance, which has incorrect type (as DBAL has no idea what custom type is to be used without comments, so it guesses best match from default types)
  2. then Column::getType()::getSQLDeclaration() is called to obtain incorrect SQL

And on the new schema, we get correct type for column and call its getSQLDeclaration()

These do not match.


Proposed solution:

Why not leverage Column::$_columnDefinition for the old schema? Lets populate this field inside AbstractSchemaManager::_getPortableTableColumnDefinition.

I dont understand why AbstractPlatform::columnsEqual() unsets this. But if we dont unset it and specify it explicitly for old schema, the comparison would be done properly

@greg0ire WDYT?

@greg0ire
Copy link
Member

greg0ire commented Jul 4, 2024

Sounds good, feel free to give it a try, and see if anything breaks 👍

@michnovka
Copy link
Author

I tried to go this way and I am afraid this would create tons of unexpected issues. There are specific test cases that ensure that

Schema diff is empty, since only columnDefinition changed from null (not detected) to a defined one

AbstractComparatorTestCase::testWillNotProduceSchemaDiffOnTableWithAddedCustomSchemaDefinition

so for whatever reason this is considered important. Even the comment in the AbstractPlatform::columsEqual talks about this specifically to ignore the column definition.

So maybe a solution would be to add comments back so that the type can be deduced properly?

@greg0ire
Copy link
Member

greg0ire commented Jul 5, 2024

I don't think that's the direction we want to go, no. Re-reading the cookbook you mentioned I see

In this case however Schema-Tool update will have a hard time not to request changes for this column on each call.

It seems like that might have been fixed in doctrine/dbal#5224, which might make solution 1 the way to go here.

@berkut1
Copy link

berkut1 commented Jul 6, 2024

@michnovka
I don't know why you ignored my solution #1441 (comment). For reference, I had a very similar problem, not with enum, but with the inet type, which is unknown to Doctrine. It tried, as in your case, to convert it to VARCHAR in the down() method and constantly generated diff migrations.

As I mentioned in my comment on the instructions, I solved the problem for myself in this way:

doctrine:
    dbal:
        mapping_types:
            inet: my_inet

        types:
            my_inet: { class: 'App\Model\InetType' }

I want to clarify, as stated in the instructions at https://www.doctrine-project.org/projects/doctrine-orm/en/3.2/cookbook/mysql-enums.html, it is recommended to do it like this

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

but this solution works only for DBAL3. For DBAL4, you need to refer not to string but to your newly registered custom type, as I indicated in my previous comment.

However, if you haven't tried this solution at all, you can first refer to string; it might work with enum.

@michnovka
Copy link
Author

michnovka commented Jul 7, 2024

@berkut1 Thanks for the suggestion, but this is not a solution for my case.

It works in your case when you have 1:1 mapping between DB type INET and your class MY_INET. So your code is 100% certain, when it finds INET type in DB to use that specific type.

But in my case, every ENUM is different. The example I provided is ENUM('submit','cancel') and it uses custom type ActionTypeType extends AbstractEnumType. AbstractEnumClass is my base class for enums, and I have many others. There is e.g. ColorEnumType extends AbstractEnumType which creates ENUM('black','red'). So DB ENUM type maps to multiple PHP Type classes.

@berkut1
Copy link

berkut1 commented Jul 7, 2024

@michnovka

Yes, I understand, which is why I initially suggested creating an abstract Enum class and registering it, then inheriting from it for custom types.
If I understand correctly how Doctrine works with types, when comparing schemas, it only compares the types of SQL, and the comparison of ColumnDeclarationSQL happens only at the final stage: https://github.com/doctrine/dbal/blob/90424473eb144659a89fb1b5c9bca37297085351/src/Platforms/AbstractPlatform.php#L2199-L2204

In other words, you need to make Doctrine understand that it has a base Enum class for comparison, with which it will compare all the descendants of this class.
This test for DBAL3 with disabled type comments, show the logic:
https://github.com/doctrine/dbal/blob/893417fee2bc5a94a10a2010ae83cab927e21df3/tests/Platforms/AbstractPlatformTestCase.php#L1448

@PowerKiKi
Copy link
Contributor

remove ENUM from DBAL

ENUM does not exist in DBAL. There is no code that specifically mention that (except one single occurence in test). So there is nothing to remove really.

But I am a bit shocked that you suggested it. We've been asking to keep a way for us to implement our own support for ENUM, and you suggest the exact opposite. I feel I am not being heard at all here. I get it that DBAL has no interest to support it out-of-the-box. That's fair. But it feels like you are actively trying to prevent us to achieve our goal, whereas there could be a reasonable compromise to be made, but you don't seem to consider them seriously at all.

I would like to hear the opinion of other people too. Someone with much more insights that the both of us, such as @greg0ire or @derrabus ? would you accept a PR that avoid truncating type declaration for ENUM and SET ?

@berkut1
Copy link

berkut1 commented Aug 13, 2024

But I am a bit shocked that you suggested it. We've been asking to keep a way for us to implement our own support for ENUM, and you suggest the exact opposite

@PowerKiKi Not exactly. As far as I understand, the main contributors are strongly against bringing back comment hints, or something similar. If they're against it, then why keep non-functional pieces of code at all?

@michnovka
Copy link
Author

@berkut1 you cannot remove ENUM from DBAL4 bacause it simply is not there. What you can remove is an example on docs for how to make custom ENUM() column. While it still works to create the DBAL schema, it will fail with migrations.

@PowerKiKi I think this thread has the underlying issue described to the very detail:

The issue is that oldSchema is created based on database ONLY. It ignores PHP code. And thats good, as the php code defines the new schema, if it took the PHP code as a hint of the type, then migration has no point, as it would be the same type always.

Any type which is not natively supported by DBAL will face this issue. We have a one-way function from PHP code -> DB representation, and we have no way to reverse this with accuracy, because

  1. multiple PHP representations can lead to the same DBAL type
  2. only default built-in DBAL types can be compared in migrations

With ENUMs specifically it is further complicated by the fact that basically every ENUM is a different type. DBAL was not meant to handle such case. It can handle VARCHAR(16) != VARCHAR(63) but this is because length is a valid property of Column. The list of valid properties is:

  • name
  • length
  • precision
  • scale
  • unsigned
  • fixed
  • notnull
  • default
  • autoincrement

There is no way to specify ENUM cases. So the best we could get with current DBAL is ENUM(1,2,3) = ENUM('a','b','c').

But that still assumes that ENUM would be a valid type in DBAL, which it is not (and there are no plans to make it so, because it is not abstract enough. Many DBMS do not support this. Its really a MySQL-family thing)

I do understand the reluctancy to bring back comments, but when the decision was made, I do not believe enough thought was given to the real implications. This is not just ENUMs. This is ANY nonstandard DBAL types. INET, SET etc. Exotic ones, yes, but 4.x breaks them.

While I agree to discourage from using typehints, I think this option should be brought back for the few cases, where without them 4.x is broken without a fix where 3.x worked just fine.

@derrabus
Copy link
Member

Maybe we need some kind of verbatim type that all unknown database types are being mapped to for the sole purpose of making two introspections comparable.

@stof
Copy link
Member

stof commented Aug 13, 2024

@derrabus as long as AbstractSchemaManager::getPortableTableColumnDefinition does not allow to access the original definition for enum fields during introspection, this verbatim type won't solve it.

@derrabus
Copy link
Member

Sure that needs to be solved along with it.

@PowerKiKi
Copy link
Contributor

kind of verbatim type

It sounds like a way to say to DBAL "stop your magic, I'll handle everything from here". That's exactly what comment did. I suppose we can re-create the same concept with a different API. As long as I can use that new hypothetical API to implement a custom type to support ENUM and SET, I'd be happy. But at the same time, I can't help to wonder: why not just restore comments ? its code is simple, the concept is well-known and proven to work even (especially!) from user code.

The only rationale we were given for comment removal was "DBAL core does not need it anymore". But I think we've proven that there are reasonable use-cases, implemented outside DBAL core, that requires it. What is the reason to block restoring comments ?

@stof
Copy link
Member

stof commented Aug 13, 2024

@PowerKiKi comments were solving some cases, and were creating the same kind of issue (endless change) in other cases.
this verbatim type used during introspection to make it use the actual unmodified column declaration (which is then compared to the one generated by your custom type) looks like the proper fix for the root cause of the issue (solution 1 in #1441 (comment))

@greg0ire
Copy link
Member

as long as AbstractSchemaManager::getPortableTableColumnDefinition does not allow to access the original definition for enum fields during introspection, this verbatim type won't solve it.

@stof @derrabus should we use Doctrine\DBAL\Schema::$_columnDefinition for this? Or is it a bad idea/meant for something else?

@PowerKiKi
Copy link
Contributor

@derrabus, I'd be willing to help introduce a "verbatim type", but I'm not quite sure what you mean codewise. Would you be able to write some pseudo-code to help me understand ? or would you prefer to prototype it yourself ?

@WubbleWobble
Copy link

WubbleWobble commented Aug 16, 2024

Running into similar issues after upgrading.

We had a DateTimeMicroseconds type as based upon various code snippets in doctrine/dbal#2873, but instead of replacing the datetime type completely, we complemented it so that we could have some columns that were just seconds-based, and other columns that supported microseconds.

Now that DBAL doesn't support comments, this behaviour has been broken.

platform->getDoctrineTypeMapping(dbType) seems to base its decision purely on DATETIME, so I don't think I can get it to return a different type based upon the column length.

Similarly type->convertToDatabaseValue and type->convertToPHPValue don't provide column meta, so I don't think I can see the column size in order to use a different datetime format string depending upon the column length.

I could try to override AbstractPlatform->getDateTimeFormatString to add ".u", but the type->convertToPHPValue would break if provided a non-microsecond string.

The remaining potential options I have identified are:
a) Go back to DBAL 3.x which supports comments
b) Override datetime to overwrite the dateTimeFormat string used, and switch all of my columns over the DATETIME(6) - i.e. no more DATETIME(0)
c) Override datetime and add logic to type->convertToPHPValue to inspect whether the string coming out has a ".123456" microsecond part, and then using either the normal or microsecond dateTimeFormat string based upon that.

The comments stuff was useful.

In my particular case, I could make the type decision if I had access to more column meta - via hypothetical method changes such as getDoctrineTypeMapping($dbType, $columnMeta) or convertToDatabaseValue($value, $platform, $columnMeta)

(N.B. Not a Doctrine expert - I only know what I've gleamed from reading comments here and digging through the code. If I'm missing the elephant in the room and what I'm trying to do is actually possible, please let me know! :D)

@derrabus
Copy link
Member

@derrabus, I'd be willing to help introduce a "verbatim type", but I'm not quite sure what you mean codewise. Would you be able to write some pseudo-code to help me understand ? or would you prefer to prototype it yourself ?

If you're motivated to work on this, please go ahead. I will support you if I can.

The idea is that during introspection DBAL maps column to this verbatim type if it cannot map it to a registered DBAL type. And since your custom enum type and the verbatim type should ideally generate the same SQL, no migration is generated.


We had a DateTimeMicroseconds type

Multiple attempts have been made to support DATETIME fields with configurable precision natively in DBAL. In your case, I'd rather try to finish that feature. This feature is supported by most DBMS and you're not the only one who needs this. Instead of wasting time on documenting workarounds, we should really focus on getting that implemented properly.

@WubbleWobble
Copy link

WubbleWobble commented Aug 16, 2024

Multiple attempts have been made to support DATETIME fields with configurable precision natively in DBAL. In your case, I'd rather try to finish that feature. This feature is supported by most DBMS and you're not the only one who needs this. Instead of wasting time on documenting workarounds, we should really focus on getting that implemented properly.

Well - that's kind one of the angles I was looking at - having the DateTimeType react appropriately depending upon the precision, but convertToPHPValue and convertToDatabaseValue don't have access to the column information and so can't change their behaviour based upon it.

The next hop up from that was where types are determined - i.e. getDoctrineTypeMapping(string $dbType) - and that too didn't have the information (i.e. it only knew "datetime" and as such gave out a DateTimeType), so it wasn't possible there either as far as I could see.

So my understanding ended up being that implementing it would require a change in the API / interfaces.

@berkut1
Copy link

berkut1 commented Aug 19, 2024

@PowerKiKi I'd be willing to help introduce a "verbatim type", but I'm not quite sure what you mean codewise. Would you be able to write some pseudo-code to help me understand ? or would you prefer to prototype it yourself ?

I think we need to introduce a new type that will just store the database data type as it is. For example, right now, if a type like enum('apple','orange','pear') comes in, then here:

https://github.com/doctrine/dbal/blob/4.1.x/src/Schema/MySQLSchemaManager.php#L121

It turns into just enum, and we permanently lose the details of the enum. We need to keep the full enum('apple','orange','pear'), possibly in a new property Doctrine\DBAL\Schema::$_ColumnDefinitionAsItIs (we probably can't use old $_columnDefinition, because of https://github.com/doctrine/dbal/blob/a41d81d7d255d4f855606b4a29c792e062acb554/src/Platforms/AbstractPlatform.php#L2222-L2223) and also let Doctrine know that this type will be verbatim. For example, register it like this:

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

And then in the custom type, just do something like this:

class EnumType extends VerbatimType
{
    public function getSQLDeclaration(array $column, AbstractPlatform $platform): string
    {
        return "enum('apple','orange','pear')";
    }
}

VerbatimType probably will looks like this:

class VerbatimType extends Type
{
    public function getSQLDeclaration(array $column, AbstractPlatform $platform): string
    {
        return $column['сolumnDefinitionAsItIs']; //this seems not right :)
    }
}

However, we need to make it work for any unknown types, and this might require a lot of research on how different DBMSs store data types that are unknown to Doctrine, and whether we can simply save all of them in a сolumnDefinitionAsItIs.

P.S Of course, my idea might be wrong.

UPD:
Probably, a verbatim type will even allow us to skip the type registration step. For example, right now, simple unknown types that don't contain anything except the TYPE_NAME (like in PostgreSQL with INET, CIDR, etc), meaning without (), need to be registered like this:

doctrine:
    dbal:
        mapping_types:
            inet: alias_name

        types:
            alias_name: { class: 'App\Entity\InetType' }

Maybe, with a verbatim type, we can skip mapping_types if Doctrine automatically marks all unknown types as verbatim (currently, Doctrine throws an exception or registers as string).

@tomsykes
Copy link

Setting the Enum case aside for a moment (as I think it is muddying the water WRT what I think is the underlying issue)…

As of DBAL 4, the representation in the database is no longer an exact match for the schema definition from code, so when the comparator makes its comparison, there can be a difference even when nothing has changed.


Take the example of a custom type that is an RGB hex value represented by an object with 3 integer properties.

class RGB {
    public int $red;
    public int $green;
    public int $blue;
}

This is mapped to a varchar in the database using a custom type: e.g. {red: 255, green: 255, blue: 255} => FFFFFF

At runtime, DBAL correctly maps the varchar data from the database to my custom RGB object using App\DBAL\Types\RGBType (and vice-versa), because it is using the schema def from code - no problem.

When using the comparator to look for schema updates:

  • loads the schema from the database, and since the column type is varchar, resolves that to Doctrine\DBAL\Types\StringType
  • loads the schema from code (which maps the column to App\DBAL\Types\RGBType)
  • they're different - problem

Prior to the removal of database comments, the first step above would have resolved the column to the correct type.

@derrabus
Copy link
Member

derrabus commented Aug 19, 2024

When using the comparator to look for schema updates:

  • loads the schema from the database, and since the column type is varchar, resolves that to Doctrine\DBAL\Types\StringType
  • loads the schema from code (which maps the column to App\DBAL\Types\RGBType)
  • they're different - problem

No. As long as both types generate the same DDL statements, they are to be be considered equal.

@tomsykes
Copy link

No. As long as both types generate the same DDL statements, they are to be be considered equal.

But the comparator isn't comparing DDL statements, it's comparing Doctrine\DBAL\Schema\Column definitions, and the type property in Column is a resolved DBAL type, not a DDL type.

image
image

i.e. it's Doctrine\DBAL\Types\StringType and not "VARCHAR"

if this were comparing DDL statements, it would be working fine

@WubbleWobble
Copy link

WubbleWobble commented Aug 19, 2024

But the comparator isn't comparing DDL statements, it's comparing Doctrine\DBAL\Schema\Column definitions, and the type property in Column is a resolved DBAL type, not a DDL type.

This is exactly the cause of my problems. My custom type works fine in use, but the comparator thinks that something has changed and causes schema-update / migration diff to perpetually recommend an "ALTER TABLE" on said column.

image

Here I've dumped the $oldSchema and $newSchema when running doctrine:schema:update --dump-sql.

It thinks they are different because it has two different DBAL types, whereas if it compared raw column types, it'd realise that the schema wants a DATETIME, and the database already has a DATETIME, and all would be fine! :)

@derrabus
Copy link
Member

But the comparator isn't comparing DDL statements

Yes, it does.

https://github.com/doctrine/dbal/blob/a41d81d7d255d4f855606b4a29c792e062acb554/src/Platforms/AbstractPlatform.php#L2225-L2230

Only if two columns are considered to be not equal, a ColumnDiff object is created which contains the method from your screenshot.

https://github.com/doctrine/dbal/blob/a41d81d7d255d4f855606b4a29c792e062acb554/src/Schema/Comparator.php#L183-L187

@WubbleWobble
Copy link

Thanks @derrabus - that gives me somewhere further to dig to try to understand what is going on! :)

@WubbleWobble
Copy link

But the comparator isn't comparing DDL statements

Yes, it does.

I've looked into this further - thanks for the pointer! - and it's comparing the DDL statements generated based by the resolved Doctrine DBAL types.

https://github.com/doctrine/dbal/blob/a41d81d7d255d4f855606b4a29c792e062acb554/src/Platforms/AbstractPlatform.php#L1392

So essentially in my case:
a) It's calling DateTimeType->getSQLDeclaration() and the results of getColumnDeclarationSQL() end up as being DATETIME NOT NULL (old schema)
b) It's calling DateTimeMicrosecondType->getSQLDeclaration() and the results of getColumnDeclarationSQL() end up as being DATETIME(6) NOT NULL (new schema)

It's then raising a ColumnDiff as you suggested.

However, what is actually in the database is indeed a DATETIME(6) NOT NULL, so the DDL generated for "old schema" is not actually what is in the database.

Ultimately I've tracked this down to AbstractMysqlPlatform->getDateTimeDeclarationSQL() which doesn't seem to care about the fsp value on DATETIME - I declare a "length" (fsp) of 6, but it's not using it:

    public function getDateTimeTypeDeclarationSQL(array $column): string
    {
        if (isset($column['version']) && $column['version'] === true) {
            return 'TIMESTAMP';
        }

        return 'DATETIME';
    }

It therefore seems I could override this method to resolve my comparator woes?

I could also make a quick patch for this issue - it looks pretty trivial - if this sounds correct to you?
(i.e. For all non-null/zero column "lengths", append that "length" - e.g. DATETIME(3) or TIMESTAMP(6). Valid values for MySQL are 0-6. I would need to check whether this "fsp" has always been supported on MySQL and if not, when it was introduced etc)


Also it's a bit odd, in that it's not comparing the actual old column definition, but rather looking at the old column definition, parsing it into bits, deciding upon a DBAL Type class, and then asking that Type class to generate what it thinks the column definition should look like based upon those parsed bits, and then using that. I guess there are nuanced reasons to do that, but on the face of it, it does seem rather around the houses.

@derrabus
Copy link
Member

It therefore seems I could override this method to resolve my comparator woes?

That could probably work.

I could also make a quick patch for this issue - it looks pretty trivial - if this sounds correct to you? (i.e. For all non-null/zero column "lengths", append that "length" - e.g. DATETIME(3) or TIMESTAMP(6). Valid values for MySQL are 0-6. I would need to check whether this "fsp" has always been supported on MySQL and if not, when it was introduced etc)

You can have a look at the previous attempt (doctrine/dbal#5961) and try to complete it. The workaround that you need might be trivial. However, properly supporting DATETIME with variable precision is not. You could also open an issue on the DBAL repository first where we discuss the obstacles that you might run into.

@WubbleWobble
Copy link

You can have a look at the previous attempt (doctrine/dbal#5961) and try to complete it

I've looked at it, and it looks like the original author has put a lot of work into this and as far as I can see has resolved every change request.

I can see the entitled muppet (that's the polite form) throwing a spanner in the works there with his rudeness, but what further works needs to be done to it / how can people help to progress it?

@derrabus
Copy link
Member

It has to be ported to the 4.2 branch at least. A lot of time has passed since then, I don't recall the remaining blockers right now. But please, let's discuss this on a separate issue on the DBAL repository. This is getting more and more off-topic.

@berkut1
Copy link

berkut1 commented Aug 19, 2024

@derrabus What do you think about this idea for implementing the verbatim type #1441 (comment)? Or do you have a different approach in mind?

@derrabus
Copy link
Member

@berkut1 There should be no need to register or extend that verbatim type. It is used only during introspection, if a DB column type is encountered that DBAL has no mapping for.

@b3n3d1k7

This comment was marked as resolved.

@derrabus
Copy link
Member

derrabus commented Sep 3, 2024

@b3n3d1k7 No, it doesn't. Please read the full thread before commenting, sorry.

@uncaught
Copy link

uncaught commented Sep 5, 2024

We've had this issue for ages with our own custom enums and solved it with a patch for MySQLSchemaManager.php:

diff --git a/src/Schema/MySQLSchemaManager.php b/src/Schema/MySQLSchemaManager.php
index bd383b012..556e3df4e 100644
--- a/src/Schema/MySQLSchemaManager.php
+++ b/src/Schema/MySQLSchemaManager.php
@@ -278,6 +278,8 @@ class MySQLSchemaManager extends AbstractSchemaManager
 
         $column = new Column($tableColumn['field'], Type::getType($type), $options);
 
+        $column->setPlatformOption('hcCurrentTableColumnType', $tableColumn['type']);
+
         if (isset($tableColumn['characterset'])) {
             $column->setPlatformOption('charset', $tableColumn['characterset']);
         }

Then in our type we simply do this to return the actual current SQL declaration for comparism:

  public function getSQLDeclaration(array $column, AbstractPlatform $platform): string {
    // This is patched at the end of MySQLSchemaManager::_getPortableTableColumnDefinition to allow diffs on enums:
    if (isset($column['hcCurrentTableColumnType'])) {
      return $column['hcCurrentTableColumnType'];
    }
    //... return your `enum(...)`, but this must be lower case for comparism!

In case this helps anyone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests