fadion / Bouncy

Map Elasticsearch results to Eloquent models
MIT License
71 stars 26 forks source link

Laravel 5 Pagination Support #3

Open drakkex opened 9 years ago

drakkex commented 9 years ago

Hi,

Can you add support for Laravel 5 Pagination? The current implementation doesn't work and it throws an error "Class 'Illuminate\Support\Facades\Paginator' not found - since it doesn't exist", And the errors keeps on coming for every fix made.

Hopefully you will provide a fix soon.

Thanks.

fadion commented 9 years ago

Thanks for reporting. I don't have a running ElasticSearch setup, so you'd help if you can test. L5 has changed a bit how the Paginator works and removed the facade.

drakkex commented 9 years ago

I performed a search with $pages = Page::search( $params )->paginate( 10 ) option and this time it didn't show any error, I am using $pages-render() method to show pagination links instead of $pages->links() since this method no longer exists.

Anyways this just shows results for the first page and pagination never gets displayed.

Dumping the $pages variable, I get

Paginator {#408 ▼
  #hasMore: false
  #items: Collection {#411 ▶}
  #perPage: 10
  #currentPage: 1
  #path: "/"
  #query: []
  #fragment: null
  #pageName: "page"
}

And without the paginate() option i..e $pages = Page::search( $params ) , I get the results

ElasticCollection {#206 ▼
  #response: array:4 [▼
    "took" => 1
    "timed_out" => false
    "_shards" => array:3 [▼
      "total" => 5
      "successful" => 5
      "failed" => 0
    ]
    "hits" => array:3 [▼
      "total" => 149
      "max_score" => 2.7690523
      "hits" => array:149 [ …149]
    ]
  ]
  #instance: Page {#208 ▶}
  #items: array:149 [▶]
}

So even though it doesn't show any errors, The pagination links are not displayed.

drakkex commented 9 years ago

And i confirm the pagination works when i directly change the page parameter in url &page=1, &page=2, ... &page=10 in the url (in this case results per page is 15, and total records are 149).

And except the first page &page=1, For every other page i can see the pagination links, However the links url is set as base/domain url and not the current page/route url.

fadion commented 9 years ago

The new paginator is either buggy or I don't understand it fully. Having an incorrect url is understandable, but not having the links display on the first page makes no sense. I'll investigate.

ravanscafi commented 9 years ago

I managed to fix both wrong URLs and not displaying at the first page (at least for my use case) by changing paginate() to:

    /**
     * Paginates the Elasticsearch results.
     *
     * @param int $perPage
     * @return mixed
     */
    public function paginate($perPage = 15)
    {
        $page   = Paginator::resolveCurrentPage() ?: 1;
        $path   = '/' . \Request::path();
        $sliced = array_slice($this->items, ($page - 1) * $perPage, $perPage);

        return new Paginator($sliced, $perPage, $page, compact('path'));
    }
ravanscafi commented 9 years ago

Oh, and btw this is Laravel 5' simple pagination, so $results->total() will not work.

The L5 equivalent to paginate() is:

use Illuminate\Pagination\LengthAwarePaginator;
...
    /**
     * Paginates the Elasticsearch results.
     *
     * @param int $perPage
     * @return mixed
     */
    public function paginate($perPage = 15)
    {
        $page   = Paginator::resolveCurrentPage() ?: 1;
        $path   = '/' . \Request::path();
        $sliced = array_slice($this->items, ($page - 1) * $perPage, $perPage);
        $total  = count($this->items);

        return new LengthAwarePaginator($sliced, $total, $perPage, $page, compact('path'));
    }
fadion commented 9 years ago

@rscafi can you create a pull request on the l5 branch using your modification? Preferably using the new LengthAwarePaginator.

ravanscafi commented 9 years ago

@fadion sure I can. I'm just testing it a little longer. I have a few ideas for this package, are you still using it in production? Is it open for "breaking changes" or should I just fork it?

fadion commented 9 years ago

It would be great! I can create a "dev" branch and give you commit access. What do you think?

vikram0460 commented 7 years ago

@fadion I have modified the code according to AlexanderWright's code it worked fine. Kindly accept his merge request.