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

Remove search callback call #15

Merged
merged 1 commit into from
Jul 14, 2023

Conversation

cbaconnier
Copy link
Contributor

@cbaconnier cbaconnier commented Jul 12, 2023

Fix the issue #14

I'm still not sure why we need this. But this is currently broken when we use a callback as the method does not uses the same arguments and would need to add, at least, the search method in the store since it's being called in the callback (and potentially others methods given to the users by Algolia|Meilisearch clients).

Exemple of callback

 // Currently, when running tests, $meilisearch is an instance of ArrayStore
App\Models\User::search('John Doe', function($meilisearch, string $query, array $options) {
    $options['sort'] = ['name:asc'];
    $options['filter'] = 'email IS NOT NULL';

    /** @var Meilisearch\Endpoints\Indexes $meilisearch */
    return $meilisearch->search($query, $options);
})->get();

See cbaconnier/laravel-scout-array-bug@cdb3620

I also mentioned in the issue, as alternative, we could store the callback(s) to allow the users to tests them separately.

Search::assertSearchedWithCallback(function(Indexes $meiliSearch, string $query, array $options){
    return true;
}); 

But I haven't got the time to investigate how to adapt the Store for this.
What do you think?

Edit:
I don't think we can't add assertSearchedWithCallback test since we cannot compare closure.
Or at least I have no knowledge of it.

@cbaconnier cbaconnier closed this Jul 14, 2023
@Sti3bas Sti3bas reopened this Jul 14, 2023
@Sti3bas
Copy link
Owner

Sti3bas commented Jul 14, 2023

@cbaconnier I will merge this in as a temporary solution until I will release the changes mentioned here.

@Sti3bas Sti3bas merged commit eeab28d into Sti3bas:master Jul 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants