Fix xPDOQuery::set() string quoting and introduce xPDOExpression for query layer#280
Merged
opengeek merged 1 commit intomodxcms:3.xfrom Apr 7, 2026
Merged
Conversation
…ry layer Fixes modxcms#54: xPDOQuery::set() was calling isConditionalClause() on plain string values, which left any string containing SQL keywords (IN, LIKE, NOT, etc.) with $type = null, causing them to be inlined unquoted in the SET clause — a SQL injection surface and a data correctness bug. - Plain string values in set() now always receive PDO::PARAM_STR and are quoted; isConditionalClause() is removed from the set() value-type path. - New final class xPDO\Om\xPDOExpression wraps a raw SQL fragment and is recognised by set() and select() in all four drivers (mysql, pgsql, sqlite, sqlsrv), which inline it verbatim without quoting. - New xPDO::expression(string $expr): xPDOExpression factory method. - Ten new PHPUnit tests cover the factory, the value object, the quoting fix, and xPDOExpression passthrough in both SET and SELECT contexts. Supersedes PR modxcms#273 (closed); restricted to query layer only per Rollins architectural review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed and why
Fixes #54:
xPDOQuery::set()was callingisConditionalClause()on plain stringvalues in the SET clause. Any string containing a SQL keyword (
IN,LIKE,NOT,etc.) caused
$typeto remainnull, leaving the value inlined unquoted in thegenerated SQL. This is both a data correctness bug and a SQL injection surface for
any application that passes user-influenced data through
updateCollection().The fix removes
isConditionalClause()from theset()value-type detection path.Plain strings now always receive
PDO::PARAM_STRand are quoted. The only way toinline a verbatim SQL fragment in a SET value is to wrap it in the new
xPDOExpressionvalue object.This also introduces
xPDO\Om\xPDOExpression— afinalvalue object for passingraw SQL fragments through
xPDOQuery::set()andxPDOQuery::select()withoutquoting or identifier escaping. Supersedes PR #273 (closed); scope is restricted to
the query layer only, per architectural review.
Files and methods changed
src/xPDO/Om/xPDOExpression.php— newfinal class xPDOExpression: wraps a raw SQL string; exposesgetExpression(): stringsrc/xPDO/xPDO.php—expression(string $expr): factory method delegating toxPDOExpressionconstructorsrc/xPDO/Om/xPDOQuery.php—set(): removedisConditionalClause()from value-type path;sortby(): addedinstanceof xPDOExpressionbranch;groupby(): addedinstanceof xPDOExpressionbranch;parseConditions(): unifiedelseifgate with inner expression check; barexPDOExpressiontowhere()handled beforeisConditionalClause()src/xPDO/Om/mysql/xPDOQuery.php—construct(): xPDOExpression inline in SET and SELECTsrc/xPDO/Om/pgsql/xPDOQuery.php—construct(): xPDOExpression inline in SET and SELECTsrc/xPDO/Om/sqlite/xPDOQuery.php—construct(): xPDOExpression inline in SET and SELECTsrc/xPDO/Om/sqlsrv/xPDOQuery.php—construct(): xPDOExpression inline in SET and SELECT;parseConditions()override unifiedCompatibility impact
All four supported drivers updated: mysql, pgsql, sqlite, sqlsrv. Changes are applied identically in each driver's
construct()method.Test coverage
Six test files added/extended — 458 tests total, 647 assertions, 0 failures. New coverage:
xPDOExpressionTest,xPDOQuerySetTest,xPDOQuerySelectTest,xPDOQueryJoinTest,xPDOQueryWhereTest(10 tests),xPDOQuerySortByTest(2 new methods including groupby fix).Breaking change assessment
The string-quoting fix is a behavior change for any caller passing raw SQL fragments as plain strings to
xPDOQuery::set(). That usage was undocumented and exploitable — not a supported API. Callers needing raw SQL in a SET value must now use$xpdo->expression('...'). No public API signature changes. Safe for patch-level consumers using the documented API.AI Disclosure
This fix was developed using an AI-assisted workflow. AI agents (Claude) wrote tests, code, and performed initial code review under the direction of the project maintainer (@opengeek), who reviewed the final diff, validated the approach, and takes responsibility for the submission.
Contributors
Issue #54 originally reported the xPDOQuery::set() string quoting problem. PR #273 (@opengeek) established the xPDOExpression concept; this PR corrects the scope to the query layer only.