-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Support for native PHP enums #9021
Comments
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! |
I disagree.
Yes, but others don't and I don't see why we should forbid using PHP enums on e.g. SQL Server.
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. |
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. |
I think @Nek- was talking about database 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 |
This commit does not contains a test because AFAIK for the moment there is no test scoped to a specific version of PHP. It fixes doctrine#9021
This commit does not contains a test because AFAIK for the moment there is no test scoped to a specific version of PHP. It fixes doctrine#9021
Related to issue doctrine/orm#9021 Related to PR ORM doctrine/orm#9149
Related to issue doctrine/orm#9021 Related to PR ORM doctrine/orm#9149
I believe we can support this on the ORM level if we add a new option to a fieldMapping #[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.
Transformation could be done with either a new ReflectionEnumProperty on the setValue/getvalue level or with a check for |
@beberlei Could we maybe implement this in more generic manner? If we expose not very specific to that particular use case
Then, those value converters could attached like that:
Converters should be also attachable via the Conversion process should probably look like that:
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. |
@kadet1090 it might be better to make |
@greg0ire Personally I think that we could have both - |
Moreover, |
a huge advantage for not using database 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. |
Related discussion at the DBAL level: doctrine/dbal#2841 |
Yes, this is an important question that hasn't been addressed so far. 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 |
The question in this case is: Is it really the data that's broken, or is just a I forgot to mention above that there's also A use case for "converting" them to null might be:
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 Edit: For the type safety: I didn't have arbitrary stings in mind as "fallback" value, but only other valid enum cases (e.g. |
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.
… and then we should've run a migration on our database, but we didn't. 🙃
If we substituted the broken value, the form would not reflect the broken data either, would it?
Right. A migration. |
As an average user who can't wait for enum support; my vote goes for option 1:
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 Even worse case is if the value is nullable and the logic depends on it. Suddenly some entities that had |
You're (essentially) saying:
I'm saying: Yes, it shouldn't. But it could happen. You're (essentially) saying:
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 |
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. |
Sure, then set it to
Right. If the user wants it to be this way, it wouldn't be visible. Just like |
I actually @ThomasLandauer here. But should the developer be forced to make that choice from the start or should there be a default? I personally think that if there should be a default the default should be |
Default! Forcing a choice is always bad, even more if the question is rather complicated.
I think |
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. |
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 I think that first of all we should decide if we want for every enum type to have dedicated DBAL type as suggested in doctrine/dbal#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:
Of course this is just an example, and parameters could be passed differently, maybe even like that:
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. |
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 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 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 . '[]')] |
My take on native enum type: doctrine/dbal#5147 |
This is an application level responsibility where an exception can be caught and handled. |
Feature Request
Summary
PHP 8.1 has introduced native enums to the language.
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:The text was updated successfully, but these errors were encountered: