ModelNotFoundException should have ItemNotFoundException as an ancestor

[dfidler]

PROBLEM STATEMENT

Eloquent Builder and [Illuminate\Support] Collection both have rich filtering functionality and there is a huge overlap between the two subsystems.

Consider the following two statements:

return $user->tenant->where('name', 'bob')->firstOrFail()  // ex1
return $user->tenant()->where('name', 'bob')->firstOrFail()  // ex2

Functionally, these achieve the same outcome and, in fact, if "tenant" isn't loaded in ex1, laravel does the +1 query to load it (per the relationship) and returns the appropriate tenant.

Aside: And, in my opinion, ex1 is almost always the better syntax (because we all make extensive use of with/load/loadMissing during our first query, right 😄).

However, if tenant "bob" doesn't exist, the two pieces of code produce different results...

return $user->tenant->where('name', 'bob')->firstOrFail()  // ex1 - throws Illuminate\Collections\ItemNotFoundException
return $user->tenant()->where('name', 'bob')->firstOrFail()  // ex2 - throws Illuminate\Database\Eloquent\ModelNotFoundException

The family tree for these two exceptions is as follows:

ex1: ItemNotFoundException (henceforth INFE) <-- RuntimeException
ex2: ModeNotFoundException (henceforth MNFE) <-- RecordsNotFoundException (henceforth RNFE) <-- RuntimeException

I've recently had a production failure because of this exact thing; the dev used syntax from ex1 and wrapped it with a try/catch (ModelNotFoundException).

To me, this seems like a pretty reasonable (and harmless) mistake and that two features, with so much overlap, shouldn't produce a production outage when it happens.

Note: As a result of this, I've done some regexp'ing on our code base and have two more examples where this will, eventually, cause a failure (because they are catching MNFE insteand of INFE).

PROPOSED SOLUITON

It seems reasonable to insert INFE as the ancestor to RecordsNotFoundException (although it does mean that Eloquent would then have a dependency on Support\Collections - which doesn't bother me, but it might bother some).

I also don't foresee any potential impact in the laravel code base as RNFE is only used in four places (none of which make use of Support Collections), though I can't speak for any other laravel plugins.

It seems like a pretty innocuous change and I'm wondering how many other code bases have (and will have in the future) a customer/production UX issue as a result of this small syntactical difference.

Thoughts?

[ghost]

@dfidler I will be blamed for this, but laravel is there to generate as many billable hours as possible. It offers 10000000 ways of doing things just for that purpose.

It is the developer's job to learn it, love it at first and then hate it.

I use try catch with modelNotFoundException, but I always take care to query something there and not to search in php in collections. It makes sense to throw ItemNotFoundException in the collections situation (thanks, I did not knew it behaves like this),I used instanceof comparison or null comparison coupled with find().

A solution would be catch (ModelNotFoundException | ItemNotFoundException $e)

Side note. I always expect exceptions from db queries, cache queries etc. My bad I don't expect exceptions from Log::error('text'); but that is because I log to stdout. If the logger can't write into a file for example it will throw something.

[ghost]

I work with loads of data so a query is faster than a collection search for me:) I never cache one to many relations in code to avoid memory problems.

Yes, that is an easy workaround but isn't this cleaner?

Eloquent is about models. Symfony is about items.

[dfidler]

I work with loads of data so a query is faster than a collection search for me:) I never cache one to many relations in code to avoid memory problems.

Then it doesn't sound like you have any skin in this game :)

But if you did... (like I do)...

Eloquent is about models. Symfony is about items

That's demonstrably untrue 😄

BaseCollection (\Illuminate\Support\Collection)

source: https://github.com/laravel/framework/blob/11.x/src/Illuminate/Collections/Collection.php)

BaseCollection is a trait and the most relevant methods (to EloquentCollection), along with the exceptions thrown are:

• sole():
  • ItemNotFoundException
  • MultipleItemsFoundException
• firstOrFail():
  • ItemNotFoundException

EloquentCollection (Illuminate\Database\Eloquent\Collection)

source: https://github.com/laravel/framework/blob/11.x/src/Illuminate/Database/Eloquent/Collection.php

This class, which is the return type for call falls to Builder::get() calls.

https://github.com/laravel/framework/blob/d95f6a5dcb19403269eaa2df459b70944c1c9d4f/src/Illuminate/Database/Eloquent/Collection.php#L10C1-L20C2

# Illuminate\Database\Eloquent\Collection.php
use Illuminate\Support\Collection as BaseCollection;
...
class Collection extends BaseCollection implements QueueableCollection {

As you can see, EloquentCollection EXTENDS BaseCollection.

And this opens up a world of hurt...

For example:

1. EloquentCollection doesn't override "sole()" meaning that if you call EloquentCollection::sole(), your exceptions returned will be

• ItemNotFoundException (which if you're right, should throw ModelNotFoundException)
• MultipleItemsException (should throw MultipleModelsException - but that doesn't exist. There is a MultipleRecordsFoundException, but that can only be thrown when querying the database using Builder/Query)

2. Overrides "firstOrFail()" and throws "ModelNotFoundException"

So Eloquent isn't just "Models". 😄

Summary

All Laravel "Collections" are just BaseCollections that specialize for specific use cases. Laravel also adds patterns to treat all data sources as collections.

1. BaseCollection - collection of data/classes, called "Items"
2. Builder/Query - virtual collections of records (while there is no "DatabaseCollection" specialized class, the database is, semantically, the collection and Builder/Query provides the collection interface to it)
3. Eloquent - collections of Model instances (which are records that have been transformed into Models)

And because all three of those concepts share the same interface (they all have where/first/firstOrFail), they should all have a unified class hierarchy and a unified exception class hierarchy. Records, instanced as Models are still just "Items" in a collection.

I can understand the resistance btw - when you start using Laravel, you are taught to use ModelNotFoundException. We've all grown up doing it that way, so thinking of a Model as an "item" seems like a foreign concept and maybe even feels a bit dirty.

But from an OOP perspective, it's still the right thing to do.

[ghost]

Then it doesn't sound like you have any skin in this game :)

You are correct. I am just a "stupid" guy who sees herds implementing happy flow solutions and claiming they are bulletproof.

I am curious if a possible PR from your part will be merged into laravel. I have nothing against it. What I have against is breaking changes that don't bring much benefit.

It seems reasonable to insert INFE as the ancestor to RecordsNotFoundException (although it does mean that Eloquent would then have a dependency on Support\Collections - which doesn't bother me, but it might bother some).

What you wrote above is the key. Introducing a dependency means a breaking change.

PS.
ItemNotFoundException that you mention is not found in laravel 8-11(Illuminate\Collections\ItemNotFoundException)
but Illuminate\Support\ItemNotFoundException is.

[dfidler]

You are correct. I am just a "stupid" guy...

I'm sorry if that's what you took that away from my reply. I don't think you're "stupid" (quite the opposite actually) and implying that you were "stupid" was absolutely not my intention.

What I was challenging (apparently quite badly) was you saying "I don't do that", which seemed like you were trivialising my use case - because you don't have that same use case. I do cache data in memory when I know (with some degree of certainty) that there won't be "lots" of records brought over the wire.

[ghost]

I used quotes. No issue. ;)