Changed AggregatorObserver delete & forceDelete Methods to respect Aggregator syncing status #301
Conversation
The Delete and ForceDelete methods were checking whether syncing was disabled for the model rather than the searchable. This fixes that and updates the Delete method to reflect changes in the parent ModelObserver class.
There was a problem hiding this comment.
Hi @goldmerc ,
Thank you for this PR! I left some comments, please let me know what you think and if you have any questions.
| @@ -85,12 +85,14 @@ public function saved($model): void | |||
| */ | |||
| public function deleted($model): void | |||
| { | |||
| if (static::syncingDisabledFor($model)) { | |||
| if (!$model->wasSearchableBeforeDelete()) { | |||
There was a problem hiding this comment.
This check here won't always work. Aggregators can contain classes that don't have the Searchable trait on them. Without this trait, this check will fail because the method is not defined. We should probably add a check on whether or not the model includes the Searchable trait (you can see how here).
There was a problem hiding this comment.
I didn't know that. Are you sure aggregators work if the underlying models don't have the Searchable trait? The docs for scout extended recommend disabling syncing for models if you don't want them to create their own individual model indexes. If what you say is correct, that wouldn't be needed.
There was a problem hiding this comment.
Yes, this is something that's possible to do. This is not well documented, but the tests ensure this works as well (in AggregatorTest.php, we use the News aggregator that includes the Post model, which doesn't include the Searchable trait).
There was a problem hiding this comment.
Interesting. In that case, maybe best to check if the method exists rather than whether the Searchable trait is being used. That way an aggregated class without the Searchable trait can still implement a wasSearchableBeforeDelete method if wanted.
There was a problem hiding this comment.
Yes, that works for me too! 🙂
…led for search indexes Co-authored-by: Devin Beeuwkes <[email protected]>
|
Having the same issue as #285, I'd like to bump this PR. We've patched it in our application with a very similar fix (simply removing the Do you need someone to take it over? I could create my own PR if desired |
Describe your change
This PR changes the AggregatorObserver delete and forceDelete methods to check syncingDisabledFor using the Aggregator class rather than the Model class. This brings these methods inline with the behaviour of the saved method.
It also updates the delete method to reflect changes made in the parent ModelObserver class.
What problem is this fixing?
If you want to use an aggregator but don't want to index individually the models being aggregated, the algolia/scout-extended docs recommend to simply disable syncing for those models, eg...
Laravel\Scout\ModelObserver::disableSyncingFor(Article::class);
Laravel\Scout\ModelObserver::disableSyncingFor(Event::class);
However, the AggregatorObserver delete and forceDelete methods were both checking syncingDisabledFor using the Model class rather than the Aggregator class. This meant that if you deleted a model which had syncing disabled (because you only wanted an Aggregated Index and not individual indexes), it was not deleted from the aggregated index.
Conversely, the saved method does check syncingDisabledFor with the Aggregator class, which I believe should be the correct behaviour.