NelmioApiDocBundle icon indicating copy to clipboard operation
NelmioApiDocBundle copied to clipboard

PHPStan error when using Model in Response attribute

Open lucasmirloup opened this issue 3 years ago • 8 comments

Hi, When using a Model in a Response attribute like in the documentation : #[OA\Response( response: 200, description: 'Successful response', content: new Model(type: User::class) )]

I get this PHPStan error : Parameter $content of attribute class OpenApi\Attributes\Response constructor expects array<OpenApi\Annotations\JsonContent|OpenApi\Annotations\MediaType|OpenApi\Annotations\XmlContent>|OpenApi\Annotations\JsonContent|OpenApi\Annotations\MediaType|OpenApi\Annotations\XmlContent|null, Nelmio\ApiDocBundle\Annotation\Model given.

Is the documentation incorrect or does the problem come from the code ? Thank you in advance.

lucasmirloup avatar Aug 31 '22 15:08 lucasmirloup

This is a limitation in the type-hints in swagger-php.

Seems with the advance of type-hints we are getting more of these situations where properties are 'extended' with new value types in order to implement additional features.

Not quite sure how this can be solved in a simple way that doesn't require to water down type-hints every time a new use-case pops up...

DerManoMann avatar Aug 31 '22 20:08 DerManoMann

We may advise to use attachables instead of content for using @Model, I believe this works just as fine although maybe less syntactically pleasant.

Attachables actually may be the suggested extension point for 3rd party libraries now, isn't it? I think we created our annotations before their creation. Probably would need to update our code to make @Model extend Attachable and to completely respect the type hints from zircote/swagger-php.

GuilhemN avatar Aug 31 '22 21:08 GuilhemN

We may advise to use attachables instead of content for using @Model, I believe this works just as fine although maybe less syntactically pleasant.

You are right, of course. Just like x-.. is the vendor namespace for custom spec data attachables allow to attach any custom class/instance to an annotation/attribute. I agree that just stuffing everything custom in the attachable property is not very intuitive.

In that respect annotations work a lot better than attributes as you can just nest whatever annotations you create.

Attachables actually may be the suggested extension point for 3rd party libraries now, isn't it?

Yup. Ironic that I forgot to mention that considering I implemented them :)

I think we created our annotations before their creation. Probably would need to update our code to make @Model extend Attachable and to completely respect the type hints from zircote/swagger-php.

That's how things work over time. I admit that swagger-php changed a lot in the last few years (mea culpa).

OTOH, just using the attachables property does feel like the cheap way out. I wonder if we could also add a AttachableInterface and add that as typehint to all properties of interest for 3rd party libraries. Behaviour could be the same in that for serialization those properties would be ignored (treated like Generator::UNDEFINED).

DerManoMann avatar Aug 31 '22 22:08 DerManoMann

Thank you for your answers :+1:

I don't mind using attachables if it conveys the same information as content and doesn't trigger a PHPStan error, but unfortunately that's not the case.

I get this PHPStan error when using it : Parameter $attachables of attribute class OpenApi\Attributes\Response constructor expects array<OpenApi\Attributes\Attachable>|null, array<int, Nelmio\ApiDocBundle\Annotation\Model> given.

Ideally, I would like to avoid silencing PHPStan. If I understand well, that's not currently possible ?

lucasmirloup avatar Sep 02 '22 14:09 lucasmirloup

I'm not sure we have the need for an AttachableInterface in this case, our annotations either directly extend AbstractAnnotation, or are not meant to be nested. Extending Attachable instead should work fine.

And supporting attachables in content would be a nice way out I think :) I think we only support @Model as content in Response and RequestBody.

Allowing attachables on those, and updating our annotations here would do fine in my opinion. @DerManoMann would you like me to open a PR with this on swagger-php?

GuilhemN avatar Sep 03 '22 09:09 GuilhemN

@lucasmirloup indeed then this doesn't work for now, will do the change and extend Attachable in our annotations.

GuilhemN avatar Sep 03 '22 09:09 GuilhemN

@GuilhemN sure. Always easier to discuss something concrete. Personally I'd prefer an interface so you are not limited by having to extend from yet another class but we'll see.

DerManoMann avatar Sep 03 '22 10:09 DerManoMann

We can leave this open for now to keep track of https://github.com/zircote/swagger-php/pull/1307

GuilhemN avatar Sep 03 '22 10:09 GuilhemN

Hi @DerManoMann @GuilhemN :wave: Thanks for the fix ! I kinda forgot about this issue.. It works now AFAIR, can we close it ?

lucasmirloup avatar Mar 17 '23 13:03 lucasmirloup