Repository navigation
Bring the Sql DML classes to Mago compliance - #232
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Bugs fixed: - Preparing an expression with a null parameter threw a TypeError. processExpressionParameterName() now accepts null. - A float value in Update::set() or Insert::values() made getSqlString() throw a TypeError. resolveColumnValue() accepts floats. - An unkeyed expression in a join's column list was passed to quoteIdentifier() and threw. It is now aliased ExpressionN, as the main column list does. - Join::current() returned an undefined index past the last join. It returns null. Select, Update and Delete keep their WHERE, HAVING and JOIN clauses lazy, but the backing properties are renamed whereClause, havingClause and joinClause. They shared their names with the @Property tags for __get(), so the analyzer read every access as the non-null magic property. Select's table resolution moves to a private resolveSelectTable(). It overrode resolveTable() with a different return shape, which forced AbstractSql::resolveTable() to declare string|array|null. It now returns ?string, which also clears the three processTable() findings left in Ddl. The specification renderer is split into steps that match a format, render each parameter and format a parameter list, each with exact types. A slot without a parameter specification now rejects a list with the catalogued parameter-count exception instead of printing "Array". Single-string specifications are read through formatSpecification(), which throws RuntimeException::forStringSpecificationRequired() for a specification that is not a string. Parameters are bound only when a driver is present to format their names, falling back to quoting the value. A ColumnValue alias types Update's set values and Insert's column values. The thrown RuntimeException is documented through every method it reaches, the DDL processors among them. processExpressionList() replaces the four loops in AlterTable and CreateTable that rendered expression lists.
Every top-level Sql class, interface and enum is marked @api rather than final, as in Predicate and Ddl. Select, Insert, Update and AbstractSql join the too-many-methods exclusions, and Select the too-many-properties exclusion: the builders carry one method or property per clause, and AbstractSql one hook per clause it renders. ArgumentType gets a scoped analyzer ignore for invalid-enum-case-value, which rejects ::class constants as values of a string-backed enum. Seventeen DML findings stay baselined: - class-level cyclomatic-complexity and kan-defect on AbstractSql, Select, Insert, Update and Combine, which would need the statement classes split up; - halstead on Select::reset(), an eleven-way dispatcher; - excessive-parameter-list on processExpressionOrSelect(), a protected signature; - string-member-selector and mixed-assignment in buildSqlString() and localizeVariables(), which dispatch dynamically by design; - missing-return-type on __clone(), __isset() and __unset(), left untyped so subclass overrides stay compatible. The typed predicate and column signatures move thirteen findings onto the TableGateway and RowGateway callers, for the gateway pass.
simon-mundy
force-pushed
the
qa/mago-sql-dml
branch
from
October 11, 2026 23:43
04d72bb to
69d0c73
Compare
simon-mundy
added
bug
Predicate's ExpressionParameter import named \PhpDb\Sql\Expression in full, because Expression alone resolves to Predicate\Expression in this namespace. It now imports the class under a SqlExpression alias.
The @throws tags added to AlterTable, CreateTable and DropTable named \PhpDb\Sql\Exception\RuntimeException in full. Each class now imports it.
HydratingResultSet documented \Laminas\Hydrator\Exception\RuntimeException in full; it now imports it as HydratorRuntimeException, since the class already imports the ResultSet RuntimeException. TableIdentifierFactory linked \PhpDb\ConfigProvider in full; it now imports ConfigProvider.
…ullable This reverts the whereClause, havingClause and joinClause renaming in Select, Update and Delete. The lazy clauses are stored in $where, $having and $joins again, so subclasses that use those protected properties keep working. The analyzer read every $this->where inside these classes as the @Property tag for __get(), which declared a non-null Where, and reported the null checks and null assignments that make the clauses lazy. The tags now declare ?Where, ?Having and ?Join, matching the properties. __get() still always returns a clause at runtime. Also prunes three baseline entries for AbstractPdo that the createStatement() docblock fix resolved.
This reverts the $parent renaming, to match the clause properties on Select, Update and Delete. The private property is $unnest again, and the @Property tags for unnest and UNNEST declare ?Predicate so the analyzer accepts the null checks and null assignments inside the class. __get('unnest') still returns the parent or throws when the predicate is not nested.
tyrsson
approved these changes
Oct 12, 2026
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
2 participants
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.
Takes the DML classes (
AbstractSql,Select,Insert,Update,Delete,Combine,Join,Sql,Expression,ArgumentType) from 194 baselined findings to 17, and fixes four bugs along the way. It also clears the threeprocessTable()findings left in Ddl.Bugs fixed
new Expression('COALESCE(?, 1)', [null])threw aTypeErrornullis bound as a parameter(new Update('t'))->set(['a' => 1.5])->getSqlString()threw aTypeError, as did the same value inInsert::values()'1.5'is quoted$select->join('b', 'a.id = b.id', [new Expression('COUNT(*)')])threw aTypeErrorCOUNT(*) AS Expression1, as in the main column listJoin::current()past the last join read an undefined indexnullEach has a regression test that fails on #231.
Design changes
Select,UpdateandDeletestore their lazy clauses in?Where $where,?Having $havingand?Join $joins. Their@propertytags for__get()declared non-null types, and Mago applies those tags to$this->whereinside the class too, so it reported every null check and null assignment that makes the clauses lazy (28 findings). The tags now declare?Where,?Havingand?Join.__get()still always returns a clause at runtime. The trade-off is that analysers will see$select->whereas nullable in calling code.Select's table resolution is its own method.Select::resolveTable()overrode the parent with a different return shape ([table, fromTable]), which forcedAbstractSql::resolveTable()to declarestring|array|null. TheSelectlogic is now a privateresolveSelectTable(), andresolveTable()returns?string. No platform package calls or overrides it.createSqlFromSpecificationAndParameters()now delegates to three private steps: match a format, render each parameter, and format a parameter list. A slot that has no parameter specification now rejects a list with the existingforUnsupportedParameterCountOf()exception. It used to printArray.formatSpecification()replaces thestr_replace()calls on$specifications. It substitutes values exactly as before, and throws the newRuntimeException::forStringSpecificationRequired()if a specification isn't a string. That case used to end in aTypeError.null->formatParameterName().ColumnValuealias typesUpdate::set()andInsert::values().Sql::getAdapter()andgetSqlPlatform()no longer declare anullthey never return.@throwstags: the thrownRuntimeExceptionis documented through every method it reaches, including the DDL processors.processExpressionList()replaces four identical loops inAlterTableandCreateTable.@apiandmago.toml:@api, as in Bring Sql\Predicate to full Mago compliance #230 and Bring Sql\Ddl to Mago compliance #231.Select,Insert,UpdateandAbstractSqljoin thetoo-many-methodsexclusions, andSelectthetoo-many-propertiesexclusion.ArgumentTypegets a scoped analyzer ignore. Mago rejects::classconstants as values of a string-backed enum.Still baselined (17)
cyclomatic-complexityandkan-defectonAbstractSql,Select,Insert,UpdateandCombine. Clearing these needs the statement classes split up, for example by extracting the renderer.halsteadonSelect::reset(), an eleven-way dispatcher.excessive-parameter-liston the protectedprocessExpressionOrSelect().string-member-selectorandmixed-assignmentinbuildSqlString()andlocalizeVariables(), which dispatch dynamically by design.missing-return-typeon__clone(),__isset()and__unset(). These are left untyped so subclass overrides stay compatible.The typed signatures move 13 findings onto the
TableGatewayandRowGatewaycallers, for the gateway pass.Result
src/SqloutsideSql\Platform, both baselinesLine and method coverage of every changed
src/Sqlclass is 100%. Infection on the changed files reports 92% covered-code MSI. Most of the remaining survivors are protected-to-private visibility mutants and lines this PR does not change.