Extract and Implement ResultSetBuilder #1065
Conversation
| @@ -462,7 +463,7 @@ public function search($query = '', $options = null) | |||
| $params | |||
| ); | |||
|
|
|||
| return ResultSet::create($response, $query); | |||
| return $this->_client->getResultSetBuilder()->buildResultSet($response, $query); | |||
There was a problem hiding this comment.
I would prefer to inject the ResultSetBuilder directly into the Search object, but I think given the way Elastica is structured that developers will be creating these objects directly and we dont want to introduce such a major change..
merk
force-pushed
the
resultset-builder
branch
6 times, most recently
from
d497713
to
b9a158e
Compare
| */ | ||
| protected $_connectionPool = null; |
There was a problem hiding this comment.
This breaks BC in case someone extended the Client. Not something that prevents us from moving forward, but worth mentioning in the CHANGELOG.md.
There was a problem hiding this comment.
The connection pool remains protected, my habit of alphabetising properties strikes here, its just now on line 54.
|
As mentioned before I think this definitively goes in the right direction. I did quite a brief review but I saw that the changes to the public APIs and tests are minimal which is great to make it easier for people to migrate (which in most cases is not needed). Could you update the CHANGELOG.md with the BC breaks? |
|
Regarding the ResultSet $_results variable, to make it simpler we could leave it protected and deprecate it for a major bump instead? That would mean this change has almost no effect on any current users.. |
|
@merk That would be great, if we could deprecate it first. Like this we could do the release as following:
This gives people the opportunity to update the lib to the most recent version first without breaking their app but they already see what they have to change. |
merk
force-pushed
the
resultset-builder
branch
3 times, most recently
from
5a8930a
to
4e2fa40
Compare
|
I have moved ResultSet properties back to protected with deprecations and updated the CHANGELOG file. |
|
@merk Sorry, took me some time here to get back to this one. Could you rebase on top of master so I can merge this one in? |
|
@merk BTW: ping when you rebase, as Github does not send notifications for this :-( |
|
Rebased. |
|
Argh, seems you have to rebase again as it conflicts with the logging PR :-( |
|
Squashed and rebased |
|
Merged. Thanks. |
As per #1017, this PR contains the extraction of ResultSetBuilder.
Please consider this for merging.
This PR technically creates a BC break for anyone using ResultSet::$_class and ResultSet::create().