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

Add Node::loadDescendants method #48

Closed
wants to merge 1 commit into from
Closed

Add Node::loadDescendants method #48

wants to merge 1 commit into from

Conversation

tremby
Copy link

@tremby tremby commented Mar 5, 2014

For cases where I already have a Node object and now want to recurse over all
descendants I find this method very useful. It loads all descendants and sets
the children relation so no further database queries need to be made when
looping through children and children's children.

Thanks to @shaddowgh for the help.

This fixes #45.

@etrepat
Copy link
Owner

etrepat commented Mar 12, 2014

Thanks for contributing!

I was reviewing this patch but something popped out: Is this not accomplishable
by the usual eager-loading methods provided in Eloquent?

I mean:

If you wanted to eager-load the children relation on every query you could
add to your model:

...
protected $with = array('children');
...

You may use the with method to instruct Eloquent\Builder to eager-load the
children on some queries:

$node->descendants()->with('children')->get();

Or, to load the relation in a particular model which was not eager-loaded
previously, you could:

$node->load('children');

@tremby
Copy link
Author

tremby commented Mar 27, 2014

This took me a while to get to, since I'm investigating another baum-related bug. But yes, you're absolutely right. Thanks.

@tremby tremby closed this Mar 27, 2014
@tremby
Copy link
Author

tremby commented Apr 2, 2014

Hold up -- doesn't with('children') or load('children') only load the next layer of children, non-recursively? That's no good for me.

@tremby tremby reopened this Apr 2, 2014
@etrepat
Copy link
Owner

etrepat commented May 2, 2014

Calling $node->descendants()->with('children')->get(); will get all of the $node's descendants from the database and for each of those it will populate the children relation. It will do this with exactly 2 queries.

Of course, the children relation will not contain a nested hierarchy-style (like those returned by toHierarchy) but you can traverse them in that fashion if needed without hitting the database again, so I think it is way better to leave the relation objects behave as they should and not filling them with nested Collection objects.

@tremby
Copy link
Author

tremby commented Jun 27, 2014

Should that load all descendents even if they're several layers deeper?

@etrepat
Copy link
Owner

etrepat commented Jul 1, 2014

Yes it should because Nested Sets work linearly. When you ask for the descendants of a node, you get all of them one after the other (the default sorting is by the lft column). Only by calling toHierarchy you get them in a hierarchical fashion (nested). By using the usual eager-loading methods you should accomplish this hitting the database only twice (2 queries) regardless the depth level of the descendant nodes.

@tremby
Copy link
Author

tremby commented Jul 3, 2014

I want to do what you suggest, but for all roots at the same time. Is there a way to do that? Right now I have MenuItem::roots()->with('children')->get() but this has not loaded the children. When I recurse through I get some 60 queries, one for each node. I'm having to do MenuItem::roots()->with(['children', 'children.children', 'children.children.children'])->get() for a reasonable number of queries.

@etrepat
Copy link
Owner

etrepat commented Jul 4, 2014

Well in the case you're showcasing what I said does not work. The descendants queries include the children of each level, so eager-loading works. But in your case, the roots() call obviously only includes root nodes, so eager-loading only loads the first level of children.

But, Isn't getting the roots with all their descendants just getting the whole tree?

The following queries will produce the desired results returning the full hierarchy in exactly 2 queries:

MenuItem::query()->with('children')->get()->toHierarchy();
MenuItem::where(... some scope conditions ...)->with('children')->get()->toHierarchy();

@etrepat
Copy link
Owner

etrepat commented Aug 25, 2014

I understand that having a collection of root nodes and trying to eager-load all possible nested children is not working right now in your use-case.

I sincerely think that your best bet here is to include: protected $with = array('children'); in your models to achieve that effect if that is what you need. I don't think that it is advisable to mess with the eager-loading methods provided by the framework, Baum already does some patching of its own which I'm already thinking how to remove.

I'm soon to start rewriting some bits of this package to allow for trait-based usage and maybe I can re-think/re-do some things for your use-case to work.

@etrepat etrepat closed this Aug 25, 2014
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

Successfully merging this pull request may close these issues.

Eager loading children
2 participants