From 4f61cc61322f496441a7f892299b312d0ce05d97 Mon Sep 17 00:00:00 2001 From: Simon Mundy Date: Tue, 13 Oct 2026 10:25:01 +1100 Subject: [PATCH 1/2] Bind Select parameters in placeholder order for positional drivers 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. --- src/Sql/Select.php | 48 ++++- test/unit/Sql/SelectParameterOrderTest.php | 233 +++++++++++++++++++++ 2 files changed, 277 insertions(+), 4 deletions(-) create mode 100644 test/unit/Sql/SelectParameterOrderTest.php diff --git a/src/Sql/Select.php b/src/Sql/Select.php index 183750be9..27836f023 100644 --- a/src/Sql/Select.php +++ b/src/Sql/Select.php @@ -12,6 +12,7 @@ use function array_key_exists; use function array_key_first; +use function array_keys; use function count; use function explode; use function gettype; @@ -617,6 +618,10 @@ protected function processOrder( /** * Process the select part * + * The FROM table is resolved before the columns and the quantifier after them, so the columns and + * the table bind into their own containers and are appended after the quantifier, keeping the + * parameters in the order their placeholders appear in the SQL. + * * @return list>> * @throws Exception\RuntimeException */ @@ -627,7 +632,10 @@ protected function processSelect( ): array { $expr = 1; - [$table, $fromTable] = $this->resolveSelectTable($this->table, $platform, $driver, $parameterContainer); + $tableParameters = $parameterContainer instanceof ParameterContainer ? new ParameterContainer() : null; + $columnParameters = $parameterContainer instanceof ParameterContainer ? new ParameterContainer() : null; + + [$table, $fromTable] = $this->resolveSelectTable($this->table, $platform, $driver, $tableParameters); $columns = []; foreach ($this->columns as $columnIndexOrAs => $column) { if (self::SQL_STAR === $column) { @@ -643,7 +651,7 @@ protected function processSelect( ], $platform, $driver, - $parameterContainer, + $columnParameters, is_string($columnIndexOrAs) ? $columnIndexOrAs : 'column', ); $columnAs = match (true) { @@ -659,7 +667,7 @@ protected function processSelect( foreach ($this->getJoins()->getJoins() as $join) { $joinName = is_array($join['name']) ? array_key_first($join['name']) : $join['name']; - $joinName = $this->resolveTable($joinName, $platform, $driver, $parameterContainer); + $joinName = $this->resolveTable($joinName, $platform, $driver, $columnParameters); foreach ($join['columns'] as $jKey => $jColumn) { $jColumns = []; @@ -674,7 +682,7 @@ protected function processSelect( ], $platform, $driver, - $parameterContainer, + $columnParameters, is_string($jKey) ? $jKey : 'column', ); $jColumnAs = match (true) { @@ -699,6 +707,8 @@ protected function processSelect( : $this->quantifier; } + $this->appendParameters($parameterContainer, $columnParameters, $tableParameters); + if (null === $table) { return [$columns]; } @@ -748,6 +758,36 @@ protected function processWhere( ]; } + /** + * Append the parameters of separately bound parts, in the order given + */ + private function appendParameters(?ParameterContainer $target, ?ParameterContainer ...$sources): void + { + foreach ($sources as $source) { + if (null === $target || null === $source) { + continue; + } + + $this->copyParameters($source, $target); + } + } + + /** + * Copy each parameter with its type and maximum length, keeping their order + */ + private function copyParameters(ParameterContainer $source, ParameterContainer $target): void + { + foreach (array_keys($source->getNamedArray()) as $name) { + $name = (string) $name; + $target->offsetSet( + $name, + $source->offsetGet($name), + $source->offsetHasErrata($name) ? $source->offsetGetErrata($name) : null, + $source->offsetHasMaxLength($name) ? $source->offsetGetMaxLength($name) : null, + ); + } + } + private function getHaving(): Having { return $this->having ??= new Having(); diff --git a/test/unit/Sql/SelectParameterOrderTest.php b/test/unit/Sql/SelectParameterOrderTest.php new file mode 100644 index 000000000..b4f25c536 --- /dev/null +++ b/test/unit/Sql/SelectParameterOrderTest.php @@ -0,0 +1,233 @@ +}> + */ + public static function outOfOrderSelectProvider(): array + { + return [ + 'quantifier before column' => [ + static fn(): Select => (new Select('t'))->columns(['c' => new Expression('?', ['COLUMN'])]) + ->quantifier(new Expression('TOP ?', ['QUANTIFIER'])), + 'SELECT TOP ? ? AS "c" FROM "t"', + ['QUANTIFIER', 'COLUMN'], + ], + 'column before from sub-select' => [ + static fn(): Select => (new Select(['t' => (new Select('src'))->where(['x' => 'FROM'])])) + ->columns(['c' => new Expression('?', ['COLUMN'])]), + 'SELECT ? AS "c" FROM (SELECT "src".* FROM "src" WHERE "x" = ?) AS "t"', + ['COLUMN', 'FROM'], + ], + 'quantifier before from sub-select' => [ + static fn(): Select => (new Select(['t' => (new Select('src'))->where(['x' => 'FROM'])])) + ->quantifier(new Expression('TOP ?', ['QUANTIFIER'])), + 'SELECT TOP ? "t".* FROM (SELECT "src".* FROM "src" WHERE "x" = ?) AS "t"', + ['QUANTIFIER', 'FROM'], + ], + 'join column before from sub-select' => [ + static fn(): Select => (new Select(['t' => (new Select('src'))->where(['x' => 'FROM'])])) + ->join(['j' => 'jt'], 'j.id = t.id', ['jc' => new Expression('?', ['JOIN-COLUMN'])]), + 'SELECT "t".*, ? AS "jc" FROM (SELECT "src".* FROM "src" WHERE "x" = ?) AS "t"' + . ' INNER JOIN "jt" AS "j" ON "j"."id" = "t"."id"', + ['JOIN-COLUMN', 'FROM'], + ], + 'column sub-select before from sub-select' => [ + static fn(): Select => (new Select(['t' => (new Select('src'))->where(['x' => 'FROM'])])) + ->columns(['c' => (new Select('cs'))->where(['y' => 'COLUMN-SUB-SELECT'])]), + 'SELECT (SELECT "cs".* FROM "cs" WHERE "y" = ?) AS "c"' + . ' FROM (SELECT "src".* FROM "src" WHERE "x" = ?) AS "t"', + ['COLUMN-SUB-SELECT', 'FROM'], + ], + 'quantifier, column and from sub-select' => [ + static fn(): Select => (new Select(['t' => (new Select('src'))->where([ + 'x' => 'FROM', + ])]))->columns(['c' => new Expression('?', ['COLUMN'])]) + ->quantifier(new Expression('TOP ?', ['QUANTIFIER'])), + 'SELECT TOP ? ? AS "c" FROM (SELECT "src".* FROM "src" WHERE "x" = ?) AS "t"', + ['QUANTIFIER', 'COLUMN', 'FROM'], + ], + 'nested from sub-select' => [ + static fn(): Select => new Select([ + 'outer' => (new Select(['inner' => (new Select('src'))->where(['x' => 'INNER-FROM'])])) + ->columns(['c' => new Expression('?', ['OUTER-COLUMN'])]), + ]), + 'SELECT "outer".* FROM (SELECT ? AS "c"' + . ' FROM (SELECT "src".* FROM "src" WHERE "x" = ?) AS "inner") AS "outer"', + ['OUTER-COLUMN', 'INNER-FROM'], + ], + 'every clause' => [ + static fn(): Select => (new Select(['t' => (new Select('src'))->where([ + 'x' => 'FROM', + ])]))->columns(['c' => new Expression('?', ['COLUMN'])]) + ->quantifier(new Expression('TOP ?', ['QUANTIFIER'])) + ->join( + ['j' => (new Select('js'))->where(['z' => 'JOIN-TABLE'])], + new Operator('j.k', Operator::OPERATOR_EQUAL_TO, 'JOIN-ON'), + ['jc' => new Expression('?', ['JOIN-COLUMN'])], + ) + ->where(['w' => 'WHERE']) + ->group(new Expression('?', ['GROUP'])) + ->having(['h' => 'HAVING']) + ->order(new Expression('?', ['ORDER'])) + ->limit(5) + ->offset(10), + 'SELECT TOP ? ? AS "c", ? AS "jc" FROM (SELECT "src".* FROM "src" WHERE "x" = ?) AS "t"' + . ' INNER JOIN (SELECT "js".* FROM "js" WHERE "z" = ?) AS "j" ON "j"."k" = ?' + . ' WHERE "w" = ? GROUP BY ? HAVING "h" = ? ORDER BY ? LIMIT ? OFFSET ?', + [ + 'QUANTIFIER', + 'COLUMN', + 'JOIN-COLUMN', + 'FROM', + 'JOIN-TABLE', + 'JOIN-ON', + 'WHERE', + 'GROUP', + 'HAVING', + 'ORDER', + 5, + 10, + ], + ], + ]; + } + + #[Test] + public function appendsAfterParametersAlreadyInTheContainer(): void + { + $select = (new Select(['t' => (new Select('src'))->where(['x' => 'FROM'])])) + ->columns(['c' => new Expression('?', ['COLUMN'])]); + + $parameterContainer = new ParameterContainer(['existing' => 'EXISTING']); + + $this->prepare($select, $this->createDriver('positional'), $parameterContainer); + + static::assertSame(['EXISTING', 'COLUMN', 'FROM'], $parameterContainer->getPositionalArray()); + } + + /** + * @param Closure(): Select $build + * @param list $expectedValues + */ + #[DataProvider('outOfOrderSelectProvider')] + #[Test] + public function bindsPositionalParametersInPlaceholderOrder( + Closure $build, + string $expectedSql, + array $expectedValues, + ): void { + $parameterContainer = new ParameterContainer(); + + $sql = $this->prepare($build(), $this->createDriver('positional'), $parameterContainer); + + static::assertSame( + [$expectedSql, $expectedValues], + [$sql, $parameterContainer->getPositionalArray()], + ); + } + + #[Test] + public function keepsNamedParameterNamesAndSql(): void + { + $select = (new Select(['t' => (new Select('src'))->where(['x' => 'FROM'])]))->columns([ + 'c' => new Expression('?', ['COLUMN']), + 'sq' => (new Select('cs'))->where(['y' => 'COLUMN-SUB-SELECT']), + ]) + ->quantifier(new Expression('TOP ?', ['QUANTIFIER'])) + ->join(['j' => 'jt'], 'j.id = t.id', ['jc' => new Expression('?', ['JOIN-COLUMN'])]) + ->where(['w' => 'WHERE']) + ->limit(5) + ->offset(10); + + $parameterContainer = new ParameterContainer(); + + $sql = $this->prepare($select, $this->createDriver('named'), $parameterContainer); + + $namedParameters = $parameterContainer->getNamedArray(); + ksort($namedParameters); + + static::assertSame( + [ + 'SELECT TOP :quantifier1 :c1 AS "c", (SELECT "cs".* FROM "cs" WHERE "y" = :subselect2where1) AS "sq",' + . ' :jc1 AS "jc" FROM (SELECT "src".* FROM "src" WHERE "x" = :subselect1where1) AS "t"' + . ' INNER JOIN "jt" AS "j" ON "j"."id" = "t"."id" WHERE "w" = :where1' + . ' LIMIT :limit OFFSET :offset', + [ + 'c1' => 'COLUMN', + 'jc1' => 'JOIN-COLUMN', + 'limit' => 5, + 'offset' => 10, + 'quantifier1' => 'QUANTIFIER', + 'subselect1where1' => 'FROM', + 'subselect2where1' => 'COLUMN-SUB-SELECT', + 'where1' => 'WHERE', + ], + ], + [$sql, $namedParameters], + ); + } + + #[Test] + public function keepsParameterTypeOfReorderedParameters(): void + { + $select = (new Select(['t' => (new Select('src'))->limit(5)])) + ->columns(['c' => new Expression('?', ['COLUMN'])]); + + $parameterContainer = new ParameterContainer(); + + $this->prepare($select, $this->createDriver('positional'), $parameterContainer); + + static::assertSame( + [['COLUMN', 5], ParameterContainer::TYPE_INTEGER], + [$parameterContainer->getPositionalArray(), $parameterContainer->offsetGetErrata('subselect1limit')], + ); + } + + private function createDriver(string $prepareType): DriverInterface + { + $formatParameterName = static fn(string $name): string => 'named' === $prepareType ? ":{$name}" : '?'; + + $driver = $this->createStub(DriverInterface::class); + $driver->method('getPrepareType')->willReturn($prepareType); + $driver->method('formatParameterName')->willReturnCallback($formatParameterName); + + return $driver; + } + + private function prepare(Select $select, DriverInterface $driver, ParameterContainer $parameterContainer): string + { + $adapter = $this->createStub(AdapterInterface::class); + $adapter->method('getPlatform')->willReturn(new TrustingSql92Platform()); + $adapter->method('getDriver')->willReturn($driver); + + return (string) $select->prepareStatement($adapter, new StatementContainer(null, $parameterContainer)) + ->getSql(); + } +} From 41c22d670f60b8d2fa6e0d8c8d377ce84d402743 Mon Sep 17 00:00:00 2001 From: Simon Mundy Date: Wed, 4 Nov 2026 15:59:09 +1100 Subject: [PATCH 2/2] Bind the quantifier first and keep one table container Process the quantifier before the columns, so the quantifier and column parameters bind straight into the statement's container in placeholder order. Only the FROM table, which must be resolved before the columns, needs its own container, appended after them. The join name never binds parameters, so it no longer receives a container. This drops a container allocation and copy per Select, removes the redundant null checks in appendParameters(), and drops a cast that was only there for the analyzer. A new test pins that an empty quantifier renders no quantifier. --- src/Sql/Select.php | 55 ++++++++++------------ test/unit/Sql/SelectParameterOrderTest.php | 2 +- test/unit/Sql/SelectTest.php | 11 +++++ 3 files changed, 37 insertions(+), 31 deletions(-) diff --git a/src/Sql/Select.php b/src/Sql/Select.php index 27836f023..fa5429d68 100644 --- a/src/Sql/Select.php +++ b/src/Sql/Select.php @@ -618,9 +618,9 @@ protected function processOrder( /** * Process the select part * - * The FROM table is resolved before the columns and the quantifier after them, so the columns and - * the table bind into their own containers and are appended after the quantifier, keeping the - * parameters in the order their placeholders appear in the SQL. + * The quantifier binds first and the columns after it, in the order their placeholders appear in + * the SQL. The FROM table must be resolved before the columns, which are prefixed with it, so it + * binds into its own container and its parameters are appended after the columns. * * @return list>> * @throws Exception\RuntimeException @@ -632,8 +632,8 @@ protected function processSelect( ): array { $expr = 1; - $tableParameters = $parameterContainer instanceof ParameterContainer ? new ParameterContainer() : null; - $columnParameters = $parameterContainer instanceof ParameterContainer ? new ParameterContainer() : null; + $quantifier = $this->renderQuantifier($platform, $driver, $parameterContainer); + $tableParameters = $parameterContainer instanceof ParameterContainer ? new ParameterContainer() : null; [$table, $fromTable] = $this->resolveSelectTable($this->table, $platform, $driver, $tableParameters); $columns = []; @@ -651,7 +651,7 @@ protected function processSelect( ], $platform, $driver, - $columnParameters, + $parameterContainer, is_string($columnIndexOrAs) ? $columnIndexOrAs : 'column', ); $columnAs = match (true) { @@ -667,7 +667,7 @@ protected function processSelect( foreach ($this->getJoins()->getJoins() as $join) { $joinName = is_array($join['name']) ? array_key_first($join['name']) : $join['name']; - $joinName = $this->resolveTable($joinName, $platform, $driver, $columnParameters); + $joinName = $this->resolveTable($joinName, $platform, $driver); foreach ($join['columns'] as $jKey => $jColumn) { $jColumns = []; @@ -682,7 +682,7 @@ protected function processSelect( ], $platform, $driver, - $columnParameters, + $parameterContainer, is_string($jKey) ? $jKey : 'column', ); $jColumnAs = match (true) { @@ -700,15 +700,10 @@ protected function processSelect( } } - $quantifier = null; - if ($this->quantifier) { - $quantifier = $this->quantifier instanceof ExpressionInterface - ? $this->processExpression($this->quantifier, $platform, $driver, $parameterContainer, 'quantifier') - : $this->quantifier; + if ($parameterContainer instanceof ParameterContainer && $tableParameters instanceof ParameterContainer) { + $this->copyParameters($tableParameters, $parameterContainer); } - $this->appendParameters($parameterContainer, $columnParameters, $tableParameters); - if (null === $table) { return [$columns]; } @@ -758,27 +753,12 @@ protected function processWhere( ]; } - /** - * Append the parameters of separately bound parts, in the order given - */ - private function appendParameters(?ParameterContainer $target, ?ParameterContainer ...$sources): void - { - foreach ($sources as $source) { - if (null === $target || null === $source) { - continue; - } - - $this->copyParameters($source, $target); - } - } - /** * Copy each parameter with its type and maximum length, keeping their order */ private function copyParameters(ParameterContainer $source, ParameterContainer $target): void { foreach (array_keys($source->getNamedArray()) as $name) { - $name = (string) $name; $target->offsetSet( $name, $source->offsetGet($name), @@ -810,6 +790,21 @@ private function prefixFromTable(?string $fromTable, PlatformInterface $platform : ''; } + /** @throws Exception\RuntimeException */ + private function renderQuantifier( + PlatformInterface $platform, + ?DriverInterface $driver, + ?ParameterContainer $parameterContainer, + ): ?string { + if (! $this->quantifier) { + return null; + } + + return $this->quantifier instanceof ExpressionInterface + ? $this->processExpression($this->quantifier, $platform, $driver, $parameterContainer, 'quantifier') + : $this->quantifier; + } + /** * @param Select|TableReference|null $table * @return array{0: string|null, 1: string} diff --git a/test/unit/Sql/SelectParameterOrderTest.php b/test/unit/Sql/SelectParameterOrderTest.php index b4f25c536..1d157115d 100644 --- a/test/unit/Sql/SelectParameterOrderTest.php +++ b/test/unit/Sql/SelectParameterOrderTest.php @@ -23,7 +23,7 @@ #[Group('unit')] #[CoversMethod(Select::class, 'processSelect')] -#[CoversMethod(Select::class, 'appendParameters')] +#[CoversMethod(Select::class, 'renderQuantifier')] #[CoversMethod(Select::class, 'copyParameters')] final class SelectParameterOrderTest extends TestCase { diff --git a/test/unit/Sql/SelectTest.php b/test/unit/Sql/SelectTest.php index 7bf7f2120..081ac83ee 100644 --- a/test/unit/Sql/SelectTest.php +++ b/test/unit/Sql/SelectTest.php @@ -66,6 +66,7 @@ #[CoversMethod(Select::class, '__clone')] #[CoversMethod(Select::class, 'processJoins')] #[CoversMethod(Select::class, 'processSelect')] +#[CoversMethod(Select::class, 'renderQuantifier')] #[CoversMethod(Select::class, 'processWhere')] #[CoversMethod(Select::class, 'processGroup')] #[CoversMethod(Select::class, 'processHaving')] @@ -1044,6 +1045,16 @@ public function construct(): void static::assertSame('foo', $select->getRawState('table')); } + #[Test] + public function emptyQuantifierRendersNoQuantifier(): void + { + static::assertSame( + 'SELECT "t".* FROM "t"', + (new Select('t'))->quantifier('') + ->getSqlString(new TrustingSql92Platform()), + ); + } + #[Test] #[TestDox('unit test: Test from() returns Select object (is chainable)')] public function from(): void