matchish / laravel-scout-elasticsearch

Search among multiple models with ElasticSearch and Laravel Scout
MIT License
702 stars 113 forks source link

[BUG] Possibly unexpected behaviour, when using additional callback in `search` #250

Open vgaldikas opened 1 year ago

vgaldikas commented 1 year ago

Describe the bug

Search breaks when you use callback.

Example: when you I use like this:

$result = Product::search(
            'iphone',
        )->get();

Everything is ok. But when you use search with additional callback, like so:

$result = Product::search(
            null,
            function (
                Client $client,
            ) use ($search) {
                return $client->search(['index' => $this->index->value, 'body' => $search->toArray()]);
            }
        )->get();

I get the following error:

Matchish\ScoutElasticSearch\ElasticSearch\EloquentHitsIteratorAggregate::__construct(): Argument #1 ($results) must be of type array, Elastic\Elasticsearch\Response\Elasticsearch given

To Reproduce I think bug description is clear enough about how to reproduce

Expected behavior In both cases it should return a collection with appropriate models

Additional context Is it possible that I am trying to do this the wrong way?

Version Versions of Laravel, Scout, and the package.

"matchish/laravel-scout-elasticsearch": "^6.0.2", Laravel Framework 10.2.0

vgaldikas commented 1 year ago

Ok I think I have found the issue. It seems to stem from here:

https://github.com/matchish/laravel-scout-elasticsearch/blob/master/src/Engines/ElasticSearchEngine.php#L196

When call back is passed, asArray is not called if no callback then asArray is called.

@matchish is there a reason why it is done this way, or is this a bug? If the latter, im happy to make a pull request

matchish commented 1 year ago

Hey) feel free to open the pr. But it breaking changes so probably I'll wait until will be merged to master. For now could be solved by calling toArray in callback I think

vgaldikas commented 1 year ago

@matchish yes, calling asArray in the callback does indeed seem to work as well. But I reckon that it's better that the underlying function returns same thing, regardless of if callback argument was used or not :)