Allow messenger handler to return non-resource output

[karser]

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#2860 #2810 #2803
License	MIT

Hi folks!

Let's imagine ResetPasswordRequest / ResetPasswordRequestHandler case from the api-platform docs (https://api-platform.com/docs/core/messenger/).

Before api-platform v2.4.3 it was possible to return the output dto object (ResetPasswordResponse) directly from the messenger handler. It was very convenient, and that's what dto's are for - return a custom response without exposing an entity.
As of v2.4.3 it started to complain that ResetPasswordResponse must be a resource, so this PR allows returning the non-resource output.

[teohhanhui]

I've already explained to you on Slack why this is wrong. But let me say it again: it's the job of the DataTransformer to transform a resource to the output DTO. If you want to return a non-resource, just remove the output option and it should work.

[karser]

Sorry for bothering you again with this case but I'm not seeing an alternative in the approach you suggest. Moreover, it was working in 2.4.2. Why a bugfix update breaks the backwards compatibility?

If you want to return a non-resource, just remove the output option and it should work.

the output model is auto-documented. If I remove the output, the documentation will be missing.

it's the job of the DataTransformer to transform a resource to the output DTO

the code would be too scattered if ResetPasswordRequestHandler returned a resource and ResetPasswordRequestDataTransformer returned an output object. Also, the resource doesn't have to be convertable to the output dto.

My points are:

• returning the output was described in the first version of dto docs added by @lyrixx. The messenger+output is supposed to be a successor of that approach.
• output is amazingly useful - it solves the problem of custom methods and adds the output to swagger doc

I'm wondering what is @dunglas and @soyuka opinion on this case.

[soyuka]

I've already explained to you on Slack why this is wrong. But let me say it again: it's the job of the DataTransformer to transform a resource to the output DTO. If you want to return a non-resource, just remove the output option and it should work.

Wait we'ŕe in the context of a MessageHandler, isn't it a bit different? Could the MessageHandler act as a data transformer? Shouldn't we support non-resources anyway (to me we definitely should)?

the code would be too scattered if ResetPasswordRequestHandler returned a resource and ResetPasswordRequestDataTransformer returned an output object.

To me this makes sense and removes some useless boilerplate code.

I don't really see anything wrong with the PR here, it just simplifies this use case no?

[karser]

Hey guys! What should I do to get this PR merged? Submit it again? Add some tests?

…

On Wed, Jun 26, 2019, 3:14 PM Antoine Bluchet ***@***.***> wrote: I've already explained to you on Slack why this is wrong. But let me say it again: it's the job of the DataTransformer to transform a resource to the output DTO. If you want to return a non-resource, just remove the output option and it should work. Wait we'ŕe in the context of a MessageHandler, isn't it a bit different? Could the MessageHandler act as a data transformer? Shouldn't we support non-resources anyway (to me we definitely should)? the code would be too scattered if ResetPasswordRequestHandler returned a resource and ResetPasswordRequestDataTransformer returned an output object. To me this makes sense and removes some useless boilerplate code. I don't really see anything wrong with the PR here, it just simplifies this use case no? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2878?email_source=notifications&email_token=AAMY6GLPMIWPBGGKZRIEIFDP4NMTLA5CNFSM4H3LURP2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODYTKJDY#issuecomment-505848975>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMY6GO6OOTYJYBQMPL2FADP4NMTLANCNFSM4H3LURPQ> .

[teohhanhui]

@soyuka @karser

If you want to return a non-resource, just remove the output option and it should work.

the output model is auto-documented. If I remove the output, the documentation will be missing.

This is the actual issue, a more general problem. We should allow documenting any classes.

Otherwise it's already correct. Just don't use output class if you don't want to use a DataTransformer. It's unnecessary.

[karser]

@teohhanhui You are right, the documentation - that's what I use the output for.

We should allow documenting any classes

How do you imagine that? A special option or using swagger_context?

Tweaking swagger_context won't work, looks like the DTO class has to be added to swagger definitions.

App\UseCase\ForgotPasswordRequest:
    collectionOperations:
        post:
            path: '/users/forgot-password-request'
            messenger: true
#            output: 'App\UseCase\ForgotPasswordResponse'
            status: 200
            swagger_context:
                responses:
                    200:
                        description: 'Response'
                        schema:
                            type: 'App\UseCase\ForgotPasswordResponse'
#                            $ref doesn't seem to be working as well

[teohhanhui]

Let's continue that discussion in #2147