[13.x] feat: convert QueryException to ModelNotFoundException in implicit route binding

[calebdw]

Hello!

When implicit route model binding encounters a QueryException (e.g., from integer overflow or type mismatch), convert it to a ModelNotFoundException instead of letting the 500 propagate. This ensures invalid route parameters like /users/99999999999999999999 return a 404 rather than a 500.

This has been the biggest pain, without this you have to add ->whereNumber() on every route OR you litter the AppServiceProvider with 50+ calls like Route::pattern('user', '[0-9]+');. Even with this, we've still had exceptions where the parameter was all digits but it exceeded the column int size so we had to add even more checks to prevent the exception.

Ultimately, a model was not found so end user should always receive a 404 instead of a 500.

The QueryException is still reported, but developers now have the ability to disable reporting, for example:

class AppServiceProvider extends ServiceProvider
{
    public function register(): void
    {
        Model::reportRouteModelBindingExceptions(! $this->app->isProduction());
    }
}

Thanks!

[shaedrich]

Is this intentional? 🤔

[calebdw]

Is this intentional? 🤔

Is what intentional? 🧐 I intentionally submitted this PR if that's what you're asking about 🙃

[shaedrich]

No, sorry, should have included the quote, I was looking at 😬 😅

This ensures […] a 404 rather than a 500.

[calebdw]

Lol, I'm not sure. I was trying to keep this PR simple, but if folks want the exception then I can add a way to configure it

[shaedrich]

I guess, that would be great—thanks 👍🏻

[taylorotwell]

I wonder if this would be hard to debug what is going on, especially if you expect to be getting a model back, you get some query error (maybe a global scope), but now it's masked as a 404. It feels like the current behavior is pretty helpful in local development, but maybe not ideal in prod (though you would still want the original error).

[calebdw]

Yeah, I could see maybe having some configuration options, maybe something like:

Model::throwRouteModelBindingExceptions();
// and / or maybe just this
Model::reportRouteModelBindingExceptions();

I think from the end user perspective they should always receive a 404 instead of a 500 in production

[shaedrich]

Nice! 👍🏻

[calebdw]

@taylorotwell, I've updated to report the QueryException, with the option of disabling reporting

[taylorotwell]

Hmm, let me think about this one - I'm not confident we're totally landing on the right solution.

[calebdw]

Hey @taylorotwell, just wanted to check in here---this bit us again today and it would be really nice if these could just return a 404 in prod without alerting all the devs about an exception

[calebdw]

Hey @taylorotwell, this bit us again today---any thoughts on what the right solution would be so we don't have to litter ->whereNumber() on every single route definition or have a ton of Route::pattern calls?

[GrahamCampbell]

For what it's worth, I feel like this is a bad idea. It's actually extremely expensive and dangerous to allow a string to query an int column in MySQL as it may will cause a full table scan and not use the index. Proper validation should be applied before the query is made.

[calebdw]

@GrahamCampbell, do you have any other ideas? I feel like there's bound to be a better global solution than:

• adding ->whereNumber to hundreds (if not thousands) of routes
• or adding Route::pattern() calls to a service provider (a place that's far removed from the actual route definitions) for every unique route parameter name (we have 300+ unique models)

Also, it's not just about non-numbers but also numbers that are too large (see above) for the primary key size.

It's actually extremely expensive and dangerous to allow a string to query an int column in MySQL as it may will cause a full table scan and not use the index.

• it's important to note that I'm not introducing "expensive and dangerous" queries---I'm just turning exceptions from existing queries into a 404 because: 1) there's nothing to fix, 2) the user SHOULD receive a 404 instead of a 500 error page, and 3) I don't want to get pinged from an exception
• from my understanding (and I double checked with Grok, but I suppose we both could be wrong), this is not actually a valid concern---MySQL performs implicit type coercion before comparison:
  • '123' gets converted to 123
  • '123acb' gets truncated to 123
  • 'foo' gets converted to 0