Skip to content

Commit

Permalink
Make Model::find('first') always return an array.
Browse files Browse the repository at this point in the history
  • Loading branch information
@bar
bar committed Oct 25, 2012
1 parent 528671f commit c741f60
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 14 deletions.
15 changes: 9 additions & 6 deletions lib/Cake/Model/Model.php
View file
Expand Up @@ -2613,9 +2613,12 @@ public function hasAny($conditions = null) {
* *
* Note: find(list) + database views have issues with MySQL 5.0. Try upgrading to MySQL 5.1 if you * Note: find(list) + database views have issues with MySQL 5.0. Try upgrading to MySQL 5.1 if you
* have issues with database views. * have issues with database views.
*
* Note: find(count) has its own return values.
*
* @param string $type Type of find operation (all / first / count / neighbors / list / threaded) * @param string $type Type of find operation (all / first / count / neighbors / list / threaded)
* @param array $query Option fields (conditions / fields / joins / limit / offset / order / page / group / callbacks) * @param array $query Option fields (conditions / fields / joins / limit / offset / order / page / group / callbacks)
* @return array Array of records * @return array Array of records, or Null on failure.
* @link http://book.cakephp.org/2.0/en/models/deleting-data.html#deleteall * @link http://book.cakephp.org/2.0/en/models/deleting-data.html#deleteall
*/ */
public function find($type = 'first', $query = array()) { public function find($type = 'first', $query = array()) {
Expand All @@ -2638,10 +2641,10 @@ public function find($type = 'first', $query = array()) {


if ($type === 'all') { if ($type === 'all') {
return $results; return $results;
} else { }
if ($this->findMethods[$type] === true) {
return $this->{'_find' . ucfirst($type)}('after', $query, $results); if ($this->findMethods[$type] === true) {
} return $this->{'_find' . ucfirst($type)}('after', $query, $results);
} }
} }


Expand Down Expand Up @@ -2707,7 +2710,7 @@ protected function _findFirst($state, $query, $results = array()) {
return $query; return $query;
} elseif ($state === 'after') { } elseif ($state === 'after') {
if (empty($results[0])) { if (empty($results[0])) {
return false; return array();
} }
return $results[0]; return $results[0];
} }
Expand Down
2 changes: 1 addition & 1 deletion lib/Cake/Test/Case/Model/Behavior/TranslateBehaviorTest.php
View file
Expand Up @@ -422,7 +422,7 @@ public function testMissingTranslation() {
$TestModel = new TranslatedItem(); $TestModel = new TranslatedItem();
$TestModel->locale = 'rus'; $TestModel->locale = 'rus';
$result = $TestModel->read(null, 1); $result = $TestModel->read(null, 1);
$this->assertFalse($result); $this->assertEquals(array(), $result);


$TestModel->locale = array('rus'); $TestModel->locale = array('rus');
$result = $TestModel->read(null, 1); $result = $TestModel->read(null, 1);
Expand Down
14 changes: 7 additions & 7 deletions lib/Cake/Test/Case/Model/ModelDeleteTest.php
View file
Expand Up @@ -107,7 +107,7 @@ public function testDeleteHabtmReferenceWithConditions() {
$result = $Portfolio->find('first', array( $result = $Portfolio->find('first', array(
'conditions' => array('Portfolio.id' => 1) 'conditions' => array('Portfolio.id' => 1)
)); ));
$this->assertFalse($result); $this->assertEquals(array(), $result);


$result = $Portfolio->ItemsPortfolio->find('all', array( $result = $Portfolio->ItemsPortfolio->find('all', array(
'conditions' => array('ItemsPortfolio.portfolio_id' => 1) 'conditions' => array('ItemsPortfolio.portfolio_id' => 1)
Expand Down Expand Up @@ -195,7 +195,7 @@ public function testDelete() {
$this->assertTrue($result); $this->assertTrue($result);


$result = $TestModel->read(null, 2); $result = $TestModel->read(null, 2);
$this->assertFalse($result); $this->assertEquals(array(), $result);


$TestModel->recursive = -1; $TestModel->recursive = -1;
$result = $TestModel->find('all', array( $result = $TestModel->find('all', array(
Expand All @@ -216,7 +216,7 @@ public function testDelete() {
$this->assertTrue($result); $this->assertTrue($result);


$result = $TestModel->read(null, 3); $result = $TestModel->read(null, 3);
$this->assertFalse($result); $this->assertEquals(array(), $result);


$TestModel->recursive = -1; $TestModel->recursive = -1;
$result = $TestModel->find('all', array( $result = $TestModel->find('all', array(
Expand Down Expand Up @@ -448,16 +448,16 @@ public function testRecursiveDel() {


$TestModel->recursive = 2; $TestModel->recursive = 2;
$result = $TestModel->read(null, 2); $result = $TestModel->read(null, 2);
$this->assertFalse($result); $this->assertEquals(array(), $result);


$result = $TestModel->Comment->read(null, 5); $result = $TestModel->Comment->read(null, 5);
$this->assertFalse($result); $this->assertEquals(array(), $result);


$result = $TestModel->Comment->read(null, 6); $result = $TestModel->Comment->read(null, 6);
$this->assertFalse($result); $this->assertEquals(array(), $result);


$result = $TestModel->Comment->Attachment->read(null, 1); $result = $TestModel->Comment->Attachment->read(null, 1);
$this->assertFalse($result); $this->assertEquals(array(), $result);


$result = $TestModel->find('count'); $result = $TestModel->find('count');
$this->assertEquals(2, $result); $this->assertEquals(2, $result);
Expand Down

8 comments on commit c741f60

@func0der
Copy link
Contributor

Choose a reason for hiding this comment

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

$result = false;
$result['aa'] <-- would not produce any PHP Notice.

$result = array();
$result['aa'] <-- would produce a PHP Notice.

This is just an example of the advantage of false which should not occure anyway, because you'd check with isset().

I don't get this change. Why does $Model->find('first') should return an array instead of false?

@lorenzo
Copy link
Member

Choose a reason for hiding this comment

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

I think it is pretty obvios that $result['aa'] should return a notice if you access something that does not exist, I don't get why would you consider that something desirable

@markstory
Copy link
Member

Choose a reason for hiding this comment

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

The thinking was that it was the only find type to return false. find(count) clearly will return an int at all times. But find(all, list, neighbors) all return arrays even on failure.

@dereuromark
Copy link
Member

Choose a reason for hiding this comment

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

@ func0der: You do not check with isset() here but with !emtpy(). isset() would only work for NULL return values.

You can also just check the result directly. So using an empty array you have the exact same behavior as false if you do:

$result = $this->Model->find('first');
if ($result) {
    $value = $result['Model']['value'];
} else {
    // no record found - do not access the array
}

If there is no result you should not try to access any keys/values in that array.

@func0der
Copy link
Contributor

Choose a reason for hiding this comment

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

@dereuromark You used the wrong name :P
I meant to use "isset($result['aa'])" to check for an index in an array to be set. Isset() won't work on an empty array, that's true.
I tested your code before posting my initial comment, and recognized that, too. It was one of my main concernes about this change. But you are right, there is no real difference.

@markstory Ahh. Okay. Thank you for that information. I thought find('all') returns also false, but I was wrong there as it seems. Considering that you are trying to unify all the finds shouldn't read() be changed to this behavior, too? The return comment on read() is wrong at the moment: " * @return array Array of database fields, or false if not found". It does return false only if the id is NULL or FALSE. If there were no entries in the database for that special id it woud return an empty array, because it is just a wrapper method for find('first') with primary key as condition.

@lorenzo You are right, PHP should return something. But if the content of the variable is actually FALSE and not an array it would not trigger any notice messages if the requested index is not present. This saves just some lines of code here and there and is not really best or even good practice. I just mentioned it, because maybe some applications are relying on that fact. My core question was: Why the change. Which markstory answered already ;)

@markstory
Copy link
Member

Choose a reason for hiding this comment

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

read() is a warty method on Model. It will probably be removed in 3.x as its inconsistent and bad.

@bar
Copy link
Contributor Author

@bar bar commented on c741f60 Feb 1, 2013

Choose a reason for hiding this comment

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

@func0der to late here but the rationale was #917

@func0der
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. You are right. I used it so much before is recognized it kills my whole application because model states are saved in classregistry.

Whatever. Thank all of you for your explanation. :)

Please sign in to comment.