Using firstWhere() inside when() does not yield the expected result

[troccoli]

• Laravel Version: 9.35.1
• PHP Version: 8.1.11
• Database Driver & Version:

Description:

When using firstWhere() inside a when() on a collection I don't get what I think I should.

Steps To Reproduce:

On a brand new Laravel project, add the following Unit test

<?php

namespace Tests\Unit;

use PHPUnit\Framework\TestCase;

class FirstWhereTest extends TestCase
{
    public function test_firstWhere(): void
    {
        $collection = collect([
            ['name' => 'John'],
            ['name' => 'Paul'],
        ]);

        $notFound = $collection->firstWhere('name', '=', 'Peter');

        $this->assertNull($notFound);

        $notFoundUsingWhen = $collection->when(
            true,
            fn ($collection) => $collection->firstWhere('name', '=', 'Peter')
        );

        $this->assertNull($notFoundUsingWhen);
    }
}

The second assertion fails, as $notFoundUsingWhen is actually the whole collection, rather that null

Note
The unit test passes in Laravel 8.83.25

[driesvints]

Heya, thanks for reporting.

We'll need more info and/or code to debug this further. Can you please create a repository with the command below, commit the code that reproduces the issue as one separate commit on the main/master branch and share the repository here? Please make sure that you have the latest version of the Laravel installer in order to run this command. Please also make sure you have both Git & the GitHub CLI tool properly set up.

laravel new bug-report --github="--public"

Please do not amend and create a separate commit with your custom changes. After you've posted the repository, we'll try to reproduce the issue.

Thanks!

[eduance]

I was able to reproduce the test.

This is intended behavior in my opinion, a callback is any reference to executable code that is passed as an argument to another piece of code; that code is expected to call back the callback function as part of it jobs. When an expression is true, it should execute a piece of code, but callbacks aren't made for returning values. This is also notable in the documentation when looking at how the when methods are used. When an expression is true, we push something into a collection or we execute another piece of code.

I do have to say that I was confused by this too, but after looking way too much at the Laravel codebase (especially Taylors code), you get really comfortable with callbacks.

It would be a dream to get the inner value back, but that would demolish codebases.

[troccoli]

@eduance

I don't agree with you, but even if that was the intended behaviour, then it's not BC, as the test above passes in Laravel 8.83.25. It should have at least being mentioned in the upgrade notes.

You also said that "callbacks are not meant to return values", but if firstWhere() actually finds a record then that is returned.

For example, the following code does return the proper record.

$collection->when(
   true,
   fn ($collection) => $collection->firstWhere('name', '=', 'John')
);

Also, I don't think it's much about the callback returning a value but rather the callback actually changing the original collection.

But as I said I don't think it's intended behaviour, for a couple of reasons.

1. It works as expected (at least expected by me) if instead of firstWhere() if I use first(). The following test passes on both Laravel 9 and Laravel 8, meaning the return value from first() is indeed passed back outside the when()

<?php

namespace Tests\Unit;

use PHPUnit\Framework\TestCase;

class FirstTest extends TestCase
{
    public function test_first(): void
    {
        $collection = collect([
            ['name' => 'John'],
            ['name' => 'Paul'],
        ]);

        $john = $collection->first();

        $this->assertNotNull($john);

        $stillJohn = $collection->when(
            true,
            fn ($collection) => $collection->first()
        );

        $this->assertNotNull($stillJohn);
    }
}

2. It works as expected (again, as expected by me) when used on a query. Consider the following

User::factory()->create(['name' => 'John']);
User::factory()->create(['name' => 'Paul']);

$peterQuery = User::query()->when(
    true,
    fn (Builder $query) => $query->firstWhere('name', '=', 'Peter')
);

The SQL for $peterQuery is select * from "users" where "name" = 'Peter' limit 1. So, as you can see, the call modifies the original query. Also, I'm not sure what you could do with a callback if they could not change the original query.

@driesvints
I will try and create a new repository today.

[eduance]

@troccoli @driesvints Thanks for the clear explainer. After diving into the code, this is caused due to the null coalescing operator returning $this, where $this refers to the collection from the Conditionable trait.

if ($value) {
    return $callback($this, $value) ?? $this;
} elseif ($default) {
    return $default($this, $value) ?? $this;
}

@driesvints The expression causes a lot more issues when the initial value is not found, especially with firstWhere(), first() and more when the $callback is null. This has been in the when functionality since the beginning.

This has to return $this so the builder eventually gets returned.

[troccoli]

@driesvints

Here's the repo

https://github.com/troccoli/laravel-bug-report

[driesvints]

I believe returning the whole collection for when is the proper expected behavior.

[troccoli]

I have to say I am a bit disappointed. But at the very least I think it should be documented in the upgrade notes, as it's a breaking change in behaviour.