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.

Sign up for GitHub

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

Jump to bottom

Allow enum discriminator columns #10288

Merged
merged 1 commit into from
Dec 13, 2022
Merged

Allow enum discriminator columns #10288

merged 1 commit into from
Dec 13, 2022

Conversation

michnovka
Copy link
Contributor

@michnovka michnovka commented Dec 11, 2022 •
edited
Loading

In all hydrators, discriminator columns are for all purposes converted to string using explicit (string) conversion. This is not allowed with PHP enums. Therefore, I added code which checks whether the variable is a BackedEnum instance and if so, instead its ->value is used (which can be cast to string without issues as its a scalar)

The test builds upon #6141 which allowed objects to be discriminator column values. This fix further extends the use-case to allow PHP enums also.

@michnovka michnovka force-pushed the 2.13.x-enum-discriminator-columns branch from 1471a77 to 0ef8084 Compare December 11, 2022 16:51
@michnovka
Copy link
Contributor Author

@greg0ire please review

greg0ire
greg0ire reviewed Dec 11, 2022
View reviewed changes
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.

Kudos on the great commit message, it makes it really easier to understand what's happening here.

Any way you could test the scenarios Codecov is pointing at?

@derrabus
Copy link
Member

I'm a bit hesitant to treat this as a bugfix. Let's target 2.14, please.

@derrabus derrabus changed the base branch from 2.13.x to 2.14.x December 11, 2022 18:50
@michnovka michnovka force-pushed the 2.13.x-enum-discriminator-columns branch 3 times, most recently from af1350b to c1c089a Compare December 11, 2022 19:04
@michnovka
Copy link
Contributor Author

@derrabus I have rebased to 2.14.x
@greg0ire I have cleaned up the code. Turns out the only problematic place was ObjectHydrator, the other Hydrators do not convert discriminator into enum at the time when they work with it. Ive added test for SimpleHydrator as well to prove this point and throw an exception should this behavior change in the future

greg0ire
greg0ire previously approved these changes Dec 11, 2022
View reviewed changes
@derrabus derrabus added this to the 2.14.0 milestone Dec 12, 2022
derrabus
derrabus requested changes Dec 12, 2022
View reviewed changes
Copy link
Member

@derrabus derrabus left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Blocking the merge until #10288 (comment) is resolved.

@derrabus derrabus removed this from the 2.14.0 milestone Dec 12, 2022
@michnovka michnovka force-pushed the 2.13.x-enum-discriminator-columns branch 2 times, most recently from b9cb653 to 8fb653e Compare December 12, 2022 21:19
@michnovka
Copy link
Contributor Author

@derrabus I have implemented what you asked for.

enumType is now added to DiscriminatorColumn, so it can (and is) properly hydrated.

I have added tests to MappingDriverTestCase for the new option as well as specific Functional tests which tests both custom type using enum and "native doctrine" enumType.

please review

@michnovka michnovka requested a review from derrabus December 12, 2022 21:36
@michnovka michnovka force-pushed the 2.13.x-enum-discriminator-columns branch 3 times, most recently from 210a27a to ef0ff51 Compare December 13, 2022 09:40
@derrabus derrabus closed this Dec 13, 2022
@derrabus derrabus reopened this Dec 13, 2022
@michnovka
Add support for enum discriminator columns
497ee16 
This commit adds enumType option to DiscriminatorColumn as well as support for custom DBAL types which use PHP enums for PHP values.
Previously, the enumType option was completely missing, but also even using custom types that used PHP enums would end up in exception because ObjectHydrator would try to convert enums to string using (string) explicit conversion.
Apart from hydrators, ClassMetadataBuilder was extended to support specifying enumType.
Documentation was updated.
@michnovka michnovka force-pushed the 2.13.x-enum-discriminator-columns branch from b2211c6 to 497ee16 Compare December 13, 2022 10:15
@michnovka
Copy link
Contributor Author

I am happy with the PR now, I think it covers everything:

  • add support to all mapping drivers
  • add support to ClassMetadataBuilder
  • add support into hydrators
  • add tests for everything newly implemented
  • add and fix documentation
  • squash commits and add a single descriptive commit message

please review @greg0ire @derrabus

@derrabus derrabus requested a review from greg0ire December 13, 2022 18:55
@derrabus derrabus added this to the 2.14.0 milestone Dec 13, 2022
@greg0ire greg0ire merged commit 15058ca into doctrine:2.14.x Dec 13, 2022
@greg0ire
Copy link
Member

Thanks @michnovka !

@michnovka
Copy link
Contributor Author

please merge into 3.x so that I can do the cleanup. thanks!

@github-actions github-actions bot mentioned this pull request Dec 19, 2022
Merged
@greg0ire greg0ire mentioned this pull request Mar 29, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derrabus derrabus derrabus approved these changes

@greg0ire greg0ire Awaiting requested review from greg0ire

Assignees
No one assigned
Labels
New Feature
Projects
None yet
Milestone
2.14.0
Development

Successfully merging this pull request may close these issues.
3 participants
@michnovka @derrabus @greg0ire