-
Notifications
You must be signed in to change notification settings - Fork 253
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow for abstract Model typed route params #445
Conversation
This prevents an `Illuminate\Contracts\Container\BindingResolutionException` with message `Target [Illuminate\Database\Eloquent\Model] is not instantiable.` when the route parameter is typed as the abstract `Illuminate\Database\Eloquent\Model`.
That error doesn't come from Ziggy, it comes from Laravel (specifically here). I get the error even in a new Laravel app without Ziggy installed. In order to perform its route model binding, Laravel's router will try to instantiate the class used in the route parameter type, which it can't in this case because |
class MyController extends Controller
{
public function edit(Model $model, Model $modelId)
{
//
}
}
Route::bind('model', function (string $kebabCasedModelName): Model {
$modelClass = $this->kebabCasedModelNames()->search($kebabCasedModelName);
if ( ! $modelClass || ! is_subclass_of($modelClass, Model::class)) {
throw new Exception("Can't find model for `$kebabCasedModelName`.");
}
return new $modelClass();
});
Route::bind('model_id', function (string $id, Route $route): Model {
if ( ! $modelClass = $route->parameter('model')) {
throw new Exception("Route parameter `{model}` should be present for using `{model_id}`.");
}
return $modelClass::findOrFail($id);
}); Apart from this example, I think the error will also occur when you have multiple routes point to a controller method as described in this PR description. |
I want to make sure I'm understanding what those bindings are doing—the first one is a kebab-cased model class name and the second one is a model ID? So your route would look something like Can you share your route definition too? I set that snippet up locally and I actually can't reproduce your error at all now. Ziggy works as expected with that controller method and those bindings, the output in the config object looks like this: {
edit: {
uri: 'edit/{model}/{modelId}',
methods: ['GET', 'HEAD'],
bindings: {
model: 'id',
modelId: 'id',
},
},
} I'm curious why you set up the route model binding manually like that instead of using real model classes for the types. Wouldn't something like this do the same thing as your example? Route::get('blogPosts/{blogPost}/edit', [BlogPostController::class, 'edit']);
// ...
class BlogPostController
{
public function edit(BlogPost $blogPost)
{
//
}
}
That works fine for me locally, I don't think it matters if routes share a controller. |
Route configuration: Route::name('foo.bar')
->get('/{model}/{model_id}', function (\Illuminate\Database\Eloquent\Model $model, \Illuminate\Database\Eloquent\Model $modelId) {
//
});
$this->bind('model', function (string $kebabCasedModelName): Model {
//
});
$this->bind('model_id', function (string $id, \Illuminate\Routing\Route $route): Model {
//
}); You can leave the callbacks empty as it will never reach those points when calling |
Gotcha, thanks! I added a test and used |
Even better! 👍 Thank you for reviewing! |
This prevents an
Illuminate\Contracts\Container\BindingResolutionException
with messageTarget [Illuminate\Database\Eloquent\Model] is not instantiable.
when the route parameter is typed as the abstractIlluminate\Database\Eloquent\Model
: