tan-tan-kanarek / github-php-client

MIT License
184 stars 120 forks source link

Pagination example fails when there are fewer objects than page size #107

Closed ryandesign closed 8 years ago

ryandesign commented 8 years ago

The example pagination code from your readme fails when there are fewer objects than the page size. That is: your example gets commit messages 4 at a time. If pointed at a repo with fewer than 4 commits, such as https://github.com/ryandesign/test-fairly-empty which has 2 commits, it fails:

$ php test-pagination.php
================ Page 1 - 2 items ================
    a6a3406f266c8ad5e8d6ea910c22bb54a70bd5db - Update README.md
    d0b8d67f5ff769d9f39e975193e51beededde406 - Initial commit
PHP Fatal error:  Uncaught GitHubClientException: Page not defined in /path/to/github-php-client/client/GitHubClientBase.php:166
Stack trace:
#0 /path/to/test-pagination.php(31): GitHubClientBase->getNextPage()
#1 {main}
  thrown in /path/to/github-php-client/client/GitHubClientBase.php on line 166

Fatal error: Uncaught GitHubClientException: Page not defined in /path/to/github-php-client/client/GitHubClientBase.php:166
Stack trace:
#0 /path/to/test-pagination.php(31): GitHubClientBase->getNextPage()
#1 {main}
  thrown in /path/to/github-php-client/client/GitHubClientBase.php on line 166

As long as page size is set less than or equal to the number of items, the example works, but of course that defeats the purpose since I don't know in advance how many items there are and I want to set the page size to its maximum value of 100 to minimize the number of requests I send.

ryandesign commented 8 years ago

This workaround works for me for now:

--- README.md.orig  2016-08-13 23:51:58.000000000 -0500
+++ README.md   2016-08-15 15:24:53.000000000 -0500
@@ -134,7 +134,8 @@
    $client->setPageSize($pageSize);

    $commits = $client->repos->commits->listCommitsOnRepository($owner, $repo);
-   while(true)
+   $client->setPage();
+   while(count($commits))
    {
        $page = $client->getPage();
        echo "================ Page $page - " . count($commits) . " items ================\n";
tan-tan-kanarek commented 8 years ago

Thank you, could you please open a pull-request with this fix?

ryandesign commented 8 years ago

Is this really the fix you recommend? I was just using it as a workaround until you could work out the correct solution. I don't know your code well enough to understand how you intended it to work, but it doesn't seem right that I should have to call $client->setPage();, then issue the request, which clears page, then have to call $client->setPage(); a second time. Why does running the request clear page?

tan-tan-kanarek commented 8 years ago

You're right, see my fix: https://github.com/tan-tan-kanarek/github-php-client/pull/108

tan-tan-kanarek commented 8 years ago

Wait, it's not good, will get back to you.

tan-tan-kanarek commented 8 years ago

Now it's good: https://github.com/tan-tan-kanarek/github-php-client/pull/109