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

Add support for PHP 8.1 enums. #9304

Merged
merged 1 commit into from
Jan 8, 2022
Merged

Conversation

beberlei
Copy link
Member

@beberlei beberlei commented Dec 30, 2021

ORM based support for enums.

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

#[Entity]
class Card
{
    /** ... */

    #[Column(type: 'string', enumType: Suit::class)]
    public $suit;

It works based on a new ReflectionEnumProperty that wraps around other reflection property types. This avoids the problem of other approaches that hook into DBAL Type abstraction, since that would require some kind of paramterization which is not supported at the moment. In addition it would at the moment require to register each enum explicitly, which this approach does not.

This includes support for typed property detection, the previous example could be written as:

#[Column]
public Suit $suit;

As for error handling, when the database returns an invalid enum case, then the regular ValueError is not caught that BackedEnum::from throws.

@beberlei beberlei added this to the 2.11.0 milestone Dec 30, 2021
lib/Doctrine/ORM/Mapping/Column.php Outdated Show resolved Hide resolved
lib/Doctrine/ORM/Mapping/Driver/AnnotationDriver.php Outdated Show resolved Hide resolved
lib/Doctrine/ORM/Mapping/ReflectionEnumProperty.php Outdated Show resolved Hide resolved
lib/Doctrine/ORM/Mapping/ReflectionEnumProperty.php Outdated Show resolved Hide resolved
tests/Doctrine/Tests/Models/Enums/Suit.php Outdated Show resolved Hide resolved
tests/Doctrine/Tests/ORM/Functional/EnumTest.php Outdated Show resolved Hide resolved
lib/Doctrine/ORM/Mapping/ReflectionEnumProperty.php Outdated Show resolved Hide resolved
lib/Doctrine/ORM/Mapping/ReflectionEnumProperty.php Outdated Show resolved Hide resolved
@derrabus
Copy link
Member

This looks like the most promising solution I've seen so far, thanks!

@derrabus derrabus linked an issue Dec 31, 2021 that may be closed by this pull request
@derrabus
Copy link
Member

derrabus commented Dec 31, 2021

For phpcs.xml.dist:

<rule ref="Generic.WhiteSpace.ScopeIndent.Incorrect">
    <!-- see https://github.com/squizlabs/PHP_CodeSniffer/issues/3474 -->
    <exclude-pattern>your/path/here</exclude-pattern>
</rule>
<rule ref="Generic.WhiteSpace.ScopeIndent.IncorrectExact">
    <!-- see https://github.com/squizlabs/PHP_CodeSniffer/issues/3474 -->
    <exclude-pattern>your/path/here</exclude-pattern>
</rule>

If you add the paths to your enum files here, PHPCS should allow you to fix the indentation manually.

@derrabus
Copy link
Member

I think the following error scenarios should be covered by tests:

  • enumType is set to a non-string or a string that is not a value enum FQCN.
  • The database contains a non-null value that is not part of the enum.

Furthermore, I think we should be able to detect type and enumType fairly easy for a typed property.

@beberlei
Copy link
Member Author

The BackedEnum usage is making Psalm crazy, a bit at loss how to workaround the class not existing in 8.0/7.4

@derrabus
Copy link
Member

The BackedEnum usage is making Psalm crazy, a bit at loss how to workaround the class not existing in 8.0/7.4

<UndefinedDocblockClass>
    <errorLevel type="suppress">
        <referencedClass name="BackedEnum"/>
    </errorLevel>
</UndefinedDocblockClass>

Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

Nice! This does look more straightforward than other approaches.

3 things:

  • should there be checks regarding the type? What should happen if an enum backed by an int is used within a mapping that uses a stringy type? And vice versa?
  • should we stop adding features to the YAML and Annotation drivers? They are deprecated.
  • there should be docs

One more reason why it makes sense to implement this at the ORM level: what we obtain in our mapped property is a user-defined type (the enum), just like embedded classes, and those are not implemented in the DBAL.

@beberlei
Copy link
Member Author

@greg0ire about your bullets:

  • The same issue already exists with typing private int $id and declaring it as type: "string" in mappings and so on. We don't validate that kind of missmatch at the moment. Maybe something for SchemaValidator?
  • I was thinking the same, but since we target 2.11 and its only a few lines for each driver I did them anyways. Problem would be that the Exporter classes don't have support for these new mapping things, so you cannot fully convert between a yml with new features to xml for example.
  • Docs will follow once i have everything else working :)

@filiplikavcan
Copy link

#[Entity]
class Card
{
    /** ... */

    #[Column(type: 'string', enumType: Suit::class)]
    public $suit;

type: 'string' can be inferred from enum reflection. I would prefer either #[Column(type: Suit::class)] or #[Enum(type: Suit::class)].

@beberlei
Copy link
Member Author

@filiplikavcan the tests usually go for the full information, you can just declare it as the type itself and Doctrine's automatic type completion will do that:

#[Column]
public Suit $suit;

@beberlei
Copy link
Member Author

I have everything working now exception Psalm not detecting my ignoring of ReflectionEnum: https://github.com/doctrine/orm/runs/4673434142?check_suite_focus=true

phpstan-baseline.neon Outdated Show resolved Hide resolved
Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

I just created #9310 to make the baseline changes unnecessary (will require a merge up from 2.10 to 2.11)

docs/en/reference/basic-mapping.rst Outdated Show resolved Hide resolved
@derrabus
Copy link
Member

derrabus commented Jan 1, 2022

I've modified EnumTest to cover the detection of the enum type property: beberlei#1

[$metadata->fieldMappings['id']['columnName'] => $card->id]
);

$this->expectException(ValueError::class);
Copy link
Member

Choose a reason for hiding this comment

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

I think we should catch and upcast this error. There is something wrong with the user's database and the user should be made aware of that. DBAL has a ConversionException for errors like this one. Either we reuse that or we declare our own exception for that.

Copy link
Member

@greg0ire greg0ire Jan 7, 2022

Choose a reason for hiding this comment

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

I think we should have our own one, it would be less confusing. The message could look like this:

Context: trying to hydrate Property "MyClass::$foo"
Problem: That property is of enum type "My\Enum", which does not accept value "value"
Solution: migrate the database value to a supported case or add a new case to the enum

@greg0ire
Copy link
Member

greg0ire commented Jan 7, 2022

It does make Psalm green, but now Phpstan is red… for reasons that have to do with the last release of the DBAL (2.10.x is also broken)

@greg0ire greg0ire merged commit 4117ca3 into doctrine:2.11.x Jan 8, 2022
@greg0ire
Copy link
Member

greg0ire commented Jan 8, 2022

Thanks for this great feature!

@sxsws
Copy link

sxsws commented Jan 8, 2022

A much awaited feature! Well done everyone!

@zmitic
Copy link
Sponsor

zmitic commented Jan 8, 2022

@beberlei

Thank you a lot for this feature, I have already implemented it and works without a problem.

Wish list:
is it possible to have type guessing when XML is used?

class BiomarkerRange
{
    public function __construct(
        private AgeCategoryEnum $ageCategory, 
    ) {}

If defined like this:

<field name="ageCategory" enum-type="AgeCategoryEnum"/>

everything works perfectly. But when I drop enum-type:

Cannot assign string to property BiomarkerRange::$ageCategory of type AgeCategoryEnum

It is not a big deal if not possible, I am still very happy even if I have to put it. But it would make it more consistent with other guessers.

@derrabus
Copy link
Member

derrabus commented Jan 8, 2022

Type guessing is currently only implemented for attributes. Please open an issue if you want to extend the feature to other mapping types, but my gut feeling is that this is a bad idea.

@ThomasLandauer
Copy link
Contributor

@beberlei You didn't write the docs yet, did you? If you want, I could start a PR, but I'd need help from you, since I don't know all the details.

@sxsws
Copy link

sxsws commented Jan 13, 2022

Just to be clear for this PR, are non-backed enums supported? Will the name value be stored as a string value?

@beberlei
Copy link
Member Author

Non backed enums are not supported, there is no value to store them with.

@sxsws
Copy link

sxsws commented Jan 13, 2022

The 'value' is the name of the case, surely?

@rkeet
Copy link

rkeet commented Jan 13, 2022

I would have to side with @sxsws on this. To quote PHP manual:

This type of case, with no related data, is called a "Pure Case." An Enum that contains only Pure Cases is called a Pure Enum.

All Pure Cases are implemented as instances of their enum type. The enum type is represented internally as a class.

All Cases have a read-only property, name, that is the case-sensitive name of the case itself. That may sometimes be useful for debugging purposes.

<?php
print Suit::Spades->name;
// prints "Spades"
?>

This would suggest to me that it's a valid, storable, value. The caveat is that the value matches the case.

This is different from the BackedEnum where the value (int|string) does not have to match the case. Example from the linked page:

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

// other documentation

<?php
print Suit::Clubs->value;
// Prints "C"
?>

Another difference is, the value of a Pure Enum is accessed with Enum::case->name, where as a Backed Enum requires Enum::case->value usage.

Differentiating between the can be done by using new UnitEnum interface, which is applied to all enum classes and the new BackedEnum interface, which, as the name implies, is only implied to BackedEnum instances.

This page also shows the existance of static method cases(), applied to all instances of UnitEnum, which returns all available cases, like so:

<?php
Suit::cases();
// Produces: [Suit::Hearts, Suit::Diamonds, Suit::Clubs, Suit:Spades]
?>

For mapping to a BackedEnum, it's documentation page shows methods from and tryFrom to map to a scalar instance, or a scalar instance or null respectively.

The above scalar mapping for BackedEnums does not have a corresponding function mentioned for mapping a scalar to a UnitEnum.


Having said all of the above, I'm excited already with just BackedEnums :)

@derrabus
Copy link
Member

All Cases have a read-only property, name, that is the case-sensitive name of the case itself. That may sometimes be useful for debugging purposes.

It clearly says "useful for debugging purposes", not "a fallback if there is not backed value".

This would suggest to me that it's a valid, storable, value. The caveat is that the value matches the case.

Probably, but it's not meant for storing. I've heard alternative proposals that use the fully qualified name of the case or even the PHP-serialized representation or assigning a sequencial number to each case. Those are all valid proposals, but who are we to decide which of these options is the "right" one if the developer deliberately omits the backed value, the one piece of information that tells us, "Look, this is the way this enum is meant to be stored"?

The thing is, we can only guess how such a pure enum should be serialized. If you implement a custom DBAL type for your pure enum that takes the name as backed value, you implement a convention for your own enum and that absolutely fine. But we cannot infer a general rule from that. I think, for a library as Doctrine, it is fair to demand that you define your enum as backed if you want it to be stored properly. Besides, I fail to see the difficulty of implementing a backed enum instead of a pure one.

Having said all of the above, I'm excited already with just BackedEnums :)

As am I. 🙂

nicolas-grekas added a commit to symfony/symfony that referenced this pull request Jan 14, 2022
This PR was merged into the 4.4 branch.

Discussion
----------

[DoctrineBridge] Fix invalid guess with enumType

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Since doctrine/orm#9304 doctrine allows Enums with the new `enumType` option.
ie.

```php
#[ORM\Column(type: 'string', enumType: Status::class, length: 1)]
public Status $status;
```

The issue is, it break validations at several places:
- `doctrinerExtractor` guess it's a `string` (Because of `type = string`), and validation adds the constraint `Assert\Type(string)`
- `doctrineLoader` guess it's a string with a maxLength and validation adds a constrain `Assert\Length(max=1)`
-

Commits
-------

68dd218 Fix invalid guess with enumType
@bastien70
Copy link

Very good PR ! So, if we're good, a use case could be :

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

#[Entity]
class Card
{
    #[Column(type: 'string', enumType: Suit::class)]
    private ?Suit $suit = null;

    public function setSuit(Suit $suit): self
    {
        $this->suit = $suit;

        return $this;
    }

    public function getSuit(): ?Suit
    {
        return $this->suit;
    }

Is it okay ?

@ThomasLandauer
Copy link
Contributor

It clearly says "useful for debugging purposes"

I just deleted that sentence from the PHP docs, see php/doc-en#1331

@ThomasLandauer
Copy link
Contributor

When doing it like this, I'm getting this "TypeError":

Cannot assign string to property App\Entity\Card::$suit of type ?App\Enum\Suit

After deleting the entire var/cache/dev directory, it works exactly once (i.e. after refreshing the page, the error is shown again)

Is this the problem that was fixed in symfony/symfony#45012 ?

@derrabus
Copy link
Member

Is this the problem that was fixed in symfony/symfony#45012 ?

Well, there is an easy way to find out, isn't there? If you encounter a problem with enum columns, please open a bug report.

@greg0ire
Copy link
Member

If you want, I could start a PR, but I'd need help from you, since I don't know all the details.

@ThomasLandauer did you end up starting something?

ThomasLandauer added a commit to ThomasLandauer/orm that referenced this pull request Mar 2, 2022
@greg0ire This is the first (small) step towards documenting enums - see doctrine#9304 (comment)
I started reading the page from the top, and did some minor changes.
Biggest change: If you agree, I'd continue adding php attributes to all code blocks.
ThomasLandauer added a commit to ThomasLandauer/orm that referenced this pull request Mar 3, 2022
This is the first (small) step towards documenting enums - see doctrine#9304 (comment)
I started reading the page from the top, and did some minor changes.
Biggest change: If you agree, I'd continue adding php attributes to all code blocks.
@FrancescJR
Copy link

How would be the equivalent of

 #[Column(type: 'string', enumType: Suit::class)]

on xml? I can't find that anywhere on any documentation

@FrancescJR
Copy link

answering myself, (I had other mapping issues and I thought were related to that but not).

 #[Column(type: 'string', enumType: Suit::class)]

equivalent is:

 <field name="[name-on-entity-attribute-forsuit" enumType="Namespace\To\Type\Suit::class"  column="your_suit_Column_name" type="string"/>

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

Successfully merging this pull request may close these issues.

Support for native PHP enums
10 participants