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

BC Break for findBy with custom enum types #10432

Open
michnovka opened this issue Jan 19, 2023 · 8 comments
Open

BC Break for findBy with custom enum types #10432

michnovka opened this issue Jan 19, 2023 · 8 comments

Comments

@michnovka
Copy link
Contributor

BC Break Report

Q A
BC Break yes
Version 2.12.x

Summary

This commit #9453 allows to use enums in findBy. However it breaks behavior when custom DBAL types use enums as their PHP values (can be seen here: Elao/PhpEnums#199 )

Previous behavior

Previously findBy() call for a custom DBAL type field which uses enum as values would call its convertToDatabaseValue with an enum instance.

Current behavior

After this commit, the convertToDatabaseValue receives the string or int (the actual value of the BackedEnum).

@michnovka
Copy link
Contributor Author

The question here is, do we want to fix this behavior. The only way would be to check first whether this is an enum using enumType (native Doctrine enum system) or its a custom DBAL type and then do not do the conversion to the BackedEnum value.

Alternatively we may just ignore this issue and say, that if working with custom DBAL types that use enum as the PHP value representation, you have to count with the possibility of receiving the actual value, not an enum instance, into your convertToDatabaseValue function.

Either way, this was a BC break and it was not documented

@michnovka
Copy link
Contributor Author

This seems a bit harder than I thought, as BasicEntityPersister::getValues is unaware of the type associated with the values. So it would require to pass it the types as well and do value casting based on the type of the value.

Maybe we should revert the #9453 changes, and instead do the conversion of enum to its value inside Connection or even Statement from DBAL?

@Antarian
Copy link

Antarian commented Feb 7, 2023

I hit this same bug and changed convertToDatabaseValue to accept string|int also.
But you need to pass BackedEnum class name to your custom Type, so you can check if string|int is valid value against expected BackedEnum value, otherwise you risk inserting values to DB you will not be able to convert back to enum.

@jazithedev
Copy link

jazithedev commented Oct 27, 2023

Occurred at my project as well. Also voting it should be changed so convertToDatabaseValue(...) would receive BackedEnum instead of string|int.

@beberlei
Copy link
Member

Why do you have custom enum types if i may ask

@michnovka
Copy link
Contributor Author

Well for me specifically its the ability to use native SQL ENUM type.

I have made my own types also with support of https://github.com/Elao/PhpEnums and it allows me great flexibility together with PHP enum attributes.

@Antarian
Copy link

Hard to recall after that long time. 2 lines of code from Doctrine Entity class:

#[Column(type: MyEnum::class)]
protected MyEnum $myEnum;

MyEnum is just normal PHP BackedEnum class.
This will also create comment over the table column in migrations, which is handy if you are not using native DB enums.

I know there was also a reason for custom types as adapting some enum libraries to Doctrine required that. I think many projects will have mix of some enum library and PHP enums together and to simplify keep code style with/to Doctrine same. That means to keep enums as custom types even they are now real PHP enums.

@jazithedev
Copy link

Why do you have custom enum types if i may ask

My case is the same as @michnovka's. I wanted to have ENUM type at the column definition of my table, and not a VARCHAR type (which resulted from using Doctrine's enumType).

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

4 participants