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

Use jms/serializer groups detection logic #1482

Merged
merged 3 commits into from
Apr 23, 2019

Conversation

goetas
Copy link
Collaborator

@goetas goetas commented Mar 11, 2019

This PR moves outside of NelmioApiDocBundle the logic for groups detection.

In this way jms/serializer can change the groups logic without affection NelmioApiDocBundle code.

Another advantage or this solution is that will be possible to introduce easily other exclusion strategies (as versioning) without changing too much code.

TODO:

  • concept (works on my machine on relatively complex project)
  • update tests
  • release updates on the jms/serializer side

@goetas
Copy link
Collaborator Author

goetas commented Mar 11, 2019

@GuilhemN If you are OK with moving that logic outside of nelmio-apidoc, I can complete this

@GuilhemN
Copy link
Collaborator

Indeed, it probably makes more sense to include this in JMS 👍

@goetas
Copy link
Collaborator Author

goetas commented Mar 13, 2019

Great! will let you know as soon as i am done!

@goetas goetas force-pushed the use-jms-groups-logic branch 2 times, most recently from 3b7d81b to af1fa58 Compare April 17, 2019 20:03
@goetas goetas force-pushed the use-jms-groups-logic branch from af1fa58 to f415ac2 Compare April 17, 2019 20:05
@goetas
Copy link
Collaborator Author

goetas commented Apr 17, 2019

@GuilhemN excluding the composer.json changes, this is ready for review (probably a first round).

Preamble: I'm going to release jms/serializer 3.0, that reverts the nested group strategy as it was in 1.x (see schmittjoh/serializer#1071). 2.x will not be supported anymore.
Users using 1.x, are encouraged to upgrade directly to 3.x.

Because of the jms/serializer changes, I think also that naelmio/api-doc should just drop support for jms/serializer 2.x, and focus only on 1.x and 3.x.

This decision allows to not have to maintain anymore v1 and v2 tests, so I have just deleted the v2 tests.

Regarding the other changes, now jms/serializer exposes getGroupsFor() that calculates the groups for a given object. To make it work I had to simulare a graph-traversal (since the nested groups depends on the class graph).

composer.json Outdated
@@ -44,7 +44,8 @@
"api-platform/core": "^2.1.0",
"friendsofsymfony/rest-bundle": "^2.0",
"willdurand/hateoas-bundle": "^1.0|^2.0",
"jms/serializer-bundle": "^2.0|^3.0"
"jms/serializer-bundle": "^2.0|^3.0",
"jms/serializer": "dev-master as 2.99.0"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

when this jms/serializer 3.0 will be tagged, this will be just ^1.14|^3.0

composer.json Outdated
@@ -29,7 +29,7 @@
"symfony/asset": "^3.4|^4.0",
"symfony/console": "^3.4|^4.0",
"symfony/config": "^3.4|^4.0",
"symfony/validator": "^3.4|^4.0",
"symfony/validator": "^3.4|~4.1.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is a temporary fix for symfony/symfony#31152

Copy link
Collaborator

@GuilhemN GuilhemN left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@goetas goetas force-pushed the use-jms-groups-logic branch from f415ac2 to 8dc283e Compare April 23, 2019 13:12
@goetas goetas force-pushed the use-jms-groups-logic branch from 8dc283e to 4fbf096 Compare April 23, 2019 13:13
@goetas goetas changed the title [WIP] Use jms/serializer groups detection logic Use jms/serializer groups detection logic Apr 23, 2019
@goetas
Copy link
Collaborator Author

goetas commented Apr 23, 2019

@GuilhemN this is ready! jms/serializer v3 released

the test failure is related to symfony/symfony#31152

@GuilhemN GuilhemN merged commit bad6df7 into nelmio:master Apr 23, 2019
@GuilhemN
Copy link
Collaborator

Great, thank you @goetas :)

Congrats for releasing JMS v3 😃

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.

2 participants