-
Notifications
You must be signed in to change notification settings - Fork 11.3k
[12.x] Fix collection value() method to properly handle falsy values (issue #54910) #54960
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
Conversation
Co-authored-by: Steve Bauman <steven_bauman@outlook.com>
@stevebauman You're right. #54910 (comment) Should I add a test that specifically looks for null |
@phadaphunk Yea I think it'd be worth it! 👍 |
@stevebauman Done ✅ |
This could be a breaking change. |
As we are revisiting this method, I have a question: If the idea is to retrieve the value from the first item with a matching key, shouldn't we return For example, what should this return? collect([
['name' => 'Tim', 'balance' => null],
['name' => 'John', 'balance' => 200],
])->value('balance', 'NA'); // null, 200, or NA?
Mind that I am asking what it should return. Currently, it will return But with the proposed fix, it will still return If we want to retrieve the first matching key, regardless of its value, this could be a solution: <?php
use Illuminate\Support\Collection;
use Illuminate\Support\Facades\Artisan;
Artisan::command('app:test', function () {
Collection::macro('myValue', function ($key, $default = null) {
$sentinel = new \stdClass();
foreach ($this as $item) {
$value = data_get($item, $key, $sentinel);
if ($value !== $sentinel) {
return $value;
}
}
return value($default);
});
$collection = collect([
['name' => 'Tim', 'balance' => null],
['name' => 'John', 'balance' => 200],
]);
dump(
$collection->value('balance', 'NA'), // 200
$collection->value('missing', 'NA'), // NA
$collection->myValue('balance', 'NA'), // null
$collection->myValue('missing', 'NA'), // NA
);
}); It also aligns better with |
Missed it was closed as I was typing a rather long comment... @stevebauman what do you think of my comment? @phadaphunk can you send this to master instead? |
@rodrigopedra Yes thanks I will send it to master. You can move your discussion to the issue and from there we'll see I could implement it this way as it seems to be a more complex yet more complete fix. |
@rodrigopedra Yea I think your approach would be ideal for this adjustment so it works across arrays and objects 👍 |
close #54910
This PR fixes issue #54910 where the collection value() method incorrectly skips falsy values (0, false, empty string) in the first element.
The current implementation uses firstWhere() with loose comparison, causing it to skip over the first element when the requested value is 0, false, or an empty string.
This change modifies the value() method to use first() instead, ensuring it always returns the value from the first element regardless of whether that value is "falsy".
I don't think it should be considered a breaking change since it really seems to be a bug in the intended behaviour and that this fix is more in line the documentation.