Skip to content

Commit

Permalink
Fix: prevent bound parameters being [null] when no parameters are g…
Browse files Browse the repository at this point in the history
…iven to `Predicate#expression()`

Around `laminas/laminas-db:2.10.1`, a regression was introduced, in which calling
`Laminas\Db\Sql\Predicate#expression("an_expression()")` led to crashes like following
in downstream consumers:

```
Laminas\Db\Sql\Exception\RuntimeException: The number of replacements in the expression does not match the number of parameters

vendor/laminas/laminas-db/src/Sql/Expression.php:151
vendor/laminas/laminas-db/src/Sql/Predicate/PredicateSet.php:178
vendor/laminas/laminas-db/src/Sql/Predicate/PredicateSet.php:178
vendor/laminas/laminas-db/src/Sql/Predicate/PredicateSet.php:178
vendor/laminas/laminas-db/src/Sql/AbstractSql.php:129
vendor/laminas/laminas-db/src/Sql/Select.php:633
```

This was because predicates were initialized with an `array{null}` by
default, when expressions like `$sql->where->expression("some_expression()")`
were used.

The usage of `$sql->where->expression("some_expression()", "foo")` remains
unchanged with this patch.

This fix targets `2.15.x`, and attempts to make predicates safe to use when no
parameters have been given.

While an existing test has indeed been changed, this shouldn't have any effect for
downstream consumers, since `Predicate#expression(string)` didn't work (so far)
anyway, due to the kind of crash highlighted above.
  • Loading branch information
Ocramius committed Dec 5, 2022
1 parent 1125ef2 commit 575e28c
Show file tree
Hide file tree
Showing 2 changed files with 60 additions and 2 deletions.
2 changes: 1 addition & 1 deletion src/Sql/Predicate/Predicate.php
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ public function notLike($identifier, $notLike)
public function expression($expression, $parameters = null)
{
$this->addPredicate(
new Expression($expression, $parameters),
new Expression($expression, func_num_args() > 1 ? $parameters : []),
$this->nextPredicateCombineOperator ?: $this->defaultCombination
);
$this->nextPredicateCombineOperator = null;
Expand Down
60 changes: 59 additions & 1 deletion test/unit/Sql/Predicate/PredicateTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@

namespace LaminasTest\Db\Sql\Predicate;

use Laminas\Db\Adapter\Platform\Sql92;
use Laminas\Db\Sql\Expression;
use Laminas\Db\Sql\Predicate\Predicate;
use Laminas\Db\Sql\Select;
use Laminas\Stdlib\ErrorHandler;
use PHPUnit\Framework\TestCase;

class PredicateTest extends TestCase
Expand Down Expand Up @@ -240,7 +243,7 @@ public function testExpressionNullParameters()
$predicate->expression('foo = bar');
$predicates = $predicate->getPredicates();
$expression = $predicates[0][1];
self::assertEquals([null], $expression->getParameters());
self::assertEquals([], $expression->getParameters());
}

/**
Expand Down Expand Up @@ -276,4 +279,59 @@ public function testLiteral()
$predicate->getExpressionData()
);
}

public function testCanCreateExpressionsWithoutAnyBoundSqlParameters(): void
{
$where1 = new Predicate();

$where1->expression('some_expression()');

self::assertSame(
'SELECT "a_table".* FROM "a_table" WHERE (some_expression())',
$this->makeSqlString($where1)
);
}

public function testWillBindSqlParametersToExpressionsWithGivenParameter(): void
{
$where = new Predicate();

$where->expression('some_expression(?)', null);

self::assertSame(
'SELECT "a_table".* FROM "a_table" WHERE (some_expression(\'\'))',
$this->makeSqlString($where)
);
}

public function testWillBindSqlParametersToExpressionsWithGivenStringParameter(): void
{
$where = new Predicate();

$where->expression('some_expression(?)', 'a string');

self::assertSame(
'SELECT "a_table".* FROM "a_table" WHERE (some_expression(\'a string\'))',
$this->makeSqlString($where)
);
}

private function makeSqlString(Predicate $where): string
{
$select = new Select('a_table');

$select->where($where);

// this is still faster than connecting to a real DB for this kind of test.
// we are using unsafe SQL quoting on purpose here: this raises warnings in production.
ErrorHandler::start(E_USER_NOTICE);

try {
$string = $select->getSqlString(new Sql92());
} finally {
ErrorHandler::stop();
}

return $string;
}
}

0 comments on commit 575e28c

Please sign in to comment.