Handle enum instance when throwing errors involving query parameters

[jtheuerkauf]

Laravel Version

11.45+

PHP Version

8.3.22+

Database Driver & Version

No response

Description

Using an enum as an "ID" when using find() and its derivatives is supported because the where logic converts the instance via enum_value().

In Builder::findOrFail() the $id variable is passed to ModelNotFoundException::setModel() where it becomes part of the exception message. But nothing makes sure the value of the ID can be used in a string.

In 11.x up to 12.18 the line looks like:

if (is_null($result)) {
    throw (new ModelNotFoundException)->setModel(
        get_class($this->model), $id
    );
}

In setModel() method:

if (count($this->ids) > 0) {
    $this->message .= ' '.implode(', ', $this->ids);
}

I notice the same behavior would occur if the count of fetched Models doesn't match the ID count.

Steps To Reproduce

1. create table foo(id int) (no data)
2. artisan make:model Foo
3. Make new class enum FooNum: int { case One = 1; }
4. Query Foo::findOrFail(FooNum::One)

Expected error (approximately): No query results for model [Foo] 1

Got instead (approximately): PHP Fatal error: Uncaught Error: Object of class FooNum could not be converted to string

[achrafAa]

I believe the current behavior in findOrFail() reveals an architectural consideration worth discussing:

Layer Responsibility The query builder's where() clause correctly handles Enum conversion because it's specifically processing query values. However, ModelNotFoundException shouldn't need to understand Enum formatting - its job is to display the received ID.

Parameter Validation PHP type safety suggests the caller should provide properly formatted IDs (int/string). Most DBAL methods expect scalar values, and we wouldn't expect other object types to auto-convert.

Consistency Consideration If we auto-convert Enums here, should we do the same for:

Other magic methods (__get(), __call())?

All exception messages?

Every parameter that might accept an ID?

Suggested Solutions:

A) Strict Approach (Recommended) Document that Enum values must be explicitly converted:

Model::findOrFail(MyEnum::Case->value);

B) Conversion in Builder If auto-conversion is desired, it should happen in Builder::find() before the query executes, not in the exception:

public function find($id, $columns = ['*'])
{
    $id = $id instanceof BackedEnum ? $id->value : $id;
    // ... existing logic
}

Question for Maintainers Should Laravel:

Be strict about scalar IDs (requiring ->value), or Extend Enum auto-conversion to all ID parameters?

The first approach maintains cleaner separation of concerns, while the second prioritizes developer convenience. I'm happy to submit a PR for either direction once we agree on the philosophy.

[jtheuerkauf]

Other magic methods

1. Personally, I'd limit the amount of magic, as it can easily spiral out of control.
2. __call() definitely no. As noted, values are intended to be scalar going in. Allowing magic calls should be a non-starter.
3. What magic __get() pseudo-properties would be expected? Always a ->value dependency? Enforcement of that already seems shaky, and would add a lot of type-sniffing overhead.
4. __toString() This one I can get behind. It can still get out of control, but the return is predictable, and it's a one-step conversion with (string).

All exception messages? I think anywhere that parameters are key information in the error, conversion should be supported. But I'm definitely not married to that opinion. Maintainers can make a better judgment call.

Every parameter that might accept an ID? Again, judgment call for those in charge. My initial take is no, just IDs.

As for which approach to take...

A) Strict Approach

I see the benefits of this as far as predictable data and less type juggling under the hood, but doesn't that strictness lean against the Laravel philosophy? 😆

B) Conversion

Enums are already supported in a lot of the Builder code, so deprecating and retracting them doesn't seem beneficial. They can only represent a string or integer, and don't support magic methods (yet?) to incur other sorcery. To me they're safe enough to convert with locked-in predictable behavior.

One note on the suggested change:

public function find($id, $columns = ['*'])
{
    $id = enum_value($id);
    // ... existing logic
}

This would be more consistent with the rest of enum support, allowing for a UnitEnum to be converted to its ->name.

[crynobone]

Hey there, thanks for reporting this issue.

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"

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!

[jtheuerkauf]

https://github.com/jtheuerkauf/laravel-bug-55977

Just run the migration and seeder for the users table, then open a tinker prompt:

$ php artisan tinker
Psy Shell v0.12.8 (PHP 8.3.21 — cli) by Justin Hileman

> \App\Models\User::findOrFail(\App\Models\UserEnum::One)->pluck('id', 'name')->toJson();
= "{"Test User":1}"

> \App\Models\User::findOrFail(\App\Models\UserEnum::Ten)->pluck('id', 'name')->toJson();
   Error  Object of class App\Models\UserEnum could not be converted to string.

UserEnum::One will match the Test User, while UserEnum::Ten will find nothing, but fail to convert the enum to a string for the not-found error.

[crynobone]

Do you know which commit/PR implements enum support for Illuminate\Database\Eloquent\Builder::findOrFail()?

[jtheuerkauf]

Do you know which commit/PR implements enum support for Illuminate\Database\Eloquent\Builder::findOrFail()?

@crynobone If this was for me, I didn't add anything to change current behavior. The code is provided to reproduce the problem. I didn't feel it's my place to implement a fix until/unless maintainers decide on the appropriate solution.

[crynobone]

@jtheuerkauf Illuminate\Database\Eloquent\Builder::findOrFail() doesn't have any history supporting the feature and not just an alias to where(), I asked just to make sure I didn't missed anything while assessing this issue.

It seems that this should be considered a feature request, and you are free to send a PR.

[github-actions[bot]]

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

[HamidRezaLS]

@jtheuerkauf thanks for feedback. You are right any good solution needs to support arrays of enums and handle the situation where the exception is being thrown in multiple places. That brings us to a design decision in principle with two main options:

Option A: Fix it earlier on in the Builder so findOrFail will be smarter about enums even if they are arrays. This would be consistent with how where() already operates for enums.

Option B: Do it centrally in ModelNotFoundException, converting enum IDs there. That's far more DRY and covers all edge cases regardless of where the exception comes from.

So it comes down to do we want consistency in the Builder logic, or a centralized, automatic correction in the Exception layer?

Both have their advantages. I'd be interested to hear your thoughts and if @crynobone or @taylorotwell have an angle they're thinking of, that'd be great to know before going forward.

Thanks again.

[jtheuerkauf]

https://github.com/laravel/framework/pull/56169#discussion_r2174450946

@HamidRezaLS Thanks for the continued effort. The array check was, I think, a simple "gotcha" that needs to be accounted for, but it's not the only. When you look at other Builder methods that handle multi-value arguments (find, findMany, where and its family, etc.) a simple array is only one type that gets managed later in the process.

I personally don't think it's practical or necessary for the whole Builder library to take on a new way of dealing with passed values.

The handling is clean most of the way through. It's only in the occasional error scenario where complex types aren't dealt with very well.

I'm interested to get more feedback from core maintainers before pushing more attempts. Seem fair?

[HamidRezaLS]

@jtheuerkauf yeah you're right a simple is_array check is too narrow and does not cover for other types. lets wait for feedback from the maintainers.

[rafaelqueiroz]

@crynobone https://github.com/laravel/framework/pull/57568

I was checking that and the issue happens on find() method too.

The find() and findOrFail() accepting enum on first parameter and will works if they return some result. If returns null result I got the same error on find() like findOrFail().

The fix should be done in 2 places and IMO it's ok use enum_value() directly on the code, without check type. The root of issue was on whereKey method.

[jtheuerkauf]

@rafaelqueiroz I took a look at your PR. The main reason I haven't submitted one of my own was exactly because I didn't want to spot-fix the way $id types are handled.

It's still not a fully formed thought or idea, but it would seem there needs to be a routing process for methods that accept IDs in various types and structures so they get sanitized right away, then the rest of the Builder logic can expect clean values.

I'm not sure yet what that would look like, but it'll definitely be a bigger impact, and as mentioned in responses above, is best suited as a feature change.

@crynobone Considering it as a feature request, do you think this would work in a minor release, or wait for full major? To me it seems more of a "healing" change than adding anything functionally significant, but it definitely will touch a lot of spots. What are your thoughts?

[rafaelqueiroz]

@jtheuerkauf I was thinking about it yesterday and this issue come with a bit complexity and impact. Currently I notice 4 points which blow up the conversion to string issue:

Illuminate/Database/Eloquent/Builder.php:293

$id = (string) $id;
// Object of class Enum could not be converted to string

Illuminate/Database/Eloquent/Builder.php:620

if (count($result) !== count(array_unique($id))) {
// Object of class Enum could not be converted to string

Illuminate/Database/Eloquent/Builder.php:622

get_class($this->model), array_diff($id, $result->modelKeys())
// Object of class Enum could not be converted to string

Illuminate/Database/Eloquent/ModelNotFoundException.php:42

$this->message .= ' '.implode(', ', $this->ids);
// Object of class Enum could not be converted to string

Mixed params should be normalized at some point, specially Enum. PHP discourage use as "fancy strings".

I know I pushed to hard when I call the enum_value directly but this only missing check if is enum or not. I was thinking create a auxiliary function is_enum or enum which returns a boolean for check before call enum_value.

Anyway will be a very similar solution.

[shaedrich]

I was thinking create a auxiliary function is_enum or enum which returns a boolean for check before call enum_value.

I don't think, there's much benefit from that as enum_value() already does this and otherwise returns the value as is

[jtheuerkauf]

@rafaelqueiroz Personally I think the excuse for not making the enum castable to its backing value is weak. But more importantly PHP needs magic casts to other types before even further extending the reach of strings. There's just as much justification for other __to[Primitive]() methods.

I digress.

In my opinion, any solution to address this (if it's done at all) should be holistic, working across the board so all entry points to the Builder normalize ID data. Is that realistic? Will it be a lot of overhead for relatively low gain of data stability? Without a significant design change to the builder, I imagine it won't be worth it. But who's to say someone won't come up with the cure?

[shaedrich]

For quite some time, I was thinking about use cases for PHP attributes, namely method argument sanitation.

public function where(
    // You can pass in string|ColumnKeyEnum, while the SanitizeInput attribute
    // makes sure, the underlying method only ever receives a string 
    #[SanitizeInput(allowed: ['string', ColumnKeyEnum::class])] string $key,
    mixed $value,
)
{
    // use the string key …
}

public function whereDate(
    #[SanitizeInput(allowed: ['string', ColumnKeyEnum::class])] string $key,
    // You can pass in either an ISO string (or any string, DateTime/Carbon can parse),
    // a timestamp or any of the DateTime/Carbon classes. The SanitizeInput attribute
    // makes sure, that you can always work with the Laravel Carbon class
    #[SanitizeInput(allowed: [
        'string',
        'int',
        DateTime::class,
        \Carbon\Carbon::class,
        \Illuminate\Support\Carbon::class,
    ])] \Illuminate\Support\Carbon $value,
)
{
    // use date value …
}

This has one downside which on one hand, Laravel could pull of because of its incredible service container, but on the other almost surely won't because this would require all these methods to be called almost exclusively through the service container if you want argument sanitation and this would require possibly too huge changes.

[jtheuerkauf]

@shaedrich It's an interesting concept, but unless you plan to expand the Attributes to support all the types and constructs the Builder logic methods support (Closures, arrays, nested, mixes of all of it, etc.), it would have limited utility. And if you did attempt to provide broad sanitation logic via attributes, you're basically rebuilding the problem somewhere else. All that sanitation has to be robust enough to deal with crazy Builder argument scenarios, and the Builder itself is already doing that.

ORM construction is hard. I give Laravel a lot of credit for making Eloquent's learning curve relatively low, but the trade-off is complexity under the hood.

[shaedrich]

Yeah, sure, this hypothetical attribute should theoretically be able to cast any type. This only has to be configured somewhere.

The builder comes with its own challenges that wouldn't be entirely covered by this hypothetical attribute. This would need yet another solution:

// if three attributes are passed AND the second attribute has the type
// string|\Illuminate\Database\Comperator, then the method is
// overloaded by the definition in this signature
#[MethodSignature(
    column: 'string',
    operator: ['string', \Illuminate\Database\Comperator::class],
    value: 'mixed'
)]
// if only two attributes are passed, then the method is
// overloaded by the definition in this signature and the
// second argument gets its default value by the original
// method signature
#[MethodSignature(
    column: 'string',
    value: 'mixed'
)]
public function where(
    string $column,
    \Illuminate\Database\Comperator $operator = Comperator::Equals,
    mixed $value,
)
{
    // …
}