Framework: [5.3] Optional route parameter and type-hinted implicit binding resolves an empty model

[ElfSundae]

Route::get('user/{user?}', function (User $user = null) {
    // 
});

$user will be an empty model (not null) if there is no matching model instance, is this normal?

Now I fix this issue by registering user as an explicit binding:

$router->bind('user', function ($value) {
    return \App\User::find($value);
});

Route::get('user/{user?}', function ($user = null) {
    //
});

[GrahamCampbell]

$user will be an empty model (not null) if there is no matching model instance, is this normal?

It is normal, unfortunately, yeh, because it'll fallback to using dependency injection to basically app-make the App\User.

Is it a good behaviour? Questionable.

Now I fix this issue by registering user as an explicit binding:

Yeh, that's probably the best way to do it for the optional params.

[GrahamCampbell]

Ping @taylorotwell. I've seen this before, and it would be nice if we could perhaps change the way this works in L5.3.

[ElfSundae]

Thank you @GrahamCampbell

[deframe]

If it's a model you are passing then you can look at exists It might be neater than creating a binding and dropping the type-hint.

Route::get('user/{user?}', function (User $user = null) {
    if ( ! is_null($user) && ! $user->exists) {
        abort(404);
    }
});

[GrahamCampbell]

If it's a model you are passing then you can look at exists It might be neater than creating a binding and dropping the type-hint.

That's not brilliantly elegant though.

[deframe]

That's not brilliantly elegant though.

Yeah it's far from clean, plus it's annoying having to add this condition almost every time a binding is used. I might be able to look into this tomorrow if no one objects; if it's a straightforward change then I'll submit a PR to 5.3 and let Taylor decide where to go with it (maybe an empty model is desirable in some cases?)

[deframe]

I can't spend any more time on this but I found where the issue(?) occurs.

IlluminateRoutingRoute::runCallable() for closures

protected function runCallable(Request $request)
{
    $parameters = $this->resolveMethodDependencies(
        $this->parametersWithoutNulls(), new ReflectionFunction($this->action['uses'])
    );

    return call_user_func_array($this->action['uses'], $parameters);
}

IlluminateRoutingControllerDispatcher::call() for controllers

protected function call($instance, $route, $method)
{
    $parameters = $this->resolveClassMethodDependencies(
        $route->parametersWithoutNulls(), $instance, $method
    );

    return $instance->callAction($method, $parameters);
}

Any binding that returns null will be filtered out of the parameters list by the call to Illuminate\Routing\Route::parametersWithoutNulls().

Illuminate\Routing\Route::resolveMethodDependencies() and Illuminate\Routing\ControllerDispatcher::resolveClassMethodDependencies() make subsequent calls to Illuminate\Routing\RouteDependencyResolverTrait::transformDependency() which cross-references each _reflected_ parameter on the closure / controller method against the _filtered_ parameter list. If it doesn't exist within that, App::make(...) is run.

A potential way of fixing this might be to give all null-result bindings an temporary interim value so they don't get filtered out by parametersWithoutNulls() then reset them back again after the dependencies have been resolved.

[ElfSundae]

Sadly it has no change in 5.3 😢

[powelski]

$user will be an empty model (not null) if there is no matching model instance, is this normal?

It shouldn't be neither empty model _nor null_ in my opinion. It should throw ModelNotFoundException. Optional parameter doesn't mean it can be a trash.

[GrahamCampbell]

Optional parameter doesn't mean it can be a trash.

That's not what's happening. It's dependency injecting the class you typehinted. It just happens to be also used for model binding when that's present.

[powelski]

I know that. What I mean is that this behavior is inconsistent. For non-optional parameters, it's either model instance assigned to parameter or ModelNotFoundException. For optional parameters, it's not throwing ModelNotFoundException for unexisting model, it just sets the parameter to null. This is very error-prone behavior.

[deframe]

Where would the ModelNotFoundException be caught if it needed to be handled in a specific way?

[powelski]

In global handler. This is what you would do for implicit model binding for required parameters too.

[taylorotwell]

I don't really know what the behavior should be in this case. To me it seems more logical that it should be null rather than an empty model but I'm not sure how much work it would be to make that happen.

[powelski]

@taylorotwell My intuition tells me that if we have a route defined as /people/{user?}, $user should contain:

1. For model-focused call function (User $user):
   
   
   
   • Existing User instance if the User is found.
   
   
   • Empty User model if User is skipped (when we hit /people, but let's don't hit people, people).
   
   
   • Empty User model if User doesn't exist.
   
   
2. For instance-focused call function (User $user = null):
   
   
   
   • Existing User instance if the User is found.
   
   
   • null if User is skipped.
   
   
   • ModelNotFoundException should be thrown if User doesn't exist.
   
   

Current behavior:

1. For model-focused call function (User $user):
   
   
   
   • Existing User instance if the User is found. ✔
   
   
   • Empty User model if User is skipped (when we hit /people, but let's don't hit people, people). ✔
   
   
   • ModelNotFoundException is thrown if User doesn't exist. ✘
   
   
2. For instance-focused call function (User $user = null):
   
   
   
   • Existing User instance if the User is found. ✔
   
   
   • Empty User model if User is skipped. ✘
   
   
   • Empty User model if User doesn't exist. ✘
   
   

[MightyPork]

Just run into this issue while switching from my own ugly resolution to route-model binding.

I'm in favor of it being null, not "empty model". That's what I expected anyway, before discovering part of my app stopped working (yea unit testing, I know, I know..).

Checking for ->exists is far from elegant.

For now, using my helper workaround

/**
 * Helper to check if model exists.
 *
 * @param \Illuminate\Database\Eloquent\Model|null $model
 * @return bool
 */
function model_exists(Illuminate\Database\Eloquent\Model $model = null) {
    return $model !== null && $model->exists;
}

[projct1]

I have some problem in 5.3
Route: Route::get('order/{order}/sms/{sms?}', 'OrderController@sms')->name('order.sms');
Controller method:

    public function sms(Order $order, Sms $sms = null)
    {
        if (is_null($sms)) {
            $sms = $order->status->sms;
        }

        return $this->dispatch(new Replace($order, $sms->template));
    }

With request /order/1/sms expect $sms is null, but got empty Eloquent model (without attributes) :(

[alepeino]

I submitted a PR for this: #17521

[GrahamCampbell]

Thanks. Discussion can take place on the PR now, if needed.