Remove non-negative-int typehint on `FixedAdapter` `nbResults` argument

[angeleg]

Same issue as this one, but this time for the nbResults argument of FixedAdapter's constructor.

    /**
     * @param iterable<array-key, T> $results
     *
     * @phpstan-param int<0, max> $nbResults
     */
    public function __construct(int $nbResults, iterable $results)
    {
        $this->nbResults = $nbResults;
        $this->results = $results;
    }

It's type-hinted as @phpstan-param int<0, max>, which is a annoying to have to assert on our end, especially in the common use-case of passing is the result of a count query from a repository, which is never going to be negative.

Could we type-hint it as a simple int instead?

[mbabker]

This one I think I want to leave for two reasons:

1. There isn't a runtime validation on the $nbResults param like there is for the params where the constraint was removed in 1f1345306c2adca8c1cf857298aae677eb9e2a56

2. The adapters cannot have a negative number of results, so the range conveys the actual requirement better than a simple integer typehint can without adding new runtime checks (which could be considered a B/C break if new exceptions are thrown)

[ruudk]

The adapters cannot have a negative number of results, so the range conveys the actual requirement better than a simple integer typehint can without adding new runtime checks (which could be considered a B/C break if new exceptions are thrown)

So what happens when -1 is passed now? It probably fails or crashes too, right? So what's the difference then with throwing an assertion exception? That would not be BC break then? But I could be missing something.

[mbabker]

So what happens when -1 is passed now? It probably fails or crashes too, right?

Depends on the consumer mostly. The callback, fixed, and null adapters are the only adapters where someone can directly provide the getNbResults() return value, the rest are based on some kind of query result or count($countable). So for 99% of cases, the API should not be able to return a negative number.

Looking at where that value gets used in the Pagerfanta class, I don't see anything that would immediately cause crashes, but there's definitely some potential for awkward behavior (i.e. you could end up with a negative page count from Pagerfanta::getNbPages(), and calls to Pagerfanta::getPageNumberForItemAtPosition() would always throw an OutOfBoundsException unless you're passing negative numbers in there as well (in which case you're really Doing it Wrong™)).

So what's the difference then with throwing an assertion exception? That would not be BC break then?

Depends who you ask. Some folks consider adding new exceptions where one isn't already thrown to be a B/C break (and in DotNet and Adobe B/C guides they explicitly call out adding a new exception type as a break). There's already a decent amount of validation for things coming in while configuring the Pagerfanta class, but there isn't any validation for the values coming out of the adapters (specifically the result count). So if I were going to remove the range constraint from PagerfantaInterface::getNbResults() and AdapterInterface::getNbResults(), I'd also want to allow them to throw something so that an adapter can self-validate if it so chooses while the Pagerfanta class just hard aborts over the unexpected value instead of letting things flow with invalid data.

[mbabker]

This is fixed for 4.x (which is coming soon).

I'm playing this one conservatively because there is no runtime validation for the result count right now and I kind of agree with the linked B/C guides about not introducing totally new exceptions without a major. So https://github.com/BabDev/Pagerfanta/commit/6cb7b381a64e85b6080d1acc18f9f88bfdb0b791 removes the PHPStan assertion and adds a runtime check and throw in the fixed adapter's constructor, as well as allows the adapter's getNbResults() method to throw if need be (and the callback adapter does just this).