algolia / algolia/scout-extended
Honor makeAllSearchableUsing() in Aggregator?
- Dominant language
- PHP
- Stars
- 420
- Forks
- 94
- PR merge metrics
- No merged PRs in 30d
Description
- Laravel version: 10.48.10
- Algolia Scout Extended version: 3.1.0
- Algolia Client Version: 3.4.1
- Language Version:
### Description
First time experimenting with the Aggregator feature, and it's quite nice. However, I'm also a fan of the `makeAllSearchableUsing()` method in Scout which can implement a more specific query. (I'm aware that `$relations` can be used for eager loading and `shouldBeSearchable()` can be used to filter out results, but in my use case, the filtering logic relies on the result of a separate query, and this would create an n+1 query performance issue that would be resolved by putting the conditional logic in the main query to pull all searchable models.) The `makeAllSearchable()` method on the Aggregator class does not appear to honor the `makeAllSearchableUsing()` in the individual models the way the `makeAllSearchable()` method on the main `Laravel\Scout\Searchable` trait does.
I'm posing this as a question for discussion, because I realize that honoring any such `makeAllSearchableUsing()` methods in the individual models could be an undesirable behavior and/or breaking change for some folks (if they are using different logic in the individual index and the aggregator). Ideally there would be a way to optionally honor these methods, or perhaps to define them in the Aggregator class itself. I'm currently overriding the `makeAllSearchable()` method in the `Aggregator` class and chaining in that additional step along the lines of what the `Laravel\Scout\Searchable` trait does (note that this requires making the `makeAllSearchableUsing()` method public in the source model, otherwise it's not accessible to the Aggregator):
```php
getModels() as $model) {
$instance = new $model;
$softDeletes =
in_array(SoftDeletes::class, class_uses_recursive($model)) && config('scout.soft_delete', false);
// Added these two lines to determine if there's a public 'makeAllSearchableUsing' method in the model instance)
$makeAllSearchableUsingReflection = new \ReflectionMethod($instance, 'makeAllSearchableUsing');
$applyMakeAllSearchableUsing = $makeAllSearchableUsingReflection->isPublic();
$instance->newQuery()->when($softDeletes, function ($query) {
$query->withTrashed();
})->when($applyMakeAllSearchableUsing, function ($query) use ($instance) { // Added these two lines
$instance->makeAllSearchableUsing($query);
})->orderBy($instance->getKeyName())->chunk(config('scout.chunk.searchable', 500), function ($models) {
$models = $models->map(function ($model) {
return static::create($model);
})->filter->shouldBeSearchable();
$models->searchable();
event(new ModelsImported($models));
});
}
}
}
```
I don't love doing it this way, since it's possible the underlying `makeAllSearchable` logic will change in future updates and this code will need to be kept in sync. Perhaps there's a more elegant way to do this? Could we add an optionally extendable `makeAllSearchableUsing` method on the Aggregator class itself?
### Steps To Reproduce
See above
Contributor guide
Research direction
Start with Aggregator::makeAllSearchable() and compare it with Laravel Scout's Searchable::makeAllSearchable() behavior, using the issue's override as the reference implementation. Determine whether the completed behavior should optionally honor model makeAllSearchableUsing() methods or expose an Aggregator-level extension point, then verify the chosen behavior against the documented compatibility concerns.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- laravel, php
- Domain
- search
- Issue type
- Feature
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 35/100