code quality and tooling

Reading and Reviewing PHP Diffs

A diff shows what changed between two versions of files. Reading diffs well is a core job skill because most professional PHP work is reviewed through pull requests.

The goal is not just to see that lines changed. The goal is to understand whether the behaviour changed safely, whether the change matches the task, and whether the proof is good enough.

Diff symbols

In a unified diff:

lines starting with - were removed
lines starting with + were added
unchanged lines provide context

Example:

-if ($subtotal > 5000) {
+if ($subtotal >= 5000) {
     return 500;
 }

This is a behaviour change. It changes the boundary from "more than GBP 50" to "GBP 50 or more".

Read the intention first

Before reviewing the details, understand the intended change. For example:

Fix discount so orders of GBP 50 or more receive GBP 5 off.

Then read the diff against that intention. The comparison operator change above matches the stated fix.

Look for boundary changes

Many PHP bugs hide in comparisons, empty checks, null checks, and array defaults.

-if ($quantity < 0) {
+if ($quantity <= 0) {
     throw new InvalidArgumentException('Quantity must be positive.');
 }

This changes whether 0 is allowed. That may be correct, but it needs a test or explanation.

Watch for accidental defaults

Defaults can be useful, but they can also hide bad data.

-$price = $product['price'];
+$price = $product['price'] ?? 0;

This prevents a missing-key warning, but it may silently turn invalid product data into a free product. If price is required, validation is better.

Separate formatting from behaviour

This diff is mostly formatting:

-function total($items){$total=0;foreach($items as $item){$total+=$item['price'];}return $total;}
+function total(array $items): int
+{
+    $total = 0;
+
+    foreach ($items as $item) {
+        $total += $item['price'];
+    }
+
+    return $total;
+}

It also adds types, so it is not purely formatting. A reviewer should check whether the added types match all call sites.

Check tests and verification

A behaviour change should usually include proof.

For the discount boundary fix, good verification might include:

PHP example

<?php

declare(strict_types=1);

var_dump(discountForSubtotal(4999));
var_dump(discountForSubtotal(5000));
var_dump(discountForSubtotal(5001));

// Prints:
// int(0)
// int(500)
// int(500)

In a real project, that proof might be a unit test instead of a manual script.

Review the whole file list

Before reading line-by-line, look at which files changed.

Ask:

Are the changed files expected for this task?
Are generated files included accidentally?
Did formatting touch unrelated files?
Did dependency files change?
Did configuration change?
Are tests or docs updated where needed?

An unexpected file is not always wrong, but it should be understood.

Red flags in PHP diffs

Look carefully when you see:

?? 0 or ?? '' added to required data
@ error suppression
broad catch (Throwable $e) without handling
htmlspecialchars() removed near HTML output
SQL or shell strings built by concatenation
exit added inside reusable functions
production config changed
.env or secrets added
large formatter diffs mixed with a small bug fix

These patterns are not automatically wrong, but they deserve attention.

What a useful review comment looks like

Good review comments are specific and tied to risk.

This defaults a missing price to 0. Is price optional here? If it is required, can we validate and reject the product instead so we do not create a free line item?

That comment explains the concern and suggests a safer direction.

What to remember

Read diffs by asking what changed, why it changed, what behaviour is affected, what edge cases exist, and how the change was checked. In PHP, pay special attention to input validation, missing array keys, escaping, type changes, and error handling.

Before moving on, make sure you can identify whether a diff is formatting-only, behaviour-changing, or a mix of both.

Practice

Task: Review A Risky Default

Review this diff.

 function productPrice(array $product): int
 {
-    return $product['price'];
+    return $product['price'] ?? 0;
 }

The task says: "Stop warnings when a product price is missing."

Requirements

Identify what behaviour changed.
Explain why the change may be risky.
Suggest a safer alternative if price is required.
Write the review comment you would leave.

Show solution

The diff changes missing price from a warning into a value of 0.

That may be risky because a missing required price now becomes a free product instead of being rejected.

If price is required, a safer implementation is validation:

PHP example

<?php

declare(strict_types=1);

function productPrice(array $product): int
{
    if (! array_key_exists('price', $product)) {
        throw new InvalidArgumentException('Product price is required.');
    }

    return $product['price'];
}

A useful review comment:

This removes the warning by defaulting a missing price to 0. Is price optional here? If it is required, can we validate and reject the product instead so we do not accidentally create a free line item?