Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Database::read() should NOT return false when 'limit' is used and there are no results. #171

Closed
wants to merge 1 commit into from

3 participants

@ifunk

Side-effect of this patch is that 2 queries are executed even if no results are returned in the first query. I wasn't sure if there would be any negative side-effects with only running the first query.

@Howard3
Collaborator

Hello ifunk,

I'm not sure I follow the idea here. Can you provide an instance where this is causing a problem and why it should work this way?

Thanks and Best Regards,
Howard

@ifunk

The idea is that a find('all') should return an empty recordset if no records are found.

At the moment if you use find('all', array('limit' => 10)) and no records are found, a boolean is returned, not a recordset. This causes unexpected errors when a boolean is returned. eg.

$data = $model::all($options)->data();
return compact('data');
@Howard3
Collaborator

The reason for this is we recommend usage like the following:

$data = Model::all($options);
if (!$data) {
//Do stuff, no results
}

if we're returning a blank recordset, that functionality will break.

@Howard3
Collaborator

Something I'd like to add however, you can wrap your models or even the database->read functions with a filter so that you can have functionality you desire in your application without having to modify the core.

@ifunk

I'd like to point out again, find('all') only returns false if you're using the limit param. If you omit the limit param and get no results, an empty recordset is returned. This is an inconsistency that should not be determined by the limit param.

@Howard3
Collaborator

I see, that's actually unintional behavior along the other route, it should always return false when there are no results.

@ifunk

Actually, it's only when using 'limit' and 'hasMany' together when there are no records.

Your point about using if ($data) { is a bit moot since it doesn't work like that currently (for most find('all') calls).

I can understand why find('first') should return false if the record doesn't exist, but for find('all') it can make sense to have the query, field and model information available in the resultset even if there are no records.

@Howard3
Collaborator

This will be brought into core later, I want to go about a way to prevent it from doing two queries though. Thanks for the information ifunk!

@ifunk

For anyone else needing a temporary fix for this bug, I've written a filter that can be included in bootstrap.php after the connections.php include.

http://pastium.org/view/faf249843a8ddc46afafec92096a1789

@nateabele
Owner

@ifunk: Good call. Nice use of call_user_func() to isolate variables in the bootstrap process. We'll try and get the core cleaned up as soon as possible.

@nateabele
Owner

Merged manually. Thanks for the patch.

@nateabele nateabele closed this
@ifunk ifunk deleted the ifunk:patch-test-database-read-limit branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 25, 2011
  1. @ifunk
This page is out of date. Refresh to see the latest.
View
13 data/source/Database.php
@@ -286,14 +286,13 @@ public function read($query, array $options = array()) {
)
));
$ids = $self->read($subQuery, array('subquery' => true));
- if (!$ids->count()) {
- return false;
+ if ($ids->count()) {
+ $idData = $ids->data();
+ $ids = array_map(function($index) use ($key) {
+ return $index[$key];
+ }, $idData);
+ $query->limit(false)->conditions(array("{$name}.{$key}" => $ids));
}
- $idData = $ids->data();
- $ids = array_map(function($index) use ($key) {
- return $index[$key];
- }, $idData);
- $query->limit(false)->conditions(array("{$name}.{$key}" => $ids));
}
$sql = $self->renderCommand($query);
}
View
2  tests/cases/data/source/DatabaseTest.php
@@ -664,7 +664,7 @@ public function testReadWithHasManyAndLimit() {
'limit' => 1
);
$result = $this->db->read(new Query($options), $options);
- $this->assertFalse($result);
+ $this->assertTrue($result instanceof RecordSet);
}
public function testGroup() {
Something went wrong with that request. Please try again.