Repository navigation
Fix unresolvable docblock references and prune the dead baseline entries - #86
Merged
Merged
Conversation
The phpdb $resource contract change (php-db/phpdb#227, 0c4ed091) made the committed baseline stale and surfaced 21 pre-existing findings. All 21 are fixed at source: no new suppression was added, and mago pruned the 9 now-dead baseline entries it left behind (87 -> 78 blocks). Two root causes: * `@throws Exception\ExceptionInterface` resolved to PhpDb\Adapter\Exception\ExceptionInterface, which phpdb does not ship. Every affected file imports PhpDb\Adapter\Exception, so the short form named a non-existent class and the annotations were silently inert, reporting 14 throws as unhandled. Repointed to the real \PhpDb\Exception\ExceptionInterface in 16 places. * SelectDecorator::processLimit()/processOffset() tested only $parameterContainer before dereferencing the nullable ?DriverInterface $driver parameter. Adding the null check also let mago resolve the call result and removed a less-specific-nested-return-statement cascade. Runtime no-op: AbstractSql::buildSqlString() always supplies a driver, and a null driver now correctly falls through to the literal branch. Also tightens the LIMIT/OFFSET and ALTER TABLE ADD COLUMN return shapes, casts the LIMIT/OFFSET literals to string (vsprintf renders 10 and '10' identically), reindexes the ADD COLUMN sql list (the parent never reads the inner keys), and drops 4 redundant @var annotations. The lock is bumped to phpdb 0c4ed091 because CI's qa job installs from the lock; the previous reference still carried @return resource and would leave the pruned findings un-suppressed. Verified: mago format --check, lint, analyze and guard all clean; phpunit unit suite 193 tests / 385 assertions; integration suite 327 tests / 508 assertions against MySQL 8.0.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Import PhpDb\Exception\ExceptionInterface as PhpDbExceptionInterface in the eight files that reference it, and use the short name in @throws rather than the fully-qualified form. Replace the two em dashes with semicolons: the @todo in src/Pdo/Connection.php and the trigger comment in the CI workflow. The added import shifts the closure in src/Connection.php down one line, so mago's baselined mixed-return-statement entry is regenerated at its new position (118:31 -> 119:31). Same finding, same count, no new suppression.
Restore .github/workflows/continuous-integration.yml to its previous state so this PR only touches source files and the baseline.
The setSql expectation was with($this->isString()), which any string satisfies, so the container-and-driver branch of processLimit/processOffset was never actually pinned. Capture the SQL and assert the LIMIT/OFFSET placeholders. This restores the coverage that the (string) casts removed, killing the two ReturnRemoval mutants at lines 53 and 72. Refs #86.
simon-mundy
approved these changes
Oct 10, 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
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 this does
Follows up on #85, which fixed the missing
use mysqli_result;insrc/Driver.php. The unqualifiedmysqli_resultin that file's@paramdocblock was resolving to the non-existentPhpDb\Mysql\mysqli_result, producing anon-existent-class-likeplus a cascadingpossibly-invalid-argument, and #85 pruned the two baseline entries that fix made dead. That PR is merged as7685a95, and this branch is based directly on it.The phpdb
$resourcecontract merge (php-db/phpdb#227,0c4ed091) then made the committed baseline stale and surfaced 21 pre-existing findings in this repo that had been hidden behind it. All 21 are fixed at source here. No suppression was added, and none was widened. mago pruned the 9 now-dead baseline entries itself via--remove-outdated-baseline-entries, takinganalysis-baseline.tomlfrom 87 to 78 blocks. Diffed before/after: exactly 9 removed, 0 added.The two root causes
1.
@throws Exception\ExceptionInterfacenamed a class that does not exist.Every affected file does
use PhpDb\Adapter\Exception;, so the short form resolved toPhpDb\Adapter\Exception\ExceptionInterface, an interface phpdb has never shipped. The only real one isPhpDb\Exception\ExceptionInterface. The annotations were therefore silently inert, and mago correctly reported all those throws as unhandled. All 16 places, across 8 files, now import it as:and reference that short alias in
@throws, so no docblock carries a fully-qualified class name.Concrete exception classes (
Exception\RuntimeException,Exception\InvalidConnectionParametersException,Exception\InvalidArgumentException,PDOException, ...) were deliberately left as short names; those resolve fine, and were not touched.2.
SelectDecorator::processLimit()/processOffset()dereferenced a nullable parameter.The signature takes
?DriverInterface $driver = null, but the guard tested only$parameterContainerbefore calling$driver->formatParameterName(...). Added the null check. This is a runtime no-op:AbstractSql::buildSqlString()always supplies a driver, and when one is genuinely absent the code now correctly falls through to the literal branch instead of erroring. Fixing it also let mago resolve the call result, which removed aless-specific-nested-return-statementcascade.Also
processLimit()/processOffset()docblocks tightened fromstring[]|nulltoarray{0: string}|null, and the LIMIT/OFFSET literals cast to(string).vsprintfrenders10and'10'identically, so this is behaviour-preserving.(string)casts removed an accidental safety net, and this PR pays it back with real coverage. Previously the twoReturnRemovalmutants inprocessLimit()/processOffset()were killed by static analysis: removing thereturnmade execution fall through to a type-invalid raw-intreturn, which mago flagged. Once the literal is cast, that fall-through is type-valid, so mago stops killing the mutant and only a test can.SelectDecoratorTest::prepareStatementBindsLimitAndOffsetParametersalready drove that branch, but itssetSqlexpectation waswith($this->isString())-- a tautology that any string satisfies. It now captures the SQL and asserts the boundLIMIT :limit/OFFSET :offsetplaceholders. Both mutants are killed by the tests instead of by the analyzer.AlterTableDecorator::processAddColumns()docblock corrected toarray{0: list<string>}, matching the upstreamAlterTablecontract, with$sqls[]instead of$sqls[$i]. The parent'screateSqlFromSpecificationAndParameters()only iterates by value and callscount(), so the inner keys were never read.@varannotations dropped (mago had no auto-fix for these;--fix --retain-code redundant-docblock-typeproduced an empty diff).@todoatsrc/Pdo/Connection.php:191, and the trigger comment at the top of.github/workflows/continuous-integration.yml.PhpDbExceptionInterfaceintosrc/Connection.phpshifts the closure on line 118 down one line, and mago's baselinedmixed-return-statementmessage embeds that position, so the entry is regenerated as{closure:src/Connection.php:119:31}. Same code, samecount = 1, 78 blocks before and after. Regenerated by mago and diffed: exactly 1 entry replaced, 0 added.The lock bump is required, not cosmetic
composer.lockmoves phpdbb57f549b→0c4ed091. CI'sqajob runscomposer install(locked), so without this the pruned baseline would leave the findings un-suppressed and the job would go red. Measured against the old reference: the pruned baseline leaves 12 findings un-suppressed (9 errors, 3 warnings), including all three$resource-killed entries and seven from the removed@vardocblocks.No
repositoriesalias or other local scaffolding is included.Deliberately out of scope
The two remaining
src/Driver.phpbaseline entries are bothincompatible-parameter-type:That is a parameter-contravariance issue owned by phpdb core's
DriverInterface(resourceis a PHP-7-era type), not by this repo. Left alone pending a separate decision upstream. This PR does not reopen #71.Verification
mago format --check→ all files already formattedmago lint→ no issuesmago analyze→ no issues, 108 filtered by baseline (the "outdated entries" warning is now gone)mago guard→ no issuesvendor/bin/phpunit --testsuite "unit test"→ OK (193 tests, 386 assertions)vendor/bin/phpunit --testsuite "integration test"→ OK (327 tests, 508 assertions) against MySQL 8.00.5.x. This branch previously reported 30 escaped; the twoSelectDecoratormutants are the ones now killed by the test above, and no mutant escaped because of this PR.The integration suite matters here because the change touches generated SQL.