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

Add Attachable to Response::$content and RequestBody::$content #1307

Merged
merged 2 commits into from
Sep 11, 2022

Conversation

GuilhemN
Copy link
Contributor

@GuilhemN GuilhemN commented Sep 3, 2022

@DerManoMann
Copy link
Collaborator

The failing test means that Annotations and Attributes have different type-hints for properties.
You'll have to add those type hints for the annotation properties too.

@GuilhemN
Copy link
Contributor Author

GuilhemN commented Sep 7, 2022

Indeed, should be better now

@DerManoMann
Copy link
Collaborator

One more question before I merge - have you tried this? I suspect that no matter where you put your @Model it will end up in the attachables property of the parent since all nested objects are merged via the value property key (that's how doctrine nested annotations work...)

@DerManoMann
Copy link
Collaborator

Oh, well, lets see how that goes. I might extend that a bit to other properties though, seems stange to do this only for some...

@DerManoMann DerManoMann merged commit 0c1cdd3 into zircote:master Sep 11, 2022
@DerManoMann
Copy link
Collaborator

thanks @GuilhemN

@GuilhemN
Copy link
Contributor Author

Thank you for merging (and sorry for answering late).
Indeed if using attributes, or directly nested annotations, the annotation will end up in attachables. The only way to put it into content is using annotations and being explicit to put it into content.

I was thinking of just processing both fields in NelmioApiDocBundle to retrieve our custom annotations, but thinking about more generic use cases that would probably be quite weird and the behavior would be inconsistent between annotations and attributes... And from a semantic point of view, using content for attachables would mostly be syntactic sugar, so logically all Attachables should go to attachables property anyway.
Maybe a better implementation would be to detect Attachables in all properties and put them in attachables, and rework the test case that was failing to ignore Attachable in annotation type hints?

@DerManoMann
Copy link
Collaborator

I think attachables will end up in the attachables property regardless of where you put them.
So, while it is just sugar it does mean the attributes do read nice. As noted above, adding typehints to all currently strict typed annotation/attribute parameters would be good for consistency.

Hmm, I would have expected annotations would behave the same - that is what causes all nested annotations to go through the same funnel. I suppose you could subclass an explicit type and pass that into content tomake it stick, but if all your custom annotations extend Attachable you should be fine to just look at attachables.

Either way, I'll leave it to you to move this forward if needed. Otherwise it would be cool to at least create a new issue to track adding typehints to all remaining properties that take nested annotations so we do not forget to do that at some point,

@GuilhemN
Copy link
Contributor Author

To be more specific, using @Response(content=@Model(...)) would lead to Model staying in content field.

But this syntax isn't used in your tests or docs, so maybe we can simply consider it isn't supported.

I will let you know if I come up with something that would solve this nicely. I'll create the tracker issue as well, no problem.

@DerManoMann
Copy link
Collaborator

To be more specific, using @Response(content=@Model(...)) would lead to Model staying in content field.

Ah, I see. No, swagger-php only uses anonymous? nested annotations. I wasn't even aware that you can do this.
I suppose it depends on whether you custom annotations are ambiguous or not without assigning them to specific properties...

@DerManoMann
Copy link
Collaborator

Just remembered that we added object as type for $ref too in order to make static analysers happy - maybe that should also be upgraded to Attachable?

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.

2 participants