Eager loading children

[tremby]

How do I eager load all a model's descendants? I'm traversing the tree from a given node and every time I ask for a node's children it's sending out another database query. They're really adding up.

I'm using toHierarchy in a different area of my code which already seems very awkward since I have to get a single Node model, then call the method, and now I've got a collection I need to get the first element of again. I can live with that, though I'd prefer to just start with the Node and be able to grab children without a new query actually being made, having already preloaded all children. But anyway, when I traverse through the Collection toHierarchy gave me I still get new database queries getting fired off.

In that case I'm doing

$menus = [];
foreach (MenuItem::roots()->get() as $root) {
    $menus[$root->menu] = $root->getDescendantsAndSelf()->toHierarchy();
}

The view has

@foreach ($menus as $menu => $hierarchy)
    <?php $root = $hierarchy->first(); ?>
    <div class="menu panel panel-primary" data-id="{{{ $root->getKey() }}}">
...
        @include('admin.menu-items.children', ['model' => $root])
...
    </div>
@endforeach

The children partial view has

<ol class="nested-sortable">
    @foreach ($model->children as $node)
        @include('admin.menu-items.item', ['model' => $node, 'parentId' => $model->getKey()])
    @endforeach
</ol>

The item partial view prints an <li> in which the children partial is called again recursively on $model. There are more and more queries being fired off.

[ghost]

i suspect the extra queries you are seeing may be related to something i discovered when creating my #46 pull request.
when you call toHierarchy it will prepopulate all children that it has found, such that calls to $node->children will return the already retrieved values, however in the case of a leaf node it simply moves on meaning when you request the children of the leaf node it has to query the database which of course returns nothing. i fixed this by getting the toHierarchy function to set children to an empty collection for leaf nodes.

thinking about it a more general solution would be to adjust the children function of Node to return an empty collection if isLeaf() returns true as there are of course no children!.

[tremby]

Thanks -- that definitely sounds like a solution. I'll try your branch.

But what I'd really like to do is eager load a whole tree (or subtree) without using toHierarchy at all.

[tremby]

Your fixes took queries on one page down from 35 to 7. I'll be using my own tremby/baum#patched branch which has your fixes and mine until @etrepat looks in.

[ghost]

i don't follow... if you retrieve "all" nodes you are interested in and then run toHierarchy you have all nested children that you will need....
replace

$menus = [];
foreach (MenuItem::roots()->get() as $root) {
    $menus[$root->menu] = $root->getDescendantsAndSelf()->toHierarchy();
}

with

$menus = MenuItem::all()->toHierarchy();

you could restrict it using where's etc to only the ones you want rather than all.
then

@foreach ($menus as $menu => $hierarchy)
    <?php $root = $hierarchy->first(); ?>
    <div class="menu panel panel-primary" data-id="{{{ $root->getKey() }}}">
...
        @include('admin.menu-items.children', ['model' => $root])
...
    </div>
@endforeach

becomes

@foreach ($menus as $menu)
    <div class="menu panel panel-primary" data-id="{{{ $menu->getKey() }}}">
...
        @include('admin.menu-items.children', ['model' => $menu])
...
    </div>
@endforeach

and your last part shouldn't need changing.
you should only need one query that way.

there is still a problem with toHierarchy not eager loading "all" children if you were filtering the initial query somehow but i think that is probably a good thing incase you are using the children of one of the retrieved nodes outside of the initial context.

[tremby]

Well the thing which bothers me about toHierarchy is that it is called on a Node, but returns a Collection with one child which is the Node it was called on. That just seems very confusing to me -- why the outer Collection?

So I need to do this:

$menuItem = $menuItem->toHierarchy()->first();

which is kind of obtuse, and if that were someone else's code I was reading I would worry that the ->first() call might be throwing away potential other child nodes.

I've also had a few related issues with toHierarchy not existing on given objects. I believe the above will in fact error saying that toHierarchy doesn't exist on a Builder object, and I have to do $menuItem->get()->toHierarchy() instead, which I find confusing since I don't know why I had a Builder object to begin with, given that I got it with a MenuItem::where(...)->firstOrFail() call -- I thought that was meant to give me a MenuItem object.

I just tried ->getDescendantsAndSelf()->toHierarchy() as you suggested but it results in the same number of queries as just ->get()->toHierarchy(). Can you tell me what the difference is here?

But why can't I do this instead?

$menuItem->loadChildren();

and then be able to use the same existing $menuItem object and walk its tree, all already preloaded?

[ghost]

i think you may be misunderstanding...
toHierarchy is called on a Collection... it exists on the Baum\Extensions\Eloquent\Collection class.

so the idea is that you grab a collection that includes all of the nodes you want (including children), as a simple one dimensional array of objects then run the toHierarchy method to iterate over that collection nesting the objects as appropriate.

so

$menus = MenuItem::all()->toHierarchy();

first gets all the menu items and then turns it into a hierarchy. the result will be a collection containing just your root nodes with all of their descendants nested (and pre-loaded) beneath them.

if you only wanted one particular tree or subtree you would just select that node then use

$node_with_tree_loaded =$node->getDescendantsAndSelf()->toHierarchy();

where $node is a specific node that you have grabbed from any sort of query you like.

[tremby]

Okay, so there's something I'm not getting about the relationship between Builder objects and Eloquent objects.

If I do $menuItem = MenuItem::find(2); I have a MenuItem object.

But if I try to run toHierarchy on it (yes, I now understand that's only meant to be run on a Collection and so it's going to fail) I am told that it tried to find the method on a Builder object: Call to undefined method Baum\Extensions\Query\Builder::toHierarchy().

So when faced with a Builder object I know I need to run ->get() before I can run other methods (I never understood the why or the when, just that it fixes problems I run into from time to time).

So $menuItem->get() gives me a Collection, I now know. I can run toHierarchy() on that and I get another collection.

I'm with you now. And I figured out why the number of queries running was the same for me whether I ran ->get()->toHierarchy() or ->getDescendantsAndSelf()->toHierarchy() -- I had some other code which was skipping the ->children() calls in some cases. I see that the queries are not the same.

So what I want, it seems, is a shortcut, to go from a Node object directly to that same Node object with all children eager loaded. So I've written myself this:

    /**
     * Eager load all children
     */
    public function loadChildren() {
        return $this->getDescendantsAndSelf()->toHierarchy()->first();
    }

and put it in my Node, and it seems to work well. It seems to me that it'd be useful in core, so I'll make a PR.

[tremby]

Actually, I've found that though the above function works, it is not modifying the object but returning another. So if I do

$menuItem->loadChildren()->walkThroughTree();

...I get just the one query and it works as expected, but if I do

$menuItem->loadChildren();
$menuItem->walkThroughTree();

the eager-loading hasn't stuck and it runs more queries.

That doesn't seem ideal. Any thoughts?

[ghost]

Load children returns an altered copy of the menuitem object it does not alter the original object it was called on.
Thus your first version using chaining will work as the altered version is used for the walk. But in the second you don't assign the get children return back to the variable so you are using the unaltered original in the walk.

$menuitem = $menuitem->getChildren();
$menuitem->walkThroughTree();

Would work.

[tremby]

Yes, I'm aware of the differences and why each works the way it does. I was trying to highlight the shortcoming, rather than ask what I was doing wrong.

I was more asking for comment on a possible solution -- whether it's possible or not (and how) to have this loadChildren() method alter the existing object rather than return a new one.

[ghost]

Well in that case i would change getChildren to something like

$this->setRelation('children', $this->getDescendants()->toHierarchy()) ;

Should do the trick.
On 5 Mar 2014 21:06, "Bart Nagel" notifications@github.com wrote:

Yes, I'm aware of the differences and why each works the way it does. I
was trying to highlight the shortcoming, rather than ask what I was doing
wrong.

I was more asking for comment on a possible solution -- whether it's
possible or not (and how) to have this loadChildren() method alter the
existing object rather than return a new one.

Reply to this email directly or view it on GitHubhttps://github.com//issues/45#issuecomment-36793457
.

[tremby]

That's perfect. Thanks. That ought to be available on Node by default.

[tremby]

Closing in favour of pull request.