Популярные ошибки разработчиков

Принципы SOLID, DRY, KISS, YAGNI и прочие, являются само собой разумеющимися и обсуждать их - тема, которая выходит за рамки данного ресурса. Для этого есть Telegram чат .

Рекомендую также прочитать раздел «Плохие привычки» » из книги «Архитектура сложных веб-приложений» указанной на странице «Ссылки» .

# Логика в контроллерах

Наверняка ты уже слышал фразу «Толстые контроллеры - это плохо».

Приводить объёмные примеры логики в контроллере не вижу смысла. Рекомендую почитать книгу указанную выше. В ней присутствуют хорошие примеры и объяснения что делать. Также смотри определение контроллера по GRASP

Здесь же скажу кратко: Контроллер принимает входящий запрос, вызывает необходимые действия (сервисы, экшены, команды) и возвращает ответ клиенту. Ничего более в нем быть не должно.

# Слишком тонкие контроллеры

Некоторые разработчики буквально воспринимают понятие "тонкие контроллеры" и пишут такой код:

Bad
1public function store(Request $request, PostService $postService)
2{
3 return $postService->create($request);
4}
highlight by torchlight.dev

Здесь явное нарушение SRP: PostService зависит от Request и отвечает за Response, В результате такой сервис нельзя нигде переиспользовать, а смысл контроллера сводится к нулю.

# DI в конструкторе контроллера

Следующий пример - результат наличия логики в контроллере и, как следствие, попытка "разгрузить" методы. Требовать зависимости необходимо в конструкторе сервиса, а не контроллера.

Bad
1public function __construct(MyService $myService)
2{
3 $this->myService = $myService;
4}
5 
6public function store()
7{
8 $this->myService->handle();
9}
10 
11public function update()
12{
13 $this->myService->handle();
14}
highlight by torchlight.dev

Что в этом коде такого, спросишь ты?

  1. Он бесполезен. Визуально кажется, что сервис используется в нескольких методах и логично инициализацию вынести в одно место, но публичные методы контроллера не должны вызывать друг друга. Следовательно сервис используется только в одном методе на один запрос и больше не будет вызван в рамках жизненного цикла для текущего контроллера, поэтому использование $this->service в контроллере обычно не имеет смысла. Исключением могут быть приватные методы контроллера, но размещать логику в таких методах может быть не самым удачным решением по сравнению с сервисными классами.
  2. Не всем методам может понадобиться один и тот же сервис, зачем его загружать для всех запросов? Незачем.
  3. И самое важное, в жизненном цикле конструктор контроллера вызывается до middlewares. Согласно документации , в конструкторе контроллера можно указывать «middleware» (для порядка и удобства рекомендую указывать их только в роутах), а значит сам контроллер создаётся до этапа выполнения "мидлварок" и в момент внедрения зависимостей ещё будут недоступны некоторые функции, например, сессия и куки, а значит отсутствовать и авторизация. И если конструктор сервиса зависит от подобных вещей, то работать правильно он не сможет, а отладка бага может занять некоторое время.

Поэтому запрашивай необходимые сервисы в методах контроллера:

Good
1public function store(MyService $myService)
2{
3 $myService->handle();
4}
5 
6public function update(MyService $myService)
7{
8 $myService->handle();
9}
highlight by torchlight.dev

Последний пример не является нарушением DRY. Даже если в разных методах сервис используется одинаково, то это всё равно не повод выносить куда-либо, ибо "экшены" контроллера независимы друг от друга и в любой момент может понадобиться внести изменения в один из них. И если методы получились полностью одинаковыми, то всё равно вызов логики - это не сама логика.

# Логика в шаблонах major

Иногда начинающих ставит в тупик требование не использовать логику в шаблонах, «как же тогда выводить данные?».

Всё просто, всё что можно подготовить заранее - вынеси и подготовь заранее. В шаблоне должна остаться только логика отвечающая за само отображение. Можно вызывать сервисы и хелперы, обращаться к константам, но никаких запросов к БД из шаблонов быть не должно.

Bad
1@foreach (Product::get() as $product)
2 @php($total += $product->price * $product->qty)
3 
4 <img src="{{ Storage::disk('products')->url($product->cover) }}" />
5 
6 {{ new \Carbon\Carbon($product->created_at)->format('Y-m-d') }}
7@endforeach
8 
9Total price: {{ $total }}
highlight by torchlight.dev

Кейсы бывают разные и порой сложно полностью отказаться от использования php и определения новых переменных, но всегда старайся свести к минимуму, особенно избавиться от каких-то вычислений не связанных с вёрсткой. В примере выше, подсчитывается общая цена при переборе товаров. Это ужаснейшая идея реализовывать подобную логику в шаблоне. Либо считай заранее в сервисе/контроллере, либо вызывай сервис из шаблона, который подсчитает, но не пиши прямо в блейде.

Представь, что тебе надо будет изменить формулу ценообразования и учитывать текущие акции, код может вырасти на десятки строк, а у тебя 5 разных шаблонов, где отображается товар, в каждый будешь копипастить? Вроде смешно, но это история из реального проекта и смешного в ней нет ничего.

Чем чище html, тем лучше.

Good
1@foreach ($products as $product)
2 <img src="{{ $product->cover_url }}" />
3 
4 {{ $product->created_at->format('Y-m-d') }}
5@endforeach
6 
7Total price: {{ $totalPrice }}
highlight by torchlight.dev

Также не стоит забывать, что шаблон может быть переиспользован на странице несколько раз и если в нем присутствует тяжёлая логика, например, какой-то цикл с вызовом сервиса, то каждое использование шаблона будет повторно запускать этот код, поэтому крайне рекомендуется передавать в шаблон максимально подготовленные данные, на сколько позволяет ситуация, конечно же.

# Логика в маршрутах major

В документации логика в роутах используется для наглядности примеров, не более.

В роутах можно что-то протестировать быстро для себя, но потом обязательно удалить или перенести в контроллер, хуже места для написания любой логики придумать сложно.

Bad
1Route::get('posts', static function () {
2 $posts = Post::active()->paginate(config('post.per_page'));
3 
4 return view('post.index', compact('posts'));
5});
highlight by torchlight.dev

Даже что-то крайне простое (возврат шаблонов, редиректы) лучше вынести в контроллер или использовать готовые для этого методы.

Good
1Route::get('posts', [PostController::class, 'index']);
2 
3Route::view('posts', 'posts.index');
4 
5Route::redirect('old-posts', 'posts');
highlight by torchlight.dev

Стоит отметить, что на практике использование Route::view() неоправданно, т.к. это вносит двойственность (где-то шаблоны возвращаются роутами, а где-то контроллерами) и чтобы добавить какую-либо логику, необходимо всё равно создавать контроллер.

# Валидация в контроллере

Сама по себе валидация в контроллере не критична, т.к. контроллер отвечает за обработку запроса и ответ в случае какой-либо ошибки. Но это загрязняет методы контроллера и всё же нарушает SRP, тем более в Laravel есть готовый механизм для таких задач.

Bad
1public function store(Request $request)
2{
3 $data = $request->validate([
4 'heading' => ['required', 'string', 'max:255'],
5 'content' => ['nullable', 'string'],
6 'status' => ['required', 'boolean'],
7 ]);
8 
9 Post::create($data);
10}
highlight by torchlight.dev

Вместо валидации в контроллерах используй FormRequest

Good
1public function store(PostCreateRequest $request)
2{
3 Post::create($request->validated());
4}
5 
6public function update(PostUpdateRequest $request, $id)
7{
8 Post::where('id', $id)->update($request->validated());
9}
highlight by torchlight.dev

Обрати внимание:

  • для каждого запроса используется свой FormRequest. Не стоит делать общий PostRequest, даже если правила в конечном итоге получились одинаковыми.
  • Не используй в контроллере ручное создание валидатора Validator::make() без явной на то необходимости. В случае возникновения ошибки валидации, фреймворк сам вернёт ошибку и завершит выполнение. Иначе при использовании Validator::make() необходимо также вручную сформировать ответ.

# Запросы в циклах major

Код ниже сгенерирует по 2 запроса на каждую итерацию. Если в $ids 100 элементов, значит запросов будет 200, если 500, то 1000...

Bad
1foreach ($ids as $id) {
2 $post = Post::first('id', $id);
3 $post->update(['status' => 1]);
4}
highlight by torchlight.dev

Есть редкие кейсы, когда выгоднее или проще создать много запросов, чем писать несколько, но сложных, особенно если код будет выполняться в фоне, не важно сколько времени это может занять и какую нагрузку дать на сервер.

Но в подавляющем большинстве случаев необходимо избегать использования запросов к БД из циклов.

Good
1Post::whereIn($ids)->update(['status' => 1]);
highlight by torchlight.dev

Также обязательно прочти «Ленивая загрузка связей», т.к. тема тоже относится к данной проблеме, но менее интуитивна для начинающих.

# Ленивая загрузка связей major

Сколько запросов к БД происходит в примере ниже? Попробуй посчитать самостоятельно, при условии, что каждый пост имеет 5 комментариев и всего выбрано 10 постов (такие вопросы могут задавать на собеседованиях).

Bad
1public function index()
2{
3 $posts = Post::active()->paginate(10);
4 
5 return view('posts.index', compact('posts'));
6}
1@foreach ($posts as $post)
2 {{ $post->heading }}
3 
4 @foreach($post->comments as $comment)
5 {{ $comment->message }}
6 {{ $comment->user->name }}
7 @endforeach
8@endforeach
highlight by torchlight.dev

Посчитал? Теперь давай разберёмся. Мы выбираем 10 постов из БД одним запросом и передаём коллекцию в шаблон. В шаблоне мы перебираем посты и обращаемся на связь $post->comments. Каждое такое обращение это +1 запрос к таблице comments, в котором подгружаются комментарии к посту. Значит на 10 постов +10 запросов, итого уже 11. Далее происходит перебор комментариев (мы условились что их 5 у каждого поста) и у каждого комментария есть обращение на связь $comment->user, чтобы получить автора комментария, т.е. ещё +1 запрос на каждый комментарий, а значит 10*5 = +50 запросов. Итого имеем 61 запрос к БД.

Эта называется «проблема N+1». В реальности постов и комментариев на одной странице может быть больше и количество запросов легко переходит в сотни (например 10 постов по 30 комментариев в итоге даст 311 запросов).

Чтобы исправить данную проблему существует понятие «Eager Loading» или «Жадная загрузка», смысл которой заранее получить все необходимые данные для отображения, а не делать запросы из циклов по требованию (ленивая загрузка). В Laravel это предусмотрено, читай раздел документации «Eager Loading» .

Исправим наш запрос в контроллере с помощью with():

Good
1public function index()
2{
3 $posts = Post::with('comments.user')->active()->paginate(10);
4 
5 return view('posts.index', compact('posts'));
6}
highlight by torchlight.dev

По итогу получаем всего 3 запроса к БД вместо 61: посты, комментарии и авторы комментариев.

Обрати внимание:

  • Начиная с 8-ой версии появилась возможность отключить ленивую загрузку, чтобы случайно не обратиться к связям, которые не были загружены заранее. Подробнее читай в документации
  • Laravel сам загрузит все необходимые связи указанные в цепочке. Нет необходимости указывать отдельно: with('comments', 'comments.user'), хоть это и не будет ошибкой.

# Получение текущего пользователя major

Популярнейшая проблема встречается в коде начинающих:

Bad
1User::find(Auth::user()->id);
2// or
3User::find(Auth::id());
highlight by torchlight.dev

В примере выше происходит бесполезная выборка из БД. Auth::user() уже возвращает модель авторизированного пользователя. Нет смысла вытаскивать данные ещё раз через User::find()

Good
1$user = Auth::user();
2$user->email;
3 
4$userId = Auth::id();
highlight by torchlight.dev

Обрати внимание:

  • Если пользователь не авторизирован, то Auth::user() вернёт null
  • Фасад Auth и хелпер auth() эквивалентны

# Обновление данных minor

Чтобы обновить данные, часто можно встретить такой код:

1$post = Post::find($id);
2 
3$post->update($data);
4// or
5$post->fill($data);
6$post->save();
highlight by torchlight.dev

В данном примере нет проблем, если получаемые данные нужны для каких-либо действий, например, для ответа клиенту.

Если же задача просто обновить данные в БД, то Post::find() не нужен, т.к. это лишний SELECT запрос. Достаточно выполнить UPDATE запрос напрямую, без выборки:

1Post::where('id', $id)->update($data);
highlight by torchlight.dev

Следует различать методы модели и билдера:

Illuminate/Database/Eloquent/Model::update
Illuminate/Database/Eloquent/Builder::update

В первом примере используется метод модели, во втором - билдера. Оба варианта правильные, но второй предпочтителен, если нужно только обновить данные. Стоит отметить, что события, при таком обновлении, не будут вызваны.

# Mass assignment minor

Немного спорный вопрос, балансирующий между "краткостью" и "контролем".

В первом примере заполнение полей происходит вручную и код выглядит громоздко.

1$post = new Post();
2$post->heading = $request->get('heading');
3$post->description = $request->get('description');
4$post->content = $request->get('content');
5$post->save();
highlight by torchlight.dev

Во втором примере используется массовое заполнение и код выглядит компактно.

1$post = Post::create($request->validated());
highlight by torchlight.dev

На практике второй вариант обычно предпочтителен, т.к. имеет лаконичный формат и снижает риск опечаток в именах полей. Требует указания $fillable в модели.

С другой стороны, первый вариант, более явный, сразу видно какие поля сохраняются и удобно добавить новое, но необходимо вручную перечислять все поля.

Можно использовать оба варианта вместе:

1$post = new Post();
2$post->fill($request->validated());
3$post->user_id = Auth::id();
4$post->active = 1;
5$post->save();
highlight by torchlight.dev

# Тяжелые выборки major

В первом примере код выбирает все записи из БД. Это хорошо работает пока в таблице пару десятков или даже сотен записей.

Bad
1Post::all();
2//or
3Post::get();
highlight by torchlight.dev

Но когда количество записей в таблице заметно возрастает, то начинаются проблемы с производительностью и такие запросы долго выполняются и потребляют большое количество памяти.

Поэтому рекомендуется не использовать метод all(), а ограничивать количество записей, например, с помощью пагинации или явно указав лимит.

Good
1Post::paginate(10);
2// or
3Post::limit(100)->get();
highlight by torchlight.dev

Иногда действительно необходимо получить все записи из небольшой таблицы, например, цвета товара или статусы заказа. Даже в таком случае лучше перестраховаться и ограничить выборку.

Другой вариант используется для выборки всех записей из большой таблицы, например, нам необходимо обработать каждую запись или экспортировать в файл. Для этого необходимо использовать метод chunk(), который будет порционно доставать записи, снижая нагрузку на БД и экономя память. Подробнее можно почитать в документации

Good
1Post::chunk(100, static function ($posts) {
2 // Process the records
3 // $posts->each(...);
4});
highlight by torchlight.dev

# Делегирование выборки на коллекции

Популярная ошибка у новичков, которые не задумываются как работает их код, а смотрят лишь на результат. Оба примера ниже дают одинаковый результат, но работают с разной эффективностью.

Bad
1Post::get()->where('user_id', Auth::id());
2// or
3Post::get()->sortBy('created_at');
4// or
5Post::get()->first();
highlight by torchlight.dev

В первом случае, всё что после методов get() или all() является коллекцией моделей. Запрос к БД уже выполнен. Т.е. выбираются все записи из таблицы и затем на уровне php фильтруется/сортируется коллекция.

Правильный вариант - выбирать запросом только необходимые записи и сортировать их на уровне БД.

Good
1Post::where('user_id', Auth::id())->get();
2// or
3Post::orderBy('created_at')->get();
4// or
5Post::first();
highlight by torchlight.dev

Всё, что можно сделать на уровне БД, то лучше делать там, т.к. СУБД имеет оптимизированные алгоритмы для таких задач. Редкие кейсы со сложными сортировками и группировками, которые не позволяет сделать СУБД, можно выполнять на уровне коллекций или массивов в php.

# Хардкод

Избегай использования жёстко заданных значений прямо в коде.

Bad
1public function update(Post $post)
2{
3 $post->status = 'updated';
4 
5 Mail::to('admin@localhost')->send();
6 
7 return redirect()->withSuccess('Post successfully updated');
8}
highlight by torchlight.dev

Выноси значения в конфиги, переводы, константы и базу данных.

Good
1public function update(Post $post)
2{
3 $post->status = Post::STATUS_UPDATED;
4 
5 Mail::to(config('mail.to.admin'))->send();
6 
7 return redirect()->withSuccess(__('messages.post_updated'));
8}
highlight by torchlight.dev

# Использование env() critical

Использовать хелпер env() следует только в файлах конфигурации.

Это связано с тем, что после кэширования конфига, env() не будет возвращать значения из файла .env

Об этом говорится в документации , но к сожалению, часто упускается новичками.

Получать значения необходимо только через config()

# Высокая связанность / High coupling major

«Качественный дизайн должен обладать слабой связанностью (low coupling) и сильной связностью (high cohesion)»

Тема IoC (Inversion of Control) достаточна обширна и выходит за рамки данного ресурса, поэтому если не знаком, то рекомендую поскорее закрыть этот пробел, хотя бы в общих чертах.
Также необходимо ознакомиться с документацией Service Container .

Рассмотрим простой метод:

Bad
1public function index()
2{
3 $myService = new MyService();
4 $myService->handle();
5}
highlight by torchlight.dev

Проблема данного кода, что он имеет высокую связанность, т.к. в нем создаётся новый объект. Ситуация усугубляется ещё больше, когда данный сервис начинает требовать какие-либо зависимости, а те зависимости, в свою очередь, требуют свои зависимости и так далее...

Также такой код будет затруднительно тестировать, т.к. сервис нельзя "замокать".

Вместо прямого создания объекта необходимо использовать DI, реализация которого уже есть в Laravel:

Good
1public function index(MyService $myService)
2{
3 $myService->handle();
4}
highlight by torchlight.dev

И если следовать букве D из «SOLID», то в данном примере MyService должен быть абстракцией, а не конкретным классом (реализацией).

# Дублирование частей QueryBuilder-а

Часто необходимо выбирать данные из БД по одинаковым условиям. Чтобы избежать дублирования кода, используются «Scopes»

Bad
1public function index()
2{
3 Post::where('active', 1)->get();
4}
5 
6public function show()
7{
8 Post::where('active', 1)->first();
9}
highlight by torchlight.dev

Кроме дублирования, скоупы повышают читаемость запроса (если правильно именованы).

Good
1public function index()
2{
3 Post::active()->get();
4}
5 
6public function show()
7{
8 Post::active()->first();
9}
10 
11// App\Models\Post
12public function scopeActive(Builder $query)
13{
14 $query->where('active', 1);
15}
highlight by torchlight.dev

Также можно использовать глобальные скоупы, которые будут применяться ко всем запросам автоматически. Это может быть удобно, чтобы гарантировать выборку только активных записей (по такому принципу работает soft delete). Но рекомендую прежде подумать, что будет выгоднее для твоего приложения, прописывать везде скоупы или withoutGlobalScope(). Также глобальные скоупы снижают очевидность запроса, т.е. сложно, посмотрев на QB, сказать какие записи выбираются без проверки глобальных скоупов и этот момент придётся держать в памяти.

# Хранение файлов в public

По непонятным причинам многие новички пишут пользовательские файлы в public директорию.
Согласно документации , для хранения файлов есть директории /storage/app для приватных файлов и storage/app/public для общедоступных.

Технической проблемы "хранения в public" нет, кроме того факта, что все файлы будут всегда доступны напрямую, и нельзя программно контролировать их доступ, например, по временной ссылке. Также это не интуитивный момент для других разработчиков, они, как и сторонние пакеты, будут использовать storage/app и в проекте появится двойственность, одни файлы будут в storage, другие в public.

В Laravel есть мощная система по работе с файлами, с конфигурацией на storage/app и которую легко можно изменить под собственные нужды, вплоть до написания собственного драйвера.

Ещё одна популярная ошибка это использование хелпера storage_path() вместо фасада Storage. Вся нужная информация есть в документации и рекомендую почитать простую инструкцию «Как работать с файлами» .

# Нативная работа с датами

По умолчанию, в Laravel используется мощный пакет для работы с датами Carbon , который наследуется от нативного DateTime .

Поля модели такие как created_at, updated_at, deleted_at и другие, которые имеют каст, содержат не строку, а объект \Carbon\Carbon, поэтому нет смысла и необходимости повторно оборачивать в \DateTime

Bad
1$date = new \DateTime($post->created_at);
2$date->add(new \DateInterval('P1D'));
3$date->format('Y-m-d');
highlight by torchlight.dev

Код выше работает, потому что $post->created_at имеет метод toString(). В результате объект конвертируется в строку, чтобы создать новый объект, но уже нативный, а не Carbon

Вместо этого сразу можно работать с датой как с объектом:

Good
1$date = $post->created_at->addDay();
2$date->format('Y-m-d');
highlight by torchlight.dev

# Настройка document_root critical

Популярная ошибка у невнимательно читающих документацию , в которой сказано, что запросы должны приходить в public/index.php

Чаще всего проблема вызвана попыткой завести лару на виртуальном хостинге (shared hosting), который не позволяет управлять корневой директорией web-сервера.

В результате начинающие перемещают index.php в корень фреймворка и/или размещают .htaccess в котором настраивают перенаправление в public

Bad
1# .htaccess
2RewriteRule ^$ public/index.php [L]
3RewriteRule ^(.*)$ public/$1 [L]
4# or any variants
highlight by torchlight.dev

Это серьёзная проблема безопасности, а в случае с Apache ещё страдает и производительность. Приложение должно находиться за пределами публичной директории, а web-сервер "смотреть" в public

Good
1# Nginx
2server {
3 root /srv/example.com/public;
4}
1# Apache
2<VirtualHost>
3 DocumentRoot /srv/example.com/public
4</VirtualHost>
highlight by torchlight.dev

Если нет возможности изменить document_root, то читай «Как установить Laravel на хостинг».

# Именования

Данная тема не относится к самому фреймворку и вроде всё понятно и нечего обсуждать, но каждый день в чатах скидывают код, в котором совершены одни и те же ошибки именования.

Bad
1$post = Post::get(); // Collection
2 
3$model = Post::first(); // Model
4 
5$user = Auth::id(); // integer
6 
7$data = $user->getPathArray(); // array
highlight by torchlight.dev

Принцип простой - названия переменных, методов, классов и т.п. должны отражать содержимое или предназначение. Обрати внимание, что "содержимое" это не тип данных.

Если переменная содержит много элементов (массив, коллекция), то имя во множественном числе, иначе в единственном.

Good
1$posts = Post::get(); // Collection
2 
3$post = Post::first(); // Model
4 
5$userId = Auth::id(); // integer
6 
7$userPhotos = $user->getPhotos(); // array
highlight by torchlight.dev