The Job And The Controller Disagree

A support ticket says a customer got refunded outside the refund window. You check the endpoint and the endpoint is right. It rejects anything past thirty days, there is a test for it, the test passes.

Then you find out the refund did not come through the endpoint. It came through the bulk refund tool that operations run after an incident, which is a console command that dispatches a job, and that job has its own copy of the rule. The copy says > where the endpoint says >=, and it has said that since March.

Nobody wrote a bug. Somebody needed the same operation from a second place, and the cheapest way to get it was to copy the body of the controller.

How it happens, every time

The sequence is always the same and there is no point in the story where anyone does anything unreasonable.

A feature gets built as an endpoint, because that is what was asked for. The decision goes in the controller, or in an action the controller calls, and validation goes in a form request because that is where validation goes.

Six months later someone needs the same operation without a browser. Bulk refunds after an incident, a webhook from the payment provider, a scheduled retry, an admin panel that runs as a different guard. They open the controller, read what it does, and write a job that does the same thing.

They cannot call the controller. Nothing calls a controller. So they copy the parts they can see.

The parts they can see are the body of the method. The parts they cannot see are the form request, which never runs outside HTTP, and the policy call, which they did not notice because authorize() is one word in the middle of a line.

That is the shape of the bug, and it is why this one is specific to frameworks that put validation at the edge. Laravel is very good at making the edge comfortable. A form request is the right place for validation, right up until the operation acquires a second entry point that does not have one.

Counting the edges

The measurement from part one was how many ways there are to run a piece of logic. Here you run it in the other direction: pick an operation and count the places it can start.

# every entry point that can begin a refund
grep -rn "RefundOrder\|refundOrder\|->refund(" app/ routes/ --include=*.php

You are looking for the operation to appear once as a definition and n times as a call. What you often find instead is that it appears n times as an implementation.

The faster version of the same question is to pick a side effect rather than the operation, because side effects are easier to grep for and they are usually the thing that drifts.

# who fires this, and under what condition
grep -rn "OrderRefunded::dispatch\|new OrderRefunded" app/

If that returns three call sites, then the rule about when an order counts as refunded is written in three places, and the useful exercise is reading the five lines above each one and checking they agree. They usually agree on the happy path and disagree about one edge, which is exactly the case nobody wrote a test for.

What the two paths actually contain

Here is the endpoint, and it is fine.

final class RefundController
{
    public function store(StoreRefundRequest $request, Order $order): JsonResponse
    {
        $this->authorize('refund', $order);

        $refund = $this->refundOrder->handle(
            order: $order,
            amount: $request->integer('amount'),
        );

        return RefundResource::make($refund)->response();
    }
}

Three things happen before the operation runs: the form request validates, the policy authorises, and the input is coerced to an integer. Only the third of those is visible in the method body.

Now the job, written six months later by someone reading that method body.

final class BulkRefundOrder implements ShouldQueue
{
    public function __construct(
        private readonly Order $order,
        private readonly int $amount,
    ) {}

    public function handle(RefundOrder $refundOrder): void
    {
        if ($this->order->purchased_at->diffInDays(now()) > 30) {
            return;
        }

        $refundOrder->handle(order: $this->order, amount: $this->amount);
    }
}

It calls the same action, which is better than a lot of what you will find in the wild. And it is still wrong in three ways.

The window check is a reimplementation, and > 30 is not the same as the form request’s <= 30, so an order refunded on day thirty behaves differently depending on which door it came through. There is no policy check, so the job will refund an order the endpoint would have refused. And it returns silently when the window has closed, so operations gets no error and assumes it worked.

None of that is visible from either file alone. You have to read both, side by side, knowing to look.

The rule the framework will not enforce

The fix is that both edges call the same thing, and the thing they call contains the whole operation rather than the interesting part of it.

final readonly class RefundOrder
{
    public function __construct(
        private RefundWindow $window,
    ) {}

    /** @throws RefundWindowClosed */
    public function handle(Order $order, int $amount, DateTimeImmutable $now): Refund
    {
        if (! $this->window->isOpenFor($order->purchased_at->toDateTimeImmutable(), $now)) {
            throw new RefundWindowClosed($order->id);
        }

        // ...
    }
}

The window check moved inside the operation. Now there is one place that knows the rule, both edges get it whether they remember to or not, and the console command fails loudly instead of returning silently, because an exception is harder to ignore than an early return.

Validation is the part people get stuck on. A form request is still the right place for shape. Is amount present, is it an integer, is it positive. That is about the HTTP payload and it belongs at the HTTP edge. What does not belong there is anything a second entry point would also need to be true, and “the refund window is open” is a fact about the order, not about the request.

The line is easier to find than it sounds. If a rule would still be a rule when the request does not exist, it is not validation.

Authorisation is the same shape with a worse failure mode, because $this->authorize() in a controller is invisible from everywhere else. If an operation must not run for a given actor, the operation should say so, and the controller calling authorize() first is a nicety that produces a better status code.

When they are supposed to disagree

This is the part that matters, because the obvious fix here can destroy a real requirement.

Sometimes the second entry point is deliberately different. An admin refunding on behalf of a customer after a complaint is supposed to be able to go past thirty days. That is not drift, that is a business rule, and merging the two paths would be the bug rather than the fix.

The problem in that case was never the duplication. It was that the difference was implicit. Nothing anywhere said “administrators may refund outside the window”, it was just a > that happened to be in a file that only administrators reached.

So the fix looks different. Instead of one path, you get one operation with the difference named.

$refundOrder->handle(
    order: $order,
    amount: $amount,
    now: $now,
    policy: RefundPolicy::AdministrativeOverride,
);

Now the two behaviours are still two behaviours, and a reader can find out that they exist without diffing two files. Somebody deciding whether to add a third entry point can see what their options are.

The test for which situation you are in: ask whether anyone chose the difference. If someone can tell you why the job is more permissive, name it. If nobody knows, it is drift, and it has probably been wrong since the day it was copied.

What it costs

Moving a rule inside an operation costs you the ability to return a nice validation error without catching something. A form request gives you a 422 with a field-level message for free, and an exception thrown three layers in gives you a 500 unless you map it.

That mapping is a real cost. It is a handler entry, or a rendered exception, and it is one more thing to remember. In Laravel it is a few lines in bootstrap/app.php, and you do it once per operation family rather than once per rule.

What you get back is that the rule cannot be bypassed by adding an entry point, which is the failure that produced the ticket at the top of this article.

What to do on Monday

Pick the operation your business cares most about. Refunds, publishing, provisioning, whatever the thing is that would generate a phone call if it happened when it should not have.

Grep for its side effect. Count the call sites. Read the five lines above each one and check they agree.

If they disagree, you have found something, and the first question is not how to merge them. It is whether anyone chose the difference.

Next in this series: the interface with one implementation, and the uncomfortable question of whether the abstraction you added last year has earned anything yet.