Sync conditional models

[arjanwestdorp]

As mentioned in #263 the models where shouldBeSearchable() returns false are not deleted from the search engine.

This PR makes sure those models are made unsearchable to prevent your search engine exposing data that shouldn't be available.

This PR fixes #249

[kfirba]

I really don’t think this behavior is desired in many cases. The shouldBeSearchable is only serving as a “gate” to algolia (or any other implementation). Maybe we should introcude a new artisan command that will sync the database with the service provider and we can fire it off as a late-night maintenance. Otherwise, we can maybe introduce a new scout configuration to conditionally unsync models. By doing that, we won’t introduce a breaking change.

Think about a scenario where you want to sync your model but “freeze” its state with the service provider. So you go and add a “frozen” column to your models and the shouldBeSesrchable method will return that column’s value. Note that I don’t want to delete the record from the service provider, simply freeze its state. What you suggest here will remove the record and result in a huge confusion.

If we decide go down that path, we need to fix the import artisan command to unsync models as-well.

[arjanwestdorp]

I've initially created this PR as #263 but Taylor replied this should be default behaviour in his opinion and I agree on that.

I get your example but wouldn't using the disableSearchSyncing() method be better for this. Since you basically want to conditionally disable the sync. We probable better refactor the syncingDisabledFor() method to be able to check this also on model level, something like this:

public static function syncingDisabledFor($class)
{
    if (is_object($class) && $class->isSyncingDisabled()) {
        return true;
    }
    
    $class = is_object($class) ? get_class($class) : $class;

    return isset(static::$syncingDisabledFor[$class]);
}

I don't think that we would have to fix the import command since that command should import the models and not sync them. Adding a sync command or passing a --sync option to the import command seems like a better solution to me. What do you think?

[kfirba]

The disableSearchSyncing() is a static method, hence we don't get access to an actual instance there. I don't think we can use that function to determine if we should stop syncing an instance. Introducing another method such as shouldBeSynced() feels like a redundant duplication. I really think the best option is what you introduced in #263.

Yea, passing a --sync flag to the command is a better way to go about it. Currently, the import command will filter out any model that returns false in their shouldBeSearchable() call: https://github.com/laravel/scout/blob/4.0/src/SearchableScope.php#L35

@taylorotwell maybe we need to re-consider PR #263 and also add an optional --sync flag to the import command.

[taylorotwell]

@kfirba that wasn't what shouldBeSearchable was intended for. It was intended to determine if the model is "searchable" or not. That's why it's called that. It's not called shouldBeFrozen.

[kfirba]

@taylorotwell well, shouldBeSearchable() is a more of a generic name. We let the developer decide what should be searchable and what not. The term "Frozen" is irrelevant here but simply serves as an example.

That being said, how would you go about syncing your models with a service provider and keep them in a specific state while your local model may change a little bit? I really think that exposing an additional configuration option is the best solution here. Why are you so reluctant to that?

[taylorotwell]

No, the library (or, really I the maintainer) decides what the method means. It's not up for interpretation. It means, the model should be searchable or not.

The whole debate about freezing the search state while your local model changes is something for an entirely new feature discussion and doesn't belong here. It's only confusing the current discussion.