Wolf-Tech Logo
Back to Blog

Symfony Refactoring Patterns: How to Improve a Running Codebase Without Stopping Feature Work

#symfony refactoring
Sandor Farkas - Founder & Lead Developer at Wolf-Tech

Sandor Farkas

Founder & Lead Developer

Expert in software development and legacy code optimization

Most Symfony refactoring advice quietly assumes a luxury nobody has: a quiet sprint, a feature freeze, and permission to break things while you sort it out. Real projects do not work that way. The codebase that needs the work is the one currently serving paying customers, and the roadmap does not pause while you clean it up.

This post is about the other kind of refactoring. Not a version upgrade, not a rewrite, but structural improvement inside a stable Symfony version, applied to code that has to keep shipping. Five patterns, each with the before, the after, and the reason the existing shape causes problems. One constraint runs through all of them: every step has to leave the application deployable. If a refactoring cannot be merged and released on a Tuesday afternoon, it is not a refactoring, it is a branch that will rot.

Pattern 1: Pull fat controllers into invokable service classes

The most common shape in an aging Symfony application is a controller action that grew a body. It loads an entity, validates a few rules, calls a gateway, mutates state, flushes, sends an email, and redirects.

#[Route('/orders/{id}/refund', methods: ['POST'])]
public function refund(
    int $id,
    Request $request,
    EntityManagerInterface $em,
    MailerInterface $mailer,
    PaymentGateway $gateway,
): Response {
    $order = $em->getRepository(Order::class)->find($id);
    if (!$order) {
        throw $this->createNotFoundException();
    }
    if ($order->getStatus() !== 'paid') {
        $this->addFlash('error', 'Only paid orders can be refunded.');

        return $this->redirectToRoute('order_show', ['id' => $id]);
    }

    $amount = (int) $request->request->get('amount');
    if ($amount <= 0 || $amount > $order->getTotal()) {
        $this->addFlash('error', 'Invalid refund amount.');

        return $this->redirectToRoute('order_show', ['id' => $id]);
    }

    $gateway->refund($order->getPaymentReference(), $amount);
    $order->setStatus($amount === $order->getTotal() ? 'refunded' : 'partially_refunded');
    $em->flush();
    $mailer->send($this->buildRefundMail($order, $amount));

    return $this->redirectToRoute('order_show', ['id' => $id]);
}

The problem is not aesthetic. It is that the refund rules are now only reachable through an HTTP request. Testing them means booting the kernel, building a request, and providing a session so addFlash does not explode. When the same refund has to happen from a console command or a payment provider webhook, nobody untangles this; they copy it, and now the rules live in two places that drift.

The extraction is mechanical. Move the decision logic into an invokable service and let it signal failure with exceptions instead of flash messages.

final class RefundOrder
{
    public function __construct(
        private OrderRepository $orders,
        private PaymentGateway $gateway,
        private EntityManagerInterface $em,
    ) {
    }

    public function __invoke(int $orderId, int $amount): Order
    {
        $order = $this->orders->find($orderId) ?? throw new OrderNotFound($orderId);

        if (!$order->isRefundable()) {
            throw new OrderNotRefundable($orderId, $order->getStatus());
        }
        if ($amount <= 0 || $amount > $order->getTotal()) {
            throw new InvalidRefundAmount($amount);
        }

        $this->gateway->refund($order->getPaymentReference(), $amount);
        $order->recordRefund($amount);
        $this->em->flush();

        return $order;
    }
}

The controller keeps only what is genuinely HTTP: reading input, catching domain exceptions, and turning them into flashes and redirects. The service is now unit testable with three mocks and no kernel, which usually cuts the runtime of that test from a second to a millisecond.

Do this one action at a time. Every other route keeps working exactly as before, so the change is releasable the moment it is green.

Pattern 2: Replace parameter soup with value objects

Long positional signatures are a reliable source of production incidents, because the compiler will happily let you swap two arguments of the same scalar type.

public function createInvoice(
    int $customerId,
    string $currency,
    int $netAmount,
    int $taxRate,
    ?string $vatId,
    bool $reverseCharge,
    \DateTimeImmutable $issuedAt,
): Invoice {

Every call site repeats the same validation, or skips it. The rule that a reverse charge invoice needs a VAT ID lives nowhere in particular, which means it lives in whichever caller happened to remember.

Push the invariants into types that cannot be constructed in an invalid state.

final readonly class Money
{
    public function __construct(
        public int $amount,
        public string $currency,
    ) {
        if ($amount < 0) {
            throw new \InvalidArgumentException('Money cannot be negative.');
        }
        if (!preg_match('/^[A-Z]{3}$/', $currency)) {
            throw new \InvalidArgumentException("Invalid currency: {$currency}");
        }
    }
}

final readonly class TaxTreatment
{
    public function __construct(
        public int $ratePercent,
        public ?string $vatId,
        public bool $reverseCharge,
    ) {
        if ($reverseCharge && null === $vatId) {
            throw new \InvalidArgumentException('Reverse charge requires a VAT ID.');
        }
    }
}

The signature collapses to createInvoice(CustomerId $customer, Money $net, TaxTreatment $tax, \DateTimeImmutable $issuedAt), and a whole class of bug becomes unrepresentable.

The part that makes this safe on a live codebase is the migration path. Do not update forty call sites in one commit. Keep the old signature as a thin deprecated wrapper that builds the value objects and delegates.

/**
 * @deprecated Use createInvoice() with value objects instead.
 */
public function createInvoiceLegacy(int $customerId, string $currency, int $netAmount, /* ... */): Invoice
{
    return $this->createInvoice(
        new CustomerId($customerId),
        new Money($netAmount, $currency),
        new TaxTreatment($taxRate, $vatId, $reverseCharge),
        $issuedAt,
    );
}

New code uses the new signature immediately. Old call sites move in whatever commits touch them anyway. When a grep for createInvoiceLegacy comes back empty, delete the wrapper. Three separate releases, none of them risky.

Pattern 3: Move business logic out of Doctrine entities

Entities that reach out to services are the reason a test suite takes eleven minutes.

class Subscription
{
    public function calculateRenewalPrice(
        PriceListRepository $prices,
        DiscountService $discounts,
    ): int {
        $base = $prices->findForPlan($this->plan)->getAmount();

        return $discounts->apply($base, $this->customer->getTier());
    }
}

An entity that needs a repository passed in to answer a question about itself is not really an entity method, it is a service with an awkward calling convention. Every test that exercises pricing now needs the persistence layer, and the entity cannot be constructed in a test without dragging half the container behind it.

Split it by asking what the object can answer from its own state. isRefundable(), recordRefund(), and isWithinTrial() stay: they read and mutate fields the entity already owns. Anything that needs to look at other aggregates moves out.

final class RenewalPricer
{
    public function __construct(
        private PriceListRepository $prices,
        private DiscountCalculator $discounts,
    ) {
    }

    public function priceFor(Subscription $subscription): Money
    {
        $base = $this->prices->findForPlan($subscription->getPlan())->amount();

        return $this->discounts->apply($base, $subscription->getCustomerTier());
    }
}

The existing tests are the obstacle here, and there is a specific trick for not breaking them. Before extracting, write a characterization test that pins the current output for a spread of inputs, including the ugly edge cases nobody documented. Then extract. Then leave the old entity method in place, delegating to the new service, so every existing caller and every existing test stays green.

/**
 * @deprecated Use RenewalPricer::priceFor().
 */
public function calculateRenewalPrice(PriceListRepository $prices, DiscountService $discounts): int
{
    return (new RenewalPricer($prices, $discounts))->priceFor($this)->amount;
}

The delegation is temporary and slightly ugly, and that is fine. It buys you a green build at every commit, which is the whole point. This kind of surgical separation is the core of most legacy code optimization work we do, and the characterization test is almost always the first thing written.

Pattern 4: Introduce Command and Handler incrementally with Messenger

Teams usually treat the move to a command bus as an architecture decision requiring a big meeting. It does not have to be. Symfony Messenger dispatches synchronously by default, so introducing a message and a handler is a purely structural change with identical runtime behaviour. You decide later, per message class, whether it goes async.

Start with the operations that are slow or that you want to retry: emails, exports, third-party API calls.

final readonly class SendRefundNotification
{
    public function __construct(
        public int $orderId,
        public int $amount,
    ) {
    }
}

#[AsMessageHandler]
final class SendRefundNotificationHandler
{
    public function __construct(
        private OrderRepository $orders,
        private MailerInterface $mailer,
    ) {
    }

    public function __invoke(SendRefundNotification $message): void
    {
        $order = $this->orders->find($message->orderId);
        if (null === $order) {
            return;
        }

        $this->mailer->send(RefundMail::for($order, $message->amount));
    }
}

Merge it running synchronously. Behaviour is unchanged, the deployment is boring, and the handler is now independently testable. When you are ready, one line of configuration moves it off the request path:

framework:
    messenger:
        transports:
            async:
                dsn: '%env(MESSENGER_TRANSPORT_DSN)%'
                retry_strategy:
                    max_retries: 3
                    delay: 1000
                    multiplier: 2
        routing:
            App\Message\SendRefundNotification: async

Two things to get right before flipping that switch. Messages must carry identifiers rather than entity objects, because a detached entity will not survive serialization. And handlers must be idempotent, because a retry means the handler runs twice on the same message. If the second run would send a second email or refund a second time, add a guard keyed on something stable before you route it to a queue.

Pattern 5: Replace hardcoded dependencies with tagged interfaces

A service that knows every implementation by name has to be edited every time a new one appears.

public function export(string $format, Report $report): string
{
    return match ($format) {
        'csv' => $this->csvExporter->export($report),
        'xlsx' => $this->xlsxExporter->export($report),
        'pdf' => $this->pdfExporter->export($report),
        default => throw new \InvalidArgumentException($format),
    };
}

Adding a format touches the constructor, the match, the service definition, and every test double that has to satisfy the constructor. That last one is why nobody wants to add the format.

Define the contract, let autoconfiguration collect the implementations, and inject them as an iterator.

interface ReportExporter
{
    public function supports(string $format): bool;

    public function export(Report $report): string;
}

final class ReportExporterRegistry
{
    /**
     * @param iterable<ReportExporter> $exporters
     */
    public function __construct(
        #[AutowireIterator('app.report_exporter')]
        private iterable $exporters,
    ) {
    }

    public function get(string $format): ReportExporter
    {
        foreach ($this->exporters as $exporter) {
            if ($exporter->supports($format)) {
                return $exporter;
            }
        }

        throw new UnsupportedExportFormat($format);
    }
}

services:
    _instanceof:
        App\Export\ReportExporter:
            tags: ['app.report_exporter']

The incremental route: first make the three existing exporters implement the interface, which changes nothing because the old match still calls them directly. Then add the registry. Then switch the single caller. Then delete the match. Four small commits, each independently shippable, and a new export format afterwards is one new class and zero edits elsewhere.

The discipline that makes Symfony refactoring safe

The patterns matter less than the rules you apply while using them. Four have earned their place in every engagement:

Separate structural and behavioural commits. A commit either moves code or changes what it does, never both. When something breaks in production, the difference between reviewing a pure move and reviewing a move plus a logic tweak is the difference between a five minute diagnosis and an hour of it.

Characterize before you extract. Untested code is not a reason to skip refactoring, it is a reason to write the test that pins current behaviour first, bugs included. You are preserving behaviour, not blessing it. Fix the bug in a later, clearly labelled commit.

Deprecate, migrate, delete, as three releases. Nothing gets removed in the same release that replaces it. This is what lets you stop mid-migration without leaving the codebase broken, which matters because you will get pulled onto something urgent mid-migration.

Ratchet with static analysis. Generate a PHPStan baseline, commit it, and make CI fail if it grows. The existing debt stays acknowledged rather than fixed all at once, and new debt cannot be added. Over a few months the baseline shrinks on its own as people touch files. A code quality review is often just this: finding where the ratchet should be set and what to fix first.

Where to start

Pick the file your team complains about most, not the one that scores worst on a metric. Complaint frequency is a better proxy for cost than cyclomatic complexity, because it tells you where the code actually slows people down. Apply the smallest pattern that fits, ship it, and see whether the next change in that area is easier. If it is not, you extracted the wrong seam, and you have lost an afternoon rather than a quarter.

Refactoring at this granularity is undramatic by design. There is no rewrite branch, no migration weekend, and no slide deck asking for a feature freeze. There is just a codebase that gets slightly easier to work in every week, while the roadmap keeps moving.

If you are looking at a Symfony codebase that has become expensive to change and you want a second opinion on which seams to cut first, that is the kind of read we do at Wolf-Tech. Write to hello@wolf-tech.io or have a look at wolf-tech.io, and we will tell you where the leverage is.