Position behavior about closuretable HOT 6 CLOSED

Futhermore, when u do Table::getTree() or Table::getChildren() (for example
to display all tree in ul), the returned collection is not sorted by
position.

2014-04-05 22:59 GMT+02:00 tomzx [email protected]:

Hello,

What is supposed to be the behavior of setting positions which do not make
sense?

For instance, let say you have a tree as such:

• Item A (position 0) -- Item B (position 0) -- Item C (position 1)
• Item D (position 1)

Now let say you decide to moveTo(5000) Item D. All newer items would end
up with a position = 5000 + x, meanwhile there would be a huge hole between
position 1 and 4999.

Would it make more sense to set the positions to be inclusive between 0
and the current number of siblings? Otherwise, what is the use case of
having unlimited freedom on the position? Furthermore this also means that
it is possible to set negative positions...

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/85
.

from closuretable.

I was thinking about it almost from the very beginning but never did an incoming position check. So, we could do a check what position we're given. If it is a nonsense, we will set the last position at least.

from closuretable.

Alright, that makes sense. I'll see what needs to be done to get this working properly.

from closuretable.

I've done an initial change in https://github.com/tomzx/ClosureTable/tree/fixes/improve-tests. It does position clamping between 0 and the next_id of the parent entity. Thus, it currently costs an additional SQL query per position change (only if the position is dirty will it try to clamp the position) to obtain the next_id value.

Meanwhile I've discovered a potential issue. Some of the code does checks such as old_parent_id == parent_id, this is most likely a bug in situations where parent_id = NULL (you've set the entity to be at the root) and old_parent_id = NULL because the parent hasn't changed, not because the old_parent_id is the root.

from closuretable.

I've also explored the problem of parent_id and old_parent_id comparison and their nulliness. For example, we cannot isset() the old_parent_id because it would be false if the old_parent_id was null. I tried different comparisons but they all failed. I think that, in such circumstances, we must use equality check to false. I mean === false. And may be by default to set old_parent_id to false. And turn that:

https://github.com/tomzx/ClosureTable/blob/fixes/improve-tests/src/Franzose/ClosureTable/Models/Entity.php#L1356

to this:

$this->old_parent_id !== false && $this->old_parent_id == $this->parent_id

May be something like that?

from closuretable.

Hey @franzose,

I'd suggest we do something like this in the Entity model instead:

    public function newFromBuilder($attributes = array())
    {
        $instance = parent::newFromBuilder($attributes);
        $instance->old_parent_id = $instance->parent_id;
        $instance->old_position = $instance->position;
        return $instance;
    }

Basically, loaded models will have their proper parent_id and position set when loaded and thus only when they are changed will they trigger any update (as it should). The nice thing with this is that we don't need to change any code to add an additional check if old_parent_id !== false for instance. I've implemented it in my fixes/improve-tests branch for which I'll be submitting a PR soon.

from closuretable.