FriendsOfSymfony / FOSElasticaBundle

Elasticsearch PHP integration for your Symfony project using Elastica.
http://friendsofsymfony.github.io
MIT License
1.25k stars 793 forks source link

[RFC] New pagination #689

Closed merk closed 4 years ago

merk commented 10 years ago

We've got a few outstanding PRs to add aggregations or other features available to Elastica into our Pagination interfaces, but to me this exposes that the interfaces of this bundle are not flexible and need to be improved.

I propose the following way I believe that pagination should work:


$search = new \Elastica\Search($this->container->get('fos_elastica.client'));
$search->addIndex($this->container->get('fos_elastica.index.index_name'));
$search->addType($this->container->get('fos_elastica.index.type_name1'));
$search->addType($this->container->get('fos_elastica.index.type_name2'));

$query = /** Build an instance of an \Elastica_Query object that you want to search against the Searchable */

// Using Pagerfanta:
$adapter = new ElasticaAdapter($search, $query);
$pagerfanta = new Pagerfanta($adapter);

// Using Knp Paginator
$paginator  = $this->get('knp_paginator');
$pagination = $paginator->paginate(
    new ElasticaAdapter($search, $query),
    $this->get('request')->query->get('page', 1)/*page number*/,
    10/*limit per page*/
);

It may be possible to adjust the api slightly to avoid the need to create a Search object and accept a SearchableInterface object that we would convert internally into a Search object.

I also face problems with this implementation - the return values that we can return. For 3.2 or 4.0 I plan on removing our Hybrid results completely, and instead only return the Elastica ResultSet containing only Elastica Result objects. I hope to have a method, getTransformed() on the Result to return the transformed object. This cant be done for 3.1.

The only option I can think of that lets me keep some level of forward compatibility without forcing you all to make changes is:

// In php
foreach ($pagerfanta as $result) {
  $transformed = $resultTransformer->transform($pagerfanta, $result);

 // Do stuff with $transformed
}

// In twig
{% for result in pagerfanta %}
  {% set transformed = result | fos_elastica_transform(pagerfanta) %}

  {# Do stuff with transformed #}
{% endfor %}

I would appreciate feedback from those that actually use pagination, since in my applications I dont paginate search results.

daFish commented 10 years ago

I'm fine with any change done making pagination usable even without the support for hybrid objects. In my application I want to use pagination but didn't implemented it yet because of the lacking support of hybrid results in pagination.

trsteel88 commented 10 years ago

+1 for pagination this way @merk

createproblem commented 10 years ago

:+1:

dmarko484 commented 10 years ago

+1 for aggregation support

yellow1912 commented 10 years ago

Interesting, I don't see clearly here how aggregation is supported tho?

merk commented 10 years ago

Of note here: KnpPaginator already supports Elastica Queries natively with both pagination and sortable features.

I have just submitted a PR to Pagerfanta that adds native support as well. https://github.com/whiteoctober/Pagerfanta/pull/146

I will try to get the transformation functionality listed in this PR merged into 3.1-dev tonight.

merk commented 10 years ago

I'm going to need to make more changes to the Transformers to handle the {{ object | transform() }} helper without suffering a large number of loops. That wont happen tonight.

nurikabe commented 9 years ago

+1

hugohenrique commented 9 years ago

@merk, I tested its proposal with a structure: https://www.elastic.co/guide/en/elasticsearch/reference/current/search-aggregations-metrics-top-hits-aggregation.html#_example

And it did not work, you suggest something?