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

2026-06-06 09:44:07 -07:00
parent 44c5d0237e
commit 8b89322e18
39 changed files with 19829 additions and 1 deletions
@@ -0,0 +1,704 @@
+# 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.
+
+## Table of Contents
+
+- [Quick Review Checklist](#quick-review-checklist)
+- [Type System & Modern PHP](#type-system--modern-php)
+- [Object Modeling](#object-modeling)
+- [Input, Output & Security](#input-output--security)
+- [Database Access](#database-access)
+- [Error Handling](#error-handling)
+- [Composer & Dependencies](#composer--dependencies)
+- [Performance & Resource Management](#performance--resource-management)
+- [Testing & Static Analysis](#testing--static-analysis)
+- [Review Checklist](#review-checklist)
+- [References](#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
+<?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
+<?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
+<?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
+<?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
+<?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
+<?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
+
+// ❌ 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
+<?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
+<?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
+<?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
+<?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
+<?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
+<?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
+<?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
+<?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
+<?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
+<?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
+<?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
+<?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
+<?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
+
+```json
+{
+  "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
+
+```bash
+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
+<?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
+<?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
+<?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
+<?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
+
+```bash
+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
+<?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
+
+---
+
+## References
+
+- [PHP Manual: Type declarations](https://www.php.net/manual/en/language.types.declarations.php)
+- [PHP Manual: match](https://www.php.net/match)
+- [PHP Manual: Enumerations](https://www.php.net/manual/en/language.enumerations.overview.php)
+- [PHP Manual: Properties](https://www.php.net/manual/en/language.oop5.properties.php)
+- [PHP Manual: PDO](https://www.php.net/manual/en/class.pdo.php)
+- [PHP Manual: password_hash](https://www.php.net/manual/en/function.password-hash.php)
+- [PHP Manual: random_bytes](https://www.php.net/manual/en/function.random-bytes.php)
+- [Composer documentation](https://getcomposer.org/doc/)
+- [PHPUnit documentation](https://docs.phpunit.de/)
+- [PHPStan documentation](https://phpstan.org/user-guide/getting-started)
+- [Psalm documentation](https://psalm.dev/docs/)