External roots (FK to another table)

[stifff]

В мультируте есть такой код

$this->owner->setAttribute($this->treeAttribute, $this->owner->getPrimaryKey());

Который автоматически меняет значение treeAttribute, что делает невозможным использование поля treeAttribute в связи по foreign key с другой таблицей. Может быть есть возможность разрешить использовать своё значение treeAttribute при создании root?

[stifff]

например, сделать так . - if ($this->operation === self::OPERATION_MAKE_ROOT && $this->treeAttribute !== false) { . + if ($this->operation === self::OPERATION_MAKE_ROOT && $this->treeAttribute !== false && !$this->owner->getAttribute($this->treeAttribute)) {

[creocoder]

Да, будет такая возможность.

[creocoder]

@stifff Анализ необходимых изменений показал, что такую возможность делать идеологически не правильно. Рассматривайте treeAttribute как внутренний механизм для работы multiple root mode. Если необходима связь с какой либо внешней таблицей нужно завести дополнительный ключ для связи с этой таблицей.

[creocoder]

@stifff В будущих версиях скорее всего это будет реализовано через опцию enableExternalRoots при которой корни будут существовать в каких то внешних таблицах, т.к. если просто разрешить свои значения treeAttribute это делает все существующие корни в таблице лишними.

[stifff]

А почему лишними? Своё значение treeAttribute нужно только во время создания корня.

Код с патчем из второго коммента имеет право на жизнь? Или там во время использования вылезут не очевидные, на первый взгляд, вещи?

[creocoder]

@stifff

А почему лишними?

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

Код с патчем из второго коммента имеет право на жизнь? Или там во время использования вылезут не очевидные, на первый взгляд, вещи?

Не имеет права на жизнь, т.к. дизайн на текущий момент учитывает что root === id реальных корней, которые содержатся в таблице и вся логика построена вокруг этого для multiple tree mode. Что верно.

То, что вы просите это дополнительная feature. Совершенно другой вариант multiple tree mode. Будем реализовывать, но посредством дополнительной опции. Это даст возможность делать root внешним ключем и полностью устранить корневые узлы из таблицы. Например это может быть крайне интересно для древовидных комментариев, когда есть таблица comment в которой к примеру есть post_id (root). Таким образом корнем комментариев будет являться сама сущность post.

[mikehaertl]

@creocoder I don't speak russian, but I think my issue is related. I use treeAttribute as reference to another table (shop). Whenever a new shop is created, a new root node in category should be added:

    public function afterSave($insert, $changedAttributes)
    {
        parent::afterSave($insert, $changedAttributes);
        if ($insert) {
            $category = new Category([
                'name' => $this->name,
                'shop_id' => $this->id,
            ]);
            $category->makeRoot();
        }
    }

In category, treeAttribute is configured as shop_id. The above code breaks, because the treeAttribute is always set to the pk value of the root category, when i call makeRoot().

So +1 to the suggested change: The behavior should only set the treeAttribute, if it is still empty. I think, this change is backwards compatible.

[mikehaertl]

As this is blocking me in a current project (and I really don't want to add another column to the category table), I wonder, if we can speed up this fix a little.

I think, the only real issue with backwards compatibility is, that the tree table could potentially become inconsistent.

UPDATED: Removed invalid code example. Working fix provided as PR below.

[Mihai-P]

make a fork, make a test for the change, make sure the tests passes, make a pull request. And please do not break compatibility.

[mikehaertl]

Working on it. But the tests are extremely ugly to extend as you'd have to edit dozens of data files (each one depends on the result of the previous test). So for now I'll just add the new test at the end, otherwhise it's too tedious (@creocoder: I really recommend to refactor this so it's easier to extend).