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

Support for 3rd party enum types broken #10249

Open
ovrflo opened this issue Nov 24, 2022 · 1 comment
Open

Support for 3rd party enum types broken #10249

ovrflo opened this issue Nov 24, 2022 · 1 comment

Comments

@ovrflo
Copy link

ovrflo commented Nov 24, 2022

BC Break Report

Q A
BC Break yes
Version 2.13.4

Summary

I'm using a 3rd party implementation for PHP 8.1 enums (technically, a fork of https://github.com/bpolaszek/doctrine-native-enums ) and the upgrade to v2.13.4 suddenly broke changeset computation for my enums.

Looks like the 3 lines added in UnitOfWork in #10074 indiscriminately convert any original data backed enum into its value. This happens to work because in ClassMetadata, if enumType is used, the ReflectionProperty turns into ReflectionEnumProperty which extracts the value of the enum in question, not the enum instance, so things tend to work. My implementation, however, doesn't get to benefit from ReflectionEnumProperty.

I'm having some trouble following the code, but this is what I found.
I first encountered the error when trying to insert 2 entities (both using those Enums). The error was Invalid parameter number: number of bound variables does not match number of tokens. It would generate proper INSERTs but the changeset would be wrong (missing most fields).

It generates an initial changeset which is fine. It compares original data (NULL, since it's a new entity, persisted just now) with actual data (string value of my Enum), but somewhere along the way another changeset computation gets triggered and it somehow reaches the conclusion that only 2 out of 6 fields have changed and proceeds to using that changeset as insert data.

I know this is not the path provided by the ORM, but I still think that the ORM should support (as in "tolerate", try not to break) 3rd party implementations for Enum support. Personally, I like my current implementation since it allows me to properly define columns as #[Column(type: MyEnum::class) (not the hack-ish type: 'string', enumType: MyEnum::class) and it also allows me to use MySQL ENUM in the database (instead of string/char).

I downgraded to v2.13.3 and it works as it always did. I also tested v2.13.4 with the ORM's enumType and it also works as expected. It just doesn't work with v2.13.4 and custom enums.

I could try to fix this on my end. One way would be to just manually add the enumType metadata to any enum field (this is not a great option DX-wise). Another thing I could do would be to add a listener for loadClassMetadata that would do that (though, I'm not sure the event gets triggered at the right time, before wakeUpReflection). This is also not great performance-wise -- on each entity manager boot I would check all entities and patch the enums on-the-fly. I could also start using the ORM-supported enum implementation, but that has the downside of poorer DX and lacks support for MySQL ENUM.

Previous behavior

It would keep BackedEnums as they are and would always compare those when computing changesets.

Current behavior

When comparing changesets it converts BackedEnums to their value and it seems it compares a BackedEnum with a value of that BackedEnum since custom enum implementations don't benefit from ReflectionEnumProperty.

How to reproduce

Before attempting a reproducer I would like to hear some thoughts from you guys. I'd rather not waste time on a reproducer if this is deemed intended behavior.

@michnovka
Copy link
Contributor

@ovrflo thanks for trying to describe the issue, but I dont see how that PR could make these problems. If you can please make a repo with symfony project using your package and showing the error, on a minimal example, I will look into it and try to find a fix for you.

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

2 participants