Skip to content

Reject setSpecification() on Sql\Expression instead of silently ignoring it - #239

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

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

Conversation

@simon-mundy

Copy link
Copy Markdown
Member

Problem

Sql\Expression extends AbstractExpression, so it inherits setSpecification(). But Expression::getExpressionData() builds its spec only from $this->expression and never reads $this->specification. The call is accepted and has no effect:

(new Expression('LOWER(?) = ?', ['x', 'y']))->setSpecification('IGNORED %s %s');
// renders LOWER('x') = 'y'

Predicate\Expression extends Sql\Expression, so it behaves the same way.

Fix

Expression::setSpecification() is now overridden and throws Sql\Exception\RuntimeException::forUnsupportedSpecificationOverride(). The message names the class and points to setExpression(). The signature (string $specification): static) is unchanged, so the class still satisfies AbstractExpression, and subclasses that override it keep working.

Why throw rather than alias to setExpression()

  • Nothing calls setSpecification() on an Expression. That covers docs/book, the test suite, Literal/Predicate\Literal (which don't extend AbstractExpression), TableGateway/RowGateway, and phpdb-mysql, phpdb-sqlite and phpdb-pgsql. The only platform call is Select::setSpecification() in phpdb-sqlite, which is a different method.
  • A specification and an expression are different formats. A specification is a sprintf string whose %s are already resolved. An expression uses ? placeholders and has its % escaped. Aliasing one to the other would quietly reinterpret the caller's string, and SQL could still differ from what they wrote. Throwing means the caller can never get SQL other than what they asked for.

getSpecification() is unchanged and still returns null for an Expression.

BC

Code that called setSpecification() on an Expression or Predicate\Expression now gets a RuntimeException. That call never changed the rendered SQL, so this only exposes a no-op that was already there. Operator, Like, Between, In and IsNull still honour a custom specification.

Tests

  • ExpressionTest and Predicate\ExpressionTest check that setSpecification() throws the new exception and message.
  • New Predicate\CustomSpecificationTest uses a data provider to check that Operator, Like, Between, IsNull and In still render a custom specification.
  • RuntimeExceptionTest covers the new named constructor.
  • The two ExpressionTest classes now carry #[Group('unit')].

docs/book/sql/where-having.md gains a note under the Expression API.

phpunit --testsuite "unit test" passes (2192 tests). mago format --check, mago lint and mago analyze are clean. Sql\Expression and Sql\Exception\RuntimeException have 100% line coverage.

Expression inherits setSpecification() from AbstractExpression, but
getExpressionData() only renders $this->expression, so the override was
accepted and silently dropped. Throw a RuntimeException pointing to
setExpression() so the caller cannot get different SQL than they asked for.
Predicates that honour a custom specification are unchanged.
@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