flightphp / core

An extensible micro-framework for PHP
https://docs.flightphp.com
MIT License
2.64k stars 407 forks source link

Wrong syntax of @method tags in Flight class docblock #608

Closed anthonyaxenov closed 2 weeks ago

anthonyaxenov commented 1 month ago

Some @methods are described incorrectly. Types here should be in native php syntax and there is no way to describe generic types in @methods directly. Generic types are allowed in @param, @return and @template* tags. Otherwise IDE will not parse such method correctly.

Just a small example with Flight::set():

image

image

I didn't looked up closely in other files but it definitely worth to do

Reference: https://github.com/flightphp/core/blob/master/flight/Flight.php#L21-L81

Found in mikecao/flight v3.12.0

lubiana commented 1 month ago

Hi @anthonyaxenov interesting find there and you are right about phpstorm not picking up on the complex array/iterable shapes for @method annotations. i would guess they are mostly used for static analysis here. So maybe that would be a nice feature requests for the jetbrains issue tracker, as i see no reason why phpstorm should not support this syntax. update: i added an issue in the jetbrains issue tracker https://youtrack.jetbrains.com/issue/WI-79045/Missing-support-for-arrayshapes-in-method-docblock-annotation feel free to give that issue an upvote

edit: just checked and phpstan does indeed support this syntax (https://phpstan.org/r/948cd893-1920-49e6-9f19-02413890e577) The same code in phpstorm does not correctly check the types: grafik

anthonyaxenov commented 1 month ago
  1. I feel I need to explain something. PhpStorm is my main IDE and I used to believe its warnings in 95% of cases because it has very powerfull static analyzer under the hood. I met this behavior while working on my own pet-project and I had no time at that moment to check if this syntax acceptable for another static analyzers used in Flight. Such behavior is pretty annoying but I never know when I return to this pet-project next time. Also I never used such syntax myself inside of @methods. So I created this issue just to let another developers know that something may be incorrect here.

  2. Unfortunately, there is not standard about phpdoc, only phpDocumentor docs and PSR-5 draft which looks pretty similar (don't they?) and very inconcrete. There are no explicit descriptions about its syntax. Instead of that, php ecosystem (mostly developed by community) and 3rd-party developers' solutions becomes standard de-facto. So that increases entropy in tooling, this case shows it well.

  3. I checked your example in psalm -- and yes, it also supports generic arguments in @method tags: https://psalm.dev/r/b837894b7e (UPD I noticed your psalm link in youtrack too late))

  4. I think this is good idea to create an issue in JB Youtrack, thanks for that. If I would create it they would definitely close it because of geopolitical reasons.

lubiana commented 1 month ago
  1. Yeah, I use PhpStorm myself and for coding mostly depend on its type analysis. Its an awesome tool and as you said works right almost all the time. Regarding your specific problem I think the best advice is to use the engine instance of Flight instead of the static methods on the Flight class itself. The docs currently do not advertise that way of using the framework really, but you could take a look at the skeleton project (https://github.com/flightphp/skeleton/blob/master/app/config/bootstrap.php#L17) when using the engine instance, the types picked up by phpstorm should mostly be correct.
  2. You are absolutely right, that the phpDocumentor docs are not always helpfull nowadays. Personally I stick to the phpstan documentation about declaring types (https://phpstan.org/writing-php-code/phpdoc-types) There are more and more projects using phpstan nowadays so in my experience those type declarations are often the standard way to use. But as I said, that is mostly my personal experience and preference.

Thanks again for bringing this issue up and lets hope that JetBrains will fix the type discovery on their side. If not we might need to revisit this again here and look if we can improve that in flight itselt.