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.

Sign up for GitHub

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

Jump to bottom

Fix REST API routing paths #1235

Merged
merged 1 commit into from
Sep 25, 2020
Merged

Fix REST API routing paths #1235

merged 1 commit into from
Sep 25, 2020

Conversation

wbloszyk
Copy link
Member

@wbloszyk wbloszyk commented Sep 24, 2020 •
edited
Loading

Subject

Change API was in: https://github.com/sonata-project/SonataUserBundle/pull/1185/files
Revert API routing names: https://github.com/sonata-project/SonataUserBundle/pull/1194/files

This PR will revert API routing paths.

I am targeting this branch, because it must be fix here and change respect BC.

Changelog

### Fixed
- API routing paths after move routing type from 'REST' to 'XML' in v4.6.0

@wbloszyk wbloszyk requested review from phansys and a team September 24, 2020 17:20
@wbloszyk wbloszyk changed the title Fix REST API routing Fix REST API routing paths Sep 24, 2020
phansys
phansys previously approved these changes Sep 24, 2020
View reviewed changes
Copy link
Member

@phansys phansys left a comment

Choose a reason for hiding this comment

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

I didn't realize that these routes were pluralized using the "rest" route type. Unfortunately, we hadn't tests.

VincentLanglet
VincentLanglet previously approved these changes Sep 24, 2020
View reviewed changes
@wbloszyk
Copy link
Member Author

@phansys I also made mistakes and you help me fix it. Lets make sonata code better together.

@wbloszyk
Copy link
Member Author

Can you merge it?

@phansys
Copy link
Member

phansys commented Sep 24, 2020 •
edited
Loading

AFAIK, the bug was introduced in 4.6.0, so I think there are more probabilities for this to help projects requiring previous versions than breaking code trusting on these "new" routes.
Since I am who introduced the issue, I'd like to wait for more approvals on this, since there is also the possibility for this change to require a BC layer keeping these new routes.

@wbloszyk wbloszyk requested a review from a team September 24, 2020 18:37
@wbloszyk
Copy link
Member Author

Good point. 4.6.0 was release long time ago. IMO we can keep this new routes only in legacy api (in api.xml). This will corelate new routes with legacy API and will be BC. Remove will be mark as next major, so UPGRADE-5.0.md.

@wbloszyk wbloszyk dismissed stale reviews from VincentLanglet and phansys via fcbca7d September 24, 2020 18:56
@wbloszyk
Copy link
Member Author

Last question. Should we add it to nelmio_v3 too? Technicly we don't have becouse people should have to check it (major update for nelmio/api-doc-bundle. Anyway IMO someone can miss it.

WDYT? @sonata-project/contributors

@wbloszyk wbloszyk requested review from VincentLanglet and a team September 24, 2020 19:20
@wbloszyk
Copy link
Member Author

Now It can be merge.

phansys
phansys reviewed Sep 24, 2020
View reviewed changes
@wbloszyk wbloszyk requested a review from phansys September 24, 2020 20:18
phansys
phansys requested changes Sep 24, 2020
View reviewed changes
@wbloszyk wbloszyk requested a review from phansys September 24, 2020 21:12
@wbloszyk wbloszyk force-pushed the fix_api_routing branch 2 times, most recently from eaa2a44 to 51cd15f Compare September 24, 2020 21:23
phansys
phansys reviewed Sep 24, 2020
View reviewed changes
phansys
phansys previously approved these changes Sep 24, 2020
View reviewed changes
@wbloszyk wbloszyk requested a review from a team September 24, 2020 21:52
@wbloszyk @VincentLanglet
Fix REST API routing
809b341 
Update src/Resources/config/routing/api_nelmio_v3.xml

Co-authored-by: Javier Spagnoletti <phansys@gmail.com>

Update src/Resources/config/routing/api.xml

Co-authored-by: Javier Spagnoletti <phansys@gmail.com>

Update src/Resources/config/routing/api_nelmio_v3.xml

Co-authored-by: Vincent Langlet <VincentLanglet@users.noreply.github.com>
@VincentLanglet VincentLanglet merged commit baf8349 into sonata-project:4.x Sep 25, 2020
@VincentLanglet
Copy link
Member

Thanks

@wbloszyk wbloszyk deleted the fix_api_routing branch September 25, 2020 08:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phansys phansys phansys approved these changes

@VincentLanglet VincentLanglet VincentLanglet approved these changes

Assignees
No one assigned
Labels
patch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wbloszyk @phansys @VincentLanglet @OskarStark