voskobovich / yii2-linker-behavior

This behavior makes it easy to maintain many-to-many and one-to-many relations in your ActiveRecord models in Yii2.
Other
80 stars 19 forks source link

Обновление viaTableAttributesValue #3

Closed evgen-d closed 7 years ago

evgen-d commented 7 years ago

Настраиваем поведение в модели:

    public function behaviors()
    {
        return [
            [
                'class' => LinkerBehavior::class,
                'relations' => [
                    'roles' => [
                        'relatedRoles',
                        'updater' => [
                            'class' => ManyToManySmartUpdater::class,
                            'viaTableAttributesValue' => [
                                'created_at' => time(),
                            ],
                        ],
                    ],
                ],
            ],
        ];
    }

Я ожидаю, что поле created_at будет заполняться в базе только когда добавляется новая связь. По факту получаю обновление поля при каждом сохранении модели, даже если связанные данные не менялись.

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

voskobovich commented 7 years ago

@evgen-d Я понял твой вопрос, давай разбираться.

При обновлении связи нетронутые строки (не удалены и не добавлены) в таблице не должны удаляться и создаваться заново - это верное поведение при выбранном ManyToManySmartUpdater.

Но ведь ты можешь захотеть изменить значение в ячейке нетронутой строки и поведение должно дать тебе это сделать. В твоей ситуации ты как раз попал на обновление значений в ячейке. Так как значение time() каждый раз разное - поведение его обновляет и это правильно.

А вот как решить твою задачу красиво, нужно подумать. Ты можешь использовать callback для формирования заначения атрибута. Там даже есть флаг для отличия новая\старая запись. Но там нет старого значения...

Посмотри код ниже я оставил там комментарии.

    public function behaviors()
    {
        return [
            [
                'class' => LinkerBehavior::class,
                'relations' => [
                    'roles' => [
                        'relatedRoles',
                        'updater' => [
                            'class' => ManyToManySmartUpdater::class,
                            'viaTableAttributesValue' => [
                                'created_at' => function ($thisCallback, $behavior, $relatedPk, $isNewRecord) {
                                    // $thisCallback - это callback в которм мы сейчас, зачем он? :)))
                                    // $oldValue - Этого значения здесь нет
                                    return $isNewRecord ? time() : $oldValue;
                                },
                            ],
                        ],
                    ],
                ],
            ],
        ];
    }

Мне нужно время чтобы придумать красивое решение. Если у тебя есть предложения - с радостью рассмотрю и сделаю :)

evgen-d commented 7 years ago

Сейчас работает вот такой callback:

function ($updater, $relatedPk, $isNewRecord) {}

В readme у тебя сейчас другое описание callback'а

Можно было бы пользоваться callback'ом, если бы там было старое значение атрибута, да, мне бы это подошло.

Мне кажется, что гораздо чаще нужно именно заполнить поле при сохранении новой связи. Единственное, что могу придумать в пользу обновления данных в связующей таблице - это обновление данных в зависимости от данных в основной модели, но основная модель не передается в callback. Может все таки не трогать данные viaTable после их заполнения, или придумать другой механизм для их изменения, не через viaTableAttributesValue?

voskobovich commented 7 years ago

Ну не трогать их не получиться, так как задачи действительно есть на обновлени строк в связующей таблице после создания.

Скажи, а где ты взял эти данные?

function ($updater, $relatedPk, $isNewRecord) {}

Так как я вижу другую картинку https://github.com/voskobovich/yii2-linker-behavior/blob/master/src/updaters/BaseManyToManyUpdater.php#L64

evgen-d commented 7 years ago

Все верно, там вызывается call_user_func, первым параметром передается сама функция, затем параметры, то есть сама функция выглядит так - function ($updater, $relatedPk, $isNewRecord)

voskobovich commented 7 years ago

@evgen-d Блин, точно, завтыкал!

Ок, смотри. На этой строке возвращается null.

А что если добавить проверку здесь и здесь на null и не включать эти строки в выборку, тем самым дать возмоность базе самой решать что делать?

А то сейчас получается, что мы всегда передаем в базу либо значение либо null не спрашивая у нее как она настроена.

Что скажешь?

evgen-d commented 7 years ago

Я так понимаю, что если мы передадим то же значение, что и было ранее, то строка обновляться не будет, меня бы устроило такое поведение. NULL это все таки значение, которое может присваиваться ячейке, нельзя его игнорить.

voskobovich commented 7 years ago

Да, ты прав.

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

А чтобы это можно было делать полноценно, нужно дать ему $oldValue для атрибута.

evgen-d commented 7 years ago

Да, конечно

voskobovich commented 7 years ago

Посмотри ветку issue-3. Мне реализация не очень нравится, надо будет подумать еще. Есть идеи как красивее передать $isNewRecord в паре с $oldValue? Я подумываю в объект завернуть.

Поставь себе в проект, посмотри работает или нет? Я пока тесты не пишу.

voskobovich commented 7 years ago

@evgen-d есть результаты по тестам?

evgen-d commented 7 years ago

@voskobovich сейчас нет возможности протестировать

evgen-d commented 7 years ago

Проверил, работает вот такая конструкция:

'created_at' => function ($updater, $relatedPk, $isNewRecord, $oldValue) {
    return $isNewRecord ? time() : $oldValue;
}
voskobovich commented 7 years ago

@evgen-d отлично! Я на выходных думал как лаконичнее передать в коллбек состояние строки и oldValue. Вот что я предлагаю.

  1. Создать класс ManyToManyRowCondition который будет хранить в себе 2 атрибута isNewRecord и oldValue.
  2. Далее, в коллбек вместо двух параметров передаваться будет всегда одно rowCondition.

И далее логика будет строиться как-то так:

'created_at' => function ($updater, $relatedPk, $rowCondition) {
    return $rowCondition->isNewRecord ? time() : $rowCondition->oldValue;
}

Таким образом мы завернем в один объект два связанных по логике атрибута isNewRecord и oldValue. Так как oldValue может существовать только, если isNewRecord = false.

Что скажешь? Есть идеи?

evgen-d commented 7 years ago

В принципе все равно как выглядит callback, главное, чтобы это было в readme

voskobovich commented 7 years ago

@evgen-d ок, понял. Тогда все оформлю и выпущю релиз.

voskobovich commented 7 years ago

@evgen-d Протестируй пожалуйста изменения и я готов заливать релиз если все ок. https://github.com/voskobovich/yii2-linker-behavior/tree/issue-3

liongen commented 7 years ago

@voskobovich +1 протестировал, все работает после данного фикса (до этого валилось с той же ошибкой что и у автора issue), можно узнать, когда релиз в мастер?

voskobovich commented 7 years ago

Выпустил версию 4.0.2-rc. Обновляйтесь.