[11.x] Prevent implicit route model binding parameter name mismatches in controllers

[stevebauman]

This PR is based on a question I asked on X, where I somewhat frequently scratch my head as to why a route model binding isn't working and a new model instance is tossed in via dependency injection. It's obvious when you show the routes together with the controller method (as you'll see below), but it's an easy mistake to make when you're in one file working with models named two or more words (PhoneNumber, UserInvitation, ActivityLog, etc.):

https://twitter.com/ste_bau/status/1788976209943986419

Route::name('show')->get( 
    'phone-numbers/{phoneNumber}', 
    [PhoneNumberController::class, 'show'] 
); 

// ... 

public function show(PhoneNumber $number) // Named incorrectly - new instance created
{ 
    $number->exists; // false ❌
} 

public function show(PhoneNumber $phoneNumber) // Named correctly - existing model instance given
{ 
    $phoneNumber->exists; // true ✅
} 

This PR implements a way to enable throwing an exception when a model (similarly to Model::shouldBeStrict()) binding is missed:

\Illuminate\Routing\ImplicitRouteBinding::preventModelParameterMismatch();

Then, when the first scenario above occurs, developers will immediately see the exception with clarity:

public function show(PhoneNumber $number) // ❌ throws ModelParameterMismatchException
{ 
    // ...
}

ModelParameterMismatchException: Route parameter name 'number' does not match expected model binding name 'phoneNumber'.

---

This PR doesn't affect nullable controller parameters:

public function show(PhoneNumber $number = null); // ✅

It also does not affect explicit model bindings (as these are performed prior to implicit):

Route::model('number', PhoneNumber::class); // ✅

Let me know your thoughts! No hard feelings on closure ❤️ .

[ralphjsmit]

I like the PR, nice work! It can definitely be very confusing that the container sometimes generates empty Eloquent models that don't exist in some places that are implicit (like route model binding in your case). I would even argue that this setting could go on the container, as a way of telling the container "never instantiate an empty non existent Eloquent model". The chances that instantiating such a model is erroneous are much higher than that a developer is actually doing app(User::class)->fill(...)->save() to name sth. Everyone would call Eloquent models statically User::create(...).

[sharryy-s]

I loved this PR. I have personally bitten by it several times.

[megasteve19]

This should definitely exist. Sometimes, you get trapped in this situation, and after hours of debugging, you realize it was just a wrong naming..

[ashikhasnat]

It should be added. At first, I didn’t know the name needed to be the same! I thought the model name would be enough. After a few incidents, I just stopped using route model bindings.

[hailwood]

Should this be tied in with Model::shouldBeStrict() I feel like it's related.

[Tofandel]

This might be of interest: I'm using spatie/route-attributes, which mostly avoids this issue by having the route declaration next to your controller method declaration, but yes a warning if a route binding parameter is missnamed would be nice

[flavio-schoute]

What is the current status of this PR?

[browner12]

How does it know that the incorrect variable name is not what the user actually wants? While I would say it's very unlikely the user intentionally mis-named the variable, this feels like it could cause some unexpected or unwanted issues.

[stevebauman]

@browner12 It knows because after enablement, it makes the assumption that you will only want existing model instances to be provided in controller endpoint methods. If you want a new model instance to be provided, you must be explicit.

For example, this would throw a ModelParameterMismatchException exception:

public function show(Post $newPost)
{
    // ...
}

And this would not:

public function show(Post $newPost = new Post)
{
    // ...
}

But it prevents cases like this where it's most useful, where you're provided a new model where you're expecting an existing one:

Route::patch('posts/{post}', [PostController::class, 'update']);

// ...

public function update(PostRequest $request, Post $userPost)
{
    $userPost->exists; // false
}

This would also be opt-in behaviour, so developers would be well aware of what this does before enabling.