From 015568fd2834ca7ee5001d75bd8b2e3084b621cd Mon Sep 17 00:00:00 2001 From: Simon Mundy Date: Mon, 12 Oct 2026 13:19:05 +1100 Subject: [PATCH 1/3] Give each value of an array expression parameter its own placeholder An array passed to Expression becomes an Argument\Values, which processExpression() flattens into one bound value per element, but getExpressionData() still replaced its `?` with a single `%s`. The extra values then filled the placeholders of whatever followed in the same predicate set: where([new Expression('a IN (?)', [[1, 2, 3]]), 'b' => 4]) rendered `a IN (:where1) AND :where2 = :where3`, binding 2 and 3 in place of the column "b" and its value. Each `?` is now replaced with as many placeholders as its argument binds: a comma-separated list for Values (NULL when empty) and a single `%s` otherwise. The surrounding parentheses stay in the expression, as the documented `id IN (?)` example expects. That example also called Argument::value() with an array, which is a TypeError, so it now uses Argument::values(). Signed-off-by: Simon Mundy --- docs/book/sql/advanced.md | 6 +++- src/Sql/Expression.php | 31 ++++++++++++++--- test/unit/Sql/ExpressionTest.php | 32 ++++++++++++++++++ test/unit/Sql/SelectTest.php | 58 ++++++++++++++++++++++++++++++++ 4 files changed, 122 insertions(+), 5 deletions(-) diff --git a/docs/book/sql/advanced.md b/docs/book/sql/advanced.md index 4540209d..06dc865e 100644 --- a/docs/book/sql/advanced.md +++ b/docs/book/sql/advanced.md @@ -46,7 +46,7 @@ CASE WHEN age > 18 THEN ADULT ELSE MINOR END ```php title="Array values in expressions" $expression = new Expression( 'id IN (?)', - [Argument::value([1, 2, 3, 4, 5])] + [Argument::values([1, 2, 3, 4, 5])] ); ``` @@ -56,6 +56,10 @@ Produces: id IN (?, ?, ?, ?, ?) ``` +The placeholder for an array (or `Argument\Values`) expands to one +placeholder per value, separated by commas, so write the surrounding +parentheses in the expression yourself. An empty array renders as `NULL`. + ```php title="Nested expressions" $innerExpression = new Expression('COUNT(*)'); $outerExpression = new Expression( diff --git a/src/Sql/Expression.php b/src/Sql/Expression.php index 5820bcb1..ead23e2c 100644 --- a/src/Sql/Expression.php +++ b/src/Sql/Expression.php @@ -9,11 +9,15 @@ use PhpDb\Sql\Argument\Value; use PhpDb\Sql\Argument\Values; +use function array_fill; +use function array_shift; use function array_slice; use function array_unique; use function count; +use function explode; use function func_get_args; use function func_num_args; +use function implode; use function is_array; use function preg_match_all; use function str_replace; @@ -58,6 +62,22 @@ public function __construct( $this->setParameters($parameters); } + /** + * Values are bound one by one, so each needs its own placeholder + */ + private static function placeholderFor(?ArgumentInterface $parameter): string + { + if (! $parameter instanceof Values) { + return '%s'; + } + + $count = count($parameter->getValue()); + + return $count > 0 + ? implode(', ', array_fill(0, count: $count, value: '%s')) + : 'NULL'; + } + public function getExpression(): string { return $this->expression; @@ -81,13 +101,16 @@ public function getExpressionData(): array ]; } - // assign locally, escaping % signs - $count = 0; - $specification = str_replace(self::PLACEHOLDER, replace: '%s', subject: $specification, count: $count); + $segments = explode(self::PLACEHOLDER, $specification); + $specification = array_shift($segments); + + foreach ($segments as $index => $segment) { + $specification .= self::placeholderFor($parameters[$index] ?? null) . $segment; + } // test number of replacements without considering same variable begin used many times first, which is // faster, if the test fails then resort to regex which are slow and used rarely - if ($count !== $parametersCount) { + if (count($segments) !== $parametersCount) { $matches = []; preg_match_all('/:\w*/', $specification, $matches); if (count(array_unique($matches[0])) !== $parametersCount) { diff --git a/test/unit/Sql/ExpressionTest.php b/test/unit/Sql/ExpressionTest.php index 2237448d..c88aa0f3 100644 --- a/test/unit/Sql/ExpressionTest.php +++ b/test/unit/Sql/ExpressionTest.php @@ -30,8 +30,24 @@ #[CoversMethod(Expression::class, 'setParameters')] #[CoversMethod(Expression::class, 'getParameters')] #[CoversMethod(Expression::class, 'getExpressionData')] +#[CoversMethod(Expression::class, 'placeholderFor')] final class ExpressionTest extends TestCase { + /** @return array, string}> */ + public static function arrayParameterSpecificationProvider(): array + { + return [ + 'array of three values' => ['a IN (?)', [[1, 2, 3]], 'a IN (%s, %s, %s)'], + 'empty array' => ['a IN (?)', [[]], 'a IN (NULL)'], + 'array between other parameters' => [ + '? IN (?) AND ? = ?', + [Argument::identifier('a'), [1, 2], Argument::identifier('b'), 4], + '%s IN (%s, %s) AND %s = %s', + ], + 'values argument' => ['a IN (?)', [Argument::values([1, 2])], 'a IN (%s, %s)'], + ]; + } + /** @psalm-return array */ public static function falsyExpressionParametersProvider(): array { @@ -113,6 +129,22 @@ public function getExpressionData(): void ); } + /** + * @param list $parameters + */ + #[Test] + #[Group('unit')] + #[DataProvider('arrayParameterSpecificationProvider')] + public function getExpressionDataGivesEachArrayValueItsOwnPlaceholder( + string $expression, + array $parameters, + string $expectedSpecification, + ): void { + $expressionData = (new Expression($expression, $parameters))->getExpressionData(); + + static::assertSame($expectedSpecification, $expressionData['spec']); + } + #[Test] public function getExpressionDataThrowsExceptionWhenParameterCountMismatch(): void { diff --git a/test/unit/Sql/SelectTest.php b/test/unit/Sql/SelectTest.php index eeabdc93..0c537425 100644 --- a/test/unit/Sql/SelectTest.php +++ b/test/unit/Sql/SelectTest.php @@ -4,10 +4,12 @@ namespace PhpDbTest\Sql; +use Closure; use PhpDb\Adapter\Driver\DriverInterface; use PhpDb\Adapter\Driver\StatementInterface; use PhpDb\Adapter\ParameterContainer; use PhpDb\Adapter\Platform\Sql92; +use PhpDb\Adapter\StatementContainer; use PhpDb\Sql\Argument; use PhpDb\Sql\Argument\Identifier; use PhpDb\Sql\Argument\Value; @@ -77,6 +79,28 @@ final class SelectTest extends TestCase { use AdapterTestTrait; + /** + * @return array}, string, array}> + */ + public static function arrayExpressionParameterProvider(): array + { + return [ + 'prepared' => [ + static fn(self $test, Select $select): array => $test->prepareWithNamedParameters($select), + 'SELECT "t".* FROM "t" WHERE a IN (:where1, :where2, :where3) AND "b" = :where4', + ['where1' => 1, 'where2' => 2, 'where3' => 3, 'where4' => 4], + ], + 'unprepared' => [ + static fn(self $test, Select $select): array => [ + $select->getSqlString(new TrustingSql92Platform()), + [], + ], + 'SELECT "t".* FROM "t" WHERE a IN (\'1\', \'2\', \'3\') AND "b" = \'4\'', + [], + ], + ]; + } + /** * @psalm-return array} $render + * @param array $expectedParameters + */ + #[Test] + #[Group('unit')] + #[DataProvider('arrayExpressionParameterProvider')] + public function arrayExpressionParameterDoesNotShiftValuesIntoFollowingPredicates( + Closure $render, + string $expectedSql, + array $expectedParameters, + ): void { + $select = new Select('t'); + $select->where([new Predicate\Expression('a IN (?)', [[1, 2, 3]]), 'b' => 4]); + + static::assertSame([$expectedSql, $expectedParameters], $render($this, $select)); + } + #[Test] #[TestDox('unit test: Test join() exception with bad join')] public function badJoin(): void @@ -1849,4 +1891,20 @@ public function whereReturnsSameSelectObject(): void $select = new Select(); static::assertSame($select, $select->where('x = y')); } + + /** + * @return array{?string, array} + */ + private function prepareWithNamedParameters(Select $select): array + { + $driver = $this->createStub(DriverInterface::class); + $driver->method('formatParameterName')->willReturnCallback(static fn(string $name): string => ":{$name}"); + + $parameterContainer = new ParameterContainer(); + $statement = new StatementContainer(null, $parameterContainer); + + $select->prepareStatement($this->createMockAdapter($driver, new TrustingSql92Platform()), $statement); + + return [$statement->getSql(), $parameterContainer->getNamedArray()]; + } } From 930d88eacd82e831558106eb1ee5aedc128a7046 Mon Sep 17 00:00:00 2001 From: Simon Mundy Date: Mon, 12 Oct 2026 13:47:28 +1100 Subject: [PATCH 2/3] Reject an array expression parameter outside its own parentheses An array or Argument\Values bound to a `?` expands to a comma-separated list of placeholders, which is only valid SQL when the `?` sits alone inside parentheses. `new Expression('a IN ?', [[1, 2, 3]])` rendered `a IN :p1, :p2, :p3`, so getExpressionData() now throws InvalidArgumentException::forUnenclosedValuesPlaceholder() unless the placeholder is directly wrapped by `(` and `)`, whitespace allowed. Signed-off-by: Simon Mundy --- docs/book/sql/advanced.md | 4 ++ .../Exception/InvalidArgumentException.php | 9 ++++ src/Sql/Expression.php | 16 +++++- .../InvalidArgumentExceptionTest.php | 54 ++++++++++--------- test/unit/Sql/ExpressionTest.php | 32 +++++++++++ 5 files changed, 90 insertions(+), 25 deletions(-) diff --git a/docs/book/sql/advanced.md b/docs/book/sql/advanced.md index 06dc865e..1cdf55f8 100644 --- a/docs/book/sql/advanced.md +++ b/docs/book/sql/advanced.md @@ -59,6 +59,10 @@ id IN (?, ?, ?, ?, ?) The placeholder for an array (or `Argument\Values`) expands to one placeholder per value, separated by commas, so write the surrounding parentheses in the expression yourself. An empty array renders as `NULL`. +The placeholder must sit alone inside its parentheses: `id IN ?` and +`id IN (?, ?)` with an array throw +`PhpDb\Sql\Exception\InvalidArgumentException`, since they would render +invalid SQL. ```php title="Nested expressions" $innerExpression = new Expression('COUNT(*)'); diff --git a/src/Sql/Exception/InvalidArgumentException.php b/src/Sql/Exception/InvalidArgumentException.php index 730e4c17..8066080c 100644 --- a/src/Sql/Exception/InvalidArgumentException.php +++ b/src/Sql/Exception/InvalidArgumentException.php @@ -72,6 +72,10 @@ class InvalidArgumentException extends Exception\InvalidArgumentException 'A PhpDb\Sql\Select instance cannot be provided' . ' with the merge flag'; + final public const string UNENCLOSED_VALUES_PLACEHOLDER = + 'An array or Values parameter in the expression "%s" must use a placeholder' + . ' wrapped in its own parentheses, such as IN (?)'; + final public const string UNKNOWN_ARGUMENT_TYPE = 'Unknown argument type'; final public const string UNKNOWN_COLUMN_KEY = 'The key %s was not found in this objects column list'; @@ -188,6 +192,11 @@ public static function forSelectWithMergeFlag(): self return new self(self::SELECT_WITH_MERGE_FLAG); } + public static function forUnenclosedValuesPlaceholder(string $expression): self + { + return new self(sprintf(self::UNENCLOSED_VALUES_PLACEHOLDER, $expression)); + } + public static function forUnknownArgumentType(): self { return new self(self::UNKNOWN_ARGUMENT_TYPE); diff --git a/src/Sql/Expression.php b/src/Sql/Expression.php index ead23e2c..7a854ab6 100644 --- a/src/Sql/Expression.php +++ b/src/Sql/Expression.php @@ -19,6 +19,7 @@ use function func_num_args; use function implode; use function is_array; +use function preg_match; use function preg_match_all; use function str_replace; @@ -84,6 +85,7 @@ public function getExpression(): string } /** + * @throws Exception\InvalidArgumentException * @throws Exception\RuntimeException * @inheritDoc */ @@ -105,7 +107,19 @@ public function getExpressionData(): array $specification = array_shift($segments); foreach ($segments as $index => $segment) { - $specification .= self::placeholderFor($parameters[$index] ?? null) . $segment; + $parameter = $parameters[$index] ?? null; + + if ( + $parameter instanceof Values + && ( + 1 !== preg_match('/\(\s*$/', $specification) + || 1 !== preg_match('/^\s*\)/', $segment) + ) + ) { + throw Exception\InvalidArgumentException::forUnenclosedValuesPlaceholder($this->expression); + } + + $specification .= self::placeholderFor($parameter) . $segment; } // test number of replacements without considering same variable begin used many times first, which is diff --git a/test/unit/Sql/Exception/InvalidArgumentExceptionTest.php b/test/unit/Sql/Exception/InvalidArgumentExceptionTest.php index 39ff8fd2..5a77eebf 100644 --- a/test/unit/Sql/Exception/InvalidArgumentExceptionTest.php +++ b/test/unit/Sql/Exception/InvalidArgumentExceptionTest.php @@ -38,6 +38,7 @@ #[CoversMethod(InvalidArgumentException::class, 'forNonStringValueKey')] #[CoversMethod(InvalidArgumentException::class, 'forReadOnlyConstructorState')] #[CoversMethod(InvalidArgumentException::class, 'forSelectWithMergeFlag')] +#[CoversMethod(InvalidArgumentException::class, 'forUnenclosedValuesPlaceholder')] #[CoversMethod(InvalidArgumentException::class, 'forUnknownArgumentType')] #[CoversMethod(InvalidArgumentException::class, 'forUnknownColumnKey')] #[CoversMethod(InvalidArgumentException::class, 'forValuesWithMergeFlag')] @@ -60,122 +61,127 @@ public static function foreignTableProvider(): array public static function namedConstructorProvider(): array { return [ - 'missing decimal digits' => [ + 'missing decimal digits' => [ 'forMissingDecimalDigits', ['price', 'DECIMAL'], InvalidArgumentException::MISSING_DECIMAL_DIGITS, ], - 'missing column length' => [ + 'missing column length' => [ 'forMissingColumnLength', ['name', 'VARCHAR'], InvalidArgumentException::MISSING_COLUMN_LENGTH, ], - 'invalid column length' => [ + 'invalid column length' => [ 'forInvalidColumnLength', ['age'], InvalidArgumentException::INVALID_COLUMN_LENGTH, ], - 'empty check expression' => [ + 'empty check expression' => [ 'forEmptyCheckExpression', [], InvalidArgumentException::EMPTY_CHECK_EXPRESSION, ], - 'already combined' => [ + 'already combined' => [ 'forAlreadyCombined', [], InvalidArgumentException::ALREADY_COMBINED, ], - 'empty expression' => [ + 'empty expression' => [ 'forEmptyExpression', [], InvalidArgumentException::EMPTY_EXPRESSION, ], - 'empty prefix' => [ + 'empty prefix' => [ 'forEmptyPrefix', [], InvalidArgumentException::EMPTY_PREFIX, ], - 'empty schema' => [ + 'empty schema' => [ 'forEmptySchema', [], InvalidArgumentException::EMPTY_SCHEMA, ], - 'empty separator' => [ + 'empty separator' => [ 'forEmptySeparator', [], InvalidArgumentException::EMPTY_SEPARATOR, ], - 'empty table' => [ + 'empty table' => [ 'forEmptyTable', [], InvalidArgumentException::EMPTY_TABLE, ], - 'foreign table' => [ + 'foreign table' => [ 'forForeignTable', ['users'], InvalidArgumentException::FOREIGN_TABLE, ], - 'invalid argument type' => [ + 'invalid argument type' => [ 'forInvalidArgumentType', [], InvalidArgumentException::INVALID_ARGUMENT_TYPE, ], - 'invalid from array' => [ + 'invalid from array' => [ 'forInvalidFromArray', [], InvalidArgumentException::INVALID_FROM_ARRAY, ], - 'invalid join name' => [ + 'invalid join name' => [ 'forInvalidJoinName', ['orders'], InvalidArgumentException::INVALID_JOIN_NAME, ], - 'invalid magic property' => [ + 'invalid magic property' => [ 'forInvalidMagicProperty', [], InvalidArgumentException::INVALID_MAGIC_PROPERTY, ], - 'invalid specification name' => [ + 'invalid specification name' => [ 'forInvalidSpecificationName', [], InvalidArgumentException::INVALID_SPECIFICATION_NAME, ], - 'missing values or select' => [ + 'missing values or select' => [ 'forMissingValuesOrSelect', [], InvalidArgumentException::MISSING_VALUES_OR_SELECT, ], - 'non numeric parameter' => [ + 'non numeric parameter' => [ 'forNonNumericParameter', ['PhpDb\\Sql\\Select::limit', 'string'], InvalidArgumentException::NON_NUMERIC_PARAMETER, ], - 'non string value key' => [ + 'non string value key' => [ 'forNonStringValueKey', [], InvalidArgumentException::NON_STRING_VALUE_KEY, ], - 'read only constructor state' => [ + 'read only constructor state' => [ 'forReadOnlyConstructorState', [], InvalidArgumentException::READ_ONLY_CONSTRUCTOR_STATE, ], - 'select with merge flag' => [ + 'select with merge flag' => [ 'forSelectWithMergeFlag', [], InvalidArgumentException::SELECT_WITH_MERGE_FLAG, ], - 'unknown argument type' => [ + 'unenclosed values placeholder' => [ + 'forUnenclosedValuesPlaceholder', + ['a IN ?'], + InvalidArgumentException::UNENCLOSED_VALUES_PLACEHOLDER, + ], + 'unknown argument type' => [ 'forUnknownArgumentType', [], InvalidArgumentException::UNKNOWN_ARGUMENT_TYPE, ], - 'unknown column key' => [ + 'unknown column key' => [ 'forUnknownColumnKey', ['email'], InvalidArgumentException::UNKNOWN_COLUMN_KEY, ], - 'values with merge flag' => [ + 'values with merge flag' => [ 'forValuesWithMergeFlag', [], InvalidArgumentException::VALUES_WITH_MERGE_FLAG, diff --git a/test/unit/Sql/ExpressionTest.php b/test/unit/Sql/ExpressionTest.php index c88aa0f3..5c019dba 100644 --- a/test/unit/Sql/ExpressionTest.php +++ b/test/unit/Sql/ExpressionTest.php @@ -18,6 +18,8 @@ use PHPUnit\Framework\TestCase; use TypeError; +use function sprintf; + /** * This is a unit testing test case. * A unit here is a method, there will be at least one test per method @@ -45,6 +47,7 @@ public static function arrayParameterSpecificationProvider(): array '%s IN (%s, %s) AND %s = %s', ], 'values argument' => ['a IN (?)', [Argument::values([1, 2])], 'a IN (%s, %s)'], + 'whitespace inside parentheses' => ['a IN ( ? )', [[1, 2]], 'a IN ( %s, %s )'], ]; } @@ -60,6 +63,19 @@ public static function falsyExpressionParametersProvider(): array ]; } + /** @return array}> */ + public static function unenclosedArrayParameterProvider(): array + { + return [ + 'array' => ['a IN ?', [[1, 2, 3]]], + 'empty array' => ['a IN ?', [[]]], + 'values argument' => ['a IN ?', [Argument::values([1, 2])]], + 'opening parenthesis only' => ['a IN (?', [[1, 2]]], + 'closing parenthesis only' => ['a IN ?)', [[1, 2]]], + 'parenthesis around more' => ['a IN (?, ?)', [[1, 2], 3]], + ]; + } + #[Test] #[DataProvider('falsyExpressionParametersProvider')] public function constructorWithFalsyValidParameters(mixed $falsyParameter): void @@ -145,6 +161,22 @@ public function getExpressionDataGivesEachArrayValueItsOwnPlaceholder( static::assertSame($expectedSpecification, $expressionData['spec']); } + /** + * @param list $parameters + */ + #[Test] + #[Group('unit')] + #[DataProvider('unenclosedArrayParameterProvider')] + public function getExpressionDataRejectsArrayPlaceholderOutsideParentheses( + string $expression, + array $parameters, + ): void { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage(sprintf(InvalidArgumentException::UNENCLOSED_VALUES_PLACEHOLDER, $expression)); + + (new Expression($expression, $parameters))->getExpressionData(); + } + #[Test] public function getExpressionDataThrowsExceptionWhenParameterCountMismatch(): void { From 00165cb66e573b54a3eccad477280ff84fba835a Mon Sep 17 00:00:00 2001 From: Simon Mundy Date: Tue, 13 Oct 2026 12:42:25 +1100 Subject: [PATCH 3/3] Keep Expression under the class complexity limit Merging 0.6.x removed Expression from the lint baseline, and the placeholder expansion pushed the class over Mago's cyclomatic complexity limit. The enclosure check moves into placeholderFor(), and the comma-separated list now comes from Values::getSpecification() without its parentheses rather than being rebuilt here. Signed-off-by: Simon Mundy --- src/Sql/Expression.php | 51 +++++++++++++++++------------------------- 1 file changed, 20 insertions(+), 31 deletions(-) diff --git a/src/Sql/Expression.php b/src/Sql/Expression.php index 8fa9bd0e..2386bfb9 100644 --- a/src/Sql/Expression.php +++ b/src/Sql/Expression.php @@ -9,7 +9,6 @@ use PhpDb\Sql\Argument\Value; use PhpDb\Sql\Argument\Values; -use function array_fill; use function array_shift; use function array_slice; use function array_unique; @@ -17,11 +16,11 @@ use function explode; use function func_get_args; use function func_num_args; -use function implode; use function is_array; use function preg_match; use function preg_match_all; use function str_replace; +use function substr; /** * @api @@ -65,22 +64,6 @@ public function __construct( $this->setParameters($parameters); } - /** - * Values are bound one by one, so each needs its own placeholder - */ - private static function placeholderFor(?ArgumentInterface $parameter): string - { - if (! $parameter instanceof Values) { - return '%s'; - } - - $count = count($parameter->getValue()); - - return $count > 0 - ? implode(', ', array_fill(0, count: $count, value: '%s')) - : 'NULL'; - } - public function getExpression(): string { return $this->expression; @@ -109,19 +92,7 @@ public function getExpressionData(): array $specification = array_shift($segments); foreach ($segments as $index => $segment) { - $parameter = $parameters[$index] ?? null; - - if ( - $parameter instanceof Values - && ( - 1 !== preg_match('/\(\s*$/', $specification) - || 1 !== preg_match('/^\s*\)/', $segment) - ) - ) { - throw Exception\InvalidArgumentException::forUnenclosedValuesPlaceholder($this->expression); - } - - $specification .= self::placeholderFor($parameter) . $segment; + $specification .= $this->placeholderFor($parameters[$index] ?? null, $specification, $segment) . $segment; } // test number of replacements without considering same variable begin used many times first, which is @@ -181,4 +152,22 @@ public function setParameters( return $this; } + + /** + * Values are bound one by one, so each needs its own placeholder inside the parentheses around the `?` + * + * @throws Exception\InvalidArgumentException + */ + private function placeholderFor(?ArgumentInterface $parameter, string $before, string $after): string + { + if (! $parameter instanceof Values) { + return '%s'; + } + + if (1 !== preg_match('/\(\s*$/', $before) || 1 !== preg_match('/^\s*\)/', $after)) { + throw Exception\InvalidArgumentException::forUnenclosedValuesPlaceholder($this->expression); + } + + return substr($parameter->getSpecification(), offset: 1, length: -1); + } }