Ревью PHP-кода: замечания, которые повторяются из PR в PR

Ревьюер, который пишет «переименуйте переменную» и «добавьте комментарий», тратит время команды впустую. За полгода в проекте на PHP 8.x с Laravel или без него повторяется один и тот же короткий список проблем, и почти все они — про безопасность, обработку ошибок и предсказуемость поведения, а не про стиль. Ниже — эти повторяющиеся замечания с формулировками, которые можно копировать в комментарий к PR, и с объяснением, чем именно рискует прод, если пропустить.

Материал про то, что писать в ревью, а не про то, как настроить линтер. Форматирование, порядок импортов и длину строк должны ловить PHP-CS-Fixer или PHP_CodeSniffer в CI — на них ревьюер не тратит ни строки. Ревью начинается там, где автомат бессилен: логика, границы данных, безопасность.

Что отдать автоматам до того, как открыть PR глазами

Если в пайплайне нет статического анализа, половина ваших комментариев будет про то, что машина находит быстрее и без обид. Минимальный набор для PHP 8.x:

• PHPStan или Psalm — типы, недостижимый код, обращение к возможному null, несуществующие методы. Уровень поднимайте постепенно.
• PHP-CS-Fixer — стиль по PSR-12. Всё, что про пробелы и переносы, обсуждать в комментариях бессмысленно.
• Rector — автоматическая миграция синтаксиса на новую версию языка, если вы недавно обновляли PHP.

Дальше — только то, что требует понимания задачи.

Замечания про безопасность — блокирующие всегда

SQL-инъекция через конкатенацию

Самое частое и самое опасное. Строка вида "SELECT * FROM users WHERE email = '" . $email . "'" — это дыра независимо от того, откуда пришёл $email. Правило простое: пользовательские данные попадают в запрос только через плейсхолдеры.

// Плохо — данные склеиваются в текст запроса
$sql = "SELECT * FROM orders WHERE user_id = " . $userId;
$rows = $pdo->query($sql)->fetchAll();

// Хорошо — подготовленный запрос с параметром (PDO, PHP 8.x)
$stmt = $pdo->prepare('SELECT * FROM orders WHERE user_id = :user_id');
$stmt->execute(['user_id' => $userId]);
$rows = $stmt->fetchAll();

Формулировка замечания: «Здесь пользовательский ввод конкатенируется в SQL — это инъекция. Перепишите на подготовленный запрос с именованным параметром. Блокирую до исправления.» Важно: имена таблиц и колонок через плейсхолдер не подставляются, их так не защитить — если они динамические, нужен белый список допустимых значений.

Вывод без экранирования

Данные, которые уходят в HTML, экранируются через htmlspecialchars() с явным флагом и кодировкой, а не голым echo. В Blade за это отвечает {{ }}, и замечание чаще про использование {!! !!} там, где данные не доверенные.

Секреты в коде

Ключ API, пароль от БД или токен в исходнике — блокирующее замечание, даже если «это тестовый стенд». Секреты живут в переменных окружения и читаются через конфиг, а не хардкодятся. Если секрет уже попал в историю git — его недостаточно удалить коммитом, его нужно отозвать и перевыпустить.

Разделяйте замечания на блокирующие и необязательные явно. Помечайте вкусовое как «nit:» — тогда автор не будет спорить два дня про то, что вы и сами предлагали на выбор.

Обработка ошибок — где прод падает молча

Подавленные и проглоченные исключения

Оператор @ перед вызовом и пустой catch — два способа спрятать проблему до того момента, когда она проявится данными в неверном состоянии. Пустой catch (\Throwable $e) {} означает, что ошибка произошла, но никто о ней не узнал.

// Плохо — исключение поймано и выброшено в никуда
try {
    $payment = $gateway->charge($order);
} catch (\Throwable $e) {
    // тишина
}

// Лучше — либо обрабатываем осмысленно, либо логируем и пробрасываем
try {
    $payment = $gateway->charge($order);
} catch (PaymentException $e) {
    $this->logger->error('Списание не прошло', [
        'order_id' => $order->id,
        'reason' => $e->getMessage(),
    ]);
    throw $e; // пусть выше решают, что делать с заказом
}

Формулировка: «Пустой catch скрывает ошибку списания — заказ останется в подвешенном состоянии, а мы об этом не узнаем. Либо логируем и пробрасываем, либо явно откатываем заказ.»

Ловля \Exception вместо конкретного типа

Широкий catch (\Exception $e) вокруг большого блока проглатывает не только ту ошибку, которую ждали, но и опечатку в коде тремя строками ниже. Ревью-замечание: сузить тип до ожидаемого исключения, а неожиданное — пусть падает и попадает в мониторинг.

Игнорирование результата, который может быть false или null

Многие функции PHP возвращают false при ошибке: file_get_contents(), json_decode() при некорректном JSON вернёт null. Код, который сразу обращается к результату как к массиву или объекту, упадёт с TypeError на первых же битых данных.

Замечания, специфичные для Laravel

Если проект на Laravel (проверьте по составу зависимостей — версии 10.x и 11.x в ходу), к общему списку добавляются свои повторяющиеся места.

• Массовое присвоение. Model::create($request->all()) без $fillable или с ним, но с лишними полями — путь к тому, что пользователь перезапишет is_admin. Замечание: валидировать через Form Request и передавать $request->validated(), а не весь ввод.
• Проблема N+1. Цикл по коллекции с обращением к связи внутри без with() — на списке из 500 записей это 501 запрос к базе. Ловится пакетом barryvdh/laravel-debugbar локально; в замечании просите добавить eager loading.
• Бизнес-логика в контроллере. Метод контроллера на 120 строк с транзакциями и вызовами внешних сервисов — замечание про вынос в сервис или Action-класс, чтобы это можно было покрыть тестом отдельно от HTTP-слоя.
• Тяжёлая работа в HTTP-запросе. Отправка письма или обращение к платёжному шлюзу синхронно в контроллере держит соединение открытым. Замечание: вынести в очередь через Job.

Что ревьюить в приоритетном порядке

У ревьюера ограничено внимание. Тратьте его сверху вниз — нижние строки часто закрывает автомат или они вкусовые.

Как формулировать, чтобы не начать войну в комментариях

Замечание из одного слова «переделайте» вызывает сопротивление. Работает связка «что не так → чем это грозит → что предлагаю»:

• Пишите про код, а не про автора: не «вы забыли», а «здесь не обрабатывается случай пустого массива».
• Показывайте последствие: «на проде это упадёт, когда придёт заказ без позиций» весомее, чем «так не делают».
• Отделяйте обязательное от желательного. Пометка «nit:» экономит часы споров.
• Если сомневаетесь — задавайте вопрос, а не выносите вердикт: «почему здесь catch без логирования — намеренно?»

Инвалидация правил ревью и настройка обязательного числа апрувов в GitHub или GitLab — отдельная тема, здесь речь только про содержание замечаний.

Что сделать после прочтения

Заведите в репозитории короткий чек-лист ревьюера из шести строк таблицы выше и вынесите в него договорённости команды про тесты. Настройте PHPStan хотя бы на уровень 5 и PHP-CS-Fixer в CI, чтобы стилевые замечания вообще не доходили до глаз человека. И договоритесь про метку «nit:» — это самое дешёвое улучшение культуры ревью, которое можно внедрить за один разговор.