runForMultiple unexpected-ish behaviour with empty array

[sgtlambda]

Description

Not strictly a bug nor a feature, but I'd like to bring some attention to the following situation:

\Stancl\Tenancy\Tenancy::runForMultiple runs the given callback for all tenants if $tenants is an empty array. It makes sense that this is the behaviour for null, but I'd like to make a case for why this is not the expected behaviour with an empty array. Imagine you have some code that gathers some "candidate" items and then loops over them.

$candidates = [];

// Gather candidates ...

foreach($candidates as $candidate) {
    // Do something
}

I wouldn't expect all potential candidates to have the code in the foreach block executed on if the candidates array is empty after the "gathering" phase -- it doesn't follow naturally that I need to see if the candidates array contains any items first before I move on to the foreach loop. You can see where I'm going with this:

$tenants = [];

// Gather tenants ...

tenancy()->runForMultiple($tenants, function() {
    // Do something
});

I'd argue there's a small likelihood of this causing very serious bugs. If the developer intends to run the callback for all tenants, they can use null for the $tenants argument explicitly.
Updating this behaviour would obviously be a breaking change, but it seems worthwhile to look into this.

[stancl]

Hey, thanks for reporting this. I agree that a strict null check would make much more sense here, with the current falsy check being prone to some very bad bugs. I'm not sure if this should be fixed in a minor version in v3 or if I should only change this in v4 which should be coming out soon.

I'd prefer fixing this in v3 so I'm trying to come up with cases where people may depend on the current implementation.

One reason for why the method accepts both null and a list of tenants is actually that we use it in commands, where just dynamically changing the argument passed is easier than calling a different method altogether (to run something for all tenants, instead of a just a list when no tenants are passed). Thinking about that, it might actually be that the tenants argument is either null or a list of strings directly in the CLI arg logic. So it's possible that things like this could be broken by this change.

Arguably a lesser bug than accidentally running stuff for more tenants than intended but would probably affect more people than the current implementation does.

So it's kind of a "low likelihood of a very bad bug" vs "high likelihood of a minor but logic-breaking bug" situation.