Fix return type of Header::all #46613
Conversation
| @@ -65,7 +65,7 @@ public function __toString() | ||
| * | ||
| * @param string|null $key The name of the headers to return or null to get them all | ||
| * | ||
| * @return array<string, array<int, string|null>>|array<int, string|null> | ||
| * @return $key is null ? array<string, array<int, string|null>> : array<int, string|null> |
There was a problem hiding this comment.
| * @return $key is null ? array<string, array<int, string|null>> : array<int, string|null> | |
| * @return ($key is null ? array<string, array<int, string|null>> : array<int, string|null>) |
See https://phpstan.org/r/18e18b32-1cc3-4647-875b-c756ad1ad4d7
Also, to keep IDE support I would recommend
| * @return $key is null ? array<string, array<int, string|null>> : array<int, string|null> | |
| * @return array<string, array<int, string|null>>|array<int, string|null> | |
| * @phpstan-return ($key is null ? array<string, array<int, string|null>> : array<int, string|null>) |
(or psalm-return if preferred)
There was a problem hiding this comment.
Applied the first fix, thanks.
The second I would agree but I think it's again symfony policy so leaving it as is.
Co-authored-by: Vincent Langlet <VincentLanglet@users.noreply.github.com>
|
AFAIK (cc @nicolas-grekas) we only annotate types that have broad tooling support. This is certainly not the case for this kind of conditional types. And we don't use vendor-prefixed annotations either. If that issue could be solved by the Symfony plugin for Psalm, I think I'd prefer that way. |
Stub with this conditionnal return type can be added in both phpstan-symfony and psalm-symfony plugin yes.
Is there any plan to start using prefixed annotations later ? Or what is the reason to not to ? |
|
@nicolas-grekas IMO this policy is a mistake at this point. It might have made sense a while back but PHPStan usage is spreading more and more, and having type annotations shipped with the framework would make more sense. It ensures the types are always in sync with the Symfony version and they would also benefit Symfony itself in terms of added static analysis capability. I've seen a similar thing happen in the TypeScript ecosystem where at first typings packages where provided to type things from the outside, and then eventually all major packages started shipping their own type definitions or even switching their source completely to typescript. PHPStan is now a very established tool, and Psalm also reads Just my 2c :) |
There are no plans yet.
We use Psalm merely as a code review tool these days. If you want to dicuss an initiaive to make the codebase more accessible for SA tools, it would maybe be better to discuss this in a separate issue first. |
|
Yeah, I don't have time for that crusade, so closing here, moved to phpstan/phpstan-symfony#282 - but for the record my point above stands. |
|
For the record, having phpstan+psalm+phpstorm support for the syntax is our current bar. |
Since PHPStorm 2022.3.1, the conditional return is supported by PHPStorm. So would the PR be accepted if re-opened ? @nicolas-grekas |
|
do phpstan+psalm share same syntax now? IMHO it's essential for maintenance. Unless it's not, and we choose vendor prefixes, for what is a single language :D |
|
Please resubmit after checking with the main tools yes. |
|
Sure, #49007 |
cc @derrabus