Love Fuel?    Donate

FuelPHP Forums

Ask your question about FuelPHP in the appropriate forum, or help others by answering their questions.
Using multi-tree nestedset with string tree id
  • DirkSDirkS
    Hi,

    I'm trying to use nested sets multi-tree, but with string as a tree id (as discouraged) instead of int (as suggested in the documentation). The docs say that you can set your own tree id instead of let the system auto generate one for you. I'm wondering how I'm supposed to do that?

    If I 
    a) set the tree id using set_tree_id() on a new root before saving it, then save it and check the id, it again is an auto generated (int) by Fuel
    b) try to set the tree  id as a field obj->tree_id, I get an exception that tree_id is read only

    Btw. I checked the code and found some suspicious comments in Nestedset::save():
    // get the next free tree id, and hope it's numeric...

    What is the right way to set tree id, regardless of string or numeric, when creating a new tree?

    Cheers Dirk

  • HarroHarro
    Accepted Answer
    The tree id of a new tree is, as you found, generated in:

    // multi-root tree? And no new tree id defined?
    if ( ! is_null($tree_field) and empty($this->{$tree_field}))
    {
        // get the next free tree id, and hope it's numeric...
        $this->_data[$tree_field] = $this->max($tree_field) + 1;

        // and set it as the default tree id for this node
        $this->set_tree_id($this->_data[$tree_field]);
    }

    The "I hope it is numeric" means that the max() + 1 query doesn't really work on alphanumeric keys, the result of a max() query depends on the RDBMS in use. max() either returns zero or an error if the column isn't numeric.

    If it still executes this code after you've set the id manually using set_tree_id(), that suggests there is a bug in set_tree_id().

    I have to say after all these years that code confuses me a bit. It sets (or gets) $_current_tree_id, but that variable isn't used anywhere? 

    I don't have time to test this at the moment, can you change the code to:

    public function set_tree_id($tree = null)
    {
        // get params to avoid excessive method calls
        $tree_field = static::tree_config('tree_field');

        // is this a multi-tree model?
        if ($tree_field === null)
        {
            throw new \BadMethodCallException('This is not a multi-tree model, set_tree_id() can not be used.');
        }

        // set the tree filter value to select a specific tree
        $this->_data[$tree_field] = $tree;

        // return the object for chaining
        return $this;
    }

    and see if this fixes your problem?
  • DirkSDirkS
    The fix you suggested helps on setting. However, it breaks the getting of tree_id. I believe (from what I read so far in the code) that $_current_tree_id is only accessed through get/set_tree_id().

    I'd therefore suggest the fix like this:

    public function set_tree_id($tree = null)
    {
    // is this a multi-tree model?
    if (static::tree_config('tree_field') === null)
    {
    throw new \BadMethodCallException('This is not a multi-tree model, set_tree_id() can not be used.');
    }

    // set the tree filter value to select a specific tree
    $this->_current_tree_id = $tree;

           if ($this->_is_new) {
              // ONLY! on new objects, allow setting the DB tree id as well
              $this->_data[$tree_field] = $tree;
           }

    // return the object for chaining
    return $this;
    }


    Btw. in nestedset.php, go to line 1497 and change it like this:

    ->on(\DB::identifier('ancestor.' . $left_field), 'BETWEEN', \DB::expr(($left + 1) . ' AND ' . ($right - 1) . ' AND '.\DB::identifier('ancestor.'.$tree_field).' = '.\DB::quote($this->get_tree_id())))


    Add the DB::quote (or whatever is appropriate, for me this one works) for the tree id at the end of line. With strings as tree id, quoting is incorrect. I'm using 1.8.1.6 code.

    Cheers Dirk
  • HarroHarro
    Things are slowly coming back to me.

    The idea behind the setter/getter is that you could set a tree id that would only be used for when you create a new tree (top-of-tree node), or when you need to get a new top-of-free. It should not set the models current properties. Or worse, overwrite what is already there, which is exactly why you get the exception if you try to set it yourself.

    As far as I can see all get() operations work because build_query() injects a WHERE clause with the result of get_tree_id(), so that is ok.

    But saving a new top-of-tree node doesn't use get_tree_id(), it uses the models tree-id property directly, which is wrong as it is not set.

    So the correct fix should be, from line 1030 onwards:

    // multi-root tree? And no new tree id defined?
    if ( ! is_null($tree_field) and empty($this->{$tree_field}))
    {
        // check if there is a tree-id set
        if (is_null($this->_current_tree_id))
        {
            // nope, generate the next free tree id (hope the column is numeric)...
            $this->_current_tree_id = $this->max($tree_field) + 1;
        }

        // set the tree id explicitly
        $this->_data[$tree_field] = $this->_current_tree_id;
    }

    So: if a tree-id is required  and none is set, generate it and set the generated result. Then save the id set into the properties array, and do a double check to make sure it's unique.

    And yes, the string needs to be quoted there.
  • HarroHarro
    Accepted Answer
    This would even be better:

    // multi-root tree?
    if ( ! is_null($tree_field))
    {
        // and no new tree id defined?
        if (empty($this->{$tree_field}))
        {
            // check if there is a tree-id set
            if (is_null($this->_current_tree_id))
            {
                // nope, generate the next free tree id (hope the column is numeric)...
                $this->_current_tree_id = $this->max($tree_field) + 1;
            }

            // set the tree id explicitly
            $this->_data[$tree_field] = $this->_current_tree_id;
        }

        // add the tree_id to the query
        $query->where($tree_field, '=', $this->_data[$tree_field]);
    }

  • DirkSDirkS
    Agreed. The latter seems to me the right logic.

    Cheers Dirk
  • HarroHarro
    Ok. I've commited it like this.

    Thanks for the feedback.
Add a Comment

Howdy, Stranger!

It looks like you're new here. If you want to get involved, click one of these buttons!

Sign In Apply for Membership

Categories

In this Discussion