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

Suggestion: enumType: Allow hydrating non-existing backed value as NULL (i.e. use ::tryFrom($value) instead of ::try($value)) #9433

Closed
ThomasLandauer opened this issue Jan 27, 2022 · 8 comments

Comments

@ThomasLandauer
Copy link
Contributor

Feature Request

Q A
New Feature yes
RFC no
BC Break no

Summary

TL;DR:
I'm suggesting an option for enumType so that when fetching a value from the database which is not present in the enum, the property is hydrated as null, instead of throwing a ValueError.

@derrabus: This is my answer to #9021 (comment), after gaining some real-world experience and some more thinking.

Real example: I'm having a Product entity which has an enum column type, holding some less-important information (just for internal use). Now I had to add a new case to TypeEnum. I did this only on the internal machine (where the admin backend is running), and didn't deploy it. Result: The product is not shown in production, even though - and this is the crucial part of the story - the type isn't even displayed there!

Lessons learnt: Whenever you need to reorganize the enum values, you need to have (1) the access rights, (2) the knowledge, and (3) the time to run the code deployment and the database migration simultaneously.

What I'd like to point out is that there are unimportant database columns (for statistics, internal use, whatever). So I think developers should have the choice to define enum columns as either "crucial" (i.e. exception when inconsistent) or "unimportant" (i.e. NULL when inconsistent).

I think it's happening at this line: https://github.com/doctrine/orm/blob/2.11.x/lib/Doctrine/ORM/Mapping/ReflectionEnumProperty.php#L71 So my suggestion would be to allow switching this to ::tryFrom().

@derrabus
Copy link
Member

As long as the feature is opt-in and does not generate too many edge cases, I'd be fine merging it. Do you want to provide a PR?

@ThomasLandauer
Copy link
Contributor Author

Do you want to provide a PR?

That's the answer I was most afraid of ;-)

I will give it a try - some questions in advance:

  1. Option or type? It's certainly better to add a new option to the existing enumType, rather than inventing a new type (e.g. nullableEnumType), right? So the mapping will look like this:
    #[ORM\Column(enumType: FooEnum::class, newOption: 'someValue' )]
  2. Which name? Something like nonBackedAsNull (with default false) sounds complicated, and still doesn't really convey what it's about ;-) So the best idea I'm having would be: enumMethod with possible values try (=default) and tryFrom, with the rationale: In order to make use of this, you need to understand how enums work anyway, so "try" and "tryFrom" are probably familiar terms.
  3. How can I access such an option from inside ReflectionEnumProperty::setValue()?
  4. Test: Just one additional test next to https://github.com/doctrine/orm/blob/2.11.x/tests/Doctrine/Tests/ORM/Functional/EnumTest.php#L66, asserting that null is returned and no exception thrown.

@michnovka
Copy link
Contributor

I took your idea and extended it to allow setting any default value in case DB value cannot be cast to corresponding BackedEnum type. To achieve your functionality, just set enumDefaultValue: null. If you instead want to use some actual enum value, use enumType: FooEnum::class, enumDefaultValue: FooEnum::DefaultOption

@beberlei
Copy link
Member

I am unsure about this, because why not handle all other kind of invalid data as null then? We throw exceptions when db has invalid dsta for a datetime for example.

Databases and ORMs enforce structure on data and migrations before deployment are essential.

Making an exception for enums feels wrong

@gimler
Copy link
Contributor

gimler commented Nov 8, 2023

So enumType did not allow nullable: true. I did not think it is a edge case.

So i cant use enumType instate use type: 'string' and use the getter and setter to map to php enum.

@derrabus
Copy link
Member

derrabus commented Nov 8, 2023

So enumType did not allow nullable: true.

It does.

@derrabus
Copy link
Member

derrabus commented Nov 8, 2023

To clarify: You can use enumType on a column with nullable: true. PHP null is stored as NULL in the database and SQL NULL will be hydrated as PHP null. This issue you're commenting on was about hydrating arbitrary garbage data as PHP null.

@gimler
Copy link
Contributor

gimler commented Nov 8, 2023

ahh my fail i found the problem. thanks for clarification.

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

No branches or pull requests

5 participants