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

Fix serialization of BackedEnum - annotation without name/value #1536

Merged
merged 3 commits into from
Feb 21, 2024

Conversation

iKsSs
Copy link

@iKsSs iKsSs commented Feb 6, 2024

Q A
Bug fix? yes
New feature? no
Doc updated yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT

Serialization of Backed Enum, when property has Serializer annotation with specified Enum Class but second parameter (name/value), is not provided was not behaving according to documentation.

@iKsSs iKsSs changed the title Add Backed Enum without name/value specification to test Fix serialization of BackedEnum - annotation without name/value Feb 6, 2024
@scyzoryck scyzoryck requested a review from goetas February 6, 2024 21:37
@scyzoryck
Copy link
Collaborator

@goetas could you help with review, please? :)

@goetas
Copy link
Collaborator

goetas commented Feb 19, 2024

Looks good to me. I'm surprised that i forgot to implement it from get go.

i have left only one minor comment

@@ -44,7 +44,7 @@ public function serializeEnum(
array $type,
SerializationContext $context
) {
if (isset($type['params'][1]) && 'value' === $type['params'][1]) {
if ((isset($type['params'][1]) && 'value' === $type['params'][1]) || (!isset($type['params'][1]) && is_a($enum, \BackedEnum::class, true))) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can is_a($enum, \BackedEnum::class, true) be replace with $enum instanceof \BackedEnum? seems much more readable to me.

Copy link
Author

Choose a reason for hiding this comment

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

thanks for reviewing the code...
yes, instanceof is better here 👍

@scyzoryck scyzoryck merged commit 1235c79 into schmittjoh:master Feb 21, 2024
18 checks passed
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

Successfully merging this pull request may close these issues.

4 participants