-
Notifications
You must be signed in to change notification settings - Fork 343
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
[6.0] Use engine to flush records of model #310
[6.0] Use engine to flush records of model #310
Conversation
Instead of iterating over ids from the database we use the engine itself to flush the records of an index/model. This solves the problem where models would get out of sync with the search records and old records weren't flushed. With this implementation all records are always removed. If there are engines which don't provide this functionality they can still iterate over the model ids if they want to. See #101
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything seems good to me. I have submitted some feedback. 👍
What do you think @julienbourdeau?
src/Console/FlushCommand.php
Outdated
|
||
$this->line('<comment>Flushed ['.$class.'] models up to ID:</comment> '.$key); | ||
}); | ||
|
||
$model::removeAllFromSearch(); | ||
|
||
$events->forget(ModelsFlushed::class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You also don't need this line.
{ | ||
$index = $this->algolia->initIndex($model->searchableAs()); | ||
|
||
$index->clearIndex(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I confirm. This is good. 👍
CHANGELOG.md
Outdated
@@ -0,0 +1,6 @@ | |||
# Release Notes | |||
|
|||
## [v6.0.0 (unreleased)](https://github.com/laravel/framework/compare/v5.0.3...v6.0.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be: ## [v6.0.0 (unreleased)](https://github.com/laravel/scout/compare/v5.0.3...v6.0.0)
@nunomaduro thanks Nuno! |
All good 💯 Another great news with this implementation is that it will save a lot of indexing operations for the user. This command will only be counted (or charged) as 1 operation instead of as many as table rows. |
So this removes the ModelsFlushed event? |
@taylorotwell No. With @driesvints's proposal, the method But, if the developer intencionally calls |
@@ -34,16 +34,8 @@ public function handle(Dispatcher $events) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@driesvints This class no longer needs the line use Laravel\Scout\Events\ModelsFlushed;
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
@taylorotwell @nunomaduro Taylor does bring up a good point that the Thoughts? |
Instead of iterating over ids from the database we use the engine itself to flush the records of an index/model.
This solves the problem where models would get out of sync with the search records and old records weren't flushed. With this implementation all records are always removed.
If there are engines which don't provide this functionality they can still iterate over the model ids if they want to.
Fixes #101
@julienbourdeau would you mind also taking a look at this if this is correctly implemented? I didn't have the chance to test this with a live implementation of Algolia.