Contributing a method
boazrymland opened this issue · 4 comments
Hi,
I improved the deleteNode() and refactored it to the following two methods:
- deleteNodeWithChildren() . Just renaming deleteNode() to this new name.
- deleteNodeKeepChildren(). I've added this new method. Using this method, we can delete a node but retain its descendants tree, according to the following algorithm:
- If the node is a root node:
- if we're in 'many roots mode' - its children will become roots as well and this node will be deleted
- if we're not in 'many roots mode' and more than 1 children exists - exception will be thrown. If only one child exists it will be promoted to the root and the current, 'old' root will be deleted. If no children exists, node will be deleted.
- If the node is a parent (but NOT a root node): its children (if any) will be children of this node's parent
- If the node is a leaf - normal deletion will follow
It took many many hours of close contact my with debugger (no way this could have been successfully been prepared without a debugger... :) ).
Do you want me to contribute this as a PR?
Sorry, there was no plains to add any new methods to this behavior since its totally done. You can move children before delete node to have effect like deleteNodeKeepChildren().
Yes, I know this functionality is possible with current API. In fact, what I did was using current API. I invented nothing. Yet, the task of developing this functionality is non-trivial in both its complexity and the time resources it would take. Not to mention bug following squashing. So, I thought that why not share this code so other developers would not have to go through the same thing.
I'm quoting below the code, for your consideration (apologize for indentation style. just FYI - that's the righteous indetation style ;-) ).
The final word is yours...
/**
* Deletes a node but not its descendants:
* - If the node is a root node:
* if we're in 'many roots mode' - its children will become roots as well and this node will be deleted
* if we're not in 'many roots mode' and more than 1 children exists - exception will be thrown. If only one child exists
* it will be promoted to the root and the current, 'old' root will be deleted. If no children exists, node will be
* deleted.
* - If the node is a parent (but NOT a root node):
* its children (if any) will be children of this node's parent
* - If the node is a leaf - normal deletion will follow
*
* @return boolean whether the deletion is successful.
* @throws CException
* @throws Exception
*/
public function deleteNodeKeepChildren() {
$owner = $this->getOwner();
$children = $owner->children()->findAll();
if ($owner->isRoot()) {
if (!$this->hasManyRoots) {
if (count($children) > 1) {
/*
* this is "single root" therefore its unclear how the children shall be treated
*/
throw new CException(Yii::t('yiiext', 'Cannot delete root in single root mode while this root has more ' .
'than 1 child - I do not know how to handle the children!'));
}
else if (count($children) == 1) {
/*
* 1 child in a single root use case - kill the old king and crown the heir
*/
$heir = array_pop($children);
$db = $owner->getDbConnection();
if ($db->getCurrentTransaction() === null)
$transaction = $db->beginTransaction();
try {
$result = $owner->deleteByPk($owner->primaryKey);
// clear... make a new root
$result = $result && $heir->moveAsRoot();
if (!$result) {
if (isset($transaction))
$transaction->rollback();
return false;
}
if (isset($transaction))
$transaction->commit();
} catch (Exception $e) {
if (isset($transaction))
$transaction->rollback();
throw $e;
}
}
else {
/*
* num of child of this root is zero. simply delete it... .
*/
return $owner->deleteNodeWithChildren();
}
}
else {
/*
* use case here: root node in "many roots" setup. first make each of its children a root on its own and
* then delete the root node.
*/
$db = $owner->getDbConnection();
if ($db->getCurrentTransaction() === null)
$transaction = $db->beginTransaction();
try {
foreach ($children as $child) {
// in between iterations of this foreach the children lft,rgt,root,level values are changing
// due to the operation of moveAsRoot() on their "family tree". Therefore, to maintain integrity we
// must 'refresh' each child tree before handling it, by reloading it
$child_class = get_class($child);
$reloaded_child = $child_class::model()->findByPk($child->primaryKey);
$result = $reloaded_child->moveAsRoot();
if (!$result) {
if (isset($transaction))
$transaction->rollback();
return false;
}
}
// delete the original root node. there should be no children left for it by now.
$result = $owner->deleteNodeWithChildren();
if (!$result) {
if (isset($transaction))
$transaction->rollback();
return false;
}
if (isset($transaction))
$transaction->commit();
} catch (Exception $e) {
$transaction->rollback();
if (isset($transaction))
throw $e;
}
}
}
else if (count($children) > 0) {
/*
* this is a parent node (but not root). bubble up the children to be children of this owner's parent
* and then delete owner.
*/
$db = $owner->getDbConnection();
if ($db->getCurrentTransaction() === null)
$transaction = $db->beginTransaction();
try {
$parent = $owner->parent()->find();
foreach ($children as $child) {
// get last child of $parent (it changes with each iteration of this loop!...
$to_be_brothers = $parent->children()->findAll(
array(
'condition' => 'id!=:id',
'params' => array(':id' => $owner->id),
)
);
// in between iterations of this foreach the $children lft,rgt,root,level values are changing
// due to the usage of moveAfter() on their "family tree". Therefore, to maintain integrity we
// must 'refresh' each child tree before handling it, by reloading it
$child_class = get_class($child);
$reloaded_child = $child_class::model()->findByPk($child->primaryKey);
// if $owner was the only child of its parent then to_be_brothers will be empty. therefore, we need
// to move the $reloaded child node using the correct method
if (count($to_be_brothers) == 0) {
$result = $reloaded_child->moveAsFirst($parent);
}
else {
$result = $reloaded_child->moveAfter(array_pop($to_be_brothers));
}
if (!$result) {
if (isset($transaction))
$transaction->rollback();
return false;
}
}
// delete the original parent node. Note that there should be no children for it by now so its safe
$result = $owner->deleteNodeWithchildren();
if (!$result) {
if (isset($transaction))
$transaction->rollback();
return false;
}
if (isset($transaction))
$transaction->commit();
} catch (Exception $e) {
$transaction->rollback();
if (isset($transaction))
throw $e;
}
}
else {
/*
* Not a parent or root node but rather a simple leaf. Delete it
*/
return $owner->deleteNodeWithChildren();
}
return true;
}Yes, I know this functionality is possible with current API.
Key words ;-)
Yet, the task of developing this functionality is non-trivial in both its complexity
There is a lot of such tasks. Library provide only simple methods to solve complex things.
There is a lot of such tasks. Library provide only simple methods to solve complex things.
While I agree with the principal (a lot) the refactoring of deleteNote() to deleteNodeKeepChildren() and deleteNodeWithChildren() isn't in a level that is distinctly above the level of API methods currently in the library.
Also, I would not like to release a fork of this library just for this refactoring. This clutters the 'map' of nested set library for Yii, IMHO.
Taking the two reasons above combined, if the code seems ok (and could of course be altered by you) I would have merged it into the 'trunk' of the library.
Thanks anyway,
Boaz.