Repository navigation
Quote predicate names as identifiers where fragment quoting renders broken SQL - #244
Open
simon-mundy wants to merge 1 commit into
Open
simon-mundy wants to merge 1 commit into
simon-mundy wants to merge 1 commit into
Conversation
…roken SQL Predicate identifiers render through quoteIdentifierInFragment(), which quotes every piece split out on a non-word character other than a space, dot, * or AS. A name such as x-y, a"b, nâme or my col therefore renders as adjacent quoted pieces or a bare sequence of identifiers. Such names are now quoted whole as an identifier chain, split only on the qualifier dot. Every name whose fragment quoting renders valid SQL keeps its current rendering, judged against the platform's own fragment pattern.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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
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.
Problem
A predicate's left-hand name is rendered through
AbstractPlatform::quoteIdentifierInFragment(). It splits the name on every character outside the platform's word class and quotes each piece, except spaces, dots,*andAS. A name that holds any other delimiter therefore renders as a different, invalid identifier:where->equalTo('x-y', 1)"x""-""y" = '1'"x-y" = '1'where(['t-1.x-y' => 1])"t""-""1"."x""-""y" = '1'"t-1"."x-y" = '1'where->like('a"b', …)"a""'""b" LIKE …"a'b" LIKE …where->isNull('nâme')"n""\xC3""\xA2""me" IS NULL(split bytes)"nâme" IS NULLhaving->greaterThan('my col', 1)"my" "col" > '1'"my col" > '1'This affects
equalTo,notEqualTo,lessThan(OrEqualTo),greaterThan(OrEqualTo),like/notLike,in/notIn,between/notBetween,isNull/isNotNulland where-array keys, in WHERE and HAVING on Select, and in WHERE on Update and Delete. It has the same root cause as #238 (Update SET columns) and #241 (Select, join and GROUP BY columns).Cause
AbstractSql::processExpression()renders everyArgument\IdentifierthroughquoteIdentifierInFragment(), and so doesprocessIdentifiersArgument()forArgument\Identifiers. Fragment quoting is meant for loose SQL, not for a single name.The non-breaking rule
On 0.6.x only output that is broken today may change. A new private
AbstractSql::quoteIdentifierArgument()decides per name:.and*. Treat.,*andASas operators.quoteIdentifierInFragment()as one piece (it equalsquoteIdentifier()of that part), and no two name parts are separated by spaces alone.., with each part throughquoteIdentifier()and a*part left bare. This matches the helpers in Quote Update SET columns as identifiers and bind them under valid placeholder names #238 and Quote Select, join and GROUP BY column names as identifiers #241.The check asks the platform itself, so a platform whose fragment pattern treats
-as a word character, as phpdb-mysql's does, keeps`a-b` * `c`unchanged.What stays the same
t.*,price*qty,a * b,t.a * t.b,x as y/x AS y,a,band the empty string render exactly as before.Expression,Literaland string predicates are untouched, and so are values and string joinONclauses (they callquoteIdentifierInFragment()directly with extra safe words).The same fix also reaches the other
Identifierarguments rendered throughprocessExpression(): identifier-typedExpressionparameters and DDL names (column, index and constraint names). For those too, only names that render broken today change.Context
The maintainer's long-term direction (D18) is that identifier slots take identifiers only, with SQL passed as an
Expression. That change breaks forms likeprice*qty, so it belongs in a later major version. This PR fixes only what is broken today.Tests
test/unit/Sql/PredicateIdentifierNameTest.php(#[Group('unit')]) uses data providers to cover:equalTo,notEqualTo,lessThan,greaterThan,like,in,betweenandisNull, where-array keys with a value and withnull, HAVING, Update WHERE and Delete WHERE.t.*,price*qty,a * b,t.a * t.b,x as y,x AS y,a,band empty.Argument::identifiers()list, with each name quoted by the same rule.TrustingHyphenWordPlatformtest asset), which keeps hyphenated names and arithmetic as before.Expression,Literaland string predicates left unchanged.The changed lines have 100% line coverage. The unit suite passes, and
mago format --check,mago lintandmago analyzeare clean.