-
-
Notifications
You must be signed in to change notification settings - Fork 893
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
fix-openapi-subresource #3690
fix-openapi-subresource #3690
Conversation
GregoireHebert
commented
Aug 27, 2020
•
edited
Loading
edited
Q | A |
---|---|
Bug fix? | yes |
New feature? | no |
BC breaks? | no |
Deprecations? | no |
Tickets | fixes #3666 and #1616 |
License | MIT |
Doc PR | ~ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
cd7d2f6
to
ffd05b5
Compare
I have no idea why the coverage is not satisfied... |
@@ -75,7 +75,7 @@ public function testCreate() | |||
], | |||
'route_name' => 'api_dummy_entities_subresource_get_subresource', | |||
'path' => '/dummy_entities/{id}/subresource.{_format}', | |||
'operation_name' => 'subresource_get_subresource', | |||
'operation_name' => 'api_dummy_entities_subresource_get_subresource', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this a BC break?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, IMO yes and no. Yes because, even if generated, it could have been used. But no because I think it's a bug and that it should always have been like that. And no because subresources are experimental and that part is internal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should at least be documented in the changelog as a BC break.
Works perfectly in my testing (checked against project which caused me to open #3666 ). Thanks a lot! Looking forward to seeing this released. |
@GregoireHebert @soyuka Is there any chance this fix / something to this effect can be included in 2.6? The bug outlined in #3666 is still present from what I can see. However the situation gets a bit worse still, as now in 2.6 even if the subresource is using the default normalization context, that schema doesn't get output either if that normalization context isn't used in any item or collection operations, i.e. when using the pattern of:
(although that can be worked around by having the item GET endpoint available but blocked off by security instead). Anyway, the state as it is now means when using subresources in non-trivial ways, the types are incorrectly exposed via OpenAPI. So it creates lots of room for error in API consumers, and places where type schemas need to be determined manually, losing a big part of the benefit of OpenAPI. |
This is a good patch but is definitely breaking things. I'll see if we can add this to the next release. Also related to: #3458. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |