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

Issue 53: Allow to use treeAttribute as external key #68

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mikehaertl
Copy link

Fix for issue #53

@mikehaertl mikehaertl force-pushed the 53-tree-attribute branch 2 times, most recently from 6e86f13 to 5796682 Compare April 10, 2015 12:51
*/
public function testMakeRootNewExceptionIsRaisedWhenMultiTreeIsCreatedWithExistingTreeAttribute()
{
$node = new MultipleTree(['name' => 'Root 4', 'tree' => 1]);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@creocoder Before merging, maybe you can explain why at this point there is no tree with tree Id 10 here. It is created in the previous test and the DB is not cleared as far as I can see. I'm still quite confused about the state of the DB for each test. I only came up with this solution by trial and error.

@mikehaertl
Copy link
Author

@creocoder Any news on this?

@creocoder
Copy link
Owner

@mikehaertl In general fix is much more complex than introduced in that PR. So i'll take this ticket myself. I'll explain why. There should be special "externalRoots" mode. And behavior should take that into account. For example roots() method should throw exception when used in that mode, etc. So "externalRoots" mode is not so trivial to implement. On other side its not general direction of that behavior. This is why this ticket in low priority.

@mikehaertl
Copy link
Author

Hmm, can you explain that a bit more? I don't think that such a mode is really neccessary. And why do you think that roots() should throw an exception in that mode? How would you obtain the root nodes in that case?

The only problem i can see is something like this:

$root1 = new Category(['tree' => 2, 'name' => 'test']);
$root1->makeRoot();
echo $root1->id; // == 1

$root2 = new Category(['name' => 'test2']);
$root2->makeRoot();
echo $root2->id; // == 2

The last call to makeRoot() would mess things up, as it now would use the same tree id 2 for $root2. But this could also be solved if we'd just insert another check in afterInsert(), whether the tree id does already exist.

@creocoder
Copy link
Owner

@mikehaertl Seems you misunderstood original issue. External roots mean roots in different table. So no roots in NS table at all. Your PR is about allow use custom ids for roots, but roots is still exists inside NS table. This not solve original issue.

@mikehaertl
Copy link
Author

Hmm. But won't you always need a root node to have a consistent tree in your node table?

@KnightYoshi KnightYoshi mentioned this pull request Jul 14, 2017
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.

2 participants