[5.4] Collection::random(int $amount) should always return a collection

[sebastiandedeyne]

Currently, if you call $collection->random(1), you'll get a single item back. This makes sense if there isn't a parameter set, but if 1 is explicitly passed through, it should return a new collection instance. This keeps it consistent with passing in any other integer.

Example use case: if you want to retrieve a random number of random items (useful in seeders), you can't predict the return value:

$collection->random(rand(0, 5));
// Should return a collection containing 0 to 5 items

Proposed solution: change the default value to null, so $collection->random() still returns a single item.

If there's interest I'm willing to spin up a PR!

[KKSzymanowski]

I like your proposal, but it would break backward compatibility.
I think, the better way would be to create a new method:

public function randomSubset($count)
{
    if(empty($this->items)) {
        return new static;
    }

    if($count == 1) {
        $key = array_rand($this->items);

        return new static([$this->items[$key]]);
    }

    return $this->random($count);
}

[KKSzymanowski]

Or preferably $amount instead of $count to continue Collection::random convention.

[sebastiandedeyne]

In only would break things if you explicitely call random(1), which I think is an edge case since you'd generally write random() or random($anyOtherNumber).

I don't think it would be an issue to add this in 5.4 (just edited the title for clarity)

[KKSzymanowski]

This is a valid point, but nevertheless behaviour of this method changes drastically from returning a value to returning a one element collection, which in my opinion breaks BC.

You should ping Taylor on Slack to get feedback.

[sebastiandedeyne]

I know it breaks BC, but breaking changes are allowed between Laravel minor versions (it doesn't follow semver)

[KKSzymanowski]

Yes, that's why I think you should talk to Taylor about this, so you don't waste time writing tests.

[franzliedke]

@sebastiandedeyne Great proposal, makes complete sense and is very consistent with other great APIs (thinking of Ruby's Array#first, for example).

That BC break should indeed be fine, especially as it affects an edge case that should either be considered a bug (when passing a variable) or is very easy to find (when passing 1 directly).

As for getting Taylor's feedback, that's exactly what this place is for...

[KKSzymanowski]

As for getting Taylor's feedback, that's exactly what this place is for...

You may be right.

Ping @taylorotwell

[taylorotwell]

I feel like this makes fairly good sense.