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

Allow "friendsofsymfony/rest-bundle:^3.0" #1767

Merged
merged 1 commit into from
Jul 31, 2020

Conversation

wbloszyk
Copy link
Member

@wbloszyk wbloszyk commented Jul 16, 2020

Subject

Allow "friendsofsymfony/rest-bundle:^3.0".

I am targeting this branch, because these changes respect BC.

Changelog

### Added
- Added support for "friendsofsymfony/rest-bundle:^3.0"
### Change
- support for deprecated "rest" routing type in favor for xml

@phansys
Copy link
Member

phansys commented Jul 16, 2020

The commit dadd6ab seems unrelated given the PR title and description.

@wbloszyk
Copy link
Member Author

wbloszyk commented Jul 17, 2020

I got critical error in tests:

[critical] Uncaught PHP Exception Symfony\Component\Config\Exception\LoaderLoadException: "[ERROR 1866] Element '{http://symfony.com/schema/routing}import', attribute 'name-prefix': The attribute 'name-prefix' is not allowed. (in /home/zbiru/PhpstormProjects/sonata-project/SonataMediaBundle/ - line 3, column 0)
[ERROR 1866] Element '{http://symfony.com/schema/routing}import', attribute 'name-prefix': The attribute 'name-prefix' is not allowed. (in /home/zbiru/PhpstormProjects/sonata-project/SonataMediaBundle/ - line 4, column 0) in /home/zbiru/PhpstormProjects/sonata-project/SonataMediaBundle/src/Resources/config/routing/api.xml (which is being imported from "/home/zbiru/PhpstormProjects/sonata-project/SonataMediaBundle/tests/App/routes.yml"). Make sure there is a loader supporting the "rest" type." at /home/zbiru/PhpstormProjects/sonata-project/SonataMediaBundle/vendor/symfony/config/Loader/FileLoader.php line 180

https://travis-ci.org/github/sonata-project/SonataMediaBundle/jobs/708806345#L815

Can we change name-prefix to prefix? WDYT? @phansys

@wbloszyk
Copy link
Member Author

I check Route Matching Logs in Symfony Profiler and saw this (the same for this media api):

# Route name Path Log
124 sonata_api_user_user_sonata_user_api_user_getusers /api/user/users Path does not match
125 sonata_api_user_user_sonata_user_api_user_getuser /api/user/user/(id} Path does not match
126 sonata_api_user_user_sonata_user_api_user_postuser /api/user/user Path does not match

but should be:

# Route name Path Log
124 sonata_api_user_user_get_users /api/user/users Path does not match
125 sonata_api_user_user_get_user /api/user/user/(id} Path does not match
126 sonata_api_user_user_post_user /api/user/user Path does not match

Mybe we should add name to @Router()? Like:

* @Rest\Get("/galleries", name="get_galleries")

This will generate router name: {name-prefix}{router name} = sonata_api_media_gallery_+get_galleries. WDYT? @phansys

@phansys
Copy link
Member

phansys commented Jul 22, 2020

This will generate router name: {name-prefix}{router name} = sonata_api_media_gallery_+get_galleries. WDYT? @phansys

I agree that if we must keep the previous names, we should take that responsibility.
In that case, the same change should be applied at SonataUserBundle.

@wbloszyk
Copy link
Member Author

@phansys Can you fix it for UserBundle? This can be helpful: sonata-project/dev-kit#778 (comment)

@phansys
Copy link
Member

phansys commented Jul 22, 2020

@phansys Can you fix it for UserBundle? This can be helpful: sonata-project/dev-kit#778 (comment)

Sure. I'll try to provide this fix tomorrow 👍

@wbloszyk
Copy link
Member Author

Probably last thing what we should fix is format. I cant generate xml but evan for non fixed bundles. Mybe it is my config but
.{_format} for new routing is missing.

@phansys
Copy link
Member

phansys commented Jul 24, 2020

Probably last thing what we should fix is format. I cant generate xml but evan for non fixed bundles. Mybe it is my config but
.{_format} for new routing is missing.

I can confirm this happens, since when I bumped this requirements in one of my private projects some time ago I had to add the suffix explicitly. I'll try to provide a fix for SonataUserBundle in order to check.

@wbloszyk
Copy link
Member Author

wbloszyk commented Jul 24, 2020

After change _format I still got ***.json?_format=xml. In /api/doc format must be change in right top corner. What is little confusing.

127.0.0.1:8000/api/notification/messages.xml

<result>
<page>1</page>
<per_page>10</per_page>
<last_page>0</last_page>
<total>0</total>
<entries/>
</result>

Anyway I can get result if no object is existing. Probalbly it is some problem with fos_rest.decoder.xml or serializer(but json works).

WDYT? @phansys

@phansys
Copy link
Member

phansys commented Jul 24, 2020

WDYT? @phansys

I'm not using XML, so I must check first in order to share any concern about this.

@jordisala1991
Copy link
Member

Is this one ready to review?

@wbloszyk
Copy link
Member Author

@jordisala1991 I working on test now. I will write when finish it.

tests/App/config.yml Outdated Show resolved Hide resolved
tests/App/config.yml Outdated Show resolved Hide resolved
@wbloszyk wbloszyk force-pushed the support_rest_3 branch 4 times, most recently from 19241e8 to 594429e Compare July 26, 2020 16:04
@wbloszyk wbloszyk marked this pull request as ready for review July 26, 2020 18:42
@wbloszyk wbloszyk requested a review from phansys July 26, 2020 19:00
@wbloszyk
Copy link
Member Author

@jordisala1991 @greg0ire It is RTM, but i can find why --prefer-lowest don't work.

tests/App/config.yml Outdated Show resolved Hide resolved
@wbloszyk wbloszyk force-pushed the support_rest_3 branch 3 times, most recently from d81b441 to eb9d51e Compare July 30, 2020 18:18
phansys
phansys previously approved these changes Jul 30, 2020
VincentLanglet
VincentLanglet previously approved these changes Jul 30, 2020
Copy link
Member

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

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

This could be great to make a merge 3.x into master after this. We're starting to have a lot of conflict to merge ^^

@jordisala1991
Copy link
Member

Plus, this one is a hard merge because there are entity changes. I can deal with it after the merge

@wbloszyk
Copy link
Member Author

Finally it is RTM.

If it is hard to merge 3,x to master we can split it to 2 PRs. Can you merge 3 PRs? and I will merge PRs with my work then. @jordisala1991

<route id="get_gallery_medias" path="/galleries/{id}/medias.{_format}" methods="GET" controller="Sonata\MediaBundle\Controller\Api\GalleryController::getGalleryMediasAction" format="json">
<requirement key="_format">json|xml|html</requirement>
</route>
<route id="get_gallery_galleryhasmedias" path="/galleries/{id}/galleryhasmedias.{_format}" methods="GET" controller="Sonata\MediaBundle\Controller\Api\GalleryController::getGalleryGalleryhasmediasAction" format="json">
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to have this list sorted by id, but it's not really a blocker.

Copy link
Member Author

Choose a reason for hiding this comment

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

I check it. Better keep it in this way becouse order in config is equal to order in nelmio doc. Its look much better when we keep this order.

@VincentLanglet VincentLanglet merged commit 332ad2e into sonata-project:3.x Jul 31, 2020
@VincentLanglet
Copy link
Member

@jordisala1991 @wbloszyk Have a good merge ! 😁

@wbloszyk wbloszyk deleted the support_rest_3 branch August 1, 2020 07:41
@wbloszyk
Copy link
Member Author

wbloszyk commented Aug 1, 2020

@VincentLanglet @jordisala1991 Many conflits but easy to resolve: #1775

@wbloszyk wbloszyk mentioned this pull request Aug 3, 2020
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.

6 participants