search

schmittjoh / serializer

Library for (de-)serializing data of any complexity (supports JSON, and XML)
http://jmsyst.com/libs/serializer
MIT License
2.32k stars 588 forks source link

add php 8.1 enum support #1448

Closed goetas closed 1 year ago

goetas commented 1 year ago
Q A
Bug fix? no
New feature? yes
Doc updated yes
BC breaks? maybe??
Deprecations? no
Tests pass? yes
Fixed tickets #1393 #1392 #1373
License MIT
goetas commented 1 year ago

Im thinking if is worth trying to make it backward compatible or just make a major release..., of course the alternative is to make it an opt-in feature

goetas commented 1 year ago

@Einenlum

It seems like using enum<'Color'> makes it use the name of the enum and not its value, according to what you wrote before (line 382), so I'm a bit confused.

I have updated the documentation.

Despite the fact that the RFC suggests that non-backed enums should not be serialized, I do not see anything wrong with offering such possibility.

Regarding the integer option, TBH is an overkill, i can remove it. I've dealt with some "weird" systems in which the backed enum was a string but containing integers... but maybe is not worth solving

goetas commented 1 year ago

ok, I think that this is ready for a new round.

I made it an opt-in feature

Einenlum commented 1 year ago

Thanks @goetas ! The implementation sounds way better and simple to me this way. For the integer option, sounds indeed overkill to me since it's probably to solve some edge case that shouldn't happen in most codebases.

One thing that I don't really get though (but probably a misunderstanding on my side): why make it opt-in and not do the same as with other drivers here ?

Sounds strange to make that opt-in since it sounds like a normal behavior that should work out of the box.

goetas commented 1 year ago

One thing that I don't really get though (but probably a misunderstanding on my side): why make it opt-in and not do the same as with other drivers here ?

the issue is that some people might be using already the serializer with the default enum serialization, see testEnumDisabledByDefault. the serializer was already able to serialize enums... and with this PR the format would change...

ruudk commented 1 year ago

Would be great to have this tagged so that we can start using it 💙

Einenlum commented 1 year ago

:tada: Thanks a lot for you work @goetas ! Great news for JMS Serializer users.