Same Laravel resource controller for multiple routes

Question

I am trying to use a trait as a typehint for my Laravel resource controllers.

The controller method:

public function store(CreateCommentRequest $request, Commentable $commentable)

In which the Commentable is the trait typehint which my Eloquent models use.

The Commentable trait looks like this:

namespace App\Models\Morphs;

use App\Comment;

trait Commentable
{
   /**
    * Get the model's comments.
    *
    * @return \Illuminate\Database\Eloquent\Relations\MorphMany
    */
    public function Comments()
    {
        return $this->morphMany(Comment::class, 'commentable')->orderBy('created_at', 'DESC');
    }
}

In my routing, I have:

Route::resource('order.comment', 'CommentController')
Route::resource('fulfillments.comment', 'CommentController')

Both orders and fulfillments can have comments and so they use the same controller since the code would be the same.

However, when I post to order/{order}/comment, I get the following error:

Illuminate\Contracts\Container\BindingResolutionException
Target [App\Models\Morphs\Commentable] is not instantiable.

Is this possible at all?

Looks like automatic resolving your Dependency failed. Please provide code of your Commentable file. — manniL
@manniL, I've updated the description with the content of Commentable — andershagbard
Well, why don't you use a real "Comment" model instead? That would work :D — manniL
Yeah, but if you use a Polymorphic Relationship it shouldn't be a problem :) — manniL

sepehr sepehr · Accepted Answer · 2017-09-09T12:13:09

So you want to avoid duplicate code for both order and fulfillment resource controllers and be a bit DRY. Good.

Traits cannot be typehinted

As Matthew stated, you can't typehint traits and that's the reason you're getting the binding resolution error. Other than that, even if it was typehintable, the container would be confused which model it should instantiate as there are two Commentable models available. But, we'll get to it later.

Interfaces alongside traits

It's often a good practice to have an interface to accompany a trait. Besides the fact that interfaces can be typehinted, you're adhering to the Interface Segregation principle which, "if needed", is a good practice.

interface Commentable 
{
    public function comments();
}

class Order extends Model implements Commentable
{
    use Commentable;

    // ...
}

Now that it's typehintable. Let's get to the container confusion issue.

Contexual binding

Laravel's container supports contextual binding. That's the ability to explicitly tell it when and how to resolve an abstract to a concrete.

The only distinguishing factor you got for your controllers, is the route. We need to build upon that. Something along the lines of:

# AppServiceProvider::register()
$this->app
    ->when(CommentController::class)
    ->needs(Commentable::class)
    ->give(function ($container, $params) {
        // Since you're probably utilizing Laravel's route model binding, 
        // we need to resolve the model associated with the passed ID using
        // the `findOrFail`, instead of just newing up an empty instance.

        // Assuming this route pattern: "order|fullfilment/{id}/comment/{id}"
        $id = (int) $this->app->request->segment(2);

        return $this->app->request->segment(1) === 'order'
            ? Order::findOrFail($id)
            : Fulfillment::findOrFail($id);
     });

You're basically telling the container when the CommentController requires a Commentable instance, first check out the route and then instantiate the correct commentable model.

Non-contextual binding will do as well:

# AppServiceProvider::register()
$this->app->bind(Commentable::class, function ($container, $params) {
    $id = (int) $this->app->request->segment(2);

    return $this->app->request->segment(1) === 'order'
        ? Order::findOrFail($id)
        : Fulfillment::findOrFail($id);
});

Wrong tool

We've just eliminated duplicate controller code by introducing unnecessary complexity which is as worse as that. 👍

Even though it works, it's complex, not maintainable, non-generic and worst of all, dependent to the URL. It's using the wrong tool for the job and is plain wrong.

Inheritance

The right tool to eliminate these kinda problems is simply inheritance. Introduce an abstract base comment controller class and extend two shallow ones from it.

# App\Http\Controllers\CommentController
abstract class CommentController extends Controller
{
    public function store(CreateCommentRequest $request, Commentable $commentable) {
        // ...
    }

    // All other common methods here...
}

# App\Http\Controllers\OrderCommentController
class OrderCommentController extends CommentController
{
    public function store(CreateCommentRequest $request, Order $commentable) {
        return parent::store($commentable);
    }
}

# App\Http\Controllers\FulfillmentCommentController
class FulfillmentCommentController extends CommentController
{
    public function store(CreateCommentRequest $request, Fulfillment $commentable) {
        return parent::store($commentable);
    }
}

# Routes
Route::resource('order.comment', 'OrderCommentController');
Route::resource('fulfillments.comment', 'FulfillCommentController');

Simple, flexible and maintainable.

Arrrgh, wrong language

Not so fast:

Declaration of OrderCommentController::store(CreateCommentRequest $request, Order $commentable) should be compatible with CommentController::store(CreateCommentRequest $request, Commentable $commentable).

Even though overriding method parameters works in the constructors just fine, it simply does not for other methods! Constructors are special cases.

We could just drop the typehints in both parent and child classes and go on with our lives with plain IDs. But in that case, as Laravel's implicit model binding only works with typehints, there won't be any automatic model loading for our controllers.

Ok, maybe in a better world.

🎉Update: See PHP 7.4's support for type variance 🎉

Explicit route model binding

So what we gonna do?

If we explicitly tell the router how to load our Commentable models, we can just use the lone CommentController class. Laravel's explicit model binding works by mapping route placeholders (e.g. {order}) to model classes or custom resolution logics. So, while we're using our single CommentController we can utilize separate models or resolution logics for orders and fulfillments based on their route placeholders. So, we drop the typehint and rely on the placeholder.

For resource controllers, the placeholder name depends on the first parameter you pass to the Route::resource method. Just do a artisan route:list to find out.

Ok, let's do it:

# App\Providers\RouteServiceProvider::boot()
public function boot()
{
    // Map `{order}` route placeholder to the \App\Order model
    $this->app->router->model('order', \App\Order::class);

    // Map `{fulfillment}` to the \App\Fulfilment model
    $this->app->router->model('fulfillment', \App\Fulfilment::class);

    parent::boot();
}

Your controller code would be:

# App\Http\Controllers\CommentController
class CommentController extends Controller
{
    // Note that we have dropped the typehint here:
    public function store(CreateCommentRequest $request, $commentable) {
        // $commentable is either an \App\Order or a \App\Fulfillment
    }

    // Drop the typehint from other methods as well.
}

And the route definitions remain the same.

It's better than the first solution, as it does not rely on the URL segments which are prone to change contrary to the route placeholders which rarely change. It's also generic as all {order}s will be resolved to \App\Order model and all {fulfillment}s to the App\Fulfillment.

We could alter the first solution to utilize route parameters instead of URL segments. But there's no reason to do it manually when Laravel has provided it to us.

Yeah, I know, I don't feel good, too.