Skip to content

Bind Select parameters in placeholder order for positional drivers - #237

Open
simon-mundy wants to merge 1 commit into
php-db:0.6.xfrom
simon-mundy:fix/positional-parameter-order
Open

simon-mundy wants to merge 1 commit into
php-db:0.6.xfrom
simon-mundy:fix/positional-parameter-order

Conversation

@simon-mundy

Copy link
Copy Markdown
Member

Problem

With a positional driver (? placeholders, e.g. Mysqli, or PgSql's $# which is numbered in text order), a Select could bind values to the wrong placeholders. ParameterContainer::getPositionalArray() returns values in insertion order, but Select did not always insert them in the order the placeholders appear in the SQL:

$sub = (new Select('src'))->where(['x' => 'FROM-SUBSELECT-VALUE']);
$select = (new Select(['t' => $sub]))
    ->columns(['c' => new Expression('?', ['COLUMN-VALUE'])])
    ->quantifier(new Expression('TOP ?', ['QUANTIFIER-VALUE']));

// SQL:        SELECT TOP ? ? AS "c" FROM (SELECT "src".* FROM "src" WHERE "x" = ?) AS "t"
// positional: FROM-SUBSELECT-VALUE, COLUMN-VALUE, QUANTIFIER-VALUE   (should be the reverse)

Cause

Select::processSelect() resolves the FROM table (including a FROM sub-select) before the columns and join columns, and the quantifier after them. All three bound straight into the shared container.

Fix

In processSelect() the FROM table and the columns (including join columns and column sub-selects) bind into their own temporary ParameterContainers. After the quantifier is processed, the column parameters and then the table parameters are appended to the real container, keeping each parameter's type (errata) and maximum length.

Processing order is unchanged, so parameter names, sub-select numbering and SQL text are byte-identical to before. Named drivers bind the same names and values; only the container's iteration order changes, now following the SQL text. No process*() signatures change.

Every other clause was checked and already binds in text order: Select joins (sub-select table then ON), where, group, having, order, limit/offset and combine; Update (table, joins, set, where); Insert (values, and Insert from select); Delete; Combine.

Tests

New test/unit/Sql/SelectParameterOrderTest.php (#[Group('unit')]):

  • A data provider covering each out-of-order combination: quantifier/column, column/FROM sub-select, quantifier/FROM sub-select, join column/FROM sub-select, column sub-select/FROM sub-select, all three together, a nested FROM sub-select, and a query using every clause. Each case asserts both the SQL and getPositionalArray().
  • A named-driver test asserting the SQL and parameter names and values are the same as before the change (it passes on both the old and new code).
  • Parameter types survive reordering (LIMIT inside a FROM sub-select keeps TYPE_INTEGER).
  • Parameters already in the container stay first.

The out-of-order cases fail on 0.6.x and pass with this change. The unit suite passes, mago format --check, mago lint and mago analyze are clean, and the changed code has full line coverage.

Platform packages

None needed. php-db/phpdb-mysql and php-db/phpdb-sqlite override only processLimit()/processOffset(), which already bind in text order. php-db/phpdb-pgsql has no SQL decorators. All three inherit the fixed processSelect().

processSelect() resolved the FROM table before the columns and the
quantifier after them, so a FROM sub-select's parameters were bound
ahead of column and quantifier parameters that appear earlier in the
SQL. Positional drivers bind getPositionalArray() in insertion order,
so values landed on the wrong placeholders.

The columns and the FROM table now bind into their own containers and
are appended after the quantifier in text order. Processing order is
unchanged, so parameter names and SQL text are identical.
@codecov

codecov Bot commented Oct 12, 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