Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Make Model::find('first') always return an array.

  • Loading branch information...
commit c741f60367be5bba21fb96f65ac220106148ceaa 1 parent 528671f
@bar bar authored
View
15 lib/Cake/Model/Model.php
@@ -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
* 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 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
*/
public function find($type = 'first', $query = array()) {
@@ -2638,10 +2641,10 @@ public function find($type = 'first', $query = array()) {
if ($type === 'all') {
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);
}
}
@@ -2707,7 +2710,7 @@ protected function _findFirst($state, $query, $results = array()) {
return $query;
} elseif ($state === 'after') {
if (empty($results[0])) {
- return false;
+ return array();
}
return $results[0];
}
View
2  lib/Cake/Test/Case/Model/Behavior/TranslateBehaviorTest.php
@@ -422,7 +422,7 @@ public function testMissingTranslation() {
$TestModel = new TranslatedItem();
$TestModel->locale = 'rus';
$result = $TestModel->read(null, 1);
- $this->assertFalse($result);
+ $this->assertEquals(array(), $result);
$TestModel->locale = array('rus');
$result = $TestModel->read(null, 1);
View
14 lib/Cake/Test/Case/Model/ModelDeleteTest.php
@@ -107,7 +107,7 @@ public function testDeleteHabtmReferenceWithConditions() {
$result = $Portfolio->find('first', array(
'conditions' => array('Portfolio.id' => 1)
));
- $this->assertFalse($result);
+ $this->assertEquals(array(), $result);
$result = $Portfolio->ItemsPortfolio->find('all', array(
'conditions' => array('ItemsPortfolio.portfolio_id' => 1)
@@ -195,7 +195,7 @@ public function testDelete() {
$this->assertTrue($result);
$result = $TestModel->read(null, 2);
- $this->assertFalse($result);
+ $this->assertEquals(array(), $result);
$TestModel->recursive = -1;
$result = $TestModel->find('all', array(
@@ -216,7 +216,7 @@ public function testDelete() {
$this->assertTrue($result);
$result = $TestModel->read(null, 3);
- $this->assertFalse($result);
+ $this->assertEquals(array(), $result);
$TestModel->recursive = -1;
$result = $TestModel->find('all', array(
@@ -448,16 +448,16 @@ public function testRecursiveDel() {
$TestModel->recursive = 2;
$result = $TestModel->read(null, 2);
- $this->assertFalse($result);
+ $this->assertEquals(array(), $result);
$result = $TestModel->Comment->read(null, 5);
- $this->assertFalse($result);
+ $this->assertEquals(array(), $result);
$result = $TestModel->Comment->read(null, 6);
- $this->assertFalse($result);
+ $this->assertEquals(array(), $result);
$result = $TestModel->Comment->Attachment->read(null, 1);
- $this->assertFalse($result);
+ $this->assertEquals(array(), $result);
$result = $TestModel->find('count');
$this->assertEquals(2, $result);

8 comments on commit c741f60

@func0der

$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
Owner

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
Owner

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
Collaborator

@ 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

@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
Owner

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

@bar

@func0der to late here but the rationale was #917

@func0der

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.
Something went wrong with that request. Please try again.