Yii2 уже давно пора избавиться от этих плохих поведений. И вот почему.
Возьмём в качестве примера реализацию поведения SoftDelete для ActiveRecord с использованием PHP Traits. И рассмотрим по пунктам, что нам это даёт.
Бо?льший контроль над моделью
Очевидно, что помечая записи как «удалённые», вы расчитываете на то, что они не будут подтягиваться в результаты любых ваших запросов: Cat::find()->all(), Cat::find()->one(), и т.д. Yii1 позволяло реализовать такую функциональность в событии CActiveRecord::beforeFind(). Однако после основательной переработки ActiveRecord в Yii2, такого события по понятным причинам уже нет, и задача переходит в разряд нерешаемых, т.к. поведения могут работать только с событиями AR. Напротив, при использовании traits вы вольны переопределять любой метод модели, например, так:
public static function find()
{
$query = parent::find();
// Skip deleted items
if (static::$softDelete) {
$query->andWhere([static::tableName() . '.' . static::$softDeleteAttribute => null]);
}
return $query;
}
Полная поддержка IDE
Да, используя behaviors вы можете добавить в модель новый атрибуты, но IDE об этом ничего знать не будет, и ни о каком автокомплите речь даже не идёт. А вот при использовании traits, IDE отлично подхватывает все новые методы и свойства, в том числе виртуальные:
* @property-read bool $isDeleted
*/
trait SoftDelete
И теперь свойство isDeleted будет подсвечиваться во всех объектах AR, к которым подключён trait SoftDelete:
class Cat extends ActiveRecord
{
use \common\traits\SoftDelete;
}
$cat = (new Cat)->isDeleted; // Property "isDeleted" autocompleted by IDE
Минус один велосипед
С появлением traits, behaviors на самом деле стали велосипедом. И добросовестный разработчик, рискует весьма сильно пострадать, потратив немало времени на изучение этого заменителя traits, и попытки решить вышеизложенные проблемы. Это было действительно хорошим решением в Yii1, но сейчас это оверхед на ровном месте при изучении и использовании фреймворка. Пришло время попрощаться с ними.
Только зарегистрированные пользователи могут участвовать в опросе. Войдите, пожалуйста.
Комментарии (38)
michael_vostrikov
05.01.2016 11:04+4вы расчитываете на то, что они не будут подтягиваться в результаты любых ваших запросов: Cat::find()->all()
Ну здрасьте приехали. Если я хочу найти все записи, то ожидаю, что «find all» найдет мне все записи. Если мне надо найти только активные записи, то я сделаю метод findActive(), который будет их возвращать.
А если какой-то программист навесит на all() дополнительную логику, то надо доходчиво объяснить ему, что так делать не надо. Или, например, забрать у него проект, а потом через полгода отдать ему же обратно на длительную поддержку.
uaoleg
05.01.2016 11:50Пожалуйста, читайте доки и не переходите на личности.
michael_vostrikov
05.01.2016 12:10+2По вашей ссылке я прочитал следующее:
This allows you to write query building code like the following:
$comments = Comment::find()->active()->all(); $inactiveComments = Comment::find()->active(false)->all();
о чем я, собственно, и говорил. Если нам нужны только активные записи, мы должны это явно указать, а не переопределять метод «все()», чтобы он возвращал не все.
А если вы приняли выражение «какой-то программист» на свой счет — что ж, значит я попал в точку. Не делайте так, это непонятно.uaoleg
05.01.2016 12:28Основная проблема вашего решения в том, что оно не будет подтягиваться при вызове findOne(), например.
Про выделение согласен, но если в 99% вызовах (проще говоря везде кроме отчёта в админ панели) надо делать
то, как по мне, лучше сделать это дефолтным поведением и не иметь проблем с «ожившими» записями в самых неподходящих местах.Comment::find()->active()->notDeleted()->whatEver()
А намёк ваш про «какой-то программист» был слишком толстым, не делайте так.michael_vostrikov
05.01.2016 12:51+2Хм. Ну вообще я предлагаю в 99% случаев писать «Comment::findActive()» без всяких дополнительных стрелочек.
Про findOne().
Допустим, когда-то у нас был тариф «Супер», с id=123. Сейчас он неактивен. У нас есть сотня заказов по этому тарифу. Мы хотим посмотреть его параметры. Открываем страницу «tariff/view?id=123». По-вашему, метод findOne() должен вернуть null, а мы должны получить сообщение «Not found»?uaoleg
05.01.2016 13:15Мы ушли от темы. Изначально обсуждались удалённые записи (посты, комменты, товары). Именно удалённые, а не «не активные». И в этом случае должно быть 404 везде.
michael_vostrikov
05.01.2016 13:52+1)) Ну ок, я переформулирую.
Допустим, когда-то у нас был товар «Супер-отмычка», с id=123. Сейчас он удален. У нас есть сотня выполненных заказов по этому товару, поэтому физически удалить его из базы мы не можем, хотя новые заказы мы на него не принимаем. Пользователь хочет посмотреть параметры своего заказа полугодовой давности. Пользователь открывает страницу «order/view?id=456». В заказе, в числе прочих, есть товар с id=123. По-вашему, метод findOne() должен вернуть null, а пользователь в строчке с товаром должен получить сообщение «Not found»?uaoleg
05.01.2016 14:02Нет, этот товар будет именно «не активен», «отсутствовать на складе» и т.п.
А вот если, например, пользователь соц сети удаляет свой пост, то он должен быть удалён везде-везде. И в ленте, и в репостах, и в списке моих лайков. Везде. Почувствуйте разницу (с)michael_vostrikov
05.01.2016 14:39+1А зачем вам в таком случае soft delete, если удалить надо везде?
Fesor
05.01.2016 14:50+31) быстрее работает
2) можно похранить недельку-месяц что бы чуть что восстановить
3) мало ли пригодится.michael_vostrikov
05.01.2016 16:08+2Да это все понятно. Но
Во-первых. Это тогда не Yii2 bad behaviors, так как используемый фреймворк тут ни при чем.
Во-вторых. Если запись не удаляется из базы, значит она зачем-то нужна. Если нужна, значит где-то есть метод view или list, который такие записи получает. Если получает, значит вызывает метод findOne() или findAll() в каком-то виде. Значит переопределять их нежелательно, так как придется вводить флаг типа reallyShowAll и добавлять условие where в функцию all(). Понятно, что это все будет работать, просто код будет непонятный. Вызываем all(), а возвращается не all().
Удаление потом это неплохо, но если в базе есть удаленные и неудаленные записи, то в коде для них нужны отдельные вызовы функций. О чем я и пытаюсь сказать.
В-третьих. Если так переопределить findOne(), то восстановить удаленный коммент будет нельзя, так как вызов вернет null.uaoleg
05.01.2016 16:23Во-первых. Это тогда не Yii2 bad behaviors, так как используемый фреймворк тут ни при чем.
А на чём же мы тогда реализуем эту логику? )
Во-вторых, восстановить можно без проблем, отключив режим SoftDelete
Post::$softDelete = false; $deletedPost = Post::findOne([...]); $deletedPost->restore();michael_vostrikov
05.01.2016 17:36Если кому-то надо похранить удаленные записи недельку-месяц, это явно не проблема фреймворка) Надо — делайте.
Во-вторых, это заставляет вызывающий код знать детали реализации. Да и претензия моя, собственно, не в этом, а в том, что метод all() возвращает не all(). А это, в свою очередь, заставляет программиста знать детали реализации. И это уже сложнее.
VolCh
05.01.2016 18:08+1Если запись не удаляется из базы, значит она зачем-то нужна. Если нужна, значит где-то есть метод view или list, который такие записи получает.
Совсем не обязательно она нужна приложению. Она может быть нужна базе данных — некоторые базы при некоторых условиях могут удалять запись очень долго, заметно дольше чем обновляя запись. Она может быть нужна другому приложению. Она сейчас может быть вообще не нужна никому, но решили сделать softDeletable на всякий случай.
Нужно понимать, что ожидаемое поведение по умолчанию при softDeletable такое же как и без него — запись удалили и обычным путём к ней не достучаться, какой-нибудь Comment::find()->active(false)->all(); прежде всего альтернатива разворачиванию бэкапа и восстановлению из него, а не средство посмотреть список товаров, продажи которых временно приостановлены.
tenbits
05.01.2016 14:56+1
Я о PHP знаю очень мало, о Yii ещё меньше. Но выше приведённый пример мне даже очень понятен. А вот логика ваша, как бы, не совсем — метод «все()» возвращает действително «все()» из предыдущего запроса/потока/т.д… Неужели вы будете утверждать, что стримы, пайпы и трансформации это плохо?)$comments = Comment::find()->active()->all(); $inactiveComments = Comment::find()->active(false)->all();
Fesor
05.01.2016 11:43С появлением traits, behaviors на самом деле стали велосипедом.
Traits !== Mixins. Бехейверы в Yii решают именно задачу расширения поведения моделек, трейты же использовать для этого не совсем ок. Они нужны для устранения тупого дублирования кода, но не дублирования логики.
Что вы будете делать если вашему «трейту» вдруг понадобится сторонняя зависимость? Будете использовать сервис-локатор с глобальным доступом и тем самым скрывать зависимости? А если тестировать это все (вы же пишите тесты?).
но сейчас это оверхед на ровном месте при изучении и использовании фреймворка. Пришло время попрощаться с ними.
Попрощаться с Yii?
SamDark
05.01.2016 18:55+1На самом деле можно и заменить. По крайней мере то, что у нас из коробки сделано mixin-ами мы попробовали переделать на трейты и получилось вполне удобоваримо.
Минус, конечно, в ещё более неканоническом тестировании. Плюс в возможности выпилить логику behavior-ов из базового класса и, возможно, вообще прибить этот класс.Fesor
05.01.2016 19:06А какая разница, 90% проектов на Yii всеравно только интеграционными тестами можно покрыть, чаго уж там париться.
SamDark
05.01.2016 19:2690% вообще всех проектов вообще, вне зависимости от фреймворка, можно покрыть только интеграционными или функциональными тестами.
Fesor
05.01.2016 20:05я в принципе думаю что разработка через TDD с фреймворками использующими ActiveRecord это боль и страдания и проще использовать ATDD или же писать сценарии на бихате/кодесепшене а про юнит тесты по большому счету забыть. А если так — то разницы особо нет трейтами бихейверы сделаны или отдельной сущностью.
Я не считаю что это хорошо. Но иногда оправдано.SamDark
05.01.2016 21:53Не думаю, что виноват во всём AR. То, что особенно важно тестировать, обычно не AR.
Fesor
05.01.2016 21:55вот только практика показывает что «то что важно тестировать» обычно у разработчиков попадает в контроллеры и модель. Ни то ни другое юнит тестами красиво не потестить.
SamDark
05.01.2016 23:58Ну это не проблемы AR как такового.
Fesor
06.01.2016 00:37А чьи же? Как покрывать тестами модель в контексте AR? Как мокать методы самого себя? Остаются только инетграционные тесты.
VolCh
06.01.2016 11:04AR (как паттерн) по определению плохо покрывается юнит-тестами, поскольку содержит логику работы с базой данных. В зависимости от реализации это может быть чуть проще или чуть сложнее, но всё равно плохо.
А уж если в классе, реализующем AR, в одном методе содержится и бизнес-логика, и логика работы с БД без инжектирования одного в другое (что бывает крайне редко, кто станет писать постоянно $feature->setIsActiveAndSave(true, $feature), чтобы вызывать $feature->save(), вместо хорошо если $feature->setIsActiveAndSave(true), а то и $feature->setIsActive(true), чтобы вызывать $this->save() ?), то юнит-тестирование становится практически невозможным.
vshemarov
05.01.2016 12:27+2Между traits и behaviors есть одно принципиальное отличие, о котором иногда забывают: traits — это статическая конструкция, их использование определяется на этапе объявления класса, их нельзя ни «включить», ни «выключить».
А behaviors — это динамическая конструкция, которая используется уже после объявления класса, и может быть условной. Понимаю прекрасно, что в неумелых руках это может повлечь непредсказуемое поведение класса. Но в то же время это дает гораздо большую гибкость в расширении по сравнению с трейтами.
Как бы то ни было, при своей похожести traits и behaviors — это все ж разные сущности.uaoleg
05.01.2016 12:32-2Особых проблем с «отключением» traits в принципе тоже нет, например
Cat::$softDelete = false; $cats = Cat::find()->all(); // All cats include deleted
Пример реализации
SamDark
05.01.2016 18:56+1Сущности, конечно разные. Включать и выключать трейт можно. Это равносильно подписке на события и отписке от них.
AlexGx
05.01.2016 16:26+2Мои мысли:
1. Ваш подход интересный, но трейты не заменят бихейвиоры на 100%, но где то их можно потеснить.
2. Я был бы рад, если бы softdelete был реализован на уровне ActiveRecord в самом yii
wir_wolf
ну Оке, может быть и так. Но. если поуберать все бихейверы, то с ними уйдёт и система проверки авторизации в модуле. А там довольно таки удобная система. Можно несколько методов авторизации на модуль навесить. К примеру для апишки и для обычныйх запросов сайта(если это к примеру spa). А чем это заменить?
Fesor
Автор предлагает переделать бихейверы в трейты. Не более.