nepster09
> так если реквест не должен ничего обрабатывать это нужно делать контроллеру, а это лишний код в нем.
Если у вас параметров штук 20 с кучей проверок - конкретно эти проверки можно вынести в отдельный stateless сервис, который будет бросать исключение, или возвращать объект Response - это ок.
Если на те же 20 полей вы вы будете преобразовывать Request в некий немутабельный DTO с уже проверенными данными и приведенными к правильным типам - это тоже ок.
То, что действительно важно - это вызывать подобные штуки внутри контроллера.
В противном случае вы получите неявную зависимость вашего контроллера.
Если же параметров мало - заводить отдельные сервисы валидации/фабрики DTO будет излишним.
> Относительно контроллера я имею ввиду, что из объекта реквест он получает уже проверенные и приобразованные данные, которые готовы к работе дальше.
Вы добавляете целый уровень абстракции, который в 90% случаев не нужен.
> Почему кий-велью это плохая практика ? Из-за отсутствия целостности апи ?
В целом - да. Массив - это ящик пандоры, который провалидировать довольно проблематично. Например вы ожидаете массив с двумя ключами: login(string) и pass(string). По хорошему вам нужно проверить:
1. Массив содержит ключ login.
2. Массив содержит ключ pass.
3. Количество элементов в массиве 2.
4. Элемент login - строка.
5. Элемент login - не содержит запрещенных символов и правильной длины.
6. Элемент pass - строка.
7. Элемент pass - не содержит запрещенных символов и правильной длины.
Если же использовать scalar type hinting: при проверках у вас остаются только 2 валидации: 5 и 7.
> Что скажете про коллекции ?
Это ок.
> По поводу исключений, может я не так выразился,...
Все верно
> Просто если контроллер дергает 2 или 3 сервиса или сервис
Если сервис самостоятельно это делает - имеет смысл начинать транзакцию в нем. Если вы объединяете действия нескольких сервисов - скорее всего вам стоит создать обертку для этого действия в отдельном сервисе.
> Request - занимается строго чисткой данных и приводит их к нужному типу и формату.
Request - это просто набор данных. Его задача содержать их, не больше и не меньше. Обрабатывать он ничего не должен.
> Controller - получает уже подготовленные данные, которые готовы для дальнейшей работы.
Нет, контроллер получает Request, вытягивает данные от туда, проверяет их на корректность, приводит к конечным типам и отправляет на обработку.
> Service - принимает данные в массиве, через сеттеры или через DTO.
Через массив как набор объектов - это ок. Через массив k-v - это хреновая практика.
Через сеттеры - не ок. По хорошему сервисы не должны содержать изменяемого состояния.
Через DTO - ок.
> В процессе бросает исключения, которые являются своего рода валидацией данных, которые препятствуют выполнению.
Не совсем так. Исключения бросаются, если штатное выполнение невозможно.
> View - принимает объект $errors для обработки ошибок и отображает представление.
Затрудняюсь ответить. Вариантов передачи данных в шаблон довольно таки много. Например при запросе после редиректа имеет смысл ошибки оставлять в сессии. При аякс запросе - лучше ориентироваться на коды ошибок. При обычном рендеринге можно через переменную $errors, или как-то по другому ее назвать.
> Что должно быть в catch?
Какое исключените - такая и обработка))
> Например, ситуация с отсутсвием требуемого объекта в БД. Что я должен показать пользователю?
Это целиком и полностью зависит от задачи.
> Может делать перенаправление?
Если бизнес требования такие, что нужно перенаправлять - перенаправляйте, если нужно что-то другое - сделайте это что-то другое.
> Или делать какую либо подмену объекта?
Опять же от задачи зависит, но скорее нет чем да.
> По поводу анти практик, ларавель мальца круче Yii, а вот в Yii там для рав разработки нормально поизвращались.
Yii - вообще говоря тоже УГ. Но на нем по крайней мере можно средние проекты писать.
Валидация реквеста до контроллера - это как раз одна из антипрактик.
> а что вы скажете по поводу транзакций ? Правильно ли их открывать в контроллере ?
Скорее нет чем да. По ситуации. Если у вас действите маленькое и отдельный сервис для него писать смысла нет, но с транзакцией - почему бы и да.
> Ну тоесть за какие заслуги мы контроллеру даем лишнюю ответственность
Разве у штуки, которая работает с внешним вводом нет обязанности проверять, этот внешний ввод?))
> но сервис плюнул исключение, можно это дело подмешать во вьюшку ?
Если вы по типу исключения определите отдельную текстовку для пользователя - почему бы и да. Если же на прямую getMessage - вот это уже плохо.
В моем примере не просто так 2 try-catch, результат первого - это базовая валидация пользовательского ввода и там можно безопасно возвращать сообщение исключения на прямую.
nepster09
> я про то, что там можно писать стандартные ларавелевские валидаторы
Laravel - это набор антипрактик, увы.
> а потом в сервисе нужно еще получить данные для манипуляций.
Сервис вы вызываете с вытянутыми данными из реквеста, согласно сигнатуре вызова. Сервис по хорошему даже про существование Request не должен знать.
nepster09
> Тоесть по сути исключения посланные из сервисов считаются хорошей практикой валидации ?
Исключение - это ситуация, противоречащая штатному выполнению, как правило. Если что-то мешает сервису штатно выполнить его работу - стоит бросить исключение.
> Если логика большая и сложная, то трай/кетч может прилично разрастись.
Чаще всего вам не нужно вообще-вообще все типы исключений обрабатывать. А только несколько из них.
> Как на счет создать что-то типа AlertException и засовывать информацию туда, например сообщение об ошибке и код ?
Конкретно для контроллера - ну может быть, но нужно быть очень осторожным. Пользователю можно отдавать только безопасные для вас данные. Например исключение от БД отдавать не стоит в принципе, а вот логировать его - стоит.
Для сервисов - скорее нет, чем да, зависит от назначения сервиса.
Станислав Почепко
Да все просто с исключениями))
\InvalidArgumentException и производные - вы даете на обработку какую-то хрень. Пример: вместо массива в метод суете строку.
\DomainException и производные - данные не правильны, но уже по сложнее, например пользователя с таким id не существует, хэш такого-то пароля - не правильный...
\LogicalException и производные - такого произойти не должно. Например: удаленный пользователь откомментировал сообщение.
\RuntimeException и производные - "я хрен его знает что с этим делать". Пример: пропало подключение к БД.
> Пытаюсь поднять качество кода, но до конца не понимаю где и как надо делать проверки.
Всюду. В этом случае вы получаете 2 преимущества:
- гарантию того, что вызов каждого метода будет корректным.
- нет надобности учитывать контекст в каждом методе.
Например у вас есть метод логин, принимающий на вход логин и пароль, ему не важно, а откуда зашел пользователь, а выбрал ли он remember me,... Ваш метод проверяет и обрабатывает только 2 аргумента - не пустые строки.
@adamsafr
> До него думал, что Laravel самый удобный.
Laravel - это сборник антипрактик. Как-то довелось ревьюить код на его базе, крофь_из_глаз.jpg. Что бы писать безопасный, тестабельный, чистый и производительнй код - от большей части фреймворка придется отказаться, что печально.
Timue Asgard
> но ведь это перенесется в контроллер если будет правильнее это делать не в сервисе
Почему же? Задача контроллера - взаимодействие с "внешним миром" и вашей БЛ. То, что email не существует - это исключительная ситуация, запрещающая штатное выполнение работы сервиса. Вот вы и бросаете исключение. Для "внешнего мира" конкретно в случае такой не штатной ситуации имеет смысл вернуть специфический ответ - этим и занимается контроллер в блоке catch. При этом тот же SRP ни где не нарушается. Каждый занят своим делом.
> насчет вашего второго утверждения. чем это чревато?
Тем, что код ошибки должен быть уникальным, иначе вполне возможны ситуации наложения контекстов. Если же использовать отдельные исключения - такой проблемы нет + вы можете использовать встроенные механизмы языка под разделение исключений по типу и обработки.
Сергей Протько
> Когда нужно о чем-то помнить, причем о чем-то, что не относится к делу (ну мол вас не должно ж парить что там) - это является прямым нарушением инкапсуляции.
Эмс, ну как бы используя какой-то метод вы должны помнить о его сигнатуре, инкапсуляция тут ни при чем. В приведенном примере возвращается интерфейс, работа происходит с некой реализацией этого интерфейса, что вполне норм.
> То что это в мире симфони норм практика я верю - но в мире доктрины это плохая практика.
Можно по подробнее, почему?
> У меня ж нет состояния статического, это просто методы фабрики, не нужно их мокать.
Вариант. Но не лучше ли в таком случае сделать state-less сервис, который будет уметь то же самое, но передавать его как явную зависимость?
Сергей Протько
> Но getGroups например ни в коем случае не должен вернуть ArrayCollection.
Почему же? Вполне норм практика. В конструкторе создаем $this->groups = new ArrayCollection();
Сам геттер объявляем как
public function getGroups() : Collection
Согласен, тут есть нюанс, что при гидрации зависимостей доктрина их заменит на PersistCollection, про это нужно помнить. Но в остальном - не вижу проблем, не хотите юзать коллекции - не проблема, вызываем ->toArray() и все счастливы.
> Я сейчас почти все время делаю все на билдерах, и мне дико нравится.
Как вы покрываете тестами код, который использует зависимости со статикой? Я сейчас на проекте, в котором unit тестами с моками зависимостей покрывается практически все (98%), а со статикой возникают проблемы, phpunit это дело не мокает. Из за этого на проекте введено соглашение о запрете использования статических вызовов (кроме тех, что предлагает PHP, их мы не тестируем).
magary4 Используйте статику только в исключительных ситуациях, когда по другому вот никак. Иначе вы получаете неявную зависимость.
> чтоб один и тот же обьект иметь возможность получить в нескольких вариантах
Зависит от ситуации конечно, но конкретно в вашем примере стоит использовать: addGroup/removeGroup/getGroups методы + настройки рилейшнов, что предлагает доктрина.
Виталий Инчин ☢
> Тошно уже от этой фразы :|
Что поделать, страдай))
> Синтаксическая конструкция языка не может быть злом - им является исключительно ее кривое использование.
Ой, да ладно, если бы было по вашему - то в php бы не было deprecated штук ни в одной версии. Так что ваше утверждение в корне не корректно.
Возвращаясь к глобальным переменным - в куче best practices есть упоминание о глобальных переменных и том, что их использовать плохо, на ряду с goto (да даже в официально доке намекают, что это хреновая затея php.net/manual/ru/control-structures.goto.php).
Евгений Кульбеда
> Почему зло?
Потому, что произвольный код может сломать весь ваш функционал. Что будет, если вдруг глобальная переменная будет удалена?
> А если у меня есть куча функций, которым нужна эта переменная, то мне передавать им всем в виде аргумента?
Верно.
> И если у меня есть функция которая вызывает другую функцию которой нужна эта переменная, то мне придётся передавать им обоим эту переменную?
Верно. Если ваши функции завязаны на глобальное состояние (глобальные переменные) - сломать ваш проект очень просто, достаточно заменить это состояние.
> так если реквест не должен ничего обрабатывать это нужно делать контроллеру, а это лишний код в нем.
Если у вас параметров штук 20 с кучей проверок - конкретно эти проверки можно вынести в отдельный stateless сервис, который будет бросать исключение, или возвращать объект Response - это ок.
Если на те же 20 полей вы вы будете преобразовывать Request в некий немутабельный DTO с уже проверенными данными и приведенными к правильным типам - это тоже ок.
То, что действительно важно - это вызывать подобные штуки внутри контроллера.
В противном случае вы получите неявную зависимость вашего контроллера.
Если же параметров мало - заводить отдельные сервисы валидации/фабрики DTO будет излишним.
> Относительно контроллера я имею ввиду, что из объекта реквест он получает уже проверенные и приобразованные данные, которые готовы к работе дальше.
Вы добавляете целый уровень абстракции, который в 90% случаев не нужен.
> Почему кий-велью это плохая практика ? Из-за отсутствия целостности апи ?
В целом - да. Массив - это ящик пандоры, который провалидировать довольно проблематично. Например вы ожидаете массив с двумя ключами: login(string) и pass(string). По хорошему вам нужно проверить:
1. Массив содержит ключ login.
2. Массив содержит ключ pass.
3. Количество элементов в массиве 2.
4. Элемент login - строка.
5. Элемент login - не содержит запрещенных символов и правильной длины.
6. Элемент pass - строка.
7. Элемент pass - не содержит запрещенных символов и правильной длины.
Если же использовать scalar type hinting: при проверках у вас остаются только 2 валидации: 5 и 7.
> Что скажете про коллекции ?
Это ок.
> По поводу исключений, может я не так выразился,...
Все верно