-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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 types to responses #38802
Add types to responses #38802
Conversation
The |
I think the addHeader method should be deprecated to solve the issue |
c5d818d
to
e7e71a4
Compare
Deprecation means we can remove it in 3 years. So it won't solve your issue any time soon. |
I know, I will have to add some comments about this in the OpenAPI tutorial/guidelines. |
Is there any mechanism is place to ensure this doesn't fall apart with future modifications of the code? How does a developer check if they need to annotate changed/new APIs? |
Do you mean the code of the response classes? Those are validated by psalm
Developers don't need to add any new annotations because of these changes. |
e7e71a4
to
1056cc2
Compare
1056cc2
to
9e9fd9d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean internal, right? We are not planning to remove the function
9e9fd9d
to
e838f84
Compare
e838f84
to
83f6338
Compare
@come-nc @nickvergessen I will wait for your approvals as well, just to be sure everything is alright |
f794c59
to
e524051
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but fine either way
e524051
to
7f31fb9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed one last type, but fine by me
7f31fb9
to
757b464
Compare
757b464
to
5966c73
Compare
5966c73
to
2112d75
Compare
Signed-off-by: jld3103 <[email protected]>
2112d75
to
b0001c6
Compare
🎉 |
Mail's Psalm shows that all responses have to be type-annotated now due to vimeo/psalm#6870 (comment). Example: https://github.com/nextcloud/mail/blob/b7b84f452ac6951d0d2e6d96e33b9eec91ac6098/lib/Http/AttachmentDownloadResponse.php https://psalm.dev/r/92dc4c7871
It means those responses have to specify a type for the template (for static headers and status code) https://psalm.dev/r/68242bdab6. If I spec the type for server master I'll get an error for stable27 and older: https://psalm.dev/r/7038ef2ed1 Could there be any trick to allow responses to have types but with a default? |
For subclasses you either need to implement the template parameters which should be quite easy or you just supress the error. Implementing the template parameters should be quite easy and afterwards you don't need to add the parameters when you use the type (like in return annotations), you can just leave them empty and it will use the "default" parameters. |
If I spec the type for server master I'll get an error for stable27 and older: https://psalm.dev/r/7038ef2ed1 |
Well, then suppress until you support only 28+? |
Every call to the Subsonic XML API caused and error 'Undefined array key "" at /var/www/html/lib/private/AppFramework/Http.php#128' to be logged to the nextcloud.log. This happened because the XmlResponse object had no response status set. Setting this used to happen automatically, but after the Nextcloud PR nextcloud/server#38802, the automatic setting has not happened without a call to the parent class constructor from the derived class. However, we can't call the parent constructor because it doesn't exist on ownCloud. To overcome the problem, we now set the status code explicitly in the constructor of XmlResponse. refs #1142
From #36666
Summary
Sadly the methods have to be overridden in all the subclasses, but the code for that is always mostly the same.
Checklist