Move ResultSet creation to the Client

[merk]

FOSElasticaBundle needs to override the Result object to provide additional methods. Without this change, it means we need to override many more methods in the Index and Type classes, and I'd prefer to support consumption of Elastica without making users use our Index/Type objects.

I wish to add a method, getTransformed() to the result which will lazily transform an entire ResultSet when accessed, allowing us to remain much closer to the Elastica API rather than providing our own.

[ruflin]

@merk I understand the reasoning behind the change but the part I don't like that more complexity in the code is added because of this. In case we use ResultSet at an other place in the code later where not client is available, this will break. Perhaps we can work on a solution where a ResultSet factory method can be used to pick the right "object" type.

Can you paste the link to the code in FOSElasticaBundle here so I can have a look at it?

[merk]

You're right - it does feel fragile.

A prototype that I wrote a few months ago:

https://github.com/FriendsOfSymfony/FOSElasticaBundle/blob/new-api/Elastica/TransformingSearch.php
https://github.com/FriendsOfSymfony/FOSElasticaBundle/blob/new-api/Elastica/TransformingResultSet.php
https://github.com/FriendsOfSymfony/FOSElasticaBundle/blob/new-api/Elastica/TransformingResult.php

A ResultSet factory of some description could work. I'll see what I can mock up.

[merk]

So I'm not sure of the best approach for your library, the way I'd achieve it is to inject a ResultSet factory to anything that needs a ResultSet, but that requires an additional injection into the Type and Search classes which doesnt suit the usage patterns.

The only other option I can come up with is to use a static create method on the ResultSet that can be configured which class name to use for creation. Are you okay this this approach?

    public static function create(Response $response, Query $query)
    {
         $class = static::$class;

         return new $class($response, $query);
    }

Which would then allow a ResultSet::setClass method to modify static::$class.

[merk]

I've updated the PR with a concept for using a static factory method.

[ruflin]

Please use $_class for protected variables.

[ruflin]

Ok. I like this solution much more as the previous one. It is still not the nicest one but perhaps we find a better solution at a later stage.

See my 2 comments above. Afterwards I think it is ready to be merged.

[merk]

Updated.

[fprochazka]

Oh, wow. How is creating a global static state better solution?

[ruflin]

@fprochazka I see your point, but I still think this is better then add lots of dependencies (which are not needed) to ResultSet. I'm still convinced there must be a better solution. Any good ideas?

[merk]

My original solution moved the responsibility to the Client which was worse. The alternative to this is to make a breaking change to the Type and Search constructors.

I'd like to avoid having to override Client, Index, Type, Search and ResultSet in FOSElasticaBundle and I dont see many other options without a BC break.

[coveralls]

Coverage decreased (-0.03%) when pulling 1c82738 on merk:customisable-result into ffbfa5d on ruflin:master.

[fprochazka]

On each of those places, there is already a Client used and the class is therefore dependent on it. Every time you create ResultSet, you do that by passing data from Client::request().

I don't see how adding a Client::createResultSet() is worse than current state.

In case we use ResultSet at an other place in the code later where not client is available, this will break.

Why not just focus on current problems, without building sky castles? All those "it might one day, in the future far far away" statements only bring necessary complexity to the code.

If you're not able to provide an example (apart from unit tests), where it might be usefull to create ResultSet out of thin air without doing a request to elastic before (which means dependency on the Client, so you can do that request), then the Client::createResultSet() is a good enough solution.

[ruflin]

If you look at the design of ResultSet, Document, Result, Response etc. you will see that these were on purpose designed from the beginning to be independent of Client. One of the main reason here is testability without having to connect to elasticsearch.

[ruflin]

@merk I merged in the pull request but I would suggest to open an Issue so we can look for a better solution here in the future as the global state part is really not too nice.