Repository navigation
Check QueryBuilder orderBy() clauses that use the SortDirection enum - #796
Open
whataboutpereira wants to merge 1 commit into
Open
whataboutpereira wants to merge 1 commit into
whataboutpereira wants to merge 1 commit into
Conversation
ORM 3.7 accepts the \SortDirection enum as the direction in QueryBuilder::orderBy() / addOrderBy() and Expr\OrderBy, and deprecates the string form. ArgumentsProcessor only understood constant scalars, constant arrays, class-strings and ExprType, so an enum case was treated as a dynamic argument. Because orderBy and addOrderBy are in METHODS_NOT_AFFECTING_RESULT_TYPE, the whole call was then silently dropped from the replayed QueryBuilder, the resulting DQL had no ORDER BY clause, and unknown fields in it were never reported. ArgumentsProcessor now resolves an argument that is a \SortDirection case to the real enum instance when the installed ORM is 3.7 or newer. The instance is passed instead of 'ASC'/'DESC' because ORM 3.7 matches on the enum and triggers a deprecation for strings. On older ORM, and for any other enum, nothing changes. A SortDirection instance can now reach Doctrine code that does not accept it, so two replay steps are guarded: - NewExprDynamicReturnTypeExtension catches Throwable around the constructor call and falls back to ObjectType, e.g. for new Expr\From(SortDirection::Ascending, 'e'). - QueryBuilderGetQueryDynamicReturnTypeExtension catches Throwable around getDQL(), e.g. for an Expr\Comparison holding the enum, which cannot be converted to a string. Such code already fails at runtime in Doctrine itself, so this only changes how PHPStan degrades on it: the QueryBuilder is treated as not analysable instead of having that one call skipped. Closes phpstan#792 Co-Authored-By: Claude Code
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.
Closes #792
Problem
ORM 3.7 accepts
\SortDirectionas the direction inQueryBuilder::orderBy()/addOrderBy()andExpr\OrderBy, and deprecates the string form. With the enum,QueryBuilderDqlRulereports nothing for an unknown field:ArgumentsProcessor::processArgs()does not understand enum cases, so it throwsDynamicQueryBuilderArgumentException.orderByandaddOrderByare inMETHODS_NOT_AFFECTING_RESULT_TYPE, so the call is skipped when the QueryBuilder is replayed. The rebuilt DQL has noORDER BYand the field is never validated.Changes
ArgumentsProcessor: an argument that is a\SortDirectioncase is resolved to the real enum instance when the installed ORM is 3.7 or newer. The instance is passed instead of'ASC'/'DESC'because ORM 3.7 matches on the enum and triggers a deprecation for strings. This also coversnew Expr\OrderBy('e.x', \SortDirection::Descending).NewExprDynamicReturnTypeExtension: catchesThrowablearound the constructor call and falls back toObjectType.QueryBuilderGetQueryDynamicReturnTypeExtension: catchesThrowablearoundgetDQL().The two catches are needed because a real
SortDirectioninstance can now reach Doctrine code that does not accept it, e.g.new Expr\From(SortDirection::Ascending, 'e')(TypeError) or anExpr\Comparisonholding the enum (cannot be converted to a string ingetDQL()). Without them PHPStan crashes on such code.Behaviour
SortDirection: unchanged.SortDirectionpassed somewhere that does not accept it makes the QueryBuilder not analysable, instead of having that call skipped. Such code already fails at runtime in Doctrine itself (TypeError, or "Object of class SortDirection could not be converted to string" when the DQL is built), so this only changes how PHPStan degrades on code that is already broken.Tests
QueryBuilderDqlRuleTest::testSortDirection, skipped when\SortDirectiondoes not exist:orderBy(),addOrderBy()andnew Expr\OrderBy()are reported, valid ones are not, and the two misuse cases are reported as not analysable.Verified locally on PHP 8.5:
Infection was not run locally. The new branch and both catches only execute on ORM 3.7+, so they are not covered by a default (ORM 2) install.
Not included
SortDirection $dir,string $dir) still causes theorderBy()call to be skipped, as before.Co-Authored-By: Claude Code