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

Add jms/serializer enum support #2078

Merged
merged 1 commit into from
Jan 5, 2024
Merged

Conversation

javer
Copy link
Contributor

@javer javer commented Feb 11, 2023

jms/serializer introduced enum support since v3.20 and added a new type: https://github.com/schmittjoh/serializer/blob/402a12629e3ad6d6897273769808a28c32745309/src/Metadata/Driver/EnumPropertiesDriver.php#L50

And since jms/serializer-bundle v5.2.1 and schmittjoh/JMSSerializerBundle#919 with jms_serializer.enum_support: true all existing models with enums got broken, because now enum properties are referenced to the fixed string #/components/schemas/enum instead of the correct #/components/schemas/EnumClassName:

{
    "openapi": "3.0.0",
    "info": {},
    "paths": {
        "/api/card/random": {
            "get": {
                "operationId": "get_app_randomcardaction__invoke",
                "responses": {
                    "200": {
                        "description": "OK",
                        "content": {
                            "application/json": {
                                "schema": {
                                    "$ref": "#/components/schemas/Card"
                                }
                            }
                        }
                    }
                }
            }
        }
    },
    "components": {
        "schemas": {
            "Card": {
                "properties": {
                    "rank": {
                        "type": "integer"
                    },
                    "suit": {
                        "$ref": "#/components/schemas/enum"
                    }
                },
                "type": "object"
            },
            "enum": {}
        }
    }
}

Before it was:

{
    "openapi": "3.0.0",
    "info": {},
    "paths": {
        "/api/card/random": {
            "get": {
                "operationId": "get_app_randomcardaction__invoke",
                "responses": {
                    "200": {
                        "description": "OK",
                        "content": {
                            "application/json": {
                                "schema": {
                                    "$ref": "#/components/schemas/Card"
                                }
                            }
                        }
                    }
                }
            }
        }
    },
    "components": {
        "schemas": {
            "Card": {
                "properties": {
                    "rank": {
                        "type": "integer"
                    },
                    "suit": {
                        "$ref": "#/components/schemas/Suit"
                    }
                },
                "type": "object"
            },
            "Suit": {
                "type": "string",
                "enum": [
                    "hearts",
                    "diamonds",
                    "clubs",
                    "spades"
                ]
            }
        }
    }
}

This PR adds support for enum type going from jms/serializer and returns the correct behavior.

@Yozhef
Copy link

Yozhef commented Feb 27, 2023

@theofidry саn you review please?

@theofidry
Copy link
Member

I unfortunately have no permissions on this package, only the Alice one.

@Yozhef
Copy link

Yozhef commented Mar 1, 2023

@GuilhemN саn you review please?

@javer
Copy link
Contributor Author

javer commented Apr 24, 2023

Is there any update on this?

@szepczynski
Copy link
Contributor

@GuilhemN maybe you can help with review and merge this PR?

@javer
Copy link
Contributor Author

javer commented Sep 6, 2023

Any updates on this? @GuilhemN could you please take a look at this small fix?

@DjordyKoert
Copy link
Collaborator

@javer can you please write test(s) for this new feature? Also please rebase on master 😄

@javer javer force-pushed the support-jms-enum branch 2 times, most recently from 3858954 to 448ccb1 Compare September 16, 2023 14:56
@javer
Copy link
Contributor Author

javer commented Sep 16, 2023

@DjordyKoert Done: rebased on master, added test and even fixed codestyle in master 😄
Second commit is intended for keeping git history of JMSController.

@@ -282,6 +282,15 @@ public function describeItem(array $type, OA\Schema $property, Context $context)
$property->type = 'string';
$property->format = 'date-time';
} else {
// See https://github.com/schmittjoh/serializer/blob/master/src/Metadata/Driver/EnumPropertiesDriver.php#L51
Copy link
Collaborator

Choose a reason for hiding this comment

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

Its better to make this a permalink

Comment on lines +313 to +355
if ($this->flags & self::USE_JMS && \PHP_VERSION_ID >= 80100) {
$c->loadFromExtension('jms_serializer', [
'enum_support' => true,
]);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

jms_serializer.enum_support defaults to false even in PHP 8.1 or higher according to https://github.com/schmittjoh/JMSSerializerBundle/blob/master/Resources/doc/configuration.rst#extension-reference.

enum_support: true # PHP 8.1 Enums support, false by default for backward compatibility

Might be better to test both with false & true options by adding an additional flag for this? Gonna leave this one for @GuilhemN to decide if this is needed or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With enum_support: false test passes even without my change in JMSModelDescriber, so it would be strange to write a test which is not failing without the fix.

Copy link
Collaborator

@DjordyKoert DjordyKoert left a comment

Choose a reason for hiding this comment

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

LGTM

@shakaran
Copy link
Contributor

shakaran commented Dec 7, 2023

LGTM 👍

@shakaran
Copy link
Contributor

shakaran commented Jan 4, 2024

@javer could you rebase the conflicts? Thanks

@javer javer force-pushed the support-jms-enum branch from 0929dc2 to 4adf93e Compare January 4, 2024 15:54
@javer javer force-pushed the support-jms-enum branch from 4adf93e to c6f647f Compare January 4, 2024 16:27
@javer
Copy link
Contributor Author

javer commented Jan 4, 2024

Done, rebased on the latest master, and all tests are green.

@DjordyKoert DjordyKoert merged commit 1f99898 into nelmio:master Jan 5, 2024
12 checks passed
@DjordyKoert
Copy link
Collaborator

Thank you @javer!

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.

6 participants