Skip to content

Render int and float clause parameters again (regression from #232) - #242

Open
simon-mundy wants to merge 1 commit into
php-db:0.6.xfrom
simon-mundy:fix/scalar-specification-parameters
Open

simon-mundy wants to merge 1 commit into
php-db:0.6.xfrom
simon-mundy:fix/scalar-specification-parameters

Conversation

@simon-mundy

Copy link
Copy Markdown
Member

Regression

#232 broke unprepared LIMIT/OFFSET for the MySQL and SQLite platforms. With either package's SelectDecorator, rendering an unprepared Select that has an integer limit or offset now throws:

$decorator = (new \PhpDb\Mysql\Sql\SelectDecorator())->setSubject((new Select('t'))->limit(10));
$decorator->getSqlString($platform);
// TypeError: count(): Argument #1 ($value) must be of type Countable|array, int given

Before #232 this rendered SELECT "t".* FROM "t" LIMIT 10, which is still what ->limit('10') gives. The same happens for ->offset(5) on its own and for limit plus offset, in both phpdb-mysql and phpdb-sqlite.

Cause

When the statement is not prepared, the packages' processLimit()/processOffset() return [$this->limit]/[$this->offset] with the raw int. Before #232 those values went straight to vsprintf(), which accepts any scalar. The renderer that #232 added (flattenParameterList(), formatParameterList(), renderParameter()) accepts only strings and treats anything else as a nested list, so it calls count() on the int.

I checked the other process*() overrides in phpdb-mysql, phpdb-sqlite and phpdb-pgsql, including the MySQL DDL decorators. They all return strings, so limit and offset are the only hooks affected.

Fix

  • AbstractSql turns string|int|float|bool|Stringable|null parameters into strings with sprintf('%s', …), which matches vsprintf()'s %s output exactly, float precision included. Arrays still throw RuntimeException::forUnsupportedParameterCountOf().
  • renderParameter() wraps a scalar in a list instead of using (array). That cast would have turned a Stringable object into an array of its properties.
  • The SqlParameters psalm type gains a SqlParameterValue element type. Select::processLimit()/processOffset() now declare array{0: SqlParameterValue}|null, so platform overrides that return the raw value pass analysis.

The platform packages are unchanged.

Tests

test/unit/Sql/UnpreparedScalarLimitOffsetTest.php (#[Group('unit')]) uses TestAsset\UnpreparedScalarLimitSelect. That test double extends Select and returns the raw limit/offset from its hooks the same way the package decorators do, including MySQL's offset-only LIMIT 18446744073709551615. It does not depend on the packages. Data providers cover:

  • int, float, numeric-string and Stringable limits
  • int and float offset-only
  • limit plus offset
  • an int rendered through array specifications: a slot with no parameter specification, a slot with a parameter specification, and a combinedby slot
  • float precision compared against sprintf('%s')
  • nested lists still throwing for string and array specifications

These tests fail on 0.6.x with the TypeError above and pass with this change. The unit suite, mago format --check, mago lint and mago analyze all exit 0, and every changed line in src/ is covered.

The platform SelectDecorators in phpdb-mysql and phpdb-sqlite return the raw
int limit and offset from processLimit()/processOffset() when the statement is
not prepared. The specification renderer added in php-db#232 accepted only strings
and called count() on anything else, so those queries threw a TypeError.

Scalar and Stringable parameters are now converted with sprintf('%s'), exactly
as vsprintf rendered them, while nested lists still raise the
unsupported-parameter-count exception. The SqlParameters type and the
Select::processLimit()/processOffset() return types allow scalar values so
platform overrides type-check.
@codecov

codecov Bot commented Oct 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant