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

Example of tables structure #3

Closed
parrker opened this issue Apr 24, 2013 · 24 comments
Closed

Example of tables structure #3

parrker opened this issue Apr 24, 2013 · 24 comments
Labels

Comments

@parrker
Copy link

parrker commented Apr 24, 2013

Hi, I have never used closure tables before, and I am considering to use your class in my new Laravel project. I am a bit confused about which columns must be in each of my tables. Could you provide an example migration, please?

@franzose
Copy link
Owner

Hi, parrker! I have just written an example of migrations. Let me know if something is unclear.

@parrker
Copy link
Author

parrker commented Apr 25, 2013

Hi franzose, the migrations were really helpful! Thanks!

Now I have a different problem (probably because I'm not too experienced with both Laravel and the idea of closure tables). How should I go about saving nodes in the tree? I create a new element like this:

$category = new ProductsCategory;
$category->published = 1;
$category->save();

Is this the right way? I can see that data is stored in both my tables successfully, but the ProductsCategory::fulltree() method now throws an error:

Unhandled Exception

Message:

array_merge() [function.array-merge]: Argument #1 is not an array
Location:

X:\home\ifg.dev\bundles\closuretable\closuretable.php on line 207

I have a feeling I am doing it wrong.

@franzose
Copy link
Owner

Hi, parrker) First of all, please, show me what data containts your products' categories table and tree paths one now. Then, if possible, SQL logs. I'm not able to test them at the time.

@parrker
Copy link
Author

parrker commented Apr 25, 2013

These are my migrations for creating tables:

Schema::create('products_categories', function($table) {
$table->increments('id');
$table->integer('parent_id')->unsigned()->nullable();
$table->boolean('published');

$table->integer('created_by');
$table->integer('updated_by');
$table->timestamps();
});

// and another one

Schema::create('products_categories_treepath', function($table) {
$table->increments('tpid');
$table->integer('ancestor')->unsigned();
$table->integer('descendant')->unsigned();
$table->integer('level')->unsigned();

$table->index('level');
});

As you see, I don't have any real data yet, I'll use a separate table for lang data.

And the code I was trying to run is this:

$category1 = new ProductsCategory;
$category1->published = 1;
$category1->save();

$category2 = new ProductsCategory;
$category2->published = 1;
$category2->save();

$category1 = new ProductsCategory;
$category1->published = 1;

ProductsCategory::find(1)->append_child($subcategory);

// this works fine
$categories = ProductsCategory::roots();

// this throws an error
$categories = ProductsCategory::fulltree();


@franzose
Copy link
Owner

Please, look at the closuretable.php file now around 197 and 207 lines. I changed the code slightly. Now it tries to convert a given object to array. As I know DB::query method returns either stdClass or array according to the configuration you set. And it returns stdClass by default. So, there was no conversion from stdClass to array that led to wrong argument type error.

Please, try updated code and let me know either about your or about some new errors. Thank you very much.

@parrker
Copy link
Author

parrker commented Apr 26, 2013

I have tried the updated version, and the fulltree() method works fine now. Thanks!

But now I noticed that there is an error when I try to insert a child into an existing node. I tried the following:

// These records was created the same way as in the code in my previous comment.
$subcategory        = ProductsCategory::find(3);
$top_level_category = ProductsCategory::find(1);

// First
$top_level_category->descendants()->insert($subcategory);

// Second
$top_level_category->append_child($subcategory);

// Third
$subcategory->move_to($top_level_category);

For the first and second way I get the following error:

Unhandled Exception

Message:

Cannot access static:: when no class scope is active
Location:

X:\home\ifg.dev\bundles\closuretable\closuretable.php on line 147

For the third I get this:

Unhandled Exception

Message:

Cannot use object of type stdClass as array
Location:

X:\home\ifg.dev\bundles\closuretable\treepath.php on line 162

Sorry to keep bothering you about this, but I want to make my trees work via closure tables so much!

@franzose
Copy link
Owner

franzose commented May 3, 2013

Hi, parrker. I apologize for the delay. I have tested ClosureTable for the issues you mentioned and fixed the latter one in treepath.php.

What is the version of PHP you are using? I cannot reproduce class scope error on 5.4.11...

@franzose
Copy link
Owner

franzose commented May 3, 2013

I really feel ashamed in front of those almost 120 people who have downloaded the bundle full of bugs :( Thanks for your help, parrker ;)

@franzose
Copy link
Owner

franzose commented May 3, 2013

Parrker, trouble with class scope may happen due to the fact that there are static calls from the closure. I mean 'descendants' and 'ancestors' methods. There are joins which use closure to define additional join rules. See 133-134 and 147-148 line in closuretable.php.

@franzose
Copy link
Owner

franzose commented May 3, 2013

Please, try to change 'static::' calls to 'ClosureTable::' in joins' closures of those methods ('ancestors' and 'descendants') and let me know about the results.

@parrker
Copy link
Author

parrker commented May 3, 2013

Hi franzose! For a moment I thought you forgot about me and this bundle :)

After updating the Treepath class, the third way of assigning child to a node (using the move_to() method) indeed started working. And the issue with class scope indeed goes away if you change static:: calls to ClosureTable:: calls in descendants() method of the ClosureTable class. Should I keep it like that?

P.S. I am using PHP 5.3.17 at the moment. I know that we will have PHP 5.3 on my live web server for some time, so I cannot upgrade :(

@parrker
Copy link
Author

parrker commented May 3, 2013

Now I encountered another issue: because we changed the static:: calls to ClosureTable::, my $table and $treepath properties surely are not correct when I try to grab something $category->descendants. I have to override these methods in my ProductsCategory class, changing the ClosureTable to ProductsCategory (obviously).

I spent some time researching, and this is the solution that I used in the end:

public function descendants()
    {
        $class = get_called_class();

        return $this->has_many(get_class($this), static::$parent_key)
            ->join(static::$treepath, function($join) use ($class){
                $join->on($class::$treepath.'.descendant', '=', $class::$table.'.'.$class::$key);
                $join->on($class::$treepath.'.ancestor', '<>', $class::$table.'.'.$class::$key);
        })->select(array(static::$table.'.*'));
    }

The issue was with the closure's scope. Apparently, in PHP 5.4 you don't have to add use ($class), you could simply use $this (and probably static too, that's why you did not get this error).

@franzose
Copy link
Owner

franzose commented May 3, 2013

Hey, it seems that I have found an answer. It's quirky now, but we can define join rules first and then pass them to the closure. So, instead of


    public function ancestors()
    {
        return $this->has_many(get_class($this), static::$parent_key)
            ->join(static::$treepath, function($join){
                $join->on(static::$treepath.'.ancestor', '=', static::$table.'.'.static::$key);
                $join->on(static::$treepath.'.descendant', '<>', static::$table.'.'.static::$key);
        })->select(array(static::$table.'.*'));
    }
    public function descendants()
    {
        return $this->has_many(get_class($this), static::$parent_key)
            ->join(static::$treepath, function($join){
                $join->on(static::$treepath.'.descendant', '=', static::$table.'.'.static::$key);
                $join->on(static::$treepath.'.ancestor', '<>', static::$table.'.'.static::$key);
        })->select(array(static::$table.'.*'));
    }

we will have this:


    public function ancestors()
    {
        $ancestor = static::$treepath.'.ancestor';
        $descendant = static::$treepath.'.descendant';
        $key = static::$table.'.'.static::$key;
        return $this->has_many(get_class($this), static::$parent_key)
            ->join(static::$treepath, function($join) use($ancestor, $descendant, $key) {
                $join->on($ancestor, '=', $key);
                $join->on($descendant, '<>', $key);
        })->select(array(static::$table.'.*'));
    }
    public function descendants()
    {
        $ancestor = static::$treepath.'.ancestor';
        $descendant = static::$treepath.'.descendant';
        $key = static::$table.'.'.static::$key;
        return $this->has_many(get_class($this), static::$parent_key)
            ->join(static::$treepath, function($join) use($ancestor, $descendant, $key) {
                $join->on($descendant, '=', $key);
                $join->on(static::$ancestor, '<>', $key);
        })->select(array(static::$table.'.*'));
    }

It's a bit quirky, but we now prepare values first.

@parrker
Copy link
Author

parrker commented May 3, 2013

Oh, I edited my comment without refreshing the page :) My solution is similar, I just pass the class name to closure instead of variables. Yours looks neater, I think.

@franzose
Copy link
Owner

franzose commented May 3, 2013

From the beginning the purpose was to create a plug-and-play thing. Try my version, if it works appropriately, I will submit changes :)

@parrker
Copy link
Author

parrker commented May 3, 2013

Wait, in the ancestors method you still have one static call:

$join->on($descendant, '=', $key);
$join->on(static::$ancestor, '', $key);

@franzose
Copy link
Owner

franzose commented May 3, 2013

Ooops, it's a mistake. Now it should be just $ancestor.

@parrker
Copy link
Author

parrker commented May 3, 2013

One more thing, some of your joins lost the "not equals" operator:

$join->on($descendant, '', $key);

@franzose
Copy link
Owner

franzose commented May 3, 2013

May be parser ate them...

@franzose
Copy link
Owner

franzose commented May 3, 2013

I have updated closuretable.php with this quick fix. Try it. Later I'll solve this issue in more suitable way.

@parrker
Copy link
Author

parrker commented May 3, 2013

I have another one.

I cannot grab the node's ancestors for some reason. ProductsCategory::find(8)->ancestors just returns an empty array, even though in the database i can see three paths for the descendant with ID 8: http://i.imgur.com/85WTYtC.png

The descendants are returned correctly.

@franzose
Copy link
Owner

franzose commented May 6, 2013

Sorry, parrker, but from 8 till 31 of May I will be unable to maintain the project (and fix all the bugs) due to the trip with my music band... Thank you for forking it ;)

@parrker
Copy link
Author

parrker commented May 6, 2013

I am not too sure if I get the fork to work, but it's worth a shot.

Good luck on the tour!

@franzose
Copy link
Owner

franzose commented Jun 9, 2013

Hi, parrker. I'm able to work again. See and try my fixes! Now 'ancestors' returns what it should, 'descendants' returns all descendants (not only immediate children). And I added 'children' method to get those immediate fucking children :)))

After all that fixin' I had a thought that it was all quite simple to fix and I don't know now why I missed all that before. Now I will think about your suggestion about 'fulltree' method. I suppose it's not that difficult to implement...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants