Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug doesnt set nodes as root #54

Closed
AngryUbuntuNerd opened this issue Mar 26, 2014 · 3 comments
Closed

bug doesnt set nodes as root #54

AngryUbuntuNerd opened this issue Mar 26, 2014 · 3 comments

Comments

@AngryUbuntuNerd
Copy link

Hi there,

I just ran into this bug. When having a node with parent_id set, setting parent_id to null will not result in the node being made root. I figured the reason lies in the function Node.php/storeNewParent()

public function storeNewParent() {
    $dirty = $this->getDirty();

    if ( isset($dirty[$this->getParentColumnName()]) )

Instead it should be

public function storeNewParent() {
    if ( $this->isDirty($this->getParentColumnName()) && !(!$this->exists && $this->isRoot()) ) {

As isset returns false when the value is null, the is_null in the following function moveToNewParent() can otherwise never be true.

Edit: Added check to avoid setting new root node as root again - this throws an error in makeRoot() otherwise.

@etrepat
Copy link
Owner

etrepat commented Mar 26, 2014

Yes. The isset() there can be error prone because it returns false even when the value is NULL. I'll prepare a patch to change that check to use the isDirty() function.

Thanks for spotting this & providing a possible approach for a solution.

p.s. I don't quite follow the second existance/root checks. In which case makeRoot()fails?

@AngryUbuntuNerd
Copy link
Author

Thanks for that. The second check in brackets ensures that new elements [ !$this->exists() ] without parent_id [ $this->isRoot() ] dont get set as root again. This is being done already by Node.php/setDefaultLeftAndRight() via creating-event.
Otherwise the same happens like in ticket #56

@etrepat etrepat closed this as completed in a822bfc May 2, 2014
@etrepat
Copy link
Owner

etrepat commented May 2, 2014

Merged the changes.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants