-
Notifications
You must be signed in to change notification settings - Fork 43
fix MeiliSearch\Search\SearchResult as array #86
Conversation
fix "Cannot use object of type MeiliSearch\Search\SearchResult as array"
when i do a custom search with or without options, i got this error
( doing a simple MyModel::search() does work fine ) |
@olivM I tried your PR, but I don't think it works correctly:
|
@@ -172,11 +172,11 @@ public function mapIds($results) | |||
*/ | |||
public function map(Builder $builder, $results, $model) | |||
{ | |||
if (is_null($results) || 0 === count($results['hits'])) { | |||
if (is_null($results) || 0 === $results->getHitsCount()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to check if the $results
parameter is a SearchResult
object before and then casting the object to an array.
e.g.
if ($results instanceof SearchResult) {
$results = $results->toArray();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct me if I'm wrong, but if in the custom search we use rawSearch()
instead of search()
we can keep count($results['hits'])
instead of ->getHitsCount()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shokme correct. but I think as you mentioned in #87 (comment) that the PHP SDK (I didn't check on this) changed, it may be that other integrations are broken or will break if updated to the newer versions.
With my change we could also fix older integrations that copied the example from the docs with ->search()
in their custom query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shokme the only change needed to work with the new behaviour would be my suggestion from #86 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember everything about the PHP SDK changes, but it safe as long as it allow us to work with arrays and as I remember it will.
if we do this change, we should do it for every case (I didn't check if there is more than just that one) that can throw the object as array error.
We should find a away to have the ->toArray()
conversion earlier than in each methods ?
Anyway, it is hard to me to find time at the moment. So I can't work on making everything perfect right now.
If you have free time and want to do it feel free to open another PR that will apply your change on every case that could throw an object as array error.(if this is the only one let me now and I will merge it of course)
Thank you for your time !
As mentioned here #86 (comment) I let decide @curquiza if we close this PR in favour of a new one that allow this package to work with the SearchResult object. Thank you for your time everyone. |
88: Fix SearchResult return value in custom search callable r=curquiza a=mmachatschek Attempt to fix issues from #86 and the update of the release https://github.com/meilisearch/meilisearch-php/releases/tag/v0.16.0 The `->search()` method on the `Indexes::class` is able to return either an array or an `SearchResult::class`. Fixes #87 Co-authored-by: Markus Machatschek <[email protected]>
Hello everyone! 🙂 (feel free to re-open it @shokme if I missed something) |
fix "Cannot use object of type MeiliSearch\Search\SearchResult as array"