[8.x] Adds firstOrFail to Illuminate\Support\Collections and Illuminate\Support\LazyCollections

[powellblyth]

This PR recreates the FirstOrFail methods from Eloquent collections into the Illuminate Support Collection and LazyCollection, which makes writing code a little more fluent

e.g.
OLD

 $collectionFromUser = new Collection([
            ['name' => 'foo'],
            ['name' => 'foo'],
            ['name' => 'bar'],
        ]);
// Slightly obscure code and hard to scan
if ($collectionFromUser->where('name', 'fish')->count() === 0){
    $collectionFromUser->add('fish');
}

$this->doSomethingWith($collection);

NEW

 $collectionFromUser = new Collection([
            ['name' => 'foo'],
            ['name' => 'foo'],
            ['name' => 'bar'],
        ]);

try {
   $collectionFromUser->where('name', 'fish')->firstOrFail()
}
// An Exception? This must be important, I'm glad I scanned it
catch (ItemNotFoundException){
    // Ensure user has 'fish' in their collection
    $collectionFromUser->add('fish');
}

$this->doSomethingWith($collectionFromUser);

[powellblyth]

Not sure why the checks were cancelled. I think I submitted the style fixes too early

[JosephSilber]

Are we OK that firstOrFail() on a query returns a 404, whereas this (when unhandled) will return a 500?

I mean, it kinda does makes sense, but the inconsistency can catch people by surprise.

[JosephSilber]

This also needs a test ensuring it's actually lazy, akin to the lazy test for the Sole method.

[powellblyth]

Thanks @JosephSilber , I'm not so familiar with Lazy Collections, I'll look further later and try and get that test in you suggest.

[JosephSilber]

Whoops. Seems like unless doesn't yet support the higher order proxy (it does in Laravel 9).

Guess we should just revert it back to when, and change it later in master.

Sorry.

[powellblyth]

Whoops. Seems like unless doesn't yet support the higher order proxy (it does in Laravel 9).

Guess we should just revert it back to when, and change it later in master.

Sorry.

Thought I was going mad! I'll revert tonight, but I'll add the tests. Thanks for your feedback.

[powellblyth]

Hi @JosephSilber I've added the test as requested. If I'm honest, I don't understand Lazy Collections, so I can promise I've made the tests pass, but I can't tell you that the test is correct, and I value your feedback

[JosephSilber]

Do you expect this to throw? Why?

[JosephSilber]

Same here. Why would this fail?

And really, you're not testing an actual failure, and ensuring it enumerates the whole collection once.

[powellblyth]

Further changes made. I hope I have Managed to match your suggestions @JosephSilber

[JosephSilber]

See #38449

[JosephSilber]

Nice work @powellblyth 👍

LGTM now.

Only outstanding question is about the inconsistency of the HTTP response. I honestly don't know what the right answer should be here.

[powellblyth]

Nice work @powellblyth 👍

LGTM now.

Only outstanding question is about the inconsistency of the HTTP response. I honestly don't know what the right answer should be here.

Thank you. Not sure who can help about the HTTP aspect though. However, since it's not necessarily a resource, maybe the exception is ok, as you should only use this when you are expecting it. I do understand where it could confuse though.

I'm not familiar with the framework enough to even contemplate, but could the system that shows a 404 not be altered to catch both ModelNotFound as well as ItemNotFound? . I'm just throwing that one out there.

[driesvints]

Please rebase and mark as ready.

[powellblyth]

Only outstanding question is [about the inconsistency of the HTTP response]

Further to this, if you have this in a http controller, you also get a 500

    /**
     * Display a listing of the resource.
     *
     * @return Response|View
     */
    public function index(Request $request)
    {

        $users = collect(
            [
                User::factory()->make(),
                User::factory()->make(),
            ]
        );
        return view('users.index', ['users'=>$users->sole()]);
}

so it's an existing inconsistency....

[powellblyth]

@driesvints I've re-based

[JosephSilber]

[sole() return a 500] so it's an existing inconsistency

sole() should never return a 404; that doesn't even make sense.

So that's not an inconsistency at all.