Skip to content

Use operation output class name instead Resource short name in operati... #3741

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

Merged
merged 1 commit into from
Nov 8, 2020

Conversation

maxtorete
Copy link
Contributor

…ion summary

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tickets fixes #3740
License MIT

@dunglas dunglas requested a review from soyuka October 2, 2020 08:41
@GregoireHebert
Copy link
Contributor

I quite like the idea of allowing customisation (that's why you could change the shortname), but only for the Output? It seems weird to me. The Output should just be a different representation of the same resource, so should not have a different naming per se. But I might be wrong.

@maxtorete
Copy link
Contributor Author

maxtorete commented Oct 23, 2020

I quite like the idea of allowing customisation (that's why you could change the shortname), but only for the Output? It seems weird to me. The Output should just be a different representation of the same resource, so should not have a different naming per se. But I might be wrong.

Hi GregoireHebert, thanks for your reply!

What I'm trying to achieve is a subresource custom operation, but without some of the limitations the current suresource implementation has in API-Platform. Maybe there is a better option that I haven't found.

For example, something like this /myapp/workflow/{id}/statuses will receive an Workflow object and respond with a collection of its related WorkflowStatus.

In the Workflow resource yaml I set the collection operation:

collectionOperations:
    get_workflow_status_collection:
      method: GET
      route_name: api_workflow_get_workflow_status_collection
      openapi_context:
        parameters:
          - { name: id, in: path, required: true, type: string, description: 'Workflow Resource identifier' }

And this is the route:

lince_company_api_workflow_get_workflow_status_collection:
  path: /api/lince_company/workflow/{id}/statuses
  methods: ['GET']
  defaults:
    _controller: App\Controller\WorkflowController::listWorkflowStatusSubresource
    _api_resource_class: App\Entity\WorkflowStatus
    _api_collection_operation_name: get_workflow_status_collection

The problem is that the _api_resource_class parameter in the route definition is ignored, so I changed the collection operation to this one:

collectionOperations:
    get_workflow_status_collection:
      method: GET
      route_name: api_workflow_get_workflow_status_collection
      output:
        class: App\Entity\WorkflowStatus
        name: WorkflowStatus
      openapi_context:
        parameters:
          - { name: id, in: path, required: true, type: string, description: 'Workflow Resource identifier' }

So now it works fine, even the SwaggerUI shows the right schema in the response sample value. But the only thing wrong is the Swagger description text for that Operation, as it says that it will return a collection of the parent resource, even if it shows the schema of the subresource in the example value. You can see it in action in this screenshot form the issue #3740 (comment)

Screenshot_2020-09-30 API Platform

If the output class schema is shown in the Example value schema, why its name is not show in the description?

@GregoireHebert
Copy link
Contributor

But the only thing wrong is the Swagger description text for that Operation, as it says that it will return a collection of the parent resource

You are absolutely right on this, and this one of the biggest problems on the current subResource implementation. It should be fixed thanks to #2706 which won't take place before v2.7 / v3.

But in the meantime, maybe your solution could do in 2.6? IDK.

WDYT @dunglas @soyuka?

@maxtorete
Copy link
Contributor Author

I rebased to 2.5 from upstream, the failing checks look unrelated to my commit.

@soyuka
Copy link
Member

soyuka commented Nov 8, 2020

Can you rebase ? This is correct indeed and I'd like to merge it :).

@maxtorete
Copy link
Contributor Author

Can you rebase ? This is correct indeed and I'd like to merge it :).

Rebased to upstream :-)

@soyuka soyuka merged commit db13fcc into api-platform:2.5 Nov 8, 2020
@soyuka
Copy link
Member

soyuka commented Nov 8, 2020

Thanks @maxtorete

@maxtorete
Copy link
Contributor Author

Thanks @maxtorete

You are welcome!

@BeyerJC BeyerJC mentioned this pull request Nov 27, 2020
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.

3 participants