[3.x] Re-worked pagination to not mutate the api classes

[GrahamCampbell]

This re-works pagination to avoid mutating the API classes. It took a few attempts to get the implementation details right. Let me explain why I chose to use clone:

1. If we were to use new static(...), then I would have had to have made the abstract api constructor final. This could cause problems for people mocking the api classes, and also limits the possibility to thunk up arguments to the api classes, say if we want to introduce an api class which takes an extra parameter which is always used (this is common place in the gitlab fork of this library).
2. Is there another way to do things without making the constructor final or using clone? Well, we could have every api object implement perPage for themselves. This is kinda cumbersome though, and I don't think we want to do this.

---

This PR partially addresses #904.

So, I think cloning then setting the private property, within the abstract api class is the way to go, and nicely separates responsibilities. Classes that extend the abstract api class cannot mutate the property, and outsiders also cannot either. They have to call perPage which gives a fresh instance (one we've cloned).

[acrobat]

@GrahamCampbell I'm testing this PR but it doesn't seem to work. Is it a bug or am I doing something wrong?

$paginator  = new Github\ResultPager($client);
$api = $client->api('user');
$api = $api->perPage(2);

$result = $paginator->fetch($api, 'repositories', ['acrobat']);

This fetch function still uses the default of 100 items per page

[GrahamCampbell]

Yeh, that is what I would expect to happen, and it's also how the current paginator works.

[GrahamCampbell]

The paginator intentionally doesn't allow users to change the internal number of items per page.

[acrobat]

Well actually this does work with the old pager

dev-master

<?php

require_once 'vendor/autoload.php';

$client = new \Github\Client();
$client->authenticate('xxxxxx', \Github\Client::AUTH_ACCESS_TOKEN);

$paginator  = new Github\ResultPager($client);
$api = $client->api('user');
$api->setPerPage(2);

$result = $paginator->fetch($api, 'repositories', ['knplabs']);

echo \count($result) .PHP_EOL;
while (($result = $paginator->fetchNext()) !== []) {
    echo \count($result) .PHP_EOL;
}

Output

→ php test.php
2
2
2
2
2
2
2
2
2
2
2
// Stopped the script here

pager changes code:

<?php

require_once 'vendor/autoload.php';

$client = new \Github\Client();
$client->authenticate('xxxxx', \Github\Client::AUTH_ACCESS_TOKEN);

$paginator  = new Github\ResultPager($client);
$api = $client->api('user');
$api = $api->perPage(2);

$result = $paginator->fetch($api, 'repositories', ['knplabs']);

echo \count($result) .PHP_EOL;
while (($result = $paginator->fetchNext()) !== []) {
    echo \count($result) .PHP_EOL;
}

output

→ php test.php
100
8

Previously the callApi method was used as we now call the perPage method first

php-github-api/lib/Github/ResultPager.php

Line 62 in 9b01208

$result = $this->callApi($api, $method, $parameters);

https://github.com/KnpLabs/php-github-api/pull/907/files#diff-40de580826089d3ae27bf74e6b2b9402R76

[GrahamCampbell]

Hmm, right. I suppose my question is why would anyone want to do that?

[acrobat]

I can see use cases for this like forexample the travis-ci settings page where you show all available repositories to enable. This could be a paginated list with 20 items per page. If there is no usecase for changing the per page amount, why do we have the perPage method?

[GrahamCampbell]

The perPage method allows the paginator to set the number of pages it wants. It has to be able to interface with the api classes somehow.

[acrobat]

Hmm I see there was the confusion on my part. Setting the per page amount in the resultpager constructor does the job.

$paginator  = new Github\ResultPager($client, 2);

But now I thinking that this might be confusing to for the users of this library. Could we keep the private property but use a closure bind setup to clone and update the perpage value without having a public perpage method which doesn't actually work with the paginator? What do you think @GrahamCampbell?

public function fetch(ApiInterface $api, string $method, array $parameters = [])
    {
        $paginatorPerPage = $this->perPage;
        $closure = \Closure::bind(function (AbstractApi $api) use ($paginatorPerPage) {
            $clone = clone $api;
            $clone->perPage = $paginatorPerPage;

            return $clone;
        }, null, AbstractApi::class);

        $api = $closure($api);
        $result = $api->$method(...$parameters);

        $this->postFetch();

        return $result;
    }

[acrobat]

@GrahamCampbell what do you think of the solution above? Would this be the way to finish this PR?

[acrobat]

@GrahamCampbell what do you think of the proposed solution? I would like to finish this PR and release the 3.0 😄

[GrahamCampbell]

Sorry for the delay here. I will get to this soon. NB There are currently merge conflicts.

[acrobat]

No worries! 😄

[GrahamCampbell]

Rebased and made the relevant changes you wanted. How is this looking now, @acrobat?

[acrobat]

A few things already, I will also try this change in my small test app later!

[GrahamCampbell]

I've applied the changes you asked for. :)

[acrobat]

Yes, thank you! I will try to do a review and test in my demo app later tonight!

[acrobat]

My "test app" for the pagination worked great and intuitive with the latest changes 👌 I will do a quick code review later and prepare a PR for 2.x to deprecate some things changed/removed in this PR

[acrobat]

Thank you @GrahamCampbell