Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

asserting that tree behavior generates valid sql in recover #955

Merged
merged 3 commits into from Nov 15, 2012

Conversation

Projects
None yet
5 participants
Member

dereuromark commented Nov 13, 2012

$Model->alias . '.' . $Model->primaryKey should better be used to avoid invalid sql statements

see http://cakephp.lighthouseapp.com/projects/42648-cakephp/tickets/3366-treebehavior-recover-invalid-sql

Member

ceeram commented Nov 13, 2012

Looks ok to me

Member

ADmad commented Nov 13, 2012

We might want to consider the option of calling deleteAll() with $cascade and $callbacks params false to avoid the joins.

@renan renan commented on an outdated diff Nov 13, 2012

lib/Cake/Model/Behavior/TreeBehavior.php
foreach ($missingParents as $id => $display) {
$this->errors[] = 'cannot find the parent for ' . $Model->alias . ' with id ' . $id . '(' . $display . ')';
}
return false;
- } elseif ($missingParentAction == 'delete') {
- $Model->deleteAll(array($Model->primaryKey => array_flip($missingParents)));
+ } elseif ($missingParentAction === 'delete') {
+ $Model->deleteAll(array($Model->alias . '.' . $Model->primaryKey => array_flip($missingParents)));
@renan

renan Nov 13, 2012

Member

Other places on the code uses $Model->escapeField() instead.

@markstory markstory commented on the diff Nov 14, 2012

...e/Test/Case/Model/Behavior/TreeBehaviorNumberTest.php
$this->Tree->create();
- $this->Tree->save(array('parent_id' => null, 'name' => 'Overview', $parentField => $node12, $leftField => 0, $rightField => 0));
+ $this->Tree->save(array('name' => 'Overview', $parentField => $node12, $leftField => 0, $rightField => 0));
@markstory

markstory Nov 14, 2012

Owner

Why did all these tests change?

@ADmad

ADmad Nov 14, 2012

Member

The var $parentField contains the parent field name so using the string 'parent_id' was redundant / incorrect.

@markstory markstory and 1 other commented on an outdated diff Nov 14, 2012

...e/Test/Case/Model/Behavior/TreeBehaviorNumberTest.php
@@ -225,6 +225,74 @@ public function testRecoverUsingParentMode() {
}
/**
+ * testRecoverUsingParentModeAndDelete method
+ *
+ * @return void
+ */
+ public function testRecoverUsingParentModeAndDelete() {
+ extract($this->settings);
+ $this->Tree = new $modelClass();
+ $this->Tree->Behaviors->disable('Tree');
+
+ $this->Tree->save(array('name' => 'Main', $parentField => null, $leftField => 0, $rightField => 0));
+ $node1 = $this->Tree->id;
+
+ $this->Tree->create();
+ $this->Tree->save(array('name' => 'About Us', $parentField => $node1, $leftField => 0, $rightField => 0));
+ $node11 = $this->Tree->id;
@markstory

markstory Nov 14, 2012

Owner

Style checker will complain about two spaces before =.

@dereuromark

dereuromark Nov 14, 2012

Member

darn - the original test file already contained those. it is a tab afaik. I thought I fixed em all.

Owner

markstory commented Nov 14, 2012

Ok, this looks good to me.

@markstory markstory added a commit that referenced this pull request Nov 15, 2012

@markstory markstory Merge pull request #955 from dereuromark/master-tree-fixes
Asserting that tree behavior generates valid sql in recover

Fixes #3366
7206254

@markstory markstory merged commit 7206254 into cakephp:master Nov 15, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment