Skip to content

Render every value of an In list through a custom specification - #240

Open
simon-mundy wants to merge 1 commit into
php-db:0.6.xfrom
simon-mundy:fix/in-custom-specification-values
Open

simon-mundy wants to merge 1 commit into
php-db:0.6.xfrom
simon-mundy:fix/in-custom-specification-values

Conversation

@simon-mundy

Copy link
Copy Markdown
Member

Problem

A custom specification on In/NotIn loses list values and leaves orphan parameters:

$in = (new Predicate\In('id', [1, 2]))->setSpecification('%1$s = ANY (ARRAY[%2$s])');
(new Select('t'))->where([$in]);
  • Unprepared: SELECT "t".* FROM "t" WHERE "id" = ANY (ARRAY['1']) (the 2 is gone)
  • Prepared: ... ARRAY[:where1], but the container holds where1 and where2, so PDO rejects the parameter count

Cause

In::getExpressionData() returned the custom specification unchanged with values [identifier, valueSet]. The default specification embeds the value set's own (%s, %s), which lines up with AbstractSql::flattenExpressionValues() expanding the Values argument into one value per item. A custom specification refers to the whole list with a single %2$s, so the expanded values shift: later ones are dropped from the SQL but still bound.

Fix

  • New internal Predicate\CustomSpecification::expand() rewrites a custom specification so each placeholder covers every value its argument renders. Placeholders can be plain (%s) or positional (%2$s). Output is all positional, so %2$s becomes %2$s, %3$s for a two-value list. An empty list becomes NULL. A sub-select stays a single placeholder. %% is left alone.
  • An argument that binds values (Values, Value, Select) must be referenced exactly once. If a specification repeats or omits one, or names a missing argument, it throws a Predicate\Exception\InvalidArgumentException (SPECIFICATION_ARGUMENT_NOT_REFERENCED_ONCE, SPECIFICATION_ARGUMENT_OUT_OF_RANGE) instead of producing mismatched parameters. Identifiers and literals can be repeated or omitted.
  • In::getExpressionData() applies this only when a custom specification is set. Default output is unchanged. NotIn inherits the fix.
  • docs/book/sql/where-having.md documents custom In specifications.

Other multi-value predicates were checked. Between/NotBetween take scalar Value arguments, which are never expanded, so they are not affected.

Tests

All new tests are #[Group('unit')] and use data providers.

  • CustomSpecificationTest: rewriting for 1, 2 and 3 values; plain and positional placeholders; list before the identifier; identifier repeated; escaped %%; empty list; sub-select; a Values identifier shifting the list. Also the rejection cases: list repeated, list omitted, bound identifier repeated, out-of-range positional argument, too many plain placeholders.
  • InTest: unprepared SQL and prepared SQL plus getNamedArray() for custom specifications, including a sub-select whose own parameter binds once.
  • NotInTest: unprepared SQL for 1, 2 and 3 values.
  • InvalidArgumentExceptionTest: the two new named constructors.

phpunit --testsuite "unit test", mago format --check, mago lint and mago analyze all pass. In, CustomSpecification and the predicate InvalidArgumentException have 100% line coverage.

A custom In/NotIn specification refers to the value set with one placeholder,
but the value set renders one value per list item. With more than one value
the extra values were dropped from the SQL while still being bound, leaving
orphan parameters in prepared statements.

Custom specifications are now rewritten so each placeholder covers every value
its argument renders (positional placeholders, an empty list as NULL), and a
specification that repeats or omits an argument that binds values, or names a
missing argument, throws a Predicate InvalidArgumentException.
@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