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

PHP 8.1: new enum type #4930

Closed
wants to merge 1 commit into from
Closed

PHP 8.1: new enum type #4930

wants to merge 1 commit into from

Conversation

Nek-
Copy link

@Nek- Nek- commented Oct 25, 2021

Q A
Type feature
BC Break no
Fixed issues doctrine/orm#9021

Summary

Add support for PHP 8.1 enum into doctrine.

}

if ($value instanceof UnitEnum) {
return serialize($value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very much against writing the output of PHP's serialize function to the database. This makes this value pretty much unusable for human beings and non-PHP applications and writing queries against that field is not much fun.

PHP allows us to specify backed values for enums and that is what I would expect to find in the database. For instance, if we look at the example from the RFC:

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

In that case, H is a value I'd expect to see the in the database, and not E:11:"Suit:Hearts";.

Copy link
Author

@Nek- Nek- Oct 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understandable. I will try to work on something better! You're absolutely right about keeping the benefit of the value only.

@morozov
Copy link
Member

morozov commented Oct 26, 2021

I believe we discussed adding the support for enums somewhere earlier. It's still not clear what the semantics of the enum type in a database is. Should a given column allow any value of any Enum or only values of a specific Enum?

So far, there's no way to parameterize types in the DBAL but this is exactly the case where it seems absolutely necessary.

@greg0ire
Copy link
Member

Should a given column allow any value of any Enum or only values of a specific Enum?

I would say no, for reasons listed here: http://komlenic.com/244/8-reasons-why-mysqls-enum-data-type-is-evil/

This is a bit counter-intuitive and would warrant a paragraph in the documentation.

@morozov
Copy link
Member

morozov commented Oct 26, 2021

I would say no […]

This confirms that the support of enums in the DBAL is impossible in the current type system where all types are identified by their class name w/o the ability to parameterize them.

One of the related open issues: #2841.

@Nek-
Copy link
Author

Nek- commented Oct 26, 2021

To me it's possible by using many enum types:

  1. int_enum (int)
  2. string_enum (varchar)
  3. unit_enum (encoded as json array)

I talked about it there: doctrine/orm#9149 (comment)

But as I understand you want to close the #2841 issue before implementing any enum type?

As somebody out of the team I have to admit that it's not clear what #2841 will fix out of "better code to maintain". I secretly hope the ability to make multiple columns for a type but I read here "parameterize" type which really is not helping 😅 .

@greg0ire
Copy link
Member

Should a given column allow any value of any Enum or only values of a specific Enum?

I think I read this question too fast. What I meant to say is that we should not use ENUM i.e. constrain the values more than with types such as VARCHAR, INT, etc.

That does seem impossible to do with the current type system.

I read here "parameterize" type which really is not helping

I can't speak for @morozov , but I think parameterizing types means adding support for things syntaxes such as enum(integer) where right now you can just use integer, text, float, etc.

I don't think #2841 would directly bring that either, but it would make the type system more flexible, and such evolutions easier.

@Nek-
Copy link
Author

Nek- commented Oct 26, 2021

Thank you very much 👍 . I understand better the situation.

Still, my solution works with what exists already: adding 3 types (instead of 1 with a parameter that may have 3 values).

Finally, it's still possible to support only one type of those (the string enum seems to be the better to support).

Since I'm not a maintainer and thus I can't take a decision, I need you to tell me what solution to choose!

@greg0ire
Copy link
Member

greg0ire commented Oct 27, 2021

@Nek- after giving it more thought, I think @morozov was asking if there should be a check in the type class allowing only one type of enum. That would require a parameter when instantiating the type. I don't know if we should have that check or not. When using the ORM, it would probably be redundant if you use a typed property in your entity, right? I don't know if people use DBAL types without using the ORM.

@morozov
Copy link
Member

morozov commented Oct 27, 2021

I think @morozov was asking if there should be a check in the type class allowing only one type of enum. […] I don't know if we should have that check or not

Yes. The point of an enum is to limit the number of values to a specific set. What's the point in allowing any enum, which is effectively allowing a value from any set? If the type doesn't know what enum its values belong to, how does it deserialize the scalar from the database to the enum?

I don't if people use DBAL types without using the ORM.

If it was an ORM-only feature, why would it be implemented in the DBAL? I know that the built-in types are used w/o the ORM, e.g. the date types.

@derrabus
Copy link
Member

derrabus commented Oct 27, 2021

Let's outline the use case and use the enum from the RFC again:

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

Let's image, we have a table that stores data related to poker cards. That table would have a column suit, probably CHAR(1). What we need is a type that is able to reflect that: I don't want to insert any enum instances into that table, only instances of Suit.

That means, I want to pass Suit::Clubs to $connection->insert() and expect the value C to be inserted into the database. Passing an instance of a different enum than Suit should raise an exception.

Also, I should be able to select that value from the database, funnel it through our new type class and receive Suit::Clubs again. Again, if I pass a value that is not within the range of the enum, I'd expect an exception.

The tricky part is that the type would need to know about the enum class we're expecting. This is a problem because right now, the type classes are stateless. We have no way of communicating the actual enum class to the type.

We could of course evaluate if we can build that functionality in the ORM and bypass the type system here, but I believe it would make sense to have that functionality in the DBAL.

@morozov morozov marked this pull request as draft October 30, 2021 17:03
@morozov
Copy link
Member

morozov commented Oct 30, 2021

I've converted this pull request to a draft since it's not meant to be reviewed in its current state

@greg0ire greg0ire mentioned this pull request Nov 24, 2021
9 tasks
@derrabus derrabus changed the base branch from 3.2.x to 3.3.x November 30, 2021 16:22
@morozov morozov added the Types label Dec 10, 2021
@derrabus
Copy link
Member

derrabus commented Dec 13, 2021

I'm closing this PR now. We agree that this is not the solution we're looking for. We can discuss the feature further on the linked ORM issue.

Thank you very much for your proposal though.

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

Successfully merging this pull request may close these issues.

4 participants