Skip to content
Mohammed Kharbotli

~/blog/the-first-refactor-i-make-in-a-legacy-php-application

The First Refactor I Make in a Legacy PHP Application

A practical approach to the first refactor in a legacy PHP application: isolate important behavior, preserve existing semantics, add focused tests, and create a safer starting point for future changes.

Published
Reading time
5 min

When I inherit a legacy PHP application, I usually find several things I would like to change.

Large scripts. Global database connections. Repeated queries. Business rules buried between request handling and HTML.

The temptation is to start organizing.

Create services. Introduce repositories. Move files into a cleaner directory structure.

Before doing that, I look for a smaller opportunity:

Can I make one important piece of behavior easier to verify without changing what it does?

That is usually the first refactor I want to make.

It gives me evidence about the application and a safer starting point for the next change.

1. I choose a place where change already hurts

I rarely start with the longest file simply because it is long.

I look for a business operation that is frequently changed, difficult to test, or responsible for recurring defects.

Then I ask the team where they hesitate.

“Every cancellation change seems to affect the admin panel” is useful information. It suggests shared behavior, hidden dependencies, or several implementations of the same rule.

I trace that operation before choosing the extraction.

For example, a cancellation endpoint might contain this:

session_start();
 
require __DIR__ . '/bootstrap.php';
 
$orderId = (int) $_POST['order_id'];
$actorId = (int) $_SESSION['user_id'];
 
$order = loadOrder($db, $orderId);
 
if (
    (int) $order['customer_id'] !== $actorId
    || $order['status'] !== 'pending'
    || $order['payment_status'] === 'captured'
) {
    http_response_code(409);
    exit('Order cannot be cancelled');
}
 
cancelOrder($db, $orderId);
 
header('Location: /orders.php', true, 303);
exit;

This is a shortened example; surrounding authentication, CSRF protection, and missing-record handling are omitted.

The decision is small. Testing it through this script requires session state, request input, bootstrap behavior, and a database.

That is a useful place to establish a boundary.

2. I separate the decision from the request

My first extraction might be a function:

function canCancelOrder(array $order, int $actorId): bool
{
    if ((int) $order['customer_id'] !== $actorId) {
        return false;
    }
 
    if ($order['status'] !== 'pending') {
        return false;
    }
 
    if ($order['payment_status'] === 'captured') {
        return false;
    }
 
    return true;
}

The endpoint becomes:

$order = loadOrder($db, $orderId);
 
if (!canCancelOrder($order, $actorId)) {
    http_response_code(409);
    exit('Order cannot be cancelled');
}
 
cancelOrder($db, $orderId);

There is nothing especially sophisticated about the function.

That is part of its value.

Its inputs are visible. It has no database access, session dependency, redirects, or hidden writes. I can exercise the decision without executing the endpoint.

I keep the existing comparisons and coercion deliberately. Changing how customer IDs are represented, introducing enums, and revising cancellation rules would add separate questions to the same patch.

A class may become useful later. At this point, I want a boundary I can test.

3. I capture the behavior before improving it

Before switching the endpoint to the extracted function, I establish what the existing code does for the cases that matter. If endpoint tests exist, I use them. Otherwise, I capture a small set of request-level cases in an isolated environment.

Then I add focused tests around the extracted decision.

public function testOwnerCanCancelPendingUncapturedOrder(): void
{
    $order = [
        'customer_id' => '42',
        'status' => 'pending',
        'payment_status' => 'authorized',
    ];
 
    self::assertTrue(canCancelOrder($order, 42));
}
public function testAnotherCustomerCannotCancelOrder(): void
{
    $order = [
        'customer_id' => '42',
        'status' => 'pending',
        'payment_status' => 'authorized',
    ];
 
    self::assertFalse(canCancelOrder($order, 99));
}
public function testCapturedPaymentPreventsCancellation(): void
{
    $order = [
        'customer_id' => '42',
        'status' => 'pending',
        'payment_status' => 'captured',
    ];
 
    self::assertFalse(canCancelOrder($order, 42));
}

I also cover non-pending order states and the relevant payment states the application actually stores.

That last part matters.

The original condition rejects captured. It does not explicitly allow only authorized. Replacing it with an allowlist may be a good future change, but it would change behavior.

I want that difference confirmed with the people who understand the workflow.

Characterization tests document what exists. They do not automatically establish that every existing behavior is correct.

4. I keep the first patch narrow enough to explain

In this change, I would usually avoid also:

  • Replacing the database layer
  • Changing HTTP response codes
  • Renaming stored statuses
  • Introducing a dependency injection container
  • Reorganizing the entire cancellation workflow

Each may be worthwhile. Each also increases the number of possible explanations if something breaks.

My review description should be specific:

Extracts the existing cancellation decision into a function with explicit inputs. Preserves the current comparisons and response behavior. Adds coverage for ownership, order state, and payment state.

I also verify that the endpoint still calls the decision correctly.

Unit tests for the new function cannot catch an integration mistake such as passing the wrong actor ID or accidentally bypassing the check.

If another endpoint contains similar logic, I compare the behavior before replacing it. An administrator may legitimately have different permissions. Similar-looking code is not always the same business rule.

I am cautious about deduplication until I understand why the copies differ.

5. I state what the extraction has not proved

This code still has a concurrency problem:

$order = loadOrder($db, $orderId);
 
if (!canCancelOrder($order, $actorId)) {
    throw new OrderCannotBeCancelled();
}
 
cancelOrder($db, $orderId);

Another request could change the order after the read and before the write.

The extracted function evaluates a snapshot. It does not make that snapshot remain valid.

I would record that explicitly and investigate how persistence protects the transition.

A later change might introduce a conditional update or version check:

$updated = $orders->cancelIfVersionMatches(
    orderId: $orderId,
    expectedVersion: $order['version'],
);
 
if (!$updated) {
    throw new OrderChangedDuringCancellation();
}

This is illustrative. A version check is useful only if all relevant writers maintain the version and the update applies it atomically.

If cancellation depends on payment state stored elsewhere, that relationship also needs protection. Adding a version column to one table does not settle the whole workflow.

The first refactor has made the decision easier to inspect and test. That helps me reason about the next change without confusing it with a guarantee the system does not yet provide.

6. I use the new boundary to make the next change smaller

Suppose the business later allows cancellation of authorized payments but requires voiding the authorization.

I now have a clear place to examine eligibility and tests describing the current behavior.

I can then separate the next questions:

Who owns the authorization void? What happens if the provider times out? When should the order become cancelled? Can the operation be repeated safely?

Without the initial boundary, those decisions would be mixed with session setup, redirects, SQL, and output handling.

The benefit is practical: fewer moving parts need to change at once.

Sometimes the first useful boundary is a calculation. Sometimes it is a clock dependency, an external API call, or a function that currently reads globals.

I choose based on the change the application needs.

How I Judge Whether the Refactor Was Worth Doing

I ask whether another engineer can now understand and verify this behavior with less setup.

Can they see the inputs? Can they test a meaningful case without bootstrapping unrelated infrastructure? Can they identify what remains outside the guarantee?

A small extraction that answers those questions is useful progress.

My first refactor rarely makes the application look dramatically different. It gives the next change somewhere safer to start.