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 XML for ReST API routing #1200

Merged
merged 1 commit into from
Jul 27, 2020
Merged

Conversation

phansys
Copy link
Member

@phansys phansys commented Jul 27, 2020

Subject

Use XML for ReST API routing.

I am targeting this branch, because this change respects BC.

Related to sonata-project/SonataMediaBundle#1767 (comment).

@phansys phansys requested review from a team and jordisala1991 July 27, 2020 16:38
@wbloszyk
Copy link
Member

@phansys phansys marked this pull request as ready for review July 27, 2020 16:49
@jordisala1991
Copy link
Member

Even if it is not required, isn't it better to just use the xml format anyway? With this, if someday the route changes we don't have to duplicate the controller to not break BC (like in the nelmio case).

Bonus: all the other routes are defined like this. wdyt @wbloszyk ? IMO it is worth it from every point of view.

@jordisala1991
Copy link
Member

I don't use the api at all, I didn't check if everything works. Are the api endpoints + docs generated correctly now?

@jordisala1991
Copy link
Member

Another point: You have to pick one: It is pedantic and no changelog or it is a patch/minor then it deserves a changelog.

@phansys
Copy link
Member Author

phansys commented Jul 27, 2020

I don't use the api at all, I didn't check if everything works. Are the api endpoints + docs generated correctly now?

I'm not using the API either, but since the tests are checking the routes are generated as expected, I think there is no problem.

final class RoutingTest extends WebTestCase

Swagger doc should not be affected, since it's agnostic about how routing is defined.

Another point: You have to pick one: It is pedantic and no changelog or it is a patch/minor then it deserves a changelog.

IMO, this change is pedantic. I'm just filling the changelog for internal reference.

@jordisala1991
Copy link
Member

The thing with the changelog is that is auto generated now , so if we dont remove it, it will end up in the release notes

jordisala1991
jordisala1991 previously approved these changes Jul 27, 2020
@wbloszyk
Copy link
Member

wbloszyk commented Jul 27, 2020

Even if it is not required, isn't it better to just use the xml format anyway? With this, if someday the route changes we don't have to duplicate the controller to not break BC (like in the nelmio case).

Bonus: all the other routes are defined like this. wdyt @wbloszyk ? IMO it is worth it from every point of view.

IMHO annotations is good in small close/ecommerce projects. In Sonata using xml for Rest Api, like in other places, will be awsome.

I don't use the api at all, I didn't check if everything works. Are the api endpoints + docs generated correctly now?

I use it from time to time, but not so much complex. I compare it with demo to keep it the same. We (with @phansys) added smoke tests for it and have add functional tests in plan.

Another point: You have to pick one: It is pedantic and no changelog or it is a patch/minor then it deserves a changelog.

IMHO it is BC-break for people who override API. I think it is minor and little upgrade note should be add. Otherwise they will have two diffrence system (self annotations and sonata xml) for routing.

@wbloszyk
Copy link
Member

I update sonata-project/dev-kit#778. We should know how far we are in work on Rest API and what we do.

@jordisala1991
Copy link
Member

Given the point that since not long ago the api wasnt working at all, I would not consider this as a Bc break

@phansys
Copy link
Member Author

phansys commented Jul 27, 2020

IMHO it is BC-break for people who override API. I think it is minor and little upgrade note should be add. Otherwise they will have two diffrence system (self annotations and sonata xml) for routing.

AFAIK, the only way users have to override the API routing is using their own routing definitions referencing to the existing controllers. If that is the case, I think there is no BC break at all.

Given the point that since not long ago the api wasnt working at all, I would not consider this as a Bc break

I agree. Even, the routing annotations were added just in the last release (4.6.0).

@phansys phansys mentioned this pull request Jul 27, 2020
@jordisala1991 jordisala1991 requested a review from a team July 27, 2020 17:58
@phansys
Copy link
Member Author

phansys commented Jul 27, 2020

Changelog removed in order to avoid to be picked up by release process. It was:

Changelog

### Changed
- Changed routing definition from annotations to XML.
### Removed
- Removed dev requirement for "sensio/framework-extra-bundle".

@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@jordisala1991 jordisala1991 merged commit 0785115 into sonata-project:4.x Jul 27, 2020
@jordisala1991
Copy link
Member

Thank you @phansys

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants