Skip to content

Conversation

DianaDenpa
Copy link
Contributor

Using the collection "min()" method with a key on a collection that contains null, will return null and not the smallest number as expected. Without a key the return value is 10.

Examples:

  • Current
$c = new Collection([['foo' => 10], ['foo' => 20], ['foo' => null]]);
$c->min('foo'); // null
  • Fix
$c = new Collection([['foo' => 10], ['foo' => 20], ['foo' => null]]);
$c->min('foo'); // 10
  • Without key
$c = new Collection([10, 20, null]);
$c->min(); // 10

Methods like "avg()" or "median()" have similar issues and should be checked if this goes through.

@acydburn
Copy link

Maybe one could add a new test case instead of changing the current one? But this is highly IMHO (and for consistency with the [1,2,3,4,5] check below)

@jmarcher
Copy link
Contributor

jmarcher commented Apr 17, 2018

Well, I do not think the current implementation is wrong.

In PHP:
min(10,null)=null

max(10,null)=10

Same for arrays

min([1,2,3,null])=null

So this is consistent with PHP

@DianaDenpa
Copy link
Contributor Author

@jmarcher
That would mean that the current implementation is also wrong.

Current test

$c = new Collection([1, null, 3, 4, 5]);
$this->assertEquals(1, $c->min());

@jmarcher
Copy link
Contributor

Yeah, maybe because we mainly collect objects and rarely only numbers. So your approach is right. 👍

@taylorotwell taylorotwell merged commit 44b5c1f into laravel:master Apr 25, 2018
@DianaDenpa DianaDenpa deleted the fix/collection-min branch April 27, 2018 09:38
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.

4 participants