creocoder / yii2-nested-sets

The nested sets behavior for the Yii framework.
Other
446 stars 129 forks source link

Issue 53: Allow to use treeAttribute as external key #68

Open mikehaertl opened 9 years ago

mikehaertl commented 9 years ago

Fix for issue #53

mikehaertl commented 9 years ago

@creocoder Any news on this?

creocoder commented 9 years ago

@mikehaertl In general fix is much more complex than introduced in that PR. So i'll take this ticket myself. I'll explain why. There should be special "externalRoots" mode. And behavior should take that into account. For example roots() method should throw exception when used in that mode, etc. So "externalRoots" mode is not so trivial to implement. On other side its not general direction of that behavior. This is why this ticket in low priority.

mikehaertl commented 9 years ago

Hmm, can you explain that a bit more? I don't think that such a mode is really neccessary. And why do you think that roots() should throw an exception in that mode? How would you obtain the root nodes in that case?

The only problem i can see is something like this:

$root1 = new Category(['tree' => 2, 'name' => 'test']);
$root1->makeRoot();
echo $root1->id; // == 1

$root2 = new Category(['name' => 'test2']);
$root2->makeRoot();
echo $root2->id; // == 2

The last call to makeRoot() would mess things up, as it now would use the same tree id 2 for $root2. But this could also be solved if we'd just insert another check in afterInsert(), whether the tree id does already exist.

creocoder commented 9 years ago

@mikehaertl Seems you misunderstood original issue. External roots mean roots in different table. So no roots in NS table at all. Your PR is about allow use custom ids for roots, but roots is still exists inside NS table. This not solve original issue.

mikehaertl commented 9 years ago

Hmm. But won't you always need a root node to have a consistent tree in your node table?