Комментарии 19
1. Зачем вызывать явно «response()->json»? Фреймворк сам разберется, в каком формате сериализовать данные.
2. А вот пример в Вашем коде, как не нужно делать:
Лучше отвечать кодом HTTP. Так же, фреймворк предоставляет для этого все нужное, используя настройки окружения (debug флаг) для более развернутой информации про ошибку
3. try-catch конструкция внутри контроллера — лишнее. Есть ExceptionHandler из коробки
4. Вот в этом месте
Вы рискуете получить лишние данные прямо в сервисный слой. Используйте метод "$requests->validated()"
2. А вот пример в Вашем коде, как не нужно делать:
return response()->json([
'success' => false,
'exception' => $e->getMessage()
]);
Лучше отвечать кодом HTTP. Так же, фреймворк предоставляет для этого все нужное, используя настройки окружения (debug флаг) для более развернутой информации про ошибку
3. try-catch конструкция внутри контроллера — лишнее. Есть ExceptionHandler из коробки
4. Вот в этом месте
$orderService->createOrderFromRequest($request)
Вы рискуете получить лишние данные прямо в сервисный слой. Используйте метод "$requests->validated()"
Спасибо за комментарий. Как я и описывал выше, цель статьи — показать преимущества создания слоя сервисов при разработке архитектуры веб приложений. Возврат с помощью response() имеет тут чисто показательный характер.
Да, так действительно будет гораздо лучше. Ну или определить внутри request метод, который вернет сущность (для более удобной работы) вместо массива. Спасибо за замечание)
Вы рискуете получить лишние данные прямо в сервисный слой. Используйте метод "$requests->validated()"
Да, так действительно будет гораздо лучше. Ну или определить внутри request метод, который вернет сущность (для более удобной работы) вместо массива. Спасибо за замечание)
п.2 почему так не нужно делать?
Дизайн rest api предполагает, что код ответа (http статус) уже несет в себе нужную информацию. Вы можете дополнить тело ответа информацией про ошибку, при условии отладки. Автор статьи игнорирует рекомендации по дизайну и всегда отдает сообщение из исключения, в обход ExceptionHandler'а
Какие статусы вы назначите на
Для следующих случаев:
GET /orderДля следующих случаев:
- Заказ не существует.
- Заказ не найден в рамках существующих фильтров.
Полагаю, что в Вашем вопросе есть ошибка :)
GET /order — не корректный запрос/uriGET /orders — получить коллекцию заказов. Если в рамках существующих фильтров ничего не найдено, вернуть пустую коллекцию с кодом 200 (запрос же корректно был обработан). Как альтернатива, можно вернуть код 400, но как по мне — это уже дело вкуса…GET /order/{id} — вернет 200, если заказ (ресурс) с таким id существует. 404, если заказ не найден, не корректный id или клиенту нельзя знать о существовании такого idGET /order/{id} — вернет 200, если заказ (ресурс) с таким id существует. 404, если заказ не найден, не корректный id или клиенту нельзя знать о существовании такого id
Полагаю что вы вообще не понимаете в чем суть проблемы отборажения бизнеслогики на статусы HTTP.
Первой много, а второго мало.
По шагам
404, если заказ не найден
Это ок.
404, если зне корректный id
Это не ок. Не пользователь должен догадаться из вашего "что-то пошло не так". А он должен получить внятную информацию. Особенно если это пользователь API.
404, если клиенту нельзя знать о существовании такого id
Это тоже не ок. Поскольку совершенно другая ситуация которая требует иной реакции.
Тут люди свой собственный пол сделали небинарным. А вы хотите завернуть всю широту возможных случаев в несколько десятков кодов…
Ваш код 404 не несет всей нужной информации. Т.е. нарушает сам REST по вашим же словам.
Как исправлять будете?
Можно ли использовать сервисы внутри сервисов?
Я бы однозначно не рекомендовал такую практику, так как этим вы нарушаете single responsibility принцип, делая ваш код, к тому же, достаточно запутанным.
Тут нет ошибки? Потому что во втором пункте вы по сути предлагаете использовать сервисы (OrderService и NotificationService) внутри сервиса (CreateOrderOperation)
Нет, в этом случае CreateOrderOperation — это отдельная абстракция над слоем сервисов. Тут мы можем провести аналогию с фасадом, который скрывает сложную (или не очень) систему классов за простым интерфейсом. Я использую названия в стиле ..Operation, так как обычно за один запрос нам необходимо выполнить последовательность каких-то действий (сохранить заказ, сгенерировать платежный виджет, отправить оповещение клиенту и т.д.), что по сути своей можно назвать атомарной операцией.
Только по сути ваш CreateOrderOperation завязан на контроллер, является его продолжением вынесенным в другое место, толку от него такого особо нет. На самом деле он всё таки должен относится к сервисному слою, принимать на вход какую нибудь дтошку и возвращать тоже что то понятное.
А всё что относится к http и валидации глубже контроллера не следует пропускать.
А всё что относится к http и валидации глубже контроллера не следует пропускать.
Меня одного покорежило от switch в makeNotificationEvent?
Если вы намекаете на match, то в силу текущих задач я все еще очень часто использую php ниже 8-й версии, так что пишу, как привык)
Почему сразу не передавать MailEventInterface?
NotificationService::notify(MailEventInterface $event): void;Заодно можно будет избавится от массива, а сразу передавать нужные параметры в эвент. В противном случае каждый раз надо будет заходить в реализацию makeNotificationEvent, смотреть какой эвент должен создаться при order_created и какой массив параметров он ожидает
public function createOrder(CreateOrderRequest $request)
{
$this->notificationService->notify(new OrderCreatedEvent($order));
}Теперь понял) да, это действительно имеет смысл. Но тут также может возникнуть вопрос: «Должен ли контроллер знать и уметь создавать такую зависимость?».
Ну, конкретно в данном случае есть laravel.com/docs/7.x/events#generating-events-and-listeners, где можно в заказах генерить соответствующие эвенты, а сервисный слой должен подписаться на соответствующий эвент с нужным генератором рассылки. Опять же, через этот механизм можно непринужденно можно расцепить само создание эвента и рассылку сообщений во времени, используя очереди (не задерживать обработку основного запроса).
$result = $createOrderOperation->createOrderFromRequest($request);
Это не сервисный слой, а насмешка над ним. Идея разделения на слои в том, что каждый слой отвечает за что-то свое: контроллер отвечает за взаимодействие с пользователем (прием HTTP-запросов, формирование ответов), а сервис отвечает за бизнес-логику, связанную с созданием заказа. У вас же сервис выполняет работу контроллера: анализирует HTTP-запрос и формирует HTTP-ответ.
Это не сервис, это тот же самый толстый контроллер, который вы просто назвали Operation. В то же время контроллер у вас вообще ничего не делает. Зачем он вообще нужен? Удалите контроллер и переименуйте Operation в Controller и все будет правильно.
Из-за отсутствия разделения на слои с кодом может быть неудобно работать. Если например, я захочу создать заказ из CLI скрипта: надо откуда-то взять Request (откуда?), заполнить его (чем? Это нигде не определено, откуда я узнаю, какие в нем параметры?). Если бы у вас был по настоящему отделенный от контроллера сервисный слой, вы легко могли бы создать заказ из CLI или из теста.
Можно ли использовать сервисы внутри сервисов?
Я бы однозначно не рекомендовал такую практику, так как этим вы нарушаете single responsibility принцип, делая ваш код, к тому же, достаточно запутанным.
Разве? Вот есть у нас ряд действий: создать заказ, уведомить на почту, уведомить смской,… за каждое действие отвечает свой сервис.
Теперь нам надо чтоб в каком то случае всё это произошло в определенной последовательности и чтоб мы эту операцию могли вызывать из разных мест приложения. Эта операция и становится сервисом, его область ответственности — содержать логику последовательности действий, он будет изменяться, когда мы захотим изменить набор действий, их порядок. Код станет проще, поскольку эта логика теперь лежит в одном месте и вызывающему коду о ней думать не надо.
Зарегистрируйтесь на Хабре, чтобы оставить комментарий
Еще немного про сервисный слой в PHP