Технологический консалтинг и разработка ПО
142,03
рейтинг
5 января в 08:58

Разработка → Yii2 bad behaviors

Минимальная версия PHP для Yii2 — 5.4. Минимальная версия PHP для Traits — 5.4. Совпадение? Не думаю!



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, но сейчас это оверхед на ровном месте при изучении и использовании фреймворка. Пришло время попрощаться с ними.
Судьба behaviors в Yii2.1
63%
(136)
Попрощаться
37%
(81)
Пускай остаются

Проголосовало 217 человек. Воздержалось 98 человек.

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

Автор: @uaoleg
DataArt
рейтинг 142,03
Технологический консалтинг и разработка ПО

Комментарии (38)

  • 0
    ну Оке, может быть и так. Но. если поуберать все бихейверы, то с ними уйдёт и система проверки авторизации в модуле. А там довольно таки удобная система. Можно несколько методов авторизации на модуль навесить. К примеру для апишки и для обычныйх запросов сайта(если это к примеру spa). А чем это заменить?
    • +3
      Автор предлагает переделать бихейверы в трейты. Не более.
  • +4
    вы расчитываете на то, что они не будут подтягиваться в результаты любых ваших запросов: Cat::find()->all()

    Ну здрасьте приехали. Если я хочу найти все записи, то ожидаю, что «find all» найдет мне все записи. Если мне надо найти только активные записи, то я сделаю метод findActive(), который будет их возвращать.
    А если какой-то программист навесит на all() дополнительную логику, то надо доходчиво объяснить ему, что так делать не надо. Или, например, забрать у него проект, а потом через полгода отдать ему же обратно на длительную поддержку.
    • 0
      Пожалуйста, читайте доки и не переходите на личности.
      • +2
        По вашей ссылке я прочитал следующее:

        This allows you to write query building code like the following:
        $comments = Comment::find()->active()->all();
        $inactiveComments = Comment::find()->active(false)->all();
        

        о чем я, собственно, и говорил. Если нам нужны только активные записи, мы должны это явно указать, а не переопределять метод «все()», чтобы он возвращал не все.

        А если вы приняли выражение «какой-то программист» на свой счет — что ж, значит я попал в точку. Не делайте так, это непонятно.
        • 0
          Основная проблема вашего решения в том, что оно не будет подтягиваться при вызове findOne(), например.

          Про выделение согласен, но если в 99% вызовах (проще говоря везде кроме отчёта в админ панели) надо делать
          Comment::find()->active()->notDeleted()->whatEver()
          
          то, как по мне, лучше сделать это дефолтным поведением и не иметь проблем с «ожившими» записями в самых неподходящих местах.

          А намёк ваш про «какой-то программист» был слишком толстым, не делайте так.
          • +2
            Хм. Ну вообще я предлагаю в 99% случаев писать «Comment::findActive()» без всяких дополнительных стрелочек.

            Про findOne().
            Допустим, когда-то у нас был тариф «Супер», с id=123. Сейчас он неактивен. У нас есть сотня заказов по этому тарифу. Мы хотим посмотреть его параметры. Открываем страницу «tariff/view?id=123». По-вашему, метод findOne() должен вернуть null, а мы должны получить сообщение «Not found»?
            • 0
              Мы ушли от темы. Изначально обсуждались удалённые записи (посты, комменты, товары). Именно удалённые, а не «не активные». И в этом случае должно быть 404 везде.
              • +1
                )) Ну ок, я переформулирую.

                Допустим, когда-то у нас был товар «Супер-отмычка», с id=123. Сейчас он удален. У нас есть сотня выполненных заказов по этому товару, поэтому физически удалить его из базы мы не можем, хотя новые заказы мы на него не принимаем. Пользователь хочет посмотреть параметры своего заказа полугодовой давности. Пользователь открывает страницу «order/view?id=456». В заказе, в числе прочих, есть товар с id=123. По-вашему, метод findOne() должен вернуть null, а пользователь в строчке с товаром должен получить сообщение «Not found»?
                • 0
                  Нет, этот товар будет именно «не активен», «отсутствовать на складе» и т.п.

                  А вот если, например, пользователь соц сети удаляет свой пост, то он должен быть удалён везде-везде. И в ленте, и в репостах, и в списке моих лайков. Везде. Почувствуйте разницу (с)
                  • +1
                    А зачем вам в таком случае soft delete, если удалить надо везде?
                    • +3
                      1) быстрее работает
                      2) можно похранить недельку-месяц что бы чуть что восстановить
                      3) мало ли пригодится.
                      • +2
                        Да это все понятно. Но

                        Во-первых. Это тогда не Yii2 bad behaviors, так как используемый фреймворк тут ни при чем.

                        Во-вторых. Если запись не удаляется из базы, значит она зачем-то нужна. Если нужна, значит где-то есть метод view или list, который такие записи получает. Если получает, значит вызывает метод findOne() или findAll() в каком-то виде. Значит переопределять их нежелательно, так как придется вводить флаг типа reallyShowAll и добавлять условие where в функцию all(). Понятно, что это все будет работать, просто код будет непонятный. Вызываем all(), а возвращается не all().
                        Удаление потом это неплохо, но если в базе есть удаленные и неудаленные записи, то в коде для них нужны отдельные вызовы функций. О чем я и пытаюсь сказать.

                        В-третьих. Если так переопределить findOne(), то восстановить удаленный коммент будет нельзя, так как вызов вернет null.
                        • 0
                          Во-первых. Это тогда не Yii2 bad behaviors, так как используемый фреймворк тут ни при чем.
                          А на чём же мы тогда реализуем эту логику? )

                          Во-вторых, восстановить можно без проблем, отключив режим SoftDelete
                          Post::$softDelete = false;
                          $deletedPost = Post::findOne([...]);
                          $deletedPost->restore();
                          
                          • 0
                            Если кому-то надо похранить удаленные записи недельку-месяц, это явно не проблема фреймворка) Надо — делайте.

                            Во-вторых, это заставляет вызывающий код знать детали реализации. Да и претензия моя, собственно, не в этом, а в том, что метод all() возвращает не all(). А это, в свою очередь, заставляет программиста знать детали реализации. И это уже сложнее.
                        • +1
                          Если запись не удаляется из базы, значит она зачем-то нужна. Если нужна, значит где-то есть метод view или list, который такие записи получает.

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

                          Нужно понимать, что ожидаемое поведение по умолчанию при softDeletable такое же как и без него — запись удалили и обычным путём к ней не достучаться, какой-нибудь Comment::find()->active(false)->all(); прежде всего альтернатива разворачиванию бэкапа и восстановлению из него, а не средство посмотреть список товаров, продажи которых временно приостановлены.
        • +1
          $comments = Comment::find()->active()->all();
          $inactiveComments = Comment::find()->active(false)->all();
          

          Я о PHP знаю очень мало, о Yii ещё меньше. Но выше приведённый пример мне даже очень понятен. А вот логика ваша, как бы, не совсем — метод «все()» возвращает действително «все()» из предыдущего запроса/потока/т.д… Неужели вы будете утверждать, что стримы, пайпы и трансформации это плохо?)
  • 0
    С появлением traits, behaviors на самом деле стали велосипедом.


    Traits !== Mixins. Бехейверы в Yii решают именно задачу расширения поведения моделек, трейты же использовать для этого не совсем ок. Они нужны для устранения тупого дублирования кода, но не дублирования логики.

    Что вы будете делать если вашему «трейту» вдруг понадобится сторонняя зависимость? Будете использовать сервис-локатор с глобальным доступом и тем самым скрывать зависимости? А если тестировать это все (вы же пишите тесты?).

    но сейчас это оверхед на ровном месте при изучении и использовании фреймворка. Пришло время попрощаться с ними.

    Попрощаться с Yii?

    • +1
      На самом деле можно и заменить. По крайней мере то, что у нас из коробки сделано mixin-ами мы попробовали переделать на трейты и получилось вполне удобоваримо.

      Минус, конечно, в ещё более неканоническом тестировании. Плюс в возможности выпилить логику behavior-ов из базового класса и, возможно, вообще прибить этот класс.
      • 0
        А какая разница, 90% проектов на Yii всеравно только интеграционными тестами можно покрыть, чаго уж там париться.
        • 0
          90% вообще всех проектов вообще, вне зависимости от фреймворка, можно покрыть только интеграционными или функциональными тестами.
          • 0
            я в принципе думаю что разработка через TDD с фреймворками использующими ActiveRecord это боль и страдания и проще использовать ATDD или же писать сценарии на бихате/кодесепшене а про юнит тесты по большому счету забыть. А если так — то разницы особо нет трейтами бихейверы сделаны или отдельной сущностью.

            Я не считаю что это хорошо. Но иногда оправдано.
            • 0
              Не думаю, что виноват во всём AR. То, что особенно важно тестировать, обычно не AR.
              • 0
                вот только практика показывает что «то что важно тестировать» обычно у разработчиков попадает в контроллеры и модель. Ни то ни другое юнит тестами красиво не потестить.
                • 0
                  Ну это не проблемы AR как такового.
                  • 0
                    А чьи же? Как покрывать тестами модель в контексте AR? Как мокать методы самого себя? Остаются только инетграционные тесты.
                    • 0
                      Можно логику не пихать в AR, а вместо этого выделить отдельный доменный слой для неё. Да, AR, конечно, в этом случае теряет свою привлекательность.
                      • 0
                        AR и заключается в объединение логики и доступа к базе в одном классе.
                  • 0
                    AR (как паттерн) по определению плохо покрывается юнит-тестами, поскольку содержит логику работы с базой данных. В зависимости от реализации это может быть чуть проще или чуть сложнее, но всё равно плохо.

                    А уж если в классе, реализующем AR, в одном методе содержится и бизнес-логика, и логика работы с БД без инжектирования одного в другое (что бывает крайне редко, кто станет писать постоянно $feature->setIsActiveAndSave(true, $feature), чтобы вызывать $feature->save(), вместо хорошо если $feature->setIsActiveAndSave(true), а то и $feature->setIsActive(true), чтобы вызывать $this->save() ?), то юнит-тестирование становится практически невозможным.
              • +1
                Чаще как раз самое важное в AR, если не сводить его к Table/RowGateway.
      • 0
        Саш, а вы не где не шарили это?
        • 0
          Нет. По-моему, даже нигде не сохранили. Там всё предельно просто вышло, повторить можно в любой момент.
  • +2
    Между traits и behaviors есть одно принципиальное отличие, о котором иногда забывают: traits — это статическая конструкция, их использование определяется на этапе объявления класса, их нельзя ни «включить», ни «выключить».

    А behaviors — это динамическая конструкция, которая используется уже после объявления класса, и может быть условной. Понимаю прекрасно, что в неумелых руках это может повлечь непредсказуемое поведение класса. Но в то же время это дает гораздо большую гибкость в расширении по сравнению с трейтами.

    Как бы то ни было, при своей похожести traits и behaviors — это все ж разные сущности.
    • –2
      Особых проблем с «отключением» traits в принципе тоже нет, например
      Cat::$softDelete = false;
      $cats = Cat::find()->all(); // All cats include deleted
      

      Пример реализации
      • +1
        По большому счету, это не отключение трейта, как таковое, а изменение его поведения в зависимости от условий. И это далеко не то же самое, что подключение/отключение требуемых behaviors в зависимости от условий.
        • 0
          Согласен
    • +1
      Сущности, конечно разные. Включать и выключать трейт можно. Это равносильно подписке на события и отписке от них.
  • +2
    Мои мысли:
    1. Ваш подход интересный, но трейты не заменят бихейвиоры на 100%, но где то их можно потеснить.
    2. Я был бы рад, если бы softdelete был реализован на уровне ActiveRecord в самом yii

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

Самое читаемое Разработка