search

BabDev / Pagerfanta

Pagination library for PHP applications with support for several data providers
Other
384 stars 170 forks source link

Static analysis improvements #34

Closed gjuric closed 3 years ago

gjuric commented 3 years ago

Hi,

this commit added strict phpstan parameter docblocks. While I agree that return types could be helpful I would argue that the input parameter hints will only cause consumers of this library to add more stuff to the phpstan ignore file without any real benefit.

This can also be witnessed by the fact that the ignore file had to be updated for the library itself.

mbabker commented 3 years ago

Well, that's a bit of a mixed bag.

A lot of the PHPStan ignores are in part because the return types from the various adapters can't be guaranteed to match the @phpstan-return declarations, and in part, because there are runtime assertions made that match the constraints of the @phpstan-param declarations. There are then a handful of ignores that are there because the code isn't making runtime checks on some conditions or that there are some conditions that can't be evaluated 100% of the time (like the ignore on Pagerfanta::getPreviousPage(), even with the runtime checks there isn't a way to tell PHPStan that this line will always be a positive integer).

If there are any SA declarations that get in people's way, I have no issue doing what I can to keep them from being a problem (likewise, if anyone wants to add SA declarations to improve the code (especially for Psalm which I really haven't worked with at all) then those changes are also welcome). But right now, I don't think the changes in that commit are going to be too disruptive to library consumers as all of the additions in that commit are what the API should be returning already.

mbabker commented 3 years ago

Just for reference, I did get around to updating one of my projects today and pulling in these SA changes. It forced me to add a lot of /** @phpstan-var positive-int $limit */ and /** @phpstan-var positive-int $page */ annotations to controllers reading request data where those inputs weren't previously strictly annotated; that was the only impact on that particular project.

gjuric commented 3 years ago

Not sure I agree with adding annotations for stuff like this, a better option IMHO would be treating negative input integers for the page as page 1, the limit would be trickier, but both 1 and 25 sound like a nice-enough numbers :)

Anyhow, I will be closing this issue, thank you for your input.