-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
fix(AppFramework): Adjust types so PHPStan understands them #58098
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -7,15 +7,12 @@ | |||||
| namespace OC\AppFramework\OCS; | ||||||
|
|
||||||
| use OCP\AppFramework\Http; | ||||||
| use OCP\AppFramework\Http\DataResponse; | ||||||
| use OCP\AppFramework\OCSController; | ||||||
|
|
||||||
| /** | ||||||
| * @psalm-import-type DataResponseType from DataResponse | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no generic equivalent to this psalm instruction?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not that phpstan doesn't understand this, it seems to have problems with the template-covariant referencing the alias. |
||||||
| * @template S of Http::STATUS_* | ||||||
| * @template-covariant T of DataResponseType | ||||||
| * @template H of array<string, mixed> | ||||||
| * @template-extends BaseResponse<Http::STATUS_*, DataResponseType, array<string, mixed>> | ||||||
| * @template-covariant S of Http::STATUS_* | ||||||
| * @template-covariant H of array<string, mixed> | ||||||
| * @template-extends BaseResponse<Http::STATUS_*, array|int|float|string|bool|object|null|\stdClass|\JsonSerializable, array<string, mixed>> | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Is that not the same type?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Back when this was added it didn't work with psalm, but yes that is how it is supposed to be. |
||||||
| */ | ||||||
| class V1Response extends BaseResponse { | ||||||
| /** | ||||||
|
|
||||||
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.
Is that not the same as
mixed🙃 ?Also,
\stdClass|\JsonSerializableis redundant withobject, no?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.
Nope, this is the correct type. It only allows what can be serialized to JSON.
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 how can a var be
\stdClasswithout beingobject? And same for JsonSerializable. For me eitherobjector\stdClass|\JsonSerializableshould be removed.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.
https://www.php.net/manual/en/language.types.mixed.php The only thing refused in this union is resource.