Комментарии 38
ну Оке, может быть и так. Но. если поуберать все бихейверы, то с ними уйдёт и система проверки авторизации в модуле. А там довольно таки удобная система. Можно несколько методов авторизации на модуль навесить. К примеру для апишки и для обычныйх запросов сайта(если это к примеру spa). А чем это заменить?
вы расчитываете на то, что они не будут подтягиваться в результаты любых ваших запросов: Cat::find()->all()
Ну здрасьте приехали. Если я хочу найти все записи, то ожидаю, что «find all» найдет мне все записи. Если мне надо найти только активные записи, то я сделаю метод findActive(), который будет их возвращать.
А если какой-то программист навесит на all() дополнительную логику, то надо доходчиво объяснить ему, что так делать не надо. Или, например, забрать у него проект, а потом через полгода отдать ему же обратно на длительную поддержку.
Пожалуйста, читайте доки и не переходите на личности.
По вашей ссылке я прочитал следующее:
о чем я, собственно, и говорил. Если нам нужны только активные записи, мы должны это явно указать, а не переопределять метод «все()», чтобы он возвращал не все.
А если вы приняли выражение «какой-то программист» на свой счет — что ж, значит я попал в точку. Не делайте так, это непонятно.
This allows you to write query building code like the following:
$comments = Comment::find()->active()->all(); $inactiveComments = Comment::find()->active(false)->all();
о чем я, собственно, и говорил. Если нам нужны только активные записи, мы должны это явно указать, а не переопределять метод «все()», чтобы он возвращал не все.
А если вы приняли выражение «какой-то программист» на свой счет — что ж, значит я попал в точку. Не делайте так, это непонятно.
Основная проблема вашего решения в том, что оно не будет подтягиваться при вызове findOne(), например.
Про выделение согласен, но если в 99% вызовах (проще говоря везде кроме отчёта в админ панели) надо делать
А намёк ваш про «какой-то программист» был слишком толстым, не делайте так.
Про выделение согласен, но если в 99% вызовах (проще говоря везде кроме отчёта в админ панели) надо делать
Comment::find()->active()->notDeleted()->whatEver()
А намёк ваш про «какой-то программист» был слишком толстым, не делайте так.
Хм. Ну вообще я предлагаю в 99% случаев писать «Comment::findActive()» без всяких дополнительных стрелочек.
Про findOne().
Допустим, когда-то у нас был тариф «Супер», с id=123. Сейчас он неактивен. У нас есть сотня заказов по этому тарифу. Мы хотим посмотреть его параметры. Открываем страницу «tariff/view?id=123». По-вашему, метод findOne() должен вернуть null, а мы должны получить сообщение «Not found»?
Про findOne().
Допустим, когда-то у нас был тариф «Супер», с id=123. Сейчас он неактивен. У нас есть сотня заказов по этому тарифу. Мы хотим посмотреть его параметры. Открываем страницу «tariff/view?id=123». По-вашему, метод findOne() должен вернуть null, а мы должны получить сообщение «Not found»?
Мы ушли от темы. Изначально обсуждались удалённые записи (посты, комменты, товары). Именно удалённые, а не «не активные». И в этом случае должно быть 404 везде.
)) Ну ок, я переформулирую.
Допустим, когда-то у нас был товар «Супер-отмычка», с id=123. Сейчас он удален. У нас есть сотня выполненных заказов по этому товару, поэтому физически удалить его из базы мы не можем, хотя новые заказы мы на него не принимаем. Пользователь хочет посмотреть параметры своего заказа полугодовой давности. Пользователь открывает страницу «order/view?id=456». В заказе, в числе прочих, есть товар с id=123. По-вашему, метод findOne() должен вернуть null, а пользователь в строчке с товаром должен получить сообщение «Not found»?
Допустим, когда-то у нас был товар «Супер-отмычка», с id=123. Сейчас он удален. У нас есть сотня выполненных заказов по этому товару, поэтому физически удалить его из базы мы не можем, хотя новые заказы мы на него не принимаем. Пользователь хочет посмотреть параметры своего заказа полугодовой давности. Пользователь открывает страницу «order/view?id=456». В заказе, в числе прочих, есть товар с id=123. По-вашему, метод findOne() должен вернуть null, а пользователь в строчке с товаром должен получить сообщение «Not found»?
Нет, этот товар будет именно «не активен», «отсутствовать на складе» и т.п.
А вот если, например, пользователь соц сети удаляет свой пост, то он должен быть удалён везде-везде. И в ленте, и в репостах, и в списке моих лайков. Везде. Почувствуйте разницу (с)
А вот если, например, пользователь соц сети удаляет свой пост, то он должен быть удалён везде-везде. И в ленте, и в репостах, и в списке моих лайков. Везде. Почувствуйте разницу (с)
А зачем вам в таком случае soft delete, если удалить надо везде?
1) быстрее работает
2) можно похранить недельку-месяц что бы чуть что восстановить
3) мало ли пригодится.
2) можно похранить недельку-месяц что бы чуть что восстановить
3) мало ли пригодится.
Да это все понятно. Но
Во-первых. Это тогда не Yii2 bad behaviors, так как используемый фреймворк тут ни при чем.
Во-вторых. Если запись не удаляется из базы, значит она зачем-то нужна. Если нужна, значит где-то есть метод view или list, который такие записи получает. Если получает, значит вызывает метод findOne() или findAll() в каком-то виде. Значит переопределять их нежелательно, так как придется вводить флаг типа reallyShowAll и добавлять условие where в функцию all(). Понятно, что это все будет работать, просто код будет непонятный. Вызываем all(), а возвращается не all().
Удаление потом это неплохо, но если в базе есть удаленные и неудаленные записи, то в коде для них нужны отдельные вызовы функций. О чем я и пытаюсь сказать.
В-третьих. Если так переопределить findOne(), то восстановить удаленный коммент будет нельзя, так как вызов вернет null.
Во-первых. Это тогда не Yii2 bad behaviors, так как используемый фреймворк тут ни при чем.
Во-вторых. Если запись не удаляется из базы, значит она зачем-то нужна. Если нужна, значит где-то есть метод view или list, который такие записи получает. Если получает, значит вызывает метод findOne() или findAll() в каком-то виде. Значит переопределять их нежелательно, так как придется вводить флаг типа reallyShowAll и добавлять условие where в функцию all(). Понятно, что это все будет работать, просто код будет непонятный. Вызываем all(), а возвращается не all().
Удаление потом это неплохо, но если в базе есть удаленные и неудаленные записи, то в коде для них нужны отдельные вызовы функций. О чем я и пытаюсь сказать.
В-третьих. Если так переопределить findOne(), то восстановить удаленный коммент будет нельзя, так как вызов вернет null.
Во-первых. Это тогда не Yii2 bad behaviors, так как используемый фреймворк тут ни при чем.А на чём же мы тогда реализуем эту логику? )
Во-вторых, восстановить можно без проблем, отключив режим SoftDelete
Post::$softDelete = false;
$deletedPost = Post::findOne([...]);
$deletedPost->restore();
Если кому-то надо похранить удаленные записи недельку-месяц, это явно не проблема фреймворка) Надо — делайте.
Во-вторых, это заставляет вызывающий код знать детали реализации. Да и претензия моя, собственно, не в этом, а в том, что метод all() возвращает не all(). А это, в свою очередь, заставляет программиста знать детали реализации. И это уже сложнее.
Во-вторых, это заставляет вызывающий код знать детали реализации. Да и претензия моя, собственно, не в этом, а в том, что метод all() возвращает не all(). А это, в свою очередь, заставляет программиста знать детали реализации. И это уже сложнее.
Если запись не удаляется из базы, значит она зачем-то нужна. Если нужна, значит где-то есть метод view или list, который такие записи получает.
Совсем не обязательно она нужна приложению. Она может быть нужна базе данных — некоторые базы при некоторых условиях могут удалять запись очень долго, заметно дольше чем обновляя запись. Она может быть нужна другому приложению. Она сейчас может быть вообще не нужна никому, но решили сделать softDeletable на всякий случай.
Нужно понимать, что ожидаемое поведение по умолчанию при softDeletable такое же как и без него — запись удалили и обычным путём к ней не достучаться, какой-нибудь Comment::find()->active(false)->all(); прежде всего альтернатива разворачиванию бэкапа и восстановлению из него, а не средство посмотреть список товаров, продажи которых временно приостановлены.
Я о PHP знаю очень мало, о Yii ещё меньше. Но выше приведённый пример мне даже очень понятен. А вот логика ваша, как бы, не совсем — метод «все()» возвращает действително «все()» из предыдущего запроса/потока/т.д… Неужели вы будете утверждать, что стримы, пайпы и трансформации это плохо?)$comments = Comment::find()->active()->all(); $inactiveComments = Comment::find()->active(false)->all();
С появлением traits, behaviors на самом деле стали велосипедом.
Traits !== Mixins. Бехейверы в Yii решают именно задачу расширения поведения моделек, трейты же использовать для этого не совсем ок. Они нужны для устранения тупого дублирования кода, но не дублирования логики.
Что вы будете делать если вашему «трейту» вдруг понадобится сторонняя зависимость? Будете использовать сервис-локатор с глобальным доступом и тем самым скрывать зависимости? А если тестировать это все (вы же пишите тесты?).
но сейчас это оверхед на ровном месте при изучении и использовании фреймворка. Пришло время попрощаться с ними.
Попрощаться с Yii?
На самом деле можно и заменить. По крайней мере то, что у нас из коробки сделано mixin-ами мы попробовали переделать на трейты и получилось вполне удобоваримо.
Минус, конечно, в ещё более неканоническом тестировании. Плюс в возможности выпилить логику behavior-ов из базового класса и, возможно, вообще прибить этот класс.
Минус, конечно, в ещё более неканоническом тестировании. Плюс в возможности выпилить логику behavior-ов из базового класса и, возможно, вообще прибить этот класс.
А какая разница, 90% проектов на Yii всеравно только интеграционными тестами можно покрыть, чаго уж там париться.
90% вообще всех проектов вообще, вне зависимости от фреймворка, можно покрыть только интеграционными или функциональными тестами.
я в принципе думаю что разработка через TDD с фреймворками использующими ActiveRecord это боль и страдания и проще использовать ATDD или же писать сценарии на бихате/кодесепшене а про юнит тесты по большому счету забыть. А если так — то разницы особо нет трейтами бихейверы сделаны или отдельной сущностью.
Я не считаю что это хорошо. Но иногда оправдано.
Я не считаю что это хорошо. Но иногда оправдано.
Не думаю, что виноват во всём AR. То, что особенно важно тестировать, обычно не AR.
вот только практика показывает что «то что важно тестировать» обычно у разработчиков попадает в контроллеры и модель. Ни то ни другое юнит тестами красиво не потестить.
Ну это не проблемы AR как такового.
А чьи же? Как покрывать тестами модель в контексте AR? Как мокать методы самого себя? Остаются только инетграционные тесты.
AR (как паттерн) по определению плохо покрывается юнит-тестами, поскольку содержит логику работы с базой данных. В зависимости от реализации это может быть чуть проще или чуть сложнее, но всё равно плохо.
А уж если в классе, реализующем AR, в одном методе содержится и бизнес-логика, и логика работы с БД без инжектирования одного в другое (что бывает крайне редко, кто станет писать постоянно $feature->setIsActiveAndSave(true, $feature), чтобы вызывать $feature->save(), вместо хорошо если $feature->setIsActiveAndSave(true), а то и $feature->setIsActive(true), чтобы вызывать $this->save() ?), то юнит-тестирование становится практически невозможным.
А уж если в классе, реализующем AR, в одном методе содержится и бизнес-логика, и логика работы с БД без инжектирования одного в другое (что бывает крайне редко, кто станет писать постоянно $feature->setIsActiveAndSave(true, $feature), чтобы вызывать $feature->save(), вместо хорошо если $feature->setIsActiveAndSave(true), а то и $feature->setIsActive(true), чтобы вызывать $this->save() ?), то юнит-тестирование становится практически невозможным.
Чаще как раз самое важное в AR, если не сводить его к Table/RowGateway.
Саш, а вы не где не шарили это?
Между traits и behaviors есть одно принципиальное отличие, о котором иногда забывают: traits — это статическая конструкция, их использование определяется на этапе объявления класса, их нельзя ни «включить», ни «выключить».
А behaviors — это динамическая конструкция, которая используется уже после объявления класса, и может быть условной. Понимаю прекрасно, что в неумелых руках это может повлечь непредсказуемое поведение класса. Но в то же время это дает гораздо большую гибкость в расширении по сравнению с трейтами.
Как бы то ни было, при своей похожести traits и behaviors — это все ж разные сущности.
А behaviors — это динамическая конструкция, которая используется уже после объявления класса, и может быть условной. Понимаю прекрасно, что в неумелых руках это может повлечь непредсказуемое поведение класса. Но в то же время это дает гораздо большую гибкость в расширении по сравнению с трейтами.
Как бы то ни было, при своей похожести traits и behaviors — это все ж разные сущности.
Особых проблем с «отключением» traits в принципе тоже нет, например
Пример реализации
Cat::$softDelete = false;
$cats = Cat::find()->all(); // All cats include deleted
Пример реализации
Сущности, конечно разные. Включать и выключать трейт можно. Это равносильно подписке на события и отписке от них.
Мои мысли:
1. Ваш подход интересный, но трейты не заменят бихейвиоры на 100%, но где то их можно потеснить.
2. Я был бы рад, если бы softdelete был реализован на уровне ActiveRecord в самом yii
1. Ваш подход интересный, но трейты не заменят бихейвиоры на 100%, но где то их можно потеснить.
2. Я был бы рад, если бы softdelete был реализован на уровне ActiveRecord в самом yii
Зарегистрируйтесь на Хабре, чтобы оставить комментарий
Yii2 bad behaviors