ResultPager fetchall on apps/installations incorrect result #597

Activity

1. bdelbasso commented on Jun 8, 2017

   

   bdelbasso

   on Jun 8, 2017

   Contributor

   More actions

   FYI I'm using a CustomResultPager as a workaround for this problem.

   <?php
   
   namespace AppBundle\Github;
   
   use Github\Api\ApiInterface;
   use Github\Client;
   use Github\ResultPager;
   
   /**
    * Workaround for:
    *
    * https://platform.github.community/t/pagination-differs-in-installations-api/1862
    */
   class CustomResultPager extends ResultPager
   {
   
       private $key;
   
       public function __construct(Client $client, string $key)
       {
           parent::__construct($client);
           $this->key = $key;
       }
   
       public function fetchAll(ApiInterface $api, $method, array $parameters = array())
       {
           // get the perPage from the api
           $perPage = $api->getPerPage();
   
           // set parameters per_page to GitHub max to minimize number of requests
           $api->setPerPage(100);
   
           $result = $this->decode($this->callApi($api, $method, $parameters));
           $this->postFetch();
   
           while ($this->hasNext()) {
               $next = $this->fetchNext();
   
               $result = array_merge($result, $this->decode($next));
           }
   
           // restore the perPage
           $api->setPerPage($perPage);
   
           return $result;
       }
   
       protected function decode(array $result) {
           if(! isset($result[$this->key])) {
               throw new \Exception("No key $this->key, got: ".array_keys($result));
           }
           return $result[$this->key];
       }
   
   }
   

   I see two solutions to address this issue here:

   1. Include this class in php-github-api and document when to use it.

   2. Modify the existing ResultPager to merge differently when the key totalCount is present. This would be transparent to the user but more hack-ish.

   @acrobat @Nyholm What do you think?

   👍React with 👍1Reacted by Daniel Lo Nigro
2. Daniel15 commented on Sep 3, 2017

   

   Daniel15

   on Sep 3, 2017

   More actions

   My workaround is very similar to @bdelbasso's. I instead implemented it as a separate method:

   class ResultPagerWithCustomField extends ResultPager {
     public function fetchAllUsingField(
       ApiInterface $api,
       string $method,
       string $field,
       array $parameters = array()
     ) {
       // ...
     }
   }

   I think it'd be good to include this functionality in the existing ResultPager
3. einenlum

   added

   bug

   on Sep 16, 2017