Add missing @param and @return PHPDoc attributes

[simonberger]

After the recent CS improvement batches some @param attributes had been replaced by @psalm-param which removes information for IDEs which are not interpreting those attributes. Same goes for @return.

This is the first step of two or three. What imho should follow and what I am willing to do (if you like this PR) is:

• @param which uses array<> or array{} syntax should be changed to @psalm-param and like in this PR duplicated+fixed for @param (same goes for @return again).
• Fixes for @var and @psalm-var I left this out here.

/cc @greg0ire

[simonberger]

I'll look at and fix the problems reported from the CS job after your general feedback.
I cannot really see in which file the reported issues are or do I miss the link to a detailed report? I guess in most cases it does not like the order I put the params. I wanted to discuss this here anyway.

I like to put the psalm param always directly after the normal param but there might be completely opposite opinions.

[greg0ire]

I cannot really see in which file the reported issues are or do I miss the link to a detailed report?

You can have a look at the files changed tab, it will show comments on lines that have issues. You can fix many of those by running vendor/bin/phpcbf.

[greg0ire]

I like to put the psalm param always directly after the normal param but there might be completely opposite opinions.

That might make sense, but would require a contribution to doctrine/coding-standards so that it can be enforced. Also, I think types should be aligned, like this:

@param       string[]     $foo
@psalm-param list<string> $foo

[simonberger]

That might make sense, but would require a contribution to doctrine/coding-standards so that it can be enforced. Also, I think types should be aligned, like this:

That looked like easy to achieve if I did not miss anything.
If we agree to this sort of grouping we could merge the following PR.

doctrine/coding-standard#238

[simonberger]

Hi @greg0ire,
can you please do a release of coding-standard? Then I can finish this PR.

[greg0ire]

Ok, but let's first resolve this: doctrine/coding-standard#238

[greg0ire]

@simonberger done :)

[simonberger]

@greg0ire I rebased to 2.8, resolved conflicts and went through the list of all changes afterwards.
This is ready for the round of reviews.

[greg0ire]

@orklah @Jean85 please review

[orklah]

@simonberger out of curiosity, what IDE did you have issue with and with which syntax?

[simonberger]

@orklah I hadn't really any problem yet. I just fix issues introduced in the psalm optimizations lately.
On the other hand to remove @param annotations and replace them by psalm-prefixed ones are a problem for any IDE I would say.

[orklah]

@orklah I hadn't really any problem yet. I just fix issues introduced in the psalm optimizations lately.
On the other hand to remove @param annotations and replace them by psalm-prefixed ones are a problem for any IDE I would say.

Ok. just FYI, PHPStorm in recent version is able to parse and understand array<array-key, string>, class-string and others quite well. Not sure about others though

[simonberger]

@orklah I hadn't really any problem yet. I just fix issues introduced in the psalm optimizations lately.
On the other hand to remove @param annotations and replace them by psalm-prefixed ones are a problem for any IDE I would say.

Ok. just FYI, PHPStorm in recent version is able to parse and understand array<array-key, string>, class-string and others quite well. Not sure about others though

Yes that's correct and for me it would also be enough. All other IDEs which are not PHPStorm at this point don't understand this notation though.
It is not about this PR though, because the array<> syntax is not in @param but in @psalm-param so there is no type information on a lot of parameters at all. PHPStorm nowdays uses also psalm to display errors but I doubt it uses it for autocompletion and stuff like that.
Still in a followup PR I plan to change all @param annotations including array<> notation to psalm and add a simplified @param annotation instead.

You can find some more context here #8539 (review)

[orklah]

It is not about this PR though, because the array<> syntax is not in @param but in @psalm-param so there is no type information on a lot of parameters at all. PHPStorm nowdays uses also psalm to display errors but I doubt it uses it for autocompletion and stuff like that.

It does, in the limit of their inference engine. Basically, if there's an array<string, int>, PHPStorm will interpret that as int[] (as they don't yet support offset types). This PR is not an issue, because PHPStorm will look for more detailed type into @psalm-param and such, just wanted to let you know if you didn't already

[simonberger]

If someone is willing to review this PR I am solving the conflicts a last time, Otherwise I'll close it by the end of next week.

[greg0ire]

I will review the PR. Please rebase on 2.9.x.

[simonberger]

@greg0ire Rebased on 2.9. Ready to review. Unfortunately I cannot check the pipeline. Possible that there are some small reports.
I checked inspections on 2.9 state again and did some small more or less new changes in a separate commit. I can squash them before merge.

[greg0ire]

Unfortunately I cannot check the pipeline.

You can still run Psalm and Phpstan locally, as well as vendor/bin/phpcbf 🙂

Possible that there are some small reports.

There were indeed some, I fixed them and I updated the baseline files so we can see how beneficial your changes are 🙂

I checked inspections on 2.9 state again and did some small more or less new changes in a separate commit. I can squash them before merge.

Did that for you too 🙂

[greg0ire]

Thanks @simonberger !

[simonberger]

Thank you @greg0ire

I updated the baseline files so we can see how beneficial your changes are

I think most of the changes on the baseline are unrelated to my changes though, as they are mainly solving type-holes IDEs might have. :)

[greg0ire]

Oh right, that would be previous PRs 😅