[5.4] allow random to return 0 items

[browner12]

currently passing in a 0 to the Collection->random() method will result in a php error.

this is because it passes the parameter through to the native php array_rand function which specifies the 'second argument needs to be between 1 and the number of elements in the array'.

now if we pass in 0, we will catch that early, and simply return an empty static.

[browner12]

I don't think this is breaking, unless someone was expecting a PHP warning when they passed in zero?

[laurencei]

Why would you want a 0 return?

edit: seems more logical to throw a InvalidArgumentException if you pass in 0?

[miscbits]

this wouldn't be a breaking change, but I'm not sure I see the use case being widespread enough to include this in the core. Seems like just bloat since a user could easily do this check on their own and not add an extra check for the majority of people who wouldn't ever need this.

[BrandonShar]

If there are 0 items in the collection, we'll never reach this check even though that would seem to be valid with this new logic.

[browner12]

here's my use case. I have a User model and a Drink model.

Users can have favorite Drinks.

When I seed the favorite drinks, I want to be able to randomly give them between 0 and 5 favorite drinks.

class FavoriteDrinksSeeder
{
    public function seed(){

        $drinks = \App\Drink::all();

        foreach(\App\User::all() as $user) {
            $user->favoriteDrinks()->attach($drinks->random(rand(0, 5)));
        }
    }
}

[miscbits]

I see. honestly I think that this is pretty edge though to ever want to give 0 random items. For this case, it'd probably just be better to do

class FavoriteDrinksSeeder
{
    public function seed(){

        $drinks = \App\Drink::all();

        foreach(\App\User::all() as $user) {
            $random_number = rand(0, 5);
            if($random_number) {
                $user->favoriteDrinks()->attach($drinks->random($random_number));
            }
        }
    }
}

I mean it's not as easy for you, but I think the PR in the framework is just bloat.

[browner12]

hmm.. i disagree. I would imagine this seeding scenario is common enough.

For example,

A user has made between 0 and 5 orders.

A user has read between 0 and 10 books.

A product has 0 to 3 configuration options.

etc, etc.

[miscbits]

that answers that.

[BrandonShar]

@browner12 there's still a "bug" here that I put in the review. collect()->random(0) will throw throw an InvalidArgumentException "You requested 1 items, but there are only 0 items in the collection."

[browner12]

damn you 0 and your false evaluation! 😡

the simple solution seems to be to just move the added check to the top of the function. does that work for everyone?

[BrandonShar]

Makes sense to me. Nothing else to check if all you want back is an empty collection.

[vlakoff]

Indeed, previously zero was an unsupported case, just like PHP's array_rand(). But asking for zero items and getting zero items makes sense, so +1 for this PR.

Though, for consistency the same changes should be applied to Arr::random() as well.