Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: prevent bound parameters being [null] when no parameters are given to Predicate#expression() #264

Conversation

Ocramius
Copy link
Member

@Ocramius Ocramius commented Dec 5, 2022

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.

Q A
Documentation no
Bugfix yes
BC Break no
New Feature no
RFC no
QA no

@Ocramius Ocramius added the Bug Something isn't working label Dec 5, 2022
@Ocramius Ocramius added this to the 2.15.1 milestone Dec 5, 2022
@Ocramius
Copy link
Member Author

Ocramius commented Dec 5, 2022

Note: yes, I targeted 2.15.x because I'm abusing my maintainer privileges: need this for a paying customer, and will handle merge-up myself, once I have a review :D

…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.
@Ocramius Ocramius force-pushed the fix/predicate-expressions-should-not-have-values-to-be-bound-when-none-given branch from 575e28c to 1c5ad3a Compare December 5, 2022 18:14
Copy link
Member

@internalsystemerror internalsystemerror left a comment

Choose a reason for hiding this comment

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

LGTM 🚢

@internalsystemerror internalsystemerror merged commit a03d8df into laminas:2.15.x Dec 5, 2022
@Ocramius Ocramius deleted the fix/predicate-expressions-should-not-have-values-to-be-bound-when-none-given branch December 5, 2022 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants