How to refactor a fat Laravel controller into actions

Controllers do not start fat. They start with one job, and then the same operation needs a variant, and the variant is three lines, and three lines never justify a new class. Twenty of those later you have a method nobody wants to open.

Here is the one we are going to take apart. It publishes a property listing:

public function store(StorePropertyRequest $request)
{
    $property = Property::create($request->validated());

    foreach ($request->file('photos', []) as $photo) {
        $path = $photo->store("properties/{$property->id}", 'public');
        $property->photos()->create(['path' => $path]);
    }

    $matches = Inquiry::query()
        ->where('city', $property->city)
        ->where('max_price', '>=', $property->price)
        ->get();

    foreach ($matches as $inquiry) {
        Mail::to($inquiry->email)->send(new MatchFound($property, $inquiry));
    }

    foreach (config('portals.enabled') as $portal) {
        app(PortalClient::class)->push($portal, $property);
    }

    Log::info('Property published', ['id' => $property->id]);

    return redirect()->route('properties.show', $property);
}

Nothing here is wrong. It is just that six decisions are sharing one method, and only one of them has anything to do with HTTP.

Find the operations, not the lines

The temptation is to cut by size: take the longest loop out and call it a day. That gives you a class named after a chunk of code rather than after a thing your business does.

The seam is easier to find out loud. Describe what the method does and listen for where you say "and then": create the listing, and then attach the photos, and then notify the matching inquiries, and then push it to the portals. Each of those is one decision with its own reasons to change, and each is an action.

The redirect is not. Neither is reading the form. Those are the controller's actual job, and they are what should be left behind.

Move the first one out

php artisan make:action CreateProperty

The generated class has one method. Move the code into it and make the signature say what the operation needs:

namespace App\Actions;

use EduLazaro\Laractions\Action;
use App\Models\Property;

class CreateProperty extends Action
{
    public function handle(array $attributes): Property
    {
        return Property::create($attributes);
    }
}

Two things worth doing on the way in.

Return something. An action is allowed to have a result, and run() gives you back whatever handle() returns. An action that returns the model it created is one a controller can use for a redirect and a command can use for a progress bar.

Take arguments, not the request. Passing $request into an action ties it to HTTP forever, which is exactly the knot you are trying to cut. Take the data.

The call is then this, and it reads the same from anywhere:

$property = CreateProperty::create()->run($request->validated());

create() resolves the action through the container, so anything you type hint on its constructor is injected. run() maps its arguments to handle() by name, so all three of these do the same thing:

$action->run($attributes);                          // an array, whole
$action->run(attributes: $attributes);              // named
CreateProperty::create()->run($request->validated());

Give it rules, so it holds outside the request

The form request was guarding this operation, and the moment a console command calls the same class the guard is gone. Put it on the action, keyed by the parameter names of handle():

class PushToPortals extends Action
{
    protected array $rules = [
        'portal' => 'required|string|in:idealista,fotocasa',
    ];

    public function handle(string $portal): void
    {
        // ...
    }
}

Validation runs before handle() whatever called it, and a failure throws a ValidationException, which a controller turns into a 422 for free. This is the part that makes an action safe to call from a scheduled task at four in the morning.

The ones that belong to a record

Three of our four operations are not about properties in general, they are about this property. Generate those bound to the model:

php artisan make:action PushToPortals --model=Property

That writes the class into App\Actions\Property with the property already declared:

namespace App\Actions\Property;

use EduLazaro\Laractions\Action;
use App\Models\Property;

class PushToPortals extends Action
{
    protected Property $property;

    public function handle(): void
    {
        foreach (config('portals.enabled') as $portal) {
            app(PortalClient::class)->push($portal, $this->property);
        }
    }
}

Add the trait to the model and register the ones it owns:

use EduLazaro\Laractions\Concerns\HasActions;

class Property extends Model
{
    use HasActions;

    protected array $actions = [
        'attach_photos'    => AttachPhotos::class,
        'notify_matches'   => NotifyMatchingInquiries::class,
        'push_to_portals'  => PushToPortals::class,
    ];
}

Now $property->action('push_to_portals')->run() resolves the class, injects the model into $this->property and runs it. The map is optional, you can pass the class name instead, but it earns its place fast: that array is the honest answer to "what can happen to a property", and it is a list of classes you can open one by one.

Actions calling actions

Composition is the point where this stops being file shuffling and starts paying. An action calling another action is normal:

class PublishProperty extends Action
{
    public function handle(array $attributes, array $photos = []): Property
    {
        $property = CreateProperty::create()->run($attributes);

        $property->action('attach_photos')->run($photos);
        $property->action('notify_matches')->run();
        $property->action('push_to_portals')->run();

        return $property;
    }
}

And the controller goes back to being a controller:

public function store(StorePropertyRequest $request)
{
    $property = PublishProperty::create()->run(
        $request->validated(),
        $request->file('photos', [])
    );

    return redirect()->route('properties.show', $property);
}

The importer that used to duplicate forty lines now calls PublishProperty too, and so does the test, which is the whole reason for doing this.

What not to extract

The refactor goes wrong in a predictable way: everything becomes an action, including things that were fine.

• A one liner is not an operation. $property->update(['featured' => true]) does not need a class. If the name of the action would be the name of the method it calls, leave it.
• A query is not an action. Selecting the matching inquiries belongs on a scope or in a query class. The action is what you do with them.
• Reading the form is not an action. Form requests already exist and are good at it.

The test is whether the class has a reason to change that the caller does not. Pushing to portals changes when a portal changes its API, and no controller cares. That is an action.

What it bought

The same six decisions, in six places that each fit on a screen, none of which know what an HTTP request is. The command reuses them, the test builds them directly, and the operation that used to exist in three drifting copies exists once.

And the day the portal push starts taking four seconds, it moves onto a queue by changing one word, without touching the class. That one is the next post.

The full API is in the Laractions documentation.