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 configuring input and ouput class per operation #2481

Closed
wants to merge 2 commits into from
Closed

Allow configuring input and ouput class per operation #2481

wants to merge 2 commits into from

Conversation

bendavies
Copy link
Contributor

@bendavies bendavies commented Jan 29, 2019

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

This allows specifying the input and output class per operation.
It is already possibly to specify them for subresources, so I think it was an oversight for operations, which is why i'm classifying this as a bug.

@bendavies bendavies changed the title Allow input and ouput class per operation Allow configuring input and ouput class per operation Jan 29, 2019
@bendavies bendavies changed the base branch from master to 2.4 January 29, 2019 17:34
@bendavies
Copy link
Contributor Author

unrelated CI failure

@dunglas
Copy link
Member

dunglas commented Jan 29, 2019

I don't really understand why we need that. It should already work actually

@bendavies
Copy link
Contributor Author

bendavies commented Jan 29, 2019

I want to use a different DTO for creation and updating a resource. If you have a better idea I'm happy to hear it.

I couldn't get it to work at all per operation, without this PR.

@soyuka
Copy link
Member

soyuka commented Jan 29, 2019

looks legit to me

@bendavies
Copy link
Contributor Author

@soyuka is this the correct place to implement the fix?

@dunglas
Copy link
Member

dunglas commented Jan 29, 2019

I don't understand why we need a request attribute for that. As the operation is known at this point, we should just be able to lookup the attribute value from the operation metadata (using the metadata factory). Or am I missing something?

@bendavies
Copy link
Contributor Author

bendavies commented Jan 29, 2019

I'm not really sure, but this is where I traced to where input/output classes are set. Maybe @soyuka can comment as he is the author of the feature.
the original PR is here #2235

@dunglas
Copy link
Member

dunglas commented Jan 29, 2019

The output class should be set directly on the @ApiResource annotation.
The following should already be supported:

/** @ApiResource(itemOperations={"get"={"output_class"="FooBar"}})*/

@bendavies
Copy link
Contributor Author

bendavies commented Jan 29, 2019

Can you try?

I have tried exactly that, it does not work. here is my actually attempt:

/**
 * @ApiResource(
 *     normalizationContext={"groups": {"account:read"}},
 *     collectionOperations={
 *         "get": {"method": "GET", "path": "/accounts/"},
 *     },
 *     itemOperations={
 *         "get": {"output_class": "ThrowAnError", "method": "GET", "path": "/accounts/{id}", "requirements": {"id": "%uuid_regex%"}},
 *         "put": {"input_class": "ThrowAnError", "method": "PUT", "path": "/accounts/{id}", "requirements": {"id": "%uuid_regex%"}, "denormalization_context": {"groups": {"account:update"}}}
 *     }
 * )
 */

Which should throw an error, but does nothing. The config is ignored.

@soyuka
Copy link
Member

soyuka commented Jan 30, 2019

@dunglas we were not using the operation configuration but rather the resource-level one:

https://github.com/soyuka/core/blob/cb5421abdb19392ac225909f63cf8677583cedd3/src/Bridge/Symfony/Routing/ApiLoader.php#L215-L216

Therefore, /** @ApiResource(itemOperations={"get"={"output_class"="FooBar"}})*/ is not supported yet, @ApiResource(input_class="") is though.

@bendavies yes the fix is in the correct place.

@dunglas
Copy link
Member

dunglas commented Jan 30, 2019

The input class shouldn't be in the request attribute but in our own metadata We must change this behavior before tagging 2.4.

@dunglas dunglas mentioned this pull request Feb 1, 2019
1 task
@bendavies
Copy link
Contributor Author

Closing due to #2483

@bendavies bendavies closed this Feb 1, 2019
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.

None yet

4 participants