Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5.4] Add $num optional parameter to Arr::random() #19818

Merged
merged 2 commits into from
Jun 29, 2017
Merged

[5.4] Add $num optional parameter to Arr::random() #19818

merged 2 commits into from
Jun 29, 2017

Conversation

vlakoff
Copy link
Contributor

@vlakoff vlakoff commented Jun 29, 2017

Follow-up to #19741.

There is a bit of room for improvement. Mainly, the function would fail if requesting more items than available. This can be dealt with later, here I do this PR to quickly alter the method signature while it is fresh.

ping @calebporzio

@taylorotwell taylorotwell merged commit 53f6d52 into laravel:5.4 Jun 29, 2017
@vlakoff
Copy link
Contributor Author

vlakoff commented Jun 29, 2017

array_random($array, 1) behaves as array_random($array), i.e. it directly returns the random item.

Is it ok, or should it be changed to behave as array_random($array, N), i.e. returning an array of one item?

@vlakoff
Copy link
Contributor Author

vlakoff commented Jun 29, 2017

Indeed that's what Collection::random() does. random() directly returns the item, random(1) returns an array of one item. Do you agree Arr::random() should do the same?

@calebporzio
Copy link
Contributor

I think so, it more closely matches array_rand as well - I will submit a PR as soon as I get the chance.

@vlakoff
Copy link
Contributor Author

vlakoff commented Jun 29, 2017

I'm already on it :) Also, refs #16865.

@calebporzio
Copy link
Contributor

Cool 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants