Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] PHP 8.1 EloquentHitsIteratorAggregate TypeError #215

Closed
sfdjo435 opened this issue Aug 19, 2022 · 12 comments
Closed

[BUG] PHP 8.1 EloquentHitsIteratorAggregate TypeError #215

sfdjo435 opened this issue Aug 19, 2022 · 12 comments
Labels
bug Something isn't working

Comments

@sfdjo435
Copy link

When using a custom search method: function (Client $client, Search $search) you'll get a TypeError:

Matchish\\ScoutElasticSearch\\ElasticSearch\\EloquentHitsIteratorAggregate::__construct(): Argument #1 ($results) must be of type array, Elastic\\Elasticsearch\\Response\\Elasticsearch given
@sfdjo435 sfdjo435 added the bug Something isn't working label Aug 19, 2022
@matchish
Copy link
Owner

php version? packages versions? scout, elasticsearch client

@sfdjo435
Copy link
Author

PHP 8.1, ElasticSearch 8.3.3, laravel-scout-es V6.0.2, laravel/framework v9.25.1, laravel/scout v9.4.10

@amjadbanimattar
Copy link

@matchish We are facing the same problem.
PHP 8.1, ElasticSearch 7.17.5, laravel-scout-es V6.0.2, laravel/framework v9.26.1, laravel/scout v9.4.10

@hkulekci
Copy link
Contributor

@amjadbanimattar @sfdjo435 Could you provide a sample custom search to test locally?

@hkulekci
Copy link
Contributor

I think it is related to changes about the Elasticsearch search response. Older version was returning array but the new version is returning Response object. So, for the custom search EloquentHitsIteratorAggregate expecting array. I can create a PR for solution if you can give some sample to me.

@hkulekci
Copy link
Contributor

This is a sample with an error :

$results = Product::search('*',
    static function (Client $client, Search $body) {
        $highlight = new Highlight();
        $highlight->addField('name');
        $body->addHighlight($highlight);
        return $client->search(['index' => 'products', 'body' => $body->toArray()]);
    })->raw();
$iterator = new EloquentHitsIteratorAggregate($results);
dd($iterator);

Here the error :

[2022-08-30 02:43:58] local.ERROR: Matchish\ScoutElasticSearch\ElasticSearch\EloquentHitsIteratorAggregate::__construct(): Argument #1 ($results) must be of type array, Elastic\Elasticsearch\Response\Elasticsearch given, called in /laravel-scout-elasticsearch-example/app/Http/Controllers/ProductsController.php on line 52 {"exception":"[object] (TypeError(code: 0): Matchish\\ScoutElasticSearch\\ElasticSearch\\EloquentHitsIteratorAggregate::__construct(): Argument #1 ($results) must be of type array, Elastic\\Elasticsearch\\Response\\Elasticsearch given, called in /laravel-scout-elasticsearch-example/app/Http/Controllers/ProductsController.php on line 52 at /laravel-scout-elasticsearch-example/vendor/matchish/laravel-scout-elasticsearch/src/ElasticSearch/EloquentHitsIteratorAggregate.php:28)

I think this is what we are looking for. So the quick solution for this error is using asArray() method for raw search :

$results = Product::search('*',
    static function (Client $client, Search $body) {
        $highlight = new Highlight();
        $highlight->addField('name');
        $body->addHighlight($highlight);
        return $client->search(['index' => 'products', 'body' => $body->toArray()])->asArray();
    })->raw();
$iterator = new EloquentHitsIteratorAggregate($results);
dd($iterator);

I think the solution is that we need to force the users to return a specific type for the callback to be sure the returned data for ElasticSearchEngine::performSearch() method of the Builder. It should be returned specific type. The problem performSearch using Builder::callback internally. And there is no forcing for the return type in the Scout Builder inside.

@matchish
Copy link
Owner

matchish commented Sep 1, 2022

There is another question. Why tests passes. Should fail as I understand

@hkulekci
Copy link
Contributor

hkulekci commented Sep 1, 2022

Here, in our library, we expect an array response from raw() method response. But there is no strict response type in Laravel\Scout\Builder::raw(). It is mentioned as mixed. So, raw queries start returning the response of callback directly. After version upgrade of ES Client, the search method starts returning Response object instead of an array directly. I tried to force the user to return the array for the callback of the raw search, but I could not find a short solution. So, the solution can be we can accept Response object or array at EloquentHitsIteratorAggregate constructor and resolve the object type to act according to it.

@hkulekci
Copy link
Contributor

hkulekci commented Sep 1, 2022

On the other hand, we had flaged EloquentHitsIteratorAggregate as @internal. To say this is internal, we need to create a CustomBuilder with extendingLaravel\Scout\Builder. Maybe this can be another solution, too.

@matchish
Copy link
Owner

matchish commented Sep 1, 2022

So in ci we don't use last elasticsearch lib?

@matchish
Copy link
Owner

matchish commented Sep 1, 2022

I think it will be enough to update readme with your quick solution.

@hkulekci
Copy link
Contributor

hkulekci commented Sep 1, 2022

I think so. the callback return needs to be an array. We need to mention that this is a BC Break of latest version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants