[10.x] Refactor the use of getScoutKeyName #509
Conversation
There was a problem hiding this comment.
When changing Searchable::getScoutKeyName() the method Searchable::queryScoutModelsByIds() should be updated so that the key is uses a qualified column name? What do you think?
public function queryScoutModelsByIds(Builder $builder, array $ids)
{
$query = static::usesSoftDelete()
? $this->withTrashed() : $this->newQuery();
if ($builder->queryCallback) {
call_user_func($builder->queryCallback, $query);
}
return $query->whereIn(
- $this->getScoutKeyName(), $ids
+ $this->qualifyColumn($this->getScoutKeyName()), $ids
);
}
|
@mmachatschek did some of those changes.
Should we also do this for the public static function makeAllSearchable($chunk = null)
{
$self = new static;
$softDelete = static::usesSoftDelete() && config("scout.soft_delete", false);
$self->newQuery()
->when(true, function ($query) use ($self) {
$self->makeAllSearchableUsing($query);
})
->when($softDelete, function ($query) {
$query->withTrashed();
})
- ->orderBy($self->getScoutKeyName())
+ ->orderBy($this->qualifyColumn($self->getScoutKeyName()))
->searchable($chunk);
} |
There was a problem hiding this comment.
@driesvints Yes.
Both Searchable::queryScoutModelsByIds() and Searchable::makeAllSearchable() should have the qualifiedColumn call IMO.
Additionally after the changes with the qualifiedColumn call the Algolia driver should have the same behaviour as before the changes.
@mmachatschek done, can you check again?
How do we practically do this? |
|
Okay, this is ready for review now. I tried out the changes on both the Laravel.io and blade-ui-kit.com codebases without problems. @taylorotwell feel free to give this a review. |
|
@driesvints MeiliSearch v0.21.0 was released yesterday. It brings breaking changes on the library level. |
|
Fantastic, this is a great improvement for me. I had this exact issue this week 👍 |
This PR changes the use of getScoutKeyName. The most significant change is the removal of the use of the qualified column name in the
getScoutKeyNamemethod. Other changes are using thegetScoutKeyNameinstead ofgetKeyNameso people can modify what column is used to index search records. This was already possible with Algolia but not with Meilisearch.The reason why the qualified column was removed was that if someone were to override
getScoutKeyName(with the other changes in mind from this PR) thetoSearchableArraywouldn"t be compatible anymore as it won"t append the correct column name and thus search records in Meilisearch won"t index properly anymore. If we want to makegetScoutKeyNameadjustable for Meilisearch as well (as currently stated in the docs) then this change is necessary. As pointed out by @mmachatschek in #484 (comment), Algolia doesn"t suffer from this as it"s using its own internal objectID mapping.The upgrade path for this unfortunately is that people will have to re-index their search records. If that"s not wanted then we should re-consider removing the ability to override the key name.
I"m also not entirely sure what the original reason for the use of the qualified column name was. I do not know if there will be a breaking change because if this.
I"d appreciate a review from you @mmachatschek before I mark this as ready to review for Taylor.