doctrine / migrations

Doctrine Database Migrations Library
https://www.doctrine-project.org/projects/migrations.html
MIT License
4.68k stars 388 forks source link

Migrations always generated for custom type with DBAL 4 #1441

Open michnovka opened 4 months ago

michnovka commented 4 months ago

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 https://github.com/doctrine/dbal/pull/5107 , afterwards some issues were found and fixed - https://github.com/doctrine/dbal/issues/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

derrabus commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

Sure that needs to be solved along with it.

PowerKiKi commented 3 months ago

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 commented 3 months ago

@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 https://github.com/doctrine/migrations/issues/1441#issuecomment-2285460448)

greg0ire commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

Running into similar issues after upgrading.

We had a DateTimeMicroseconds type as based upon various code snippets in https://github.com/doctrine/dbal/issues/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 commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

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:

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

derrabus commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

WubbleWobble commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

@derrabus What do you think about this idea for implementing the verbatim type https://github.com/doctrine/migrations/issues/1441#issuecomment-2295517648? Or do you have a different approach in mind?

derrabus commented 3 months ago

@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 commented 2 months ago

So solution 2 described here: https://www.doctrine-project.org/projects/doctrine-orm/en/3.2/cookbook/mysql-enums.html#solution-2-defining-a-type does not work anymore or am I missing something? This leads to infinite migrations and there is no workaround, right? image

derrabus commented 2 months ago

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

uncaught commented 2 months ago

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.

michnovka commented 1 month ago

has anybody started work on the Verbatim type? I might have some time in end of October/November to look into this.

PowerKiKi commented 1 month ago

I thought I would give it a try, but I didn't yet. And I won't have much time for a while. Feel free to start whenever you want. And notice everybody here when you do, so that we don't duplicate work.

morozov commented 1 month ago

Linking a couple more existing issues for reference: https://github.com/doctrine/dbal/issues/4470, https://github.com/doctrine/dbal/issues/5306.

VincentLanglet commented 1 month ago

I encounter the same issue with my custom types. I keep generating dozen of lines like

$this->addSql('ALTER TABLE activity_events CHANGE status status ENUM(\'DISPATCHED\', \'ONGOING\', \'SUCCEEDED\', \'FAILED\') NOT NULL');

in my migration diff, when it worked fine with COMMENT \'(DC2Type:enumActivityEventStatus)\''

It does not give a good experience with the DBAL 4 migration, and it seems like it impacts a lot of developer.

I read all the discussion and maybe I miss-understand but I feel like there is currently no solution to avoid this extra sql in every diff. Isn't it ?

The comment was really useful for custom type. If there is no known solution, what about re-introducing it back ? At least this could be optional like an option to toggle. This way if comments have bad impact for some user they can disable it.

uncaught commented 1 month ago

As I said, we have had a solution for years and it kept working with dbal 4.

I would create a PR, but it's for mysql only 🫤

meiyasan commented 1 month ago

I encounter the same issue with my custom types.

I keep generating dozen of lines like


$this->addSql('ALTER TABLE activity_events CHANGE status status ENUM(\'DISPATCHED\', \'ONGOING\', \'SUCCEEDED\', \'FAILED\') NOT NULL');

in my migration diff, when it worked fine with COMMENT \'(DC2Type:enumActivityEventStatus)\''

It does not give a good experience with the DBAL 4 migration, and it seems like it impacts a lot of developer.

I read all the discussion and maybe I miss-understand but I feel like there is currently no solution to avoid this extra sql in every diff. Isn't it ?

The comment was really useful for custom type. If there is no known solution, what about re-introducing it back ? At least this could be optional like an option to toggle. This way if comments have bad impact for some user they can disable it.

@VincentLanglet Your feeling is correct, in my opinion. I haven’t found any alternatives either. I’ve been begging for merging this PR https://github.com/doctrine/dbal/pull/6444 for months, but they don’t want to for obscure reasons. While this doesn’t have to be a permanent solution, removing those lines too early feels like a quick shot. They could just leave them in place until a more mature alternative is found.

A good legacy solution would be to follow the approach of the doctrine/annotation bundle and make it optional, but they seem to have missed the point for months.

If you’re interested, I created this package - a code modifier that reintroduces the missing lines: https://packagist.org/packages/glitchr/doctrine-dc2type. Please use with caution and inspect the package on a fresh symfony skeleton if you wish, there is nothing too complex or fancy, but it is nothing official. This is the only alternative I had for the sake of my projects.

derrabus commented 1 month ago

Your complaints have been duly noted. Now, can we please get back to discussing solutions?

I mean, you can just stay on DBAL 3 until the problem is solved or help working on a proper solution. We will still maintain DBAL 3 for some time.

VincentLanglet commented 1 month ago

Now, can we please get back to discussing solutions?

Sure, but I'm unsure how to help. I certainly lack of knowledge about how this work.

If I debug MySQLSchemaManager::_getPortableTableColumnDefinition, I feel like the logic here is incorrect https://github.com/doctrine/dbal/blob/7a8252418689feb860ea8dfeab66d64a56a64df8/src/Schema/MySQLSchemaManager.php#L118-L135

When the data_type in my DB is enum('DISPATCHED','ONGOING','SUCCEEDED','FAILED')

Personally, I could almost fix my issue if I overriden the

public function getMappedDatabaseTypes(AbstractPlatform $platform): array

method in my customs Type, but it would require to update the MySQLSchemaManager to something like

        $dbType = strtolower($tableColumn['type']);
        if (!str_starts_with($dbType, 'enum(')) {
            $dbType = strtok($dbType, '(), ');
            assert(is_string($dbType));

            $length = $tableColumn['length'] ?? strtok('(), ');
        } else {
            $length = null;
        }

Then https://www.doctrine-project.org/projects/doctrine-orm/en/latest/cookbook/mysql-enums.html could be updated to implement

public function getMappedDatabaseTypes(AbstractPlatform $platform): array
    {
        $values = array_map(function($val) { return "'".$val."'"; }, $this->values);

        return ["ENUM(".implode(",", $values).")"];
    }

(the abstract EnumType could even be provided by doctrine eventually...)

Do you think it could a good improvement @greg0ire @derrabus ? (And how avoid a BC break...)

I mean, you can just stay on DBAL 3 until the problem is solved or help working on a proper solution. We will still maintain DBAL 3 for some time.

DBAL 4 still have benefits ; I like the evolution done to bigint.

michnovka commented 1 month ago

Should the VerbatimType live inside Migration or DBAL? I would argue Migration, since it should not be used anywhere else.

And @stof

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.

This is not true, we have that, see https://github.com/doctrine/migrations/issues/1441#issuecomment-2206338070

We can just create a VerbatimType from within here and just give it this $tableColumn property. We cannot use TypeRegistry for this case, as every VerbatimType will be its own instance.

But I think we can handle all this inside SchemaManager's _getPortableTableColumnDefinition(). Seems well have to edit every single one tho, which overrides this method and introduce this logic.

Then we just make special case in comparator for VerbatimType and we're golden.

Am I missing something, or does it sound too easy to you too?

derrabus commented 1 month ago

Does doctrine/dbal#6536 solve the issue for those of you who are using enum columns?

stof commented 1 month ago

@michnovka both the schema introspection and the schema comparison are provided by DBAL. So it would definitely be used outside Migrations (and Migrations would not even use it directly but only through the usages in DBAL itself)

VincentLanglet commented 1 month ago

Does doctrine/dbal#6536 solve the issue for those of you who are using enum columns?

I'm getting No changes detected in your mapping information. with your branch when I run do:mi:diff. That's an error message I like.

Also I tried to change my mapping and/or change my enumType ; in both case the diff is correct. It's awesome, thanks a lot @derrabus

michnovka commented 1 month ago

Thanks @derrabus this indeed solves the issues with ENUM and migrations.

Only thing I noticed was that if my sqlDeclaration defined the type as lowercase enum('a','b') it would still cause migrations (where up would have ENUM('a','b') and down enum('a','b'). Is this expected?

Changing my custom type's sql declaration to uppercase ENUM resolved this.

derrabus commented 1 month ago

So, this is your fix then. 🙂

PowerKiKi commented 1 month ago

@derrabus (or @michnovka ?), could you please clarify how the new EnumType should be used ?

I updated my playground to the latest versions of all packages, and I still see varchar in my DB, instead of the expected enum. I thought it should work out of the box. Did I misunderstand something ?

The model is declared there, and Doctrine is configured in the simplest manner I know of.

Basically I am doing:

<?php

use Doctrine\ORM\Mapping as ORM;

enum UserRole: string
{
    case Visitor = 'visitor';
    case Member = 'member';
    case Admin = 'admin';
}

enum UserStatus: string
{
    case New = 'new';
    case Active = 'active';
    case Archived = 'archived';
}

#[ORM\Entity]
class User
{
    #[ORM\Id]
    #[ORM\Column(type: 'integer')]
    public ?int $id = null;

    #[ORM\Column]
    public UserRole $role = UserRole::Visitor;

    #[ORM\Column]
    public UserStatus $status = UserStatus::New;
}

And that:

function createEntityManager(): EntityManager
{
    $config = ORMSetup::createAttributeMetadataConfiguration(['my-path-to-my-entity'], true);

    $connection = DriverManager::getConnection([
        'driverClass' => Driver::class,
        'host' => '127.0.0.1',
        'dbname' => 'test',
        'user' => 'test',
        'password' => 'test',
    ], $config);

    $entityManager = new EntityManager($connection, $config);

    return $entityManager;
}

...but it still fails to have enum in my DB.

michnovka commented 1 month ago

@PowerKiKi still by default doctrine will use StringType for your enum. You will need to customize

    #[ORM\Column(type: 'enum', enumType: UserRole::class, values: [...])]
    public UserRole $role = UserRole::Visitor;

If you do not want to specify values here, and want by default all enum values to be used in DB as well, you have to use a custom type.

You can look into https://github.com/Elao/PhpEnums that makes this much easier.

You can also look into https://github.com/Elao/PhpEnums/issues/212 where I show on top my implementation which auto-registers enums as types.

derrabus commented 1 month ago

@derrabus (or @michnovka ?), could you please clarify how the new EnumType should be used ?

Please read doctrine/orm#11666.

If you do not want to specify values here, and want by default all enum values to be used in DB as well, you have to use a custom type.

No, don't do that. Custom enum types should be obsolete now.

You can look into https://github.com/Elao/PhpEnums that makes this much easier.

Not anymore, really.

michnovka commented 1 month ago

@derrabus this is so cool, thanks a lot, I was unaware of this new PR

@PowerKiKi if you still want to use MySQL enums by default, you can take advantage of TypedFieldMapper. Here is my solution for the interested:

  1. create custom EnumTypedFieldMapper
    
    <?php

declare(strict_types=1);

namespace App\Doctrine\ORM\TypedFieldMapper;

use Doctrine\DBAL\Types\Types; use Doctrine\ORM\Mapping\TypedFieldMapper; use ReflectionNamedType; use ReflectionProperty;

class EnumTypedFieldMapper implements TypedFieldMapper { /**

now you can use

#[ORM\Column]
public UserRole $role = UserRole::Visitor;

And it will make your field an ENUM type in database

Note that this requires ORM 3.3, DBAL 4.2 and DoctrineBundle 2.13

PowerKiKi commented 1 month ago

@derrabus, this is great, thank you !

My playground is now working as expected. Specifying the type of the column is an obvious solution I should have thought about, #[ORM\Column(type: 'enum')] !

@michnovka, thanks for the hint for ChainTypedFieldMapper. It is a nice to have that I might implement at some point.

From my point of view, which is only concerned about enum, this issue may be closed as resolved.

acasademont commented 3 weeks ago

Well, we still have the issue with the DATETIME(6) as @WubbleWobble pointed out in https://github.com/doctrine/migrations/issues/1441#issuecomment-2296926524

Not sure if this should remain open as a "placeholder" for that problem too.

A short patch that adds a middleware to the connection setting a custom platform (extended on MysqlPlatform80) and modifying the sql snippet to take into account the length. This now generates empty diffs for us, hope it might help someone else.

use Doctrine\DBAL\Driver;
use Doctrine\DBAL\Driver\Middleware;
use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Platforms\MySQL80Platform;
use Doctrine\DBAL\ServerVersionProvider;

class LengthAwareMysqlMiddleware implements Middleware
{
    public function wrap(Driver $driver): Driver
    {
        return new class($driver) extends Middleware\AbstractDriverMiddleware {
            public function getDatabasePlatform(ServerVersionProvider $versionProvider): AbstractPlatform
            {
                return new class extends MySQL80Platform {
                    public function getDateTimeTypeDeclarationSQL(array $column): string
                    {
                        $sql = parent::getDateTimeTypeDeclarationSQL($column);

                        if (isset($column['length']) && $column['length'] > 0) {
                            $sql .= '(' . $column['length'] . ')';
                        }

                        return $sql;
                    }
                };
            }
        };
    }
}
derrabus commented 3 weeks ago

Well, we still have the issue with the DATETIME(6) as @WubbleWobble pointed out in https://github.com/doctrine/migrations/issues/1441#issuecomment-2296926524

… and my answer to his comment still applies.

tomsykes commented 3 weeks ago

I still generally have a problem with the dropping of D2Ctype comments. By doing so. Doctrine has lost the ability to have more than one DBAL type having the same column definition.

When the comparator tries to detect if anything has changed, it's using the column definition to determine the DBAL type. When there are two DBAL types with the same column definition, it just picks the first one. When there were D2Ctype comments, the comparator could distinguish between the two DBAL types.

uncaught commented 3 weeks ago

When the comparator tries to detect if anything has changed, it's using the column definition to determine the DBAL type.

It does? I can't imagine this is the case. It would have to run through all possible parameters, too, to find the type based on a schema. Just imagine it finds a decimal(42,7) - how would that deterministically be resolved to a DBAL type?

All the migrations need to do is compare what is with what should be. And for that the comments were superfluous.

tomsykes commented 3 weeks ago

I think you're right @uncaught, you would expect the comparator to be generating a dummy schema based on the current state of the entities, and then compare that with the actual schema, but that's not what's happening.

@michnovka details how it works in their first comment

Notice the call to $platform->getDoctrineTypeMapping($dbType). It's resolving a doctrine type based on the database definition.

Edit: lets be more precise about this... the comparator isn't generating anything, it's just comparing what it's being given... but the point is the the AbstractSchemaManager + [PlatformSpecific]SchemaManager are providing the comparator with column definitions that are based on a resolved DBAL type, and not the actual underlying SQL type, so when it comes to the comparison it's not necessarily a fair representation of what's actually in the database - hence why the migrations are being continually generated.