Backport ResultPager changes to 2.x

Co-authored-by: Graham Campbell <[email protected]>

Author: acrobat

lib/Github/Api/AbstractApi.php

@@ -64,6 +64,8 @@ public function setPage($page)
     }
 
     /**
+     * @deprecated since 2.18 and will removed in 3.0. Changing items per page will be done internally by the `ResultPager`.
+     *
      * @return null|int
      */
     public function getPerPage()
@@ -72,6 +74,8 @@ public function getPerPage()
     }
 
     /**
+     * @deprecated since 2.18 and will removed in 3.0. Changing items per page will be done internally by the `ResultPager`.
+     *
      * @param null|int $perPage
      */
     public function setPerPage($perPage)

lib/Github/Api/ApiInterface.php

@@ -6,6 +6,8 @@
  * Api interface.
  *
  * @author Joseph Bielawski <[email protected]>
+ *
+ * @deprecated since 2.18 and will removed in 3.0. Changing items per page will be done internally by the `ResultPager`.
  */
 interface ApiInterface
 {

lib/Github/ResultPager.php

@@ -2,8 +2,8 @@
 
 namespace Github;
 
+use Github\Api\AbstractApi;
 use Github\Api\ApiInterface;
-use Github\Api\Search;
 use Github\HttpClient\Message\ResponseMediator;
 
 /**
@@ -14,6 +14,13 @@
  */
 class ResultPager implements ResultPagerInterface
 {
+    /**
+     * The default number of entries to request per page.
+     *
+     * @var int
+     */
+    private const PER_PAGE = 100;
+
     /**
      * The GitHub Client to use for pagination.
      *
@@ -28,6 +35,9 @@ class ResultPager implements ResultPagerInterface
      */
     protected $pagination;
 
+    /** @var int */
+    private $perPage;
+
     /**
      * The Github client to use for pagination.
      *
@@ -41,9 +51,10 @@ class ResultPager implements ResultPagerInterface
      *
      * @param \Github\Client $client
      */
-    public function __construct(Client $client)
+    public function __construct(Client $client, int $perPage = null)
     {
         $this->client = $client;
+        $this->perPage = $perPage ?? self::PER_PAGE;
     }
 
     /**
@@ -59,7 +70,25 @@ public function getPagination()
      */
     public function fetch(ApiInterface $api, $method, array $parameters = [])
     {
+        $paginatorPerPage = $this->perPage;
+        $closure = \Closure::bind(function (ApiInterface $api) use ($paginatorPerPage) {
+            $clone = clone $api;
+
+            if (null !== $api->getPerPage()) {
+                @trigger_error(sprintf('Setting the perPage value on an api class is deprecated sinc 2.18 and will be removed in 3.0. Pass the desired items per page value in the constructor of "%s"', __CLASS__), E_USER_DEPRECATED);
+
+                return $clone;
+            }
+
+            /* @phpstan-ignore-next-line */
+            $clone->perPage = $paginatorPerPage;
+
+            return $clone;
+        }, null, AbstractApi::class);
+
+        $api = $closure($api);
         $result = $this->callApi($api, $method, $parameters);
+
         $this->postFetch();
 
         return $result;
@@ -70,37 +99,24 @@ public function fetch(ApiInterface $api, $method, array $parameters = [])
      */
     public function fetchAll(ApiInterface $api, $method, array $parameters = [])
     {
-        $isSearch = $api instanceof Search;
-
-        // get the perPage from the api
-        $perPage = $api->getPerPage();
-
-        // set parameters per_page to GitHub max to minimize number of requests
-        $api->setPerPage(100);
+        return iterator_to_array($this->fetchAllLazy($api, $method, $parameters));
+    }
 
-        try {
-            $result = $this->callApi($api, $method, $parameters);
-            $this->postFetch();
+    public function fetchAllLazy(ApiInterface $api, string $method, array $parameters = []): \Generator
+    {
+        $result = $this->fetch($api, $method, $parameters);
 
-            if ($isSearch) {
-                $result = isset($result['items']) ? $result['items'] : $result;
-            }
+        foreach ($result['items'] ?? $result as $item) {
+            yield $item;
+        }
 
-            while ($this->hasNext()) {
-                $next = $this->fetchNext();
+        while ($this->hasNext()) {
+            $result = $this->fetchNext();
 
-                if ($isSearch) {
-                    $result = array_merge($result, $next['items']);
-                } else {
-                    $result = array_merge($result, $next);
-                }
+            foreach ($result['items'] ?? $result as $item) {
+                yield $item;
             }
-        } finally {
-            // restore the perPage
-            $api->setPerPage($perPage);
         }
-
-        return $result;
     }
 
     /**
@@ -187,6 +203,8 @@ protected function get($key)
     }
 
     /**
+     * @deprecated since 2.18 and will be removed in 3.0.
+     *
      * @param ApiInterface $api
      * @param string       $method
      * @param array        $parameters

lib/Github/ResultPagerInterface.php

@@ -9,6 +9,8 @@
  *
  * @author Ramon de la Fuente <[email protected]>
  * @author Mitchel Verschoof <[email protected]>
+ *
+ * @method fetchAllLazy(ApiInterface $api, string $method, array $parameters = []): iterator
  */
 interface ResultPagerInterface
 {

test/Github/Tests/ResultPagerTest.php

@@ -7,29 +7,36 @@
 use Github\ResultPager;
 use Github\Tests\Mock\PaginatedResponse;
 use Http\Client\HttpClient;
+use PHPUnit\Framework\TestCase;
 
 /**
- * ResultPagerTest.
- *
  * @author Ramon de la Fuente <[email protected]>
  * @author Mitchel Verschoof <[email protected]>
  * @author Tobias Nyholm <[email protected]>
  */
-class ResultPagerTest extends \PHPUnit\Framework\TestCase
+class ResultPagerTest extends TestCase
 {
+    public function provideFetchCases()
+    {
+        return [
+            ['fetchAll'],
+            ['fetchAllLazy'],
+        ];
+    }
+
     /**
-     * @test
+     * @test provideFetchCases
      *
-     * description fetchAll
+     * @dataProvider provideFetchCases
      */
-    public function shouldGetAllResults()
+    public function shouldGetAllResults(string $fetchMethod)
     {
         $amountLoops = 3;
         $content = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10];
         $response = new PaginatedResponse($amountLoops, $content);
 
         // httpClient mock
-        $httpClientMock = $this->getMockBuilder(\Http\Client\HttpClient::class)
+        $httpClientMock = $this->getMockBuilder(HttpClient::class)
             ->setMethods(['sendRequest'])
             ->getMock();
         $httpClientMock
@@ -47,7 +54,13 @@ public function shouldGetAllResults()
 
         // Run fetchAll on result paginator
         $paginator = new ResultPager($client);
-        $result = $paginator->fetchAll($memberApi, $method, $parameters);
+        $result = $paginator->$fetchMethod($memberApi, $method, $parameters);
+
+        if (is_array($result)) {
+            $this->assertSame('fetchAll', $fetchMethod);
+        } else {
+            $result = iterator_to_array($result);
+        }
 
         $this->assertCount($amountLoops * count($content), $result);
     }
@@ -76,7 +89,7 @@ public function shouldGetAllSearchResults()
         $response = new PaginatedResponse($amountLoops, $content);
 
         // httpClient mock
-        $httpClientMock = $this->getMockBuilder(\Http\Client\HttpClient::class)
+        $httpClientMock = $this->getMockBuilder(HttpClient::class)
             ->setMethods(['sendRequest'])
             ->getMock();
         $httpClientMock
@@ -97,19 +110,21 @@ public function shouldGetAllSearchResults()
     public function testFetch()
     {
         $result = 'foo';
-        $method = 'bar';
+        $method = 'all';
         $parameters = ['baz'];
-        $api = $this->getMockBuilder(\Github\Api\ApiInterface::class)
+        $api = $this->getMockBuilder(Members::class)
+            ->disableOriginalConstructor()
+            ->setMethods(['all'])
             ->getMock();
+        $api->expects($this->once())
+            ->method('all')
+            ->with(...$parameters)
+            ->willReturn($result);
 
-        $paginator = $this->getMockBuilder(\Github\ResultPager::class)
+        $paginator = $this->getMockBuilder(ResultPager::class)
             ->disableOriginalConstructor()
-            ->setMethods(['callApi', 'postFetch'])
+            ->setMethods(['postFetch'])
             ->getMock();
-        $paginator->expects($this->once())
-            ->method('callApi')
-            ->with($api, $method, $parameters)
-            ->willReturn($result);
 
         $paginator->expects($this->once())
             ->method('postFetch');