Skip to content

Commit

Permalink
Adding error to find(threaded).
Browse files Browse the repository at this point in the history
When the model has no parent_id trigger a warning about the impending
failure and return an empty result.

Fixes #2341
  • Loading branch information
markstory committed Jan 2, 2012
1 parent 81d7ef4 commit b4faa00
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 4 deletions.
7 changes: 7 additions & 0 deletions lib/Cake/Model/Model.php
Expand Up @@ -2818,6 +2818,13 @@ protected function _findThreaded($state, $query, $results = array()) {
foreach ($results as $result) {
$result['children'] = array();
$id = $result[$this->alias][$this->primaryKey];
if (!isset($result[$this->alias]['parent_id'])) {

This comment has been minimized.

Copy link
@ceeram

ceeram Jan 2, 2012

Contributor

Wouldn it be better to check for parent_id in schema in the before part, instead of doing this check within the foreach loop of results?

This comment has been minimized.

Copy link
@renan

renan Jan 3, 2012

Contributor

Would be nice if you could also define the parent field.
There is a chance to do it automatically, by checking belongsTo relationships using the same model, but its not 100%.

Having a parent_field key would solve it, eg:

$thread = $Apple->find('threaded', array('parent_field' => 'thread_id'));

This comment has been minimized.

Copy link
@ADmad

ADmad Jan 3, 2012

Member

I believe there's already an enhancement ticket to allow specifying the parent field, so this is a good idea.

This comment has been minimized.

Copy link
@markstory

markstory Jan 3, 2012

Author Member

I think being able to provide a custom parent_id field makes sense, we should still trigger an error if someone asks for a field that doesn't exist. As for the isset() in the loop, it could probably be moved out and done once for sure. I don't know why I didn't think of that earlier.

This comment has been minimized.

Copy link
@AD7six

AD7six Jan 4, 2012

Member

Why don't we move this logic to a new function that isn't burried in an "after" find method. E.g. Set::thread ?

This comment has been minimized.

Copy link
@ceeram

ceeram Jan 4, 2012

Contributor

@markstory as pointed out by @AD7six in irc, the isset() is needed within the loop for schemaless dbs not returning the key if no value is present.
So i think we can leave that as it was.

trigger_error(
__d('cake_dev', 'You cannot use find("threaded") on models without a "parent_id" field.'),
E_USER_WARNING
);
return $return;
}
$parentId = $result[$this->alias]['parent_id'];
if (isset($idMap[$id]['children'])) {
$idMap[$id] = array_merge($result, (array)$idMap[$id]);
Expand Down
16 changes: 12 additions & 4 deletions lib/Cake/Test/Case/Model/ModelReadTest.php
Expand Up @@ -2987,13 +2987,21 @@ public function testSelfAssociationAfterFind() {
$noAfterFindData = $noAfterFindModel->find('all');

$this->assertFalse($afterFindModel == $noAfterFindModel);
// Limitation of PHP 4 and PHP 5 > 5.1.6 when comparing recursive objects
if (PHP_VERSION === '5.1.6') {
$this->assertFalse($afterFindModel != $duplicateModel);
}
$this->assertEquals($afterFindData, $noAfterFindData);
}

/**
* find(threaded) should trigger errors whne there is no parent_id field.
*
* @expectedException PHPUnit_Framework_Error_Warning
* @return void
*/
public function testFindThreadedError() {
$this->loadFixtures('Apple', 'Sample');
$Apple = new Apple();
$Apple->find('threaded');
}

/**
* testFindAllThreaded method
*
Expand Down

0 comments on commit b4faa00

Please sign in to comment.