long/BOJ

Files

T

baozaotumao 8b89322e18 fix: 将 code-review-skill 从子模块转为普通目录

2026-06-06 09:44:07 -07:00

20 KiB

Raw Permalink Blame History

PHP Code Review Guide

PHP 8.x code review guide covering the type system, modern language features, OOP modeling, PDO data access, security, error handling, Composer dependencies, performance, and testing.

Quick Review Checklist
Type System & Modern PHP
Object Modeling
Input, Output & Security
Database Access
Error Handling
Composer & Dependencies
Performance & Resource Management
Testing & Static Analysis
Review Checklist
References

Quick Review Checklist

Must-check

New files enable declare(strict_types=1);
Public APIs have parameter, return, and property types
User input is validated; output is escaped per context
SQL uses parameterized queries or ORM binding
Passwords use password_hash() / password_verify()
File uploads validate MIME, size, extension, and storage path
composer.lock is committed; dependency ranges are reasonable
PHPUnit/Pest tests and PHPStan/Psalm static analysis are present

Common issues

Loose comparison == / != causing type-juggling vulnerabilities
md5() / sha1() used to store passwords
Concatenating SQL, HTML, shell commands, or file paths
Using @ to suppress errors
unserialize() on untrusted data
$_GET / $_POST / $_FILES flowing straight into business logic
PHP 8.2+ dynamic properties trigger a deprecation; PHP 9 may turn it into an error

Type System & Modern PHP

strict_types and explicit types

<?php

// ❌ weak boundary: passing "42" gets silently coerced
function findUser($id) {
    return User::find($id);
}

// ✅ enable strict_types at the top of the file; type the public API
declare(strict_types=1);

function findUser(int $id): ?User
{
    return User::find($id);
}

Don't leave type checking entirely to runtime input validation. Type declarations express an internal contract; input validation expresses how much to trust the boundary. You need both.

Avoid loose comparisons

<?php

// ❌ strings like "0e12345" can be treated as 0 under loose comparison
if ($providedHash == $storedHash) {
    grantAccess();
}

// ✅ strict comparison; use hash_equals() for secrets or tokens
if (hash_equals($storedHash, $providedHash)) {
    grantAccess();
}

// ✅ match uses identity checks, so fewer type-juggling surprises than switch
$status = match ($code) {
    200 => 'ok',
    404 => 'not_found',
    default => 'unknown',
};

Pay attention to ==, !=, and in_array($x, $list) (loose by default) in auth, payment, state machine, and permission logic. Use ===, !==, and in_array($x, $list, true) where it matters.

Union / intersection / nullable types

<?php

// ❌ mixed or untyped makes callers guess the return shape
function loadConfig($source) {
    return parseConfig($source);
}

// ✅ express the real contract with types
function loadConfig(string|PathInfo $source): Config
{
    return parseConfig($source);
}

// ✅ make null explicit when it's a real business state
function currentUser(): ?User
{
    return Auth::user();
}

mixed can show up at the boundary or while migrating legacy code, but in core business services it usually signals missing modeling.

The nullsafe operator shouldn't hide missing state

<?php

// ❌ chained nullsafe blurs the reason for failure
$country = $order?->customer?->profile?->country;

// ✅ branch explicitly on critical business state
if ($order === null) {
    throw new OrderNotFound();
}

$customer = $order->customer();
if ($customer === null) {
    throw new MissingCustomer($order->id);
}

$country = $customer->profile()?->country;

Distinguish "optional display field" from "business invariant that must exist." The former is a good fit for ?->; the latter should fail loudly.

Object Modeling

Use readonly properties and value objects

<?php

// ❌ public mutable fields let callers change state at will
class Money
{
    public $amount;
    public $currency;
}

// ✅ express an immutable value object with types and readonly
final readonly class Money
{
    public function __construct(
        public int $amount,
        public string $currency,
    ) {
        if ($amount < 0) {
            throw new InvalidArgumentException('Amount must be non-negative');
        }
    }
}

For DTOs, config, and domain value objects, check first whether a readonly class or readonly properties can remove hidden side effects.

Enums instead of string states

<?php

// ❌ string states are easy to typo and can't enumerate the legal set
if ($order->status === 'paied') {
    ship($order);
}

// ✅ an enum surfaces illegal states earlier
enum OrderStatus: string
{
    case Pending = 'pending';
    case Paid = 'paid';
    case Cancelled = 'cancelled';
}

if ($order->status === OrderStatus::Paid) {
    ship($order);
}

When reviewing state machines, permissions, or type fields, look for "magic string values." If the value set is stable, suggest an enum; if it comes from an external system, convert it to an internal enum before it enters the business layer.

Don't rely on dynamic properties

<?php

// ❌ PHP 8.2+ triggers a deprecation when creating a dynamic property
$user = new User();
$user->emali = 'a@example.com'; // a typo also silently creates a property

// ✅ declare properties or use a dedicated data structure
final class User
{
    public function __construct(
        public string $email,
    ) {}
}

#[AllowDynamicProperties] should be an exception for legacy compatibility, not the default for new code. Watch for serialization, ORM hydration, and test doubles that secretly rely on dynamic properties.

Don't do heavy I/O in constructors

<?php

// ❌ quietly connecting to the DB on construction makes testing and error handling hard
final class ReportService
{
    private PDO $pdo;

    public function __construct()
    {
        $this->pdo = new PDO($_ENV['DSN']);
    }
}

// ✅ inject dependencies from the outside
final class ReportService
{
    public function __construct(
        private PDO $pdo,
    ) {}
}

A constructor should establish the object's invariants — not send HTTP requests, open connections, read large files, or run complex queries.

Input, Output & Security

Validate input at the boundary

<?php

// ❌ superglobals flow straight into business logic
$user = $service->create($_POST['email'], $_POST['age']);

// ✅ validate and coerce types at the boundary first
$email = filter_input(INPUT_POST, 'email', FILTER_VALIDATE_EMAIL);
$age = filter_input(INPUT_POST, 'age', FILTER_VALIDATE_INT, [
    'options' => ['min_range' => 0, 'max_range' => 130],
]);

if ($email === false || $email === null || $age === false || $age === null) {
    throw new InvalidInput();
}

$user = $service->create($email, $age);

filter_input() only handles a slice of basic validation. Complex rules, cross-field constraints, and business constraints still need a dedicated validator or request DTO.

Escape output per context

<?php

// ❌ user input goes straight into HTML
echo "<h1>Hello {$_GET['name']}</h1>";

// ✅ use htmlspecialchars in an HTML text context
$name = (string) ($_GET['name'] ?? '');
echo '<h1>Hello ' . htmlspecialchars($name, ENT_QUOTES | ENT_SUBSTITUTE, 'UTF-8') . '</h1>';

Different contexts need different escaping: HTML text, HTML attributes, URLs, JavaScript strings, and CSS are all different. When a template engine's default escaping is turned off, treat it as a security risk.

Passwords and randomness

<?php

// ❌ md5/sha1 must not be used for password storage
$hash = md5($password);

// ✅ use PHP's built-in password API
$hash = password_hash($password, PASSWORD_DEFAULT);

if (!password_verify($password, $hash)) {
    throw new InvalidCredentials();
}

// ✅ use a CSPRNG for tokens
$token = bin2hex(random_bytes(32));
$code = random_int(100000, 999999);

Don't hand-roll salts, round migration, or password comparison. Use password_needs_rehash() when you need to upgrade the cost factor.

Deserialization and object injection

<?php

// ❌ untrusted input into unserialize can trigger object injection
$payload = unserialize($_COOKIE['state']);

// ✅ prefer JSON for external data, and validate its schema/shape
$payload = json_decode($_COOKIE['state'] ?? '{}', true, flags: JSON_THROW_ON_ERROR);

If you must process historical serialized data, at least restrict allowed_classes and make sure the relevant classes' magic methods can't produce dangerous side effects.

File uploads and paths

<?php

// ❌ building the path from the raw filename
$target = __DIR__ . '/uploads/' . $_FILES['avatar']['name'];
move_uploaded_file($_FILES['avatar']['tmp_name'], $target);

// ✅ generate a server-side filename, check the upload error and MIME
$file = $_FILES['avatar'];
if ($file['error'] !== UPLOAD_ERR_OK) {
    throw new UploadFailed();
}

$finfo = new finfo(FILEINFO_MIME_TYPE);
$mime = $finfo->file($file['tmp_name']);
if (!in_array($mime, ['image/png', 'image/jpeg'], true)) {
    throw new InvalidFileType();
}

$target = __DIR__ . '/uploads/' . bin2hex(random_bytes(16)) . '.jpg';
move_uploaded_file($file['tmp_name'], $target);

When reviewing upload features, check size limits, MIME detection, extensions, a non-executable storage directory, path traversal, overwrite protection, and any virus-scan or async-processing requirements.

Database Access

Use parameterized queries

<?php

// ❌ concatenated SQL is an injection risk
$sql = "SELECT * FROM users WHERE email = '" . $_GET['email'] . "'";
$user = $pdo->query($sql)->fetch();

// ✅ PDO prepared statement + bound value
$stmt = $pdo->prepare('SELECT id, email FROM users WHERE email = :email');
$stmt->execute(['email' => $email]);
$user = $stmt->fetch(PDO::FETCH_ASSOC);

Parameters can only bind values — not table names, column names, or sort direction. Dynamic identifiers must go through a whitelist mapping.

<?php

// ✅ whitelist the dynamic sort column
$columns = [
    'created' => 'created_at',
    'email' => 'email',
];

$column = $columns[$_GET['sort'] ?? 'created'] ?? $columns['created'];
$stmt = $pdo->query("SELECT id, email FROM users ORDER BY {$column} DESC");

Wrap multi-step writes in transactions

<?php

// ❌ multi-step writes with no transaction leave half-finished state on failure
$orderId = $orders->create($cart);
$inventory->reserve($cart);
$payments->charge($orderId);

// ✅ explicit transaction boundary
$pdo->beginTransaction();
try {
    $orderId = $orders->create($cart);
    $inventory->reserve($cart);
    $payments->recordIntent($orderId);
    $pdo->commit();
} catch (Throwable $e) {
    $pdo->rollBack();
    throw $e;
}

Don't casually put external, non-rollbackable side effects (an actual charge, an email, a message dispatch) inside a database transaction. Common patterns are an outbox, an idempotency key, or triggering after the transaction commits.

Avoid N+1 queries

<?php

// ❌ querying inside a loop
foreach ($orders as $order) {
    $customer = $customerRepo->find($order->customerId);
    render($order, $customer);
}

// ✅ batch-load, then map
$customerIds = array_unique(array_map(fn ($o) => $o->customerId, $orders));
$customers = $customerRepo->findByIds($customerIds);

foreach ($orders as $order) {
    render($order, $customers[$order->customerId] ?? null);
}

In ORMs like Laravel/Doctrine, check eager loading, join fetch, selected columns, pagination, and indexes.

Error Handling

Catch specific exceptions, keep context

<?php

// ❌ swallowing the exception leaves callers unable to know it failed
try {
    $mailer->send($message);
} catch (Exception $e) {
}

// ✅ catch a specific exception, keep context, and rethrow
try {
    $mailer->send($message);
} catch (TransportException $e) {
    throw new NotificationFailed($userId, previous: $e);
}

Empty catch blocks, error_log()-and-continue without surfacing the error, and turning every exception into RuntimeException('failed') in production code all deserve a question.

Don't suppress errors with @

<?php

// ❌ hides the real error and makes debugging hard
$content = @file_get_contents($path);

// ✅ handle failure explicitly
$content = file_get_contents($path);
if ($content === false) {
    throw new RuntimeException("Unable to read file: {$path}");
}

@ is common around file, network, array access, and legacy library calls. Push for an explicit branch, or convert third-party errors into project exceptions.

Don't leak sensitive data in logs

<?php

// ❌ writing tokens, passwords, or the full request body to the log
$logger->error('Login failed', ['request' => $_POST]);

// ✅ log non-sensitive context that still helps locate the problem
$logger->warning('Login failed', [
    'email_hash' => hash('sha256', strtolower($email)),
    'ip' => $requestIp,
]);

Check logs, exception messages, the debug toolbar, error pages, and failed-queue records. Sensitive data includes passwords, tokens, sessions, PII, payment data, and full cookies.

Composer & Dependencies

Lock reproducible dependencies

{
  "require": {
    "php": "^8.2",
    "monolog/monolog": "^3.0"
  },
  "require-dev": {
    "phpunit/phpunit": "^11.0",
    "phpstan/phpstan": "^1.10"
  }
}

When reviewing composer.json / composer.lock, watch for:

Application repos commit composer.lock; library repos usually don't
require-dev shouldn't make it into the production image
The PHP platform version matches the CI version
Autoload rules aren't too broad (don't load test or script directories)
scripts commands don't depend on a developer's local secret config

Dependency security and maintenance

composer audit
composer outdated --direct
composer validate --strict

When adding a package, look at its maintenance status — download count isn't the only signal. What matters is its security history, release cadence, minimal dependency footprint, and whether it duplicates the standard library or a framework built-in.

Performance & Resource Management

Stream large datasets with generators or pagination

<?php

// ❌ loading every record at once
$rows = $repo->all();
foreach ($rows as $row) {
    exportRow($row);
}

// ✅ paginate or use a generator to avoid a memory spike
foreach ($repo->cursor() as $row) {
    exportRow($row);
}

A PHP request lifecycle is short, but CLI jobs, queue workers, and export tasks run for a long time. For that kind of code, watch memory growth, unclosed resources, and global-state pollution especially closely.

Avoid expensive work inside loops

<?php

// ❌ re-parsing config or opening a connection on every iteration
foreach ($items as $item) {
    $client = new ApiClient($_ENV['API_KEY']);
    $client->send($item);
}

// ✅ create reusable dependencies outside the loop
$client = new ApiClient($_ENV['API_KEY']);
foreach ($items as $item) {
    $client->send($item);
}

Watch for database queries, HTTP requests, regex compilation, large array copies, accumulating array_merge() appends, and repeatedly reading env vars or config files inside loops.

Release or scope resources

<?php

// ✅ close file handles after use
$handle = fopen($path, 'rb');
if ($handle === false) {
    throw new RuntimeException('Unable to open file');
}

try {
    while (($line = fgets($handle)) !== false) {
        process($line);
    }
} finally {
    fclose($handle);
}

PDO connections are usually managed by the container, but file handles, curl handles, temp files, locks, and cached objects in queue workers still need an explicit lifecycle.

Testing & Static Analysis

Test behavior, not implementation details

<?php

// ❌ asserting an internal method call makes refactoring expensive
$mailer->expects($this->once())->method('buildTemplate');

// ✅ assert observable results
$service->sendWelcomeEmail($user);

$this->assertTrue($mailbox->hasMessageFor($user->email));

For business services, controllers, and queue jobs, prefer covering observable behavior: inputs/outputs, database state, published events, and dispatched messages.

Static analysis and formatting

vendor/bin/phpunit
vendor/bin/phpstan analyse
vendor/bin/psalm
vendor/bin/php-cs-fixer fix --dry-run --diff
vendor/bin/rector process --dry-run

When reviewing a PR, check whether the new code lowers the PHPStan/Psalm level, leans heavily on baseline ignores, or uses @phpstan-ignore-next-line to paper over a real type problem.

Isolate test data

<?php

// ❌ the test depends on real time and external services
$service->expireOldSessions();

// ✅ inject a clock and a fake gateway
$clock->setNow(new DateTimeImmutable('2026-01-01T00:00:00Z'));
$service->expireOldSessions();

Watch for database transaction rollback, fixture cleanup, randomness, time, queues, caches, and external APIs. Slow PHP tests are usually not a language problem — it's that the boundaries aren't isolated.

Review Checklist

Types & modeling

declare(strict_types=1); at the top of the file
Parameters, return values, and properties have explicit types
=== / !== used; collection lookups use strict mode
Stable state sets use an enum, not magic strings
New code doesn't rely on dynamic properties
Value objects are readonly or otherwise immutable

Security

Input is validated and type-coerced at the boundary
Output is escaped per HTML/URL/JS/CSS context
SQL uses prepared statements or ORM binding
Dynamic table/column/sort names go through a whitelist
Passwords use password_hash() / password_verify()
Tokens, codes, and filenames use random_bytes() / random_int()
Untrusted input never reaches unserialize()
File uploads check the error code, size, MIME, extension, and storage directory
No injection or leakage risk in shell commands, path building, or log output

Data & transactions

Multi-step writes have a transaction or compensation mechanism
External side effects are designed to be idempotent
N+1 queries avoided
Pagination, indexes, and selected columns are reasonable
Database errors aren't swallowed

Maintainability

Constructors don't do heavy I/O
Dependency injection is clear; no hidden global state
No @ error suppression
Exceptions preserve context and previous
Composer dependency ranges, autoload, and scripts are reasonable
Application repos commit composer.lock

Testing & tooling

PHPUnit/Pest cover the critical and failure paths
PHPStan/Psalm config doesn't lower strictness
New ignores/baselines are explained
Formatting tools and CI commands are reproducible
Tests isolate time, randomness, the database, queues, and external APIs

20 KiB Raw Permalink Blame History