Git And Collaboration Workflows
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
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:
?? 0or?? ''added to required data@error suppression- broad
catch (Throwable $e)without handling htmlspecialchars()removed near HTML output- SQL or shell strings built by concatenation
exitadded inside reusable functions- production config changed
.envor 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.
Read The Diff As A Story Of Behavior
A diff is not only a list of lines. It is the proposed change to the project contract. Start by identifying the behavior the change claims to alter: a validation rule, a database query, a dependency version, an HTTP response, a migration, a test fixture, or documentation. Then read the files in the order a user or process experiences them.
For PHP work, a useful review often starts with tests, request handlers, domain services, persistence, and configuration. Tests show the claimed behavior. Application code shows the implementation. Migrations and configuration show deployment impact. Documentation shows whether the team expects a new workflow. If those pieces tell different stories, ask for clarification before reviewing line style.
Separate Signal From Noise
Formatting-only changes, generated output, lock-file churn, and IDE rewrites can obscure the meaningful change. Use git diff --stat, git diff --name-only, and focused path diffs to understand scope. If a pull request changes one validation rule and reformats several unrelated files, ask for separation unless the repository deliberately combines formatting with feature work.
Whitespace and line-ending changes deserve attention because they can create merge conflicts and make future blame output less useful. git diff --check catches trailing whitespace and conflict markers. Word-level diff can help with long Markdown paragraphs or JSON strings, but do not rely on it for structured data where formatting has meaning.
Review PHP-Specific Surfaces
PHP diffs often hide behavior in configuration and metadata. A changed composer.lock can introduce a new package version. A changed phpstan.neon can weaken analysis. A changed .env.example can reveal a new runtime dependency. A migration can make rollback difficult even when the application diff is small.
Look for boundary changes: new public methods, constructor parameters, database columns, route names, status codes, queue messages, event payloads, serialized fields, and template variables. Those are compatibility surfaces. A private helper change is usually local; a public API response change can affect clients that are not visible in the diff.
Compare Intent, Tests, And Implementation
A strong review checks whether the test would fail before the implementation change and whether it observes the right boundary. A unit test that asserts a helper return value may be enough for a pure calculation. A change to authentication, caching, database writes, or deployment should usually include an integration or behavior-level check.
Read negative cases, not only happy paths. If the change accepts a new valid input, does it reject malformed input? If it adds a permission, does it test denial? If it adds retry behavior, does it prevent duplicate effects? Missing negative evidence is often where production defects hide.
Detect Accidental Coupling
A diff can reveal design pressure. If a small rule change touches controllers, services, jobs, templates, and migrations, the rule may have too many owners. If a new dependency appears in domain code, a provider concept may be leaking across a boundary. If tests require many unrelated fixtures, the object under test may own too much.
Use review comments to name the coupling, not merely to request a different pattern. For example: "This controller and worker now duplicate the refund eligibility rule; can the policy live in one service?" is more useful than "make this DRY."
Reviewing Lock Files And Generated Files
Lock-file diffs require a different lens from source code. Confirm whether the package changes match the stated command. A targeted update should not unexpectedly upgrade unrelated packages. Check abandoned packages, platform requirements, and security advisories when relevant.
Generated files should be reviewed against their source. If the generated output is committed, the pull request should show the source change and the generator command should be reproducible. Do not spend human review effort line-editing generated code unless the generator itself is the subject.
Practical Diff Workflow
Begin with git status --short and git diff --stat. Read the pull-request description or commit message, then inspect tests and public surfaces. Use path-limited diffs for noisy areas. When a change is large, ask for a guide: which files establish the behavior, which are mechanical, and which are generated.
Leave review comments at the level of risk. Nitpicks can be batched or automated by formatters. Behavioral questions should identify the failing scenario. Security and data-loss risks should be direct and specific.
After this lesson, you should be able to read a PHP diff for behavior, identify generated or noisy changes, review tests against the claimed intent, spot compatibility surfaces, and leave comments that improve correctness rather than merely reshuffling code.
Techniques For Large Diffs
Large diffs need navigation. Start with file names and categories, then read the smallest set of files that establish behavior. Use git diff -- path/to/file.php for focused review, git diff --word-diff for prose, and moved-code highlighting in the hosting platform when available. If a change mixes mechanical renames with behavior, ask the author to split it or provide a review map.
For dependency and generated changes, compare before and after commands. A lock-file update should match a Composer command. A generated client should match a schema change. A migration should match the model or repository change that uses it. The reviewer is not expected to mentally validate thousands of generated lines; the repository should make regeneration reproducible.
When reviewing security or money-handling code, read for missing negative cases first. A beautiful happy path is not enough if malformed input, unauthorized users, duplicate requests, or provider timeouts are untested. Put the riskiest question in the first review comment so the author knows what matters most.
End every review by re-reading the staged result from the user-facing entry point. This catches a common mistake: each individual hunk looks reasonable, but the combined request path, CLI command, migration, or rendered page no longer matches the stated behavior. The final read is where integration mistakes become visible. Do it before approval.
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
priceis 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
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?