Fix xPDOQuery::set() SQL keyword handling; introduce xPDOExpression

[opengeek]

Summary

Fixes #54. Supersedes #271.

Commit 1 — Bug fix: xPDOQuery::set() misinterprets strings containing SQL keywords

Bug: updateCollection() (and any xPDOQuery::set() call) silently produced invalid SQL when a string value contained SQL operator keywords such as IN, LIKE, or NOT. The root cause was set() calling isConditionalClause() on values — any matching string was treated as a raw SQL expression and left unquoted.

Fix: Plain PHP strings in xPDOQuery::set() are now always assigned PDO::PARAM_STR and quoted, regardless of content. isConditionalClause() is removed from set() but preserved in parseConditions() where it belongs.

Escape hatch — xPDOExpression: A new final value object (xPDO\Om\xPDOExpression) explicitly marks a value as a raw SQL fragment, bypassing automatic quoting. Use xPDO::expression() as a factory. All four driver construct() methods, xPDOQuery::set(), xPDOObject::set(), and xPDOObject::save() recognise and inline xPDOExpression values verbatim.

// Plain strings now always work correctly — never misinterpreted as SQL
$xpdo->updateCollection('MyClass', ['label' => 'The word IN is IN this strINg']);

// Use xPDOExpression for intentional raw SQL in SET clauses
$xpdo->updateCollection('MyClass', ['counter' => $xpdo->expression('counter + 1')]);
$object->set('updated_at', $xpdo->expression('NOW()'));
$object->save();

Commit 2 — Bump minimum PHP version to 7.4; remove PHP 7.2/7.3 from CI

xPDOExpression uses typed class properties (private string $expression) which require PHP 7.4. Updates composer.json and the CI workflow matrix accordingly.

Files changed

File	Change
src/xPDO/Om/xPDOExpression.php	New value object for raw SQL expressions
src/xPDO/Om/xPDOQuery.php	Fix set() to always quote strings; inline xPDOExpression verbatim
src/xPDO/xPDO.php	Add expression() factory method
src/xPDO/Om/mysql/xPDOQuery.php	Inline xPDOExpression in SET clause
src/xPDO/Om/pgsql/xPDOQuery.php	Inline xPDOExpression in SET clause
src/xPDO/Om/sqlite/xPDOQuery.php	Inline xPDOExpression in SET clause
src/xPDO/Om/sqlsrv/xPDOQuery.php	Inline xPDOExpression in SET clause
src/xPDO/Om/xPDOObject.php	Recognise xPDOExpression in set() and save()
composer.json	Bump minimum PHP to 7.4
.github/workflows/ci.yml	Remove PHP 7.2/7.3 from test matrix
test/xPDO/Test/Om/xPDOExpressionTest.php	Unit tests for xPDOExpression
test/xPDO/Test/Om/xPDOQueryTest.php	Integration tests for bug fix and xPDOExpression in SET
test/xPDO/Test/Om/xPDOObjectTest.php	Tests for xPDOExpression in xPDOObject::set() and save()

Test plan

• xPDOExpressionTest — 4 tests passing
• xPDOQueryTest — includes string-value bug fix tests and xPDOExpression in SET
• xPDOObjectTest — includes xPDOExpression in set() and save()
• xPDOQueryHavingTest, xPDOQueryLimitTest, xPDOQuerySortByTest — all passing
• xPDOQueryConditionsTest — pre-existing failures unrelated to this PR

🤖 Generated with Claude Code

[Mark-H]

The sql<<...>> syntax has me very worried about introducing SQL "injections" (not sure if that's the right word here, but, basically). It is incredibly common to either filter queries with some kind of untrusted input, relying on xPDO treating it as just a string:

$c = $modx->newQuery(Whatever::class);
$c->where([
   'some_condition' => (string)$_GET['some_filter'],
]);

There is additional data filtering that would be worthwhile for sure, but this is how it's often done.

With this PR applied, an attacker would very easily be able to submit a sql<<...>> string, which would get coerced into an xPDOExpression, and basically let them run any valid SQL in there they want - perhaps even a full subquery so long as it returns a value for the condition.

And if I understand correctly the same would be true for any sort of create/update logic that takes in some user input.

I am fairly tired at the moment so maybe I'm misreading things, but those last few tests basically read as a vulnerability instead of a feature.

I do see value in having this available and fixing the queries breaking, but instead of the magic coercion, let the implementation determine something may accept direct SQL with the $xpdo->expression($value) addition.

[opengeek]

The sql<<...>> syntax has me very worried about introducing SQL "injections" (not sure if that's the right word here, but, basically). It is incredibly common to either filter queries with some kind of untrusted input, relying on xPDO treating it as just a string:

$c = $modx->newQuery(Whatever::class);
$c->where([
   'some_condition' => (string)$_GET['some_filter'],
]);

There is additional data filtering that would be worthwhile for sure, but this is how it's often done.

With this PR applied, an attacker would very easily be able to submit a sql<<...>> string, which would get coerced into an xPDOExpression, and basically let them run any valid SQL in there they want - perhaps even a full subquery so long as it returns a value for the condition.

And if I understand correctly the same would be true for any sort of create/update logic that takes in some user input.

I am fairly tired at the moment so maybe I'm misreading things, but those last few tests basically read as a vulnerability instead of a feature.

I do see value in having this available and fixing the queries breaking, but instead of the magic coercion, let the implementation determine something may accept direct SQL with the $xpdo->expression($value) addition.

These are solid points. I think I'll remove the string syntax. It seemed like a positive feature when I was experimenting with it, but I think you are correct in thinking it would create more problems than it solves. Thanks for looking!