eloquent-tree icon indicating copy to clipboard operation
eloquent-tree copied to clipboard

setChildOf on newly created Tree

Open jbajou opened this issue 8 years ago • 5 comments

Hello,

We're using eloquent-tree to handle hierarchy between users. We spoted some performance issue when registering a new user and could track it down to setChildOf().

The problem is that when we want to assign a user newly created to as a child of the currently logged user, path and level are not filled yet. findDescendants() requests all users WHERE path LIKE '<null>%' which returns the whole users table!

I don't know if we're using the package properly, for this purpose, but in any case, I would expect a note in the documentation or something preventing to return the whole table.

What do you think ?

Thanks

jbajou avatar Jun 29 '17 11:06 jbajou

Hi,

Sorry for late response but recently I'm very busy. I don't fully understand your use case, exactly the part with assigning the new user to currently logged user. Could you explain it more to me? Which of those users are saved in DB, do they have set all tree related fields during calling setChildOf()?

AdrianSkierniewski avatar Jul 04 '17 08:07 AdrianSkierniewski

Hello,

No worries, totally understand.

We have a user currently logged in (A), created a new user (B). (A) has all tree related fields filled properly. When doing (B)->setChildOf((A)), the call to findDescendants() returns the whole users table because (A) doesn't have the tree related fields filled (which is logic, the user has just been created).

Is it more clear ?

jbajou avatar Jul 04 '17 09:07 jbajou

Yes, thanks. Ok, so this code works, but the only problem is with performance right? If I understand this correctly, we're checking for old descendants for the new node that doesn't have any, and this is why we're getting the whole table. To fix that we'd probably need to check is this is the new node case (have an id or something) and only then try to find those descendants in DB.

Here is code responsible for that: https://github.com/AdrianSkierniewski/eloquent-tree/blob/master/src/Gzero/EloquentTree/Model/Tree.php#L79

AdrianSkierniewski avatar Jul 04 '17 10:07 AdrianSkierniewski

Well, yeah, the problem is that it returns the whole table, inducing performance issue as soon as you have a few thousands users like we do.

What we do for now is we set the fields by hand, i.e. we don't call setChildOf on newly created users, but it doesn't feel right. Feels more like a temp fix than a fix for life.

jbajou avatar Jul 06 '17 14:07 jbajou

Fell free to create PR with proposed fix. If not, it will take me some time to create it due to a busy schedule.

AdrianSkierniewski avatar Jul 08 '17 10:07 AdrianSkierniewski