Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

add enum reflection property #248

Merged
merged 1 commit into from
Mar 13, 2022
Merged

Conversation

IonBazan
Copy link
Member

Reduces code duplication in doctrine/mongodb-odm#2412 and doctrine/orm#9304 by extracting the EnumReflectionProperty here. Please take note of slightly changed name and throwing native ValueException here instead of MappingException.

@malarzm malarzm added this to the 2.4.0 milestone Mar 6, 2022
@malarzm malarzm added the Enhancement New feature or request label Mar 6, 2022
@greg0ire
Copy link
Member

throwing native ValueException here instead of MappingException.

I think you mean ValueError. Besides, since doctrine/persistence has its own MappingException class, why not preserve the original behavior and wrap the ValueError in it?

@malarzm
Copy link
Member

malarzm commented Mar 13, 2022

I think you mean ValueError. Besides, since doctrine/persistence has its own MappingException class, why not preserve the original behavior and wrap the ValueError in it?

I'd say MappingException is for mapping errors :) I'm perfectly fine letting ValueError be thrown, for instance in ODM we'll catch it and wrap in our HydratorException: doctrine/mongodb-odm#2412 (comment)

@greg0ire
Copy link
Member

@malarzm I wasn't worried so much about preserving the type, which wouldn't even be achieved by my proposal, as about preserving the extra context that is added:

Context: Trying to hydrate enum property "%s::$%s"
Problem: Case "%s" is not listed in enum "%s"
Solution: Either add the case to the enum type or migrate the database column to use another case of the enum

If you have many entities/document using the same enum, and your database/document store contains an invalid value for that enum somewhere, it might help to know where.

HydratorException does seem like a better type for this, maybe we should introduce something similar in doctrine/persistence?

@malarzm
Copy link
Member

malarzm commented Mar 13, 2022

We'll totally do what you're proposing, but on ODM's level. I'm not sure how much value the exception will bring in this case. For instance in ODM the HydratorException is already extending our base exception and we can't change that. So we'd need to catch the new exception and later transform it into our one. Not sure if it's worth it :)

@greg0ire greg0ire merged commit 2e84e10 into doctrine:2.4.x Mar 13, 2022
@greg0ire
Copy link
Member

Thanks @IonBazan !

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants