doctrine / orm

Doctrine Object Relational Mapper (ORM)
https://www.doctrine-project.org/projects/orm.html
MIT License
9.87k stars 2.49k forks source link

Support for native PHP enums #9021

Closed derrabus closed 2 years ago

derrabus commented 2 years ago

Feature Request

Q A
New Feature yes
RFC no
BC Break no

Summary

PHP 8.1 has introduced native enums to the language.

enum Suit: string {
  case Hearts = 'H';
  case Diamonds = 'D';
  case Clubs = 'C';
  case Spades = 'S';
}

#[Entity]
class Card
{
    #[Column, Id]
    public int $id;

    #[Column]
    public Suit $suit;
}

I'd like to use an enum as type of a property. The ORM should store the enums in a VARCHAR field using the backed value. When hydrating such an entity, the enum should be hydrated properly.

The resulting table card would look like this:

id suit
1 H
2 H
3 S
4 C
Nek- commented 2 years ago

Using a VARCHAR looks weird because PostgresSQL, MariaDB, and MySQL support all enumerations. Is there any good reason (besides "it's work and we have nobody to do it") to not introduce the enumeration support in Doctrine?

Btw thank you very much for maintaining Doctrine!

derrabus commented 2 years ago

Using a VARCHAR looks weird

I disagree.

because PostgresSQL, MariaDB, and MySQL support all enumerations.

Yes, but others don't and I don't see why we should forbid using PHP enums on e.g. SQL Server.

Is there any good reason (besides "it's work and we have nobody to do it") to not introduce the enumeration support in Doctrine?

I personally stopped using native enums a long time ago because they rarely help. And from my point of view, they come with major drawbacks:

Even if we added support for MySQL's ENUM type to Doctrine one fine day, it think it should not be our default solution for PHP enums.

sicet7 commented 2 years ago

@derrabus

There is (at least last time I checked) no elegant way to determine a list of enum members.

Correct me if im wrong, but you should be able to call the method "cases" on the Enum to get a list of the members. Atleast from what i have read of the PHP 8.1 UnitEnum Interface.

greg0ire commented 2 years ago

I think @Nek- was talking about database ENUM here, not PHP's Enum. More reasons not to use enum here: http://komlenic.com/244/8-reasons-why-mysqls-enum-data-type-is-evil/

Also, I've used them myself at work recently, and failed to switch to varchar afterwards because the table I wanted to migrate has too many rows for the migration to go smoothly. So really, avoid ENUM.

beberlei commented 2 years ago

I believe we can support this on the ORM level if we add a new option to a fieldMapping enumType and allow setting enum fields using a new annotation, for example:

#[Enum(type: "string", class: Suit::class)]
public $suit;

#[Enum]
public Suit $autoSuit;

This way we get around the problem of DBAL types not being parameterized.

type can be string or integer or if people want their custom enum tpyes for the database.

Transformation could be done with either a new ReflectionEnumProperty on the setValue/getvalue level or with a check for $fieldMapping['enumType'] in hydrators/persisters.

kadet1090 commented 2 years ago

This way we get around the problem of DBAL types not being parameterized.

type can be string or integer or if people want their custom enum tpyes for the database.

Transformation could be done with either a new ReflectionEnumProperty on the setValue/getvalue level or with a check for $fieldMapping['enumType'] in hydrators/persisters.

@beberlei Could we maybe implement this in more generic manner? If we expose not very specific to that particular use case enumType property but ability do attach generic converters (one of them could be builtin EnumConverter) we will open more possibilities for users. To name a few of this possibilities:

Then, those value converters could attached like that:

#[Column(type: "string")]
#[ValueConverter(converter: EnumConverter::class, options: ["class" => Suit:class])]
public $suit;

Converters should be also attachable via the Events::loadClassMetadata to support more dynamic scenarios and also maybe dependency injection, as converters could have some more advanced logic.

Conversion process should probably look like that:

Hydration:
SQL Value -> Type::convertToPHPValue(...) -> ValueConverter::convertToEntityValue(..., type) (if applicable) -> PHP Value
Persistence:
PHP Value -> ValueConverter::convertToDatabaseValue(..., type) (if applicable) -> Type::convertToDatabaseValue(...) -> SQL Value

It will also allow users to have more control over how to store their enums, without limiting them to provided choices as it always will be possible to implement own converter for underlying type.

greg0ire commented 2 years ago

@kadet1090 it might be better to make Type instances parametrizable if we are going that way.

kadet1090 commented 2 years ago

@greg0ire Personally I think that we could have both - Types could specify how value is stored inside database, and ValueConverters could provide way of converting those values into something that the class wants. But said that Type parametrization would allow to implement such behavior without much effort so I think that you're right that this would be overall better approach.

greg0ire commented 2 years ago

Moreover, Type has a convertToPHPValue so they are very much about conversion already.

azjezz commented 2 years ago

a huge advantage for not using database ENUM type is that you can add/remove cases from your PHP enum without having to execute an additional migration to inform the database about the new case.

but there's also the case where you remove an enum case, and doctrine won't know what to do with the value since it has been removed from the PHP enum.

greg0ire commented 2 years ago

Related discussion at the DBAL level: https://github.com/doctrine/dbal/issues/2841

ThomasLandauer commented 2 years ago

but there's also the case where you remove an enum case, and doctrine won't know what to do with the value since it has been removed from the PHP enum.

Yes, this is an important question that hasn't been addressed so far. I'm suggesting: Let the user decide in an option:

derrabus commented 2 years ago

I'm suggesting: Let the user decide in an option

I don't see many benefits in option 2, I must admit. It breaks type safety (we would go from MyEnum to MyEnum | 'the default') and broken data is silently hidden. The DBAL usually throws ConversionExceptions if the database value cannot be parsed, see JsonType for example.

ThomasLandauer commented 2 years ago

and broken data is silently hidden

The question in this case is: Is it really the data that's broken, or is just a case missing in the Enum in PHP? From a database (i.e. DBAL) point of view, the string will always be "valid" (contrary to JSON). But you certainly have way more overview than me.

I forgot to mention above that there's also Enum::tryFrom() in PHP, which returns null if there's no value: https://www.php.net/manual/language.enumerations.backed.php

A use case for "converting" them to null might be:

We used to have 4 card suits in the past, but then we dropped Hearts.

If an exception is thrown, there's not even a way to open the (invalid) entity in a form (to manually change the value), right? So running an UPDATE in the database would be the only way out?

Edit: For the type safety: I didn't have arbitrary stings in mind as "fallback" value, but only other valid enum cases (e.g. Suit::Diamonds); so the return type would still be MyEnum (or ?MyEnum)

derrabus commented 2 years ago

and broken data is silently hidden

Is it really the data that's broken, or is just a case missing in the Enum in PHP?

From the applications PoV, where's the difference? If the enum's missing a case, either the enum's broken or someone has inserted something that they shouldn't have. Either way, this is something that should be fixed and not silently ignored.

A use case for "converting" them to null might be:

We used to have 4 card suits in the past, but then we dropped Hearts.

… and then we should've run a migration on our database, but we didn't. 🙃

If an exception is thrown, there's not even a way to open the (invalid) entity in a form (to manually change the value), right?

If we substituted the broken value, the form would not reflect the broken data either, would it?

So running an UPDATE in the database would be the only way out?

Right. A migration.

zmitic commented 2 years ago

As an average user who can't wait for enum support; my vote goes for option 1:

Just keep what PHP does natively, i.e. throw the ValueError of Enum::from('foo');

This is simple to spot and remind users to create a migration. But setting to null will create more problems for non-nullable cases:

class User
{
    public function __construct(
        private MyEnum $subscriptionLevel,
    ){}

    public function getSubscriptionLevel(): MyEnum
    {
        return $this->subscriptionLevel;
    }  
}

This would throw Return value must be of MyEnum, null returned and confuse developers, including me.


Even worse case is if the value is nullable and the logic depends on it. Suddenly some entities that had Hearts assigned get null and create chain reaction of problems, even if developer makes a typo in enum. It could take long time before the problem gets detected.

ThomasLandauer commented 2 years ago

You're (essentially) saying:

That the database is not in sync with the Enum shouldn't happen.

I'm saying: Yes, it shouldn't. But it could happen.

You're (essentially) saying:

Just setting it to null might cause problems.

I'm saying: Yes. But throwing an exception might cause problems too! Example: "I don't care what the user's favorite color was, if we don't have that anymore. But I need to see her phone number now."

So if the user wants the rule to be "Just delete those outdated values, and don't bother me." I think, this should be possible.

So I think in the end it comes down to: Let the user decide if Enum::from() or Enum::tryFrom() is used.

derrabus commented 2 years ago

You're (essentially) saying:

That the database is not in sync with the Enum shouldn't happen.

No, I'm saying: I wanna know if that happens because this has to be fixed. And you so far failed to point out a good way to make that inconsitency visible.

ThomasLandauer commented 2 years ago

I wanna know if that happens because this has to be fixed.

Sure, then set it to Enum::from() ;-) I don't want to convince you (or anybody else) to actually set it to Enum::tryFrom() in your projects. I just think it should be possible.

And you so far failed to point out a good way to make that inconsistency visible.

Right. If the user wants it to be this way, it wouldn't be visible. Just like ON DELETE SET NULL (or basically any cascade): an invisible system to preserve data integrity.

sicet7 commented 2 years ago

I actually @ThomasLandauer here. I can't see the problem in letting the developer choose which method of determining value is used, even though i would almost never choose Enum::tryFrom() it's good to have the choice.

But should the developer be forced to make that choice from the start or should there be a default? And if there is a default what should that be? :thinking:

I personally think that if there should be a default the default should be Enum::from() as it keeps the Exception closest to the source of the problem :-)

ThomasLandauer commented 2 years ago

But should the developer be forced to make that choice from the start or should there be a default?

Default! Forcing a choice is always bad, even more if the question is rather complicated.

what should that be?

I think Enum::from() too. A nice error message like "Value 'H' not found in enum Suit" is certainly better than what would be the alternative (as @zmitic pointed out above): "Return value must be of type Suit, null returned" ;-)

derrabus commented 2 years ago

I wanna know if that happens because this has to be fixed.

Sure, then set it to Enum::from() ;-)

Well, let's put it that way: If someone wants to shoot themselves in the foot, they're free to do so, but I don't have to hand them the gun. And the more winking smilies you add to your reasoning, the less I'm convinced that you take this discussion seriously. I have asked you multiple times to address the concern I've raised and your best answer is to not use the option you're proposing. I don't think that we will find a common ground this way.

We're currently working hard on this repository to remove functionality that rarely anybody uses, so we don't have to maintain that in the future. Every option creates an edge-case that needs to be maintained for years. So please forgive me if I question proposals to add new footguns.

On top of that, we're discussing adding options to a feature that does not even exist as a prototype yet. This is a waste of everybody's time. Please let's find a way to create the basic functionality before we discuss the icing on the cake.

kadet1090 commented 2 years ago

Every option creates an edge-case that needs to be maintained for years. So please forgive me if I question proposals to add new footguns.

Yes, doctrine should not provide any new footguns, but should not forbid (or make unnecessarily hard) creating them in application code where it is responsibility of the app developer to maintain them.

Probably the most reasonable default behavior would be to use Enum::from(...) semantics, but this may not be good for all applications, so there should be a way to alter this. I don't think that we will ever find one solution that fits all and that's why I think that we should implement something more generic.

I think that first of all we should decide if we want for every enum type to have dedicated DBAL type as suggested in https://github.com/doctrine/dbal/issues/2841. While I strongly disagree with that as it would mean a lot of unnecessary and repetitive boilerplate code this will solve this discussion because any developer would be able to just overwrite conversion logic to fit application needs.

Another solution would be to allow DBAL types to be parametrized, so we could have:

#[Column(type: "enum", options: ['class' => Suit::class])]

Of course this is just an example, and parameters could be passed differently, maybe even like that:

#[Column(type: new EnumType(class: Suit::class))]

In that case custom logic (which will be required at some point) could be implemented simply by creating new type (that may extend or decorate default one). This will allow to create type once and handle all similar cases - and probably there will be many of them.

emmanuelballery commented 2 years ago

~Why not considering Enum classes valid DBAL types?~ <---- As a quick workaround.

In fact, I'm more for a new Type system with more flexibility. Something like what Symfony did with Form types or ApiPlatform with its ApiFilter (it's not that flexible though).

#[Column(type: StringColumn::class, options: ['length' => 50])]
#[Column(type: EnumColumn::class, options: ['class' => Suit::class])]
#[Column(type: IntegerColumn::class, options: ['nullable' => true])]

For those waiting, here is a quick solution currently working:

enum ActionEnum: string
{
    case A = 'a';
    case B = 'b';
}

Use your enum class as type:

#[Column(type: ActionEnum::class)]

Register your enums as types:

EnumType::addEnumType(ActionEnum::class);

// ...
EnumType::addEnumType(AnotherEnum::class);
EnumType::addEnumType(AgainAnotherEnum::class);

ℹ️ The TypeRegistry could improve this step if it was able to test enum_exists and automatically register it for us;

Trick the type registry to store the class in your type:

use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Types\Type;

class EnumType extends Type
{
    private string $class;

    public static function addEnumType(string $class): void
    {
        self::addType($class, self::class);
        self::getType($class)->class = $class; // <------------------------------------------ here
    }

    public function getName(): string
    {
        return $this->class;
    }

    public function getSQLDeclaration(array $column, AbstractPlatform $platform): string
    {
        $class = $this->class;
        $values = array_map(static fn(BackedEnum $enum): string => $enum->value, $class::cases());
        $column['length'] = max(0, ...array_map('mb_strlen', $values));

        return $platform->getVarcharTypeDeclarationSQL($column);
    }

    public function convertToDatabaseValue($value, AbstractPlatform $platform): ?string
    {
        return $value?->value;
    }

    public function convertToPHPValue($value, AbstractPlatform $platform): ?BackedEnum
    {
        $class = $this->class;

        return null === $value ? null : $class::from($value);
    }
}

❗Doctrine Proxies do not support default enum value, use __construct for the defaults:

public TypeEnum $type = TypeEnum::DEFAULT; // <------ will mess up with the FQCN in the proxy

With a little more code it's also possible to support lists of enums. I personaly use the serializer syntax and store it in a JSON column:

#[Column(type: ActionEnum::class . '[]')]
filiplikavcan commented 2 years ago

My take on native enum type: https://github.com/doctrine/dbal/pull/5147

filiplikavcan commented 2 years ago

I don't want to convince you (or anybody else) to actually set it to Enum::tryFrom() in your projects. I just think it should be possible.

This is an application level responsibility where an exception can be caught and handled.