What are the "non-negotiables" I should look for in every PR?

When I'm reviewing code, the first thing I check isn't actually the logic—it's the foundation. If I don't see declare(strict_types=1); at the top of a file, that's an immediate request for change. Without it, PHP's coercive typing can hide bugs that only surface in production when a string that looks like a number gets passed into a method expecting an integer.

Beyond that, I look for "Type Completeness." In modern PHP, there is almost no excuse for missing return types or parameter types. If I see a function like public function getData($id), I'm going to ask you to be specific. Is $id an int? A UUID string? Does it return an array or a specific DTO? If it's truly "anything," use mixed, but do it explicitly so I know you thought about it.

<?php
declare(strict_types=1);

namespace App\Services;

class OrderService {
    // Bad: No types, no strict mode
    // public function calculateTotal($orderId) { ... }

    // Good: Explicit, predictable, and self-documenting
    public function calculateTotal(int $orderId): float {
        // logic here
        return 99.99;
    }
}

How can I tell if a dev is actually using PHP 8+ features or just writing PHP 5.6 in a PHP 8 environment?

This is a common struggle. A lot of developers move through versions but keep their old habits. When you're reviewing, look for "boilerplate bloat." If you see a constructor that spends ten lines assigning properties, they aren't using Constructor Property Promotion.

I also keep an eye out for long switch statements or nested if/else chains that could be a match expression. match is not just shorter; it's an expression that returns a value and throws an UnhandledMatchError if a case is missed, which is way safer than a switch that silently fails.

// Old way: Verbose and prone to missing 'break'
switch ($status) {
    case 'pending': $label = 'Waiting'; break;
    case 'active': $label = 'Live'; break;
    default: $label = 'Unknown';
}

// Modern way: Concise and strict
$label = match ($status) {
    'pending' => 'Waiting',
    'active'  => 'Live',
    default   => 'Unknown',
};

Also, look for readonly properties on DTOs. If a class is just meant to carry data and shouldn't change after instantiation, it should be readonly. It signals intent to the rest of the team.

Where do the most dangerous bugs usually hide during a review?

Usually, it's in the "invisible" areas: side effects and tight coupling. I always scan for the new keyword inside a service method. If I see $client = new GuzzleHttp\Client(); inside a method, I'm flagging it. Why? Because you can't mock that in a unit test without some serious hacking. We want dependencies injected via the constructor.

I also look for "Silent Failures." This happens when someone wraps a huge block of code in a try-catch but the catch block just does log($e) and returns null. Now the calling code thinks everything is fine, but it's actually working with a null value, leading to a "Call to a member function on null" error three stack traces later. I'll usually push for a custom Exception that the controller can actually handle.

// Red flag: Hiding the error
try {
    $user = $this->repo->find($id);
} catch (\Exception $e) {
    $this->logger->error($e->getMessage());
    return null; // This is a landmine for the caller
}

// Better: Be explicit about the failure
try {
    $user = $this->repo->find($id);
} catch (\Exception $e) {
    throw new UserNotFoundException("User $id could not be retrieved", 0, $e);
}

How do I handle "nitpicks" without sounding like a jerk?

This is the human side of engineering. If you're commenting on trailing commas or indentation, stop. That's what PHP-CS-Fixer or Pint is for. If the project doesn't have an automated linter, your first "review" should be a PR that adds one, not a hundred comments about curly braces.

When you do have a technical critique, I find it helps to frame it as a question or a suggestion for the "future self" of the developer. Instead of saying "This is inefficient," try "I'm worried this loop might struggle if the dataset grows to 10k records; should we consider a generator here?" It shifts the conversation from a critique of their skill to a shared goal of system stability.

📋 Practical Task

Exercise: Audit the LegacyPaymentProcessor

You have been handed a Pull Request for a class called LegacyPaymentProcessor. Your task is to perform a "Modern PHP Audit." Find and fix the following issues in the provided code block:

• Add strict typing for the file.
• Convert the constructor to use Constructor Property Promotion.
• Replace the switch statement with a match expression.
• Add proper type hints for parameters and return values.
• Identify the "hidden bug" regarding the try-catch block and fix it to throw a specific exception instead of returning false.

<?php
namespace App\Payments;

class LegacyPaymentProcessor {
    private $gateway;
    private $apiKey;

    public function __construct($gateway, $apiKey) {
        $this->gateway = $gateway;
        $this->apiKey = $apiKey;
    }

    public function process($amount, $currency) {
        try {
            $status = $this->gateway->charge($amount, $this->apiKey);
            
            switch ($status) {
                case 1: return 'success';
                case 2: return 'pending';
                case 3: return 'failed';
            }
        } catch (\Exception $e) {
            return false;
        }
    }
}