add BackedEnum support #2412

IonBazan · 2022-02-20T07:14:24Z

Q	A
Type	feature
BC Break	no
Fixed issues	#2404

Summary

~~Provides native support for BackedEnums. This could be later integrated with #2411 to allow even better DX.~~
Already integrated in this PR 🚀

Additional changes:

Coding Standards workflow runs on PHP 8.1 now and ignores the enum case indentation due to PHP 8.1 enum not recognized squizlabs/PHP_CodeSniffer#3474
Locks PHPStan < 1.5.0 as it reports some false positives due to a bug in template intersection types

IonBazan · 2022-02-21T07:57:31Z

This is now ready for review. Please note that the "required" Coding Standards run will not be executed because I changed the PHP version there so the required name no longer match repository settings.

Also, it will produce conflicts with #2411 - depending on which PR is merged first, the other one must be updated to bring typed property guessing for enums.

malarzm

Great you took care of porting adding the feature! Beside my remarks above please also add a relevant documentation.

Food for thought: in the past it was requested that we pass $mapping to types. While I'm still reluctant to do so it'd make the implementation easier as we'd just add new enum type that would read target enum from the mapping. Thoughts on this?

.github/workflows/coding-standards.yml

malarzm · 2022-02-21T19:49:23Z

lib/Doctrine/ODM/MongoDB/Mapping/Annotations/Field.php

+     */
+    public function __construct(
+        ?string $name = null,
+        ?string $type = 'string',


We need to remember to change the default value here as #2411 did

malarzm · 2022-02-21T19:52:49Z

lib/Doctrine/ODM/MongoDB/Mapping/Driver/XmlDriver.php

@@ -275,6 +275,10 @@ public function loadMetadataForClass($className, \Doctrine\Persistence\Mapping\C
                    $mapping['notSaved'] = ((string) $attributes['not-saved'] === 'true');
                }

+                if (isset($mapping['enum-type'])) {


This looks fishy given the check above and below are checking (and assigning from) the $attributes array.

malarzm · 2022-02-21T19:56:21Z

lib/Doctrine/ODM/MongoDB/Mapping/MappingException.php

+    public static function nonEnumTypeMapped(string $className, string $fieldName, string $enumType): self
+    {
+        return new self(sprintf(
+            'Attempting to map non-enum type %s as enum in document class %s::$%s',


Suggested change

'Attempting to map non-enum type %s as enum in document class %s::$%s',

'Attempting to map a non-enum type %s as an enum: %s::%s',

This should be the most consistent with other exceptions we have

malarzm · 2022-02-21T19:57:36Z

lib/Doctrine/ODM/MongoDB/Mapping/MappingException.php

+
+    public static function enumsRequirePhp81(string $className, string $fieldName): self
+    {
+        return new self(sprintf('Enum types require PHP 8.1 in %s::$%s', $className, $fieldName));


Suggested change

return new self(sprintf('Enum types require PHP 8.1 in %s::$%s', $className, $fieldName));

return new self(sprintf('Enum types require PHP 8.1 in %s::%s', $className, $fieldName));

We don't use $ in other exceptions

malarzm · 2022-02-21T19:59:21Z

lib/Doctrine/ODM/MongoDB/Mapping/MappingException.php

+        ValueError $previous
+    ): self {
+        return new self(sprintf(
+            'Trying to hydrate enum property "%s::$%s". Problem: Case "%s" is not listed in enum "%s"',


Suggested change

'Trying to hydrate enum property "%s::$%s". Problem: Case "%s" is not listed in enum "%s"',

'Trying to hydrate an enum property %s::%s, but "%s" is not listed as one of "%s" cases',

malarzm · 2022-02-21T20:00:24Z

lib/Doctrine/ODM/MongoDB/Mapping/MappingException.php

+     * @param class-string             $className
+     * @param class-string<BackedEnum> $enumType
+     */
+    public static function invalidEnumValue(


This one is definitely not a mapping exception but rather a https://github.com/doctrine/mongodb-odm/blob/2.3.x/lib/Doctrine/ODM/MongoDB/Hydrator/HydratorException.php

malarzm · 2022-02-21T20:03:08Z

lib/Doctrine/ODM/MongoDB/Mapping/ReflectionEnumProperty.php

+use function is_int;
+use function is_string;
+
+class ReflectionEnumProperty extends ReflectionProperty


It's a pity this class did not went into a common repository. If you're not willing to pursue this then it should be final and marked as @internal

@beberlei do you have any insights why ReflectionEnumProperty isn't a part of https://github.com/doctrine/persistence/tree/2.3.x/lib/Doctrine/Persistence/Reflection?

While waiting for doctrine/common merge and release we could add this class (final and internal). Once doctrine/common is there we'll change implementation :)

@malarzm What about the exception thrown? The doctrine/persistence one throws native ValueError, while here we are converting it to a HydratorException which is not available in doctrine/persistence. I suggest simplifying this implementation to throw native error instead too for easier migration.

The HydratorInterface doesn't make any promises on what exceptions can be thrown, but generally it throws HydratorException when something is off with data (https://github.com/doctrine/mongodb-odm/blob/2.3.x/lib/Doctrine/ODM/MongoDB/Hydrator/HydratorFactory.php#L333 and more, look for throw). If possible and relatively simple I'd wrap native ValueError with our HydratorException. If it's not simple then let's the native error be thrown, we can't save users from everything :)

lib/Doctrine/ODM/MongoDB/Mapping/ReflectionEnumProperty.php

malarzm · 2022-02-21T20:21:21Z

tests/Doctrine/ODM/MongoDB/Tests/Mapping/xml/Documents81.Card.dcm.xml

+    <document name="Documents81\Card">
+        <id />
+        <field name="suit" type="string" enum-type="Documents81\Suit" />
+        <field name="nullableSuit" type="string" enum-type="Documents81\Suit" nullable="true" />


When we go down type-inferring rule we'll need to think how to correctly choose string vs int for the type conversion.

We can check if the enum class implements StringBackedEnum or IntBackedEnum to detect the underlying type.

malarzm · 2022-03-08T16:15:43Z

#2411 was merged, this needs to be rebased (it's conflicting anyway) and adapted :)

IonBazan · 2022-03-08T17:03:48Z

@malarzm rebased and adjusted accordingly. I will keep it as separate commits for cleaner diffs but I'll squash them once approved.

lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php

IonBazan · 2022-03-22T11:13:47Z

Since doctrine/persistence v2.4 is released, we can now use it. I decided to drop the custom ReflectionProperty and keep throwing the ValueError if something goes wrong (to be consistent with normal fields).

Co-authored-by: Fran Moreno <franmomu@gmail.com>

malarzm · 2022-03-27T11:01:39Z

lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php

@@ -2556,6 +2575,15 @@ private function validateAndCompleteTypedFieldMapping(array $mapping): array
            return $mapping;
        }

+        if (PHP_VERSION_ID >= 80100 && ! $type->isBuiltin() && enum_exists($type->getName(), true)) {


Please add this feature to documentation https://github.com/doctrine/mongodb-odm/pull/2411/files#diff-239d9a128ce24a76e204d8b9821ce009ef5c7b16298d549165da9bc05d5d8f97 created

malarzm · 2022-03-27T11:06:49Z

lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php

+            $reflection = new ReflectionEnum($type->getName());
+            $type       = $reflection->getBackingType();
+
+            assert($type instanceof ReflectionNamedType);


null case is not handled (assert is just for PHPStan). Using a non-backed enum should warrant an exception

VincentLanglet · 2022-04-29T18:14:50Z

What's need to finish this PR ? Do you need help @IonBazan ?
Seems to be a blocking point for the 2.4 version with doctrine/persistence v3 support (cf #2428 (comment))

malarzm · 2022-04-29T18:36:03Z

FWIW I'll be wrapping this up over the weekend.

IonBazan · 2022-04-30T15:21:41Z

Thanks @malarzm for taking over. Sorry, I didn't have enough time to make it prod-ready.

PS. Let me know if you want me to squash anything before you add your changes.

malarzm · 2022-05-03T12:10:34Z

Merged in #2440

IonBazan added the Feature label Feb 20, 2022

IonBazan added this to the 2.4.0 milestone Feb 20, 2022

IonBazan self-assigned this Feb 20, 2022

IonBazan force-pushed the feature/enum-support branch 11 times, most recently from 598dd69 to 76d0553 Compare February 21, 2022 07:31

IonBazan requested review from malarzm, franmomu, greg0ire, alcaeus and beberlei February 21, 2022 07:56

IonBazan marked this pull request as ready for review February 21, 2022 07:56

malarzm requested changes Feb 21, 2022

View reviewed changes

IonBazan force-pushed the feature/enum-support branch from cf4e5b2 to 0184214 Compare February 22, 2022 05:42

This was referenced Feb 22, 2022

Use PHP 8.1 to run CS #2413

Merged

add enum reflection property doctrine/persistence#248

Merged

IonBazan force-pushed the feature/enum-support branch from 8fb701f to ac9913e Compare March 8, 2022 16:54

franmomu reviewed Mar 8, 2022

View reviewed changes

lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php Outdated Show resolved Hide resolved

lib/Doctrine/ODM/MongoDB/Mapping/ClassMetadata.php Outdated Show resolved Hide resolved

IonBazan requested review from franmomu and malarzm March 22, 2022 11:11

IonBazan force-pushed the feature/enum-support branch from 0d5ce15 to 44dd61d Compare March 25, 2022 11:13

IonBazan and others added 6 commits March 25, 2022 19:18

add BackedEnum support

60a2533

Fix exception messages and XML Driver

09329b5

Revert ReflectionEnumProperty change

2799b1f

add type autoconfiguration

c05b534

make enumType non-nullable

a8a337d

Co-authored-by: Fran Moreno <franmomu@gmail.com>

use doctrine/persistence v2.4

0c6d320

IonBazan force-pushed the feature/enum-support branch from 44dd61d to 0c6d320 Compare March 25, 2022 11:18

avoid PHPStan 1.5.0

ede58d4

malarzm reviewed Mar 27, 2022

View reviewed changes

IonBazan mentioned this pull request Apr 8, 2022

Add Doctrine ODM integration Elao/PhpEnums#176

Merged

7 tasks

malarzm mentioned this pull request Apr 27, 2022

Allow doctrine/persistence ^3.0 #2428

Merged

malarzm mentioned this pull request May 2, 2022

add BackedEnum support #2440

Merged

malarzm closed this May 3, 2022

malarzm mentioned this pull request May 11, 2022

Allow using enums directly in queries #2447

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add BackedEnum support #2412

add BackedEnum support #2412

IonBazan commented Feb 20, 2022 •

edited

Loading

IonBazan commented Feb 21, 2022 •

edited

Loading

malarzm left a comment

malarzm Feb 21, 2022

malarzm Feb 21, 2022 •

edited

Loading

malarzm Feb 21, 2022

malarzm Feb 21, 2022

malarzm Feb 21, 2022

malarzm Feb 21, 2022

malarzm Feb 21, 2022

malarzm Feb 21, 2022

malarzm Mar 6, 2022

IonBazan Mar 8, 2022

malarzm Mar 8, 2022 •

edited

Loading

malarzm Feb 21, 2022

IonBazan Feb 22, 2022

malarzm commented Mar 8, 2022

IonBazan commented Mar 8, 2022

IonBazan commented Mar 22, 2022 •

edited

Loading

malarzm Mar 27, 2022

malarzm Mar 27, 2022

VincentLanglet commented Apr 29, 2022 •

edited

Loading

malarzm commented Apr 29, 2022

IonBazan commented Apr 30, 2022 •

edited

Loading

malarzm commented May 3, 2022

	'Attempting to map non-enum type %s as enum in document class %s::$%s',
	'Attempting to map a non-enum type %s as an enum: %s::%s',

	return new self(sprintf('Enum types require PHP 8.1 in %s::$%s', $className, $fieldName));
	return new self(sprintf('Enum types require PHP 8.1 in %s::%s', $className, $fieldName));

	'Trying to hydrate enum property "%s::$%s". Problem: Case "%s" is not listed in enum "%s"',
	'Trying to hydrate an enum property %s::%s, but "%s" is not listed as one of "%s" cases',

add BackedEnum support #2412

add BackedEnum support #2412

Conversation

IonBazan commented Feb 20, 2022 • edited Loading

Summary

Additional changes:

IonBazan commented Feb 21, 2022 • edited Loading

malarzm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malarzm Feb 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malarzm Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malarzm commented Mar 8, 2022

IonBazan commented Mar 8, 2022

IonBazan commented Mar 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentLanglet commented Apr 29, 2022 • edited Loading

malarzm commented Apr 29, 2022

IonBazan commented Apr 30, 2022 • edited Loading

malarzm commented May 3, 2022

IonBazan commented Feb 20, 2022 •

edited

Loading

IonBazan commented Feb 21, 2022 •

edited

Loading

malarzm Feb 21, 2022 •

edited

Loading

malarzm Mar 8, 2022 •

edited

Loading

IonBazan commented Mar 22, 2022 •

edited

Loading

VincentLanglet commented Apr 29, 2022 •

edited

Loading

IonBazan commented Apr 30, 2022 •

edited

Loading