Skip to content

Commit

Permalink
Fixing pagination with complex queries having expressions in order by
Browse files Browse the repository at this point in the history
Closes #7872
  • Loading branch information
lorenzo committed Dec 19, 2015
1 parent e9b3ec5 commit 18b0ab6
Show file tree
Hide file tree
Showing 4 changed files with 67 additions and 0 deletions.
16 changes: 16 additions & 0 deletions src/Database/Expression/QueryExpression.php
Expand Up @@ -511,6 +511,22 @@ public function isCallable($c)
return is_array($c) && isset($c[0]) && is_object($c[0]) && is_callable($c);
}

/**
* Returns true if this expression contains any other nested
* ExpressionInterface objects
*
* @return boolean
*/
public function hasNestedExpression()
{
foreach ($this->_conditions as $c) {
if ($c instanceof ExpressionInterface) {
return true;
}
}
return false;
}

/**
* Auxiliary function used for decomposing a nested array of conditions and build
* a tree structure inside this object to represent the full SQL expression.
Expand Down
6 changes: 6 additions & 0 deletions src/ORM/Query.php
Expand Up @@ -723,6 +723,7 @@ protected function _performCount()
count($query->clause('union')) ||
$query->clause('having')
);

if (!$complex) {
// Expression fields could have bound parameters.
foreach ($query->clause('select') as $field) {
Expand All @@ -733,6 +734,11 @@ protected function _performCount()
}
}

if (!$complex && $this->_valueBinder !== null) {
$order = $this->clause('order');
$complex = $order === null ? false : $order->hasNestedExpression();
}

$count = ['count' => $query->func()->count('*')];

if (!$complex) {
Expand Down
21 changes: 21 additions & 0 deletions tests/TestCase/Database/Expression/QueryExpressionTest.php
Expand Up @@ -112,4 +112,25 @@ public function testDeepCloning()
$this->assertNotSame($originalParts[$i], $part);
});
}

/**
* Tests the hasNestedExpression() function
*
* @return void
*/
public function testHasNestedExpression()
{
$expr = new QueryExpression();
$this->assertFalse($expr->hasNestedExpression());

$expr->add(['a' => 'b']);
$this->assertTrue($expr->hasNestedExpression());

$expr = new QueryExpression();
$expr->add('a = b');
$this->assertFalse($expr->hasNestedExpression());

$expr->add(new QueryExpression('1 = 1'));
$this->assertTrue($expr->hasNestedExpression());
}
}
24 changes: 24 additions & 0 deletions tests/TestCase/ORM/QueryRegressionTest.php
Expand Up @@ -1206,4 +1206,28 @@ public function testFormatResultsMemory()
$endMemory = memory_get_usage() / 1024 / 1024;
$this->assertWithinRange($endMemory, $memory, 1.25, 'Memory leak in ResultSet');
}

/**
* Tests that having bound placeholders in the order clause does not result
* in an error when trying to count a query.
*
* @return void
*/
public function testCountWithComplexOrderBy()
{
$table = TableRegistry::get('Articles');
$query = $table->find();
$query->orderDesc($query->newExpr()->add(['id' => 3]));
$query->order(['title' => 'desc']);
// Executing the normal query before getting the count
$query->all();
$this->assertEquals(3, $query->count());

$table = TableRegistry::get('Articles');
$query = $table->find();
$query->orderDesc($query->newExpr()->add(['id' => 3]));
$query->order(['title' => 'desc']);
// Not executing the query first, just getting the count
$this->assertEquals(3, $query->count());
}
}

0 comments on commit 18b0ab6

Please sign in to comment.