Bring src/Adapter to Mago compliance by simon-mundy · Pull Request #248 · php-db/phpdb

simon-mundy · 2026-11-04T06:37:16Z

Takes src/Adapter from 265 baselined Mago findings to 22 (212 → 21 analyzer, 53 → 1 lint). Every finding left needs a public signature change or a class split, listed under Still baselined for a decision.

Cut from 0.6.x. #233 also adds @extends Iterator<array-key, mixed> to ResultInterface and edits both baselines, so whichever lands second needs a small rebase.

Problem

After #233, src/Adapter carries most of the remaining Mago debt: about 72% of the analyzer baseline, plus 53 lint entries. Several of those findings were real failure paths, not just missing types:

calls on null when a connection has no driver, or when connect() leaves it without a PDO;
calls on a bare PDO that it doesn't have (isConnected(), getResource(), getLastGeneratedValue());
Profiler::getLastProfile() returning false through a ?array return type;
ParameterContainer reading undefined positions.

Cause

Interfaces, traits and abstract classes were not tagged @api or @internal.
Constants were untyped.
Arrays had no element types.
Nullable collaborators ($driver, $resource, the PDO-or-wrapper $connection) were used without a guard.
ParameterContainer resolved integer names with an unchecked $this->positions[$name].

Fix

Types and tags

Every Adapter interface, trait, abstract class and leaf class is tagged @api. That also clears class-must-be-final without making any class final.
Interface and class constants are typed: public const string, and public const array for VALID_FETCH_MODES.
Array parameters and returns are typed:
- ParameterContainer|array<array-key, mixed> for statement parameters;
- a ConnectionParameters alias (array<string, mixed>) on ConnectionInterface;
- array<array-key, string> for safe words;
- array{string, string} for the identifier quote pair.
ResultInterface, Pdo\Result and ParameterContainer declare their Iterator and ArrayAccess templates.
DriverInterface::createResult() and createStatement() take an explicit mixed parameter. Untyped parameters already mean mixed, so every existing implementation still conforms.
@throws tags carry PDOException and the Adapter RuntimeException through the methods that reach them.

Guards, with new named constructors on `Adapter\Exception\RuntimeException`

Unset or unusable value	Exception
`AbstractPdo::$connection` is a bare `PDO` (`createStatement()`, `getConnection()`, `getLastGeneratedValue()`)	`forBarePdoConnection()`
`connect()` leaves `AbstractPdoConnection` without a PDO (`beginTransaction()`, `commit()`, `execute()`, `rollback()`)	`forMissingConnectionResource()`
no driver set on `AbstractPdoConnection` (`execute()`, `prepare()`)	`forMissingDriver()`
`DriverFeatureProviderTrait` composed into a driver that is not a `DriverFeatureProviderInterface`	existing `forUncomposableTrait()`

Each of these used to end in an Error (method call on null, or an undefined method on PDO) or a TypeError.

Refactors with no change in behaviour

ParameterContainer resolves integer names through nameAt(), requireName() and requireDataName(). offsetSet() resolves its key in a single match.
Pdo\Statement binds through toPdoParameter() and toPdoType().
Pdo\Result::count() goes through resolveRowCount().
AbstractPdo::formatParameterName() decides named-versus-positional in one expression.
Every isset() became ?? null or array_key_exists(), keeping isset's null semantics where it matters.

Scoped expectations, each with a docblock reason

AbstractPdoConnection::execute() has too-many-arguments. It passes the SQL as a second argument to createResult() for drivers that accept a context, such as SQLite.
AbstractPdo::formatParameterName() has redundant-cast. The analyzer reads $name as the parent's string, but this implementation also takes int.
mixed-assignment covers parameter values, which are mixed by nature, in:
- Pdo\Statement::bindParametersFromContainer();
- ParameterContainer::setFromArray();
- Adapter::getHelpers().
ParameterContainer::offsetSetReference() has possibly-undefined-string-array-index. Taking a reference creates the key.

`mago.toml`

ConnectionInterface, PlatformInterface, Pdo\Result and ParameterContainer join the too-many-methods exclusions, with a reason. Their method count is set by the interfaces they implement and by the driver contract.
Pdo\Statement::initialize() and setDriver() are registered as class initializers for $pdo and $driver.

Behaviour changes

ParameterContainer::offsetSetErrata() and offsetSetMaxLength() throw InvalidArgumentException::forMissingData() for an integer name that is not a known position. They used to raise an undefined-key warning and write under the '' key.
ParameterContainer::offsetGetErrata() and offsetGetMaxLength() return null without a warning when the parameter exists but has no errata or max length. Unknown positions throw forMissingData() as before, now without the warning.
ParameterContainer::offsetGet() returns null for an integer name that is not set. It used to throw a TypeError from ltrim().
StatementContainer::setSql(null) and Pdo\Statement::setSql(null) store '', as StatementContainerInterface::setSql(?string) allows. They used to throw a TypeError.
StatementContainer, AdapterAwareInterface and AdapterAwareTrait now declare strict_types. StatementContainer::setSql(5) throws a TypeError instead of storing "5".
DriverFeatureProviderTrait::addFeatures() checks the composing class even when given an empty list. Its methods drop #[Override], which Mago rejects on trait methods.
Adapter::getHelpers() matches helper names strictly. getHelpers(true) no longer returns the quote-identifier helper through the old loose switch.
Profiler::getLastProfile() returns null before any profile starts. It used to throw a TypeError on end()'s false.
The ProfileShape sql key is documented as string|null, because StatementContainerInterface::getSql() can return null.
AbstractPdoConnection::setResource() casts the PDO driver name to a string before lowercasing it.
AbstractPlatform::quoteIdentifierInFragment() returns '' when the split fails, without a foreach warning. The return value is unchanged.

What stays the same

No class was made final. No public or protected method signature, property type or parameter name changed, except the equivalent explicit mixed on DriverInterface's two parameters. Pdo\Statement::$isQuery, which is never written, gains a false default.
ParameterContainer::offsetUnset() still leaves the name in positions. A later write to that integer position goes back to the same name, and a new test pins this.
ParameterContainer::offsetGet() and offsetExists() keep isset() semantics: a null value counts as unset.
Binding types are unchanged:
- errata integer, null and lob map to their PDO types, and any other errata binds as a string;
- with no errata, int, bool and null values pick their PDO types, and anything else binds as a string.
formatParameterName() returns the same names for every input.
AbstractConnection::$resource stays untyped, because phpdb-mysql and phpdb-pgsql redeclare it untyped.

Still baselined: decisions needed

Each of these needs a public signature change, so it is left for a decision:

Finding	Change needed	Who it breaks
`missing-return-type` on `AdapterInterface::getHelpers()`, `ConnectionInterface::getResource()`, `StatementInterface::getResource()`	native return types (`array`, `mixed`)	implementers without a return type
`missing-property-type` and `mixed-return-statement` on `AbstractConnection::$resource` / `getResource()`	type the property	`phpdb-mysql` and `phpdb-pgsql`, which redeclare it untyped
`missing-property-type` ×7 and `missing-parameter-type` on `Pdo\Result`	native property types and `initialize(string\|int\|false\|null $generatedValue)`	subclasses that redeclare them (none in the driver packages)
`incompatible-return-type` on `Pdo\Result::next()`	`void` instead of returning the row	callers using `next()`'s return value
`missing-property-type` ×2 on `Profiler`	native types on `$profiles` and `$currentIndex`	subclasses that redeclare them
`overly-wide-return-type` on `AbstractPdo::getResultPrototype()`	return `ResultInterface`, not `?ResultInterface`	overrides declaring `?ResultInterface`
`incompatible-parameter-name` ×4 on `ParameterContainer`'s `ArrayAccess` methods	rename `$name` to `$offset`	callers passing it by name
class-level `cyclomatic-complexity` on `ParameterContainer` (lint)	split the errata and max-length metadata out of the class	none, but it is a larger refactor

The three ArrayAccess parameter-name entries for offsetExists, offsetGet and offsetSet are new in the baseline. They replace the three incompatible-parameter-type entries for the same methods, which @implements ArrayAccess<array-key, mixed> clears. A separate question is whether AbstractPdo::$connection should drop PDO from its union, since no driver method can use a bare PDO.

Tests

2313 → 2382 unit tests. Every changed src/ line is covered, and each new private helper is listed in its test's #[CoversMethod].
New tests cover:
- every guard and named constructor above;
- each binding type, checked with SQLite's typeof(), plus a PDOStatement mock for null → PDO::PARAM_NULL;
- transactions on a real SQLite PDO;
- asymmetric identifier quotes;
- the ParameterContainer position edge cases;
- the Profiler index and elapsed time.
New test assets:
- UnconnectableConnection, whose connect() leaves no PDO;
- TestNonProviderFeatureDriver, a driver without DriverFeatureProviderInterface;
- BindingObservableStatement, which exposes the protected binding hook and $parametersBound.
Infection on the 13 touched source files: 395 mutants, 383 killed by tests and 9 by static analysis, 0 escaped, covered MSI 100%. The 3 errors also occur on 0.6.x. On 0.6.x, 49 of these files' mutants were "killed" only because they raised analyzer findings the baseline didn't cover. With the files clean, 34 survived at first. The new tests kill all of them, and two of them showed redundant code that this PR removes.

Result

	Before (`0.6.x`)	After
Analyzer baseline (issues)	477	285
Lint baseline (issues)	205	153
`src/Adapter`, both baselines	265	22
PHPUnit (unit)	2313 tests	2382 tests

mago format --check, lint, analyze and guard exit 0 with MAGO_PHP_VERSION set to 8.3, 8.4 and 8.5, and PHPUnit is OK with no issues.

Tag every Adapter interface, trait, abstract class and leaf exception @api, type the interface constants, and give the array parameters and returns their element types. Connection parameters get a ConnectionParameters alias on ConnectionInterface, and ResultInterface declares its Iterator template. DriverInterface's createResult() and createStatement() take an explicit mixed parameter, which every existing implementation already satisfies.

Integer names now resolve through one helper. An unknown position throws the existing missing-data exception from the errata and max-length setters instead of writing under an empty key, offsetGet() returns null for an integer name that is not set instead of failing in ltrim(), and the errata and max-length getters return null without a warning when the parameter has none. offsetUnset() still leaves the name in its position.

AbstractPdo rejects a bare PDO connection with forBarePdoConnection() where it used to call methods PDO does not have. AbstractPdoConnection throws forMissingConnectionResource() when connect() leaves it without a PDO, and forMissingDriver() when no driver is set, instead of calling methods on null. DriverFeatureProviderTrait checks that the composing class is both a driver and a feature provider before it changes state.

Statement::bindParametersFromContainer() maps names and types through two private helpers, and passes the prepared resource to createResult(). Result::count() resolves the row count in one place and the PDOException paths are declared. Statement::setSql(null) and StatementContainer's setSql(null) store an empty string, as the interface allows null.

getHelpers() matches helper names strictly, quoteIdentifierInFragment() reads its quote pair once and tolerates a failed split, and Profiler::getLastProfile() returns null before any profile starts instead of failing on end()'s false.

Remove the src/Adapter entries these changes fix, plus one TableGateway entry that ResultInterface's Iterator template also clears. The three ArrayAccess parameter-name findings on ParameterContainer replace the parameter-type findings for the same methods. Exclude the contract-sized Adapter types from too-many-methods and register Pdo\Statement's initializers.

codecov · 2026-11-04T06:38:51Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Copilot

🟡 Changes recommended

The quote-identifier helper renames its parameter from $value to $identifier, breaking existing named-argument calls unless $value is preserved or the breaking API change is explicitly documented.

1 open issue

Preserve the quote helper’s $value named parameter · New

What changed in this PR

This PR reduces Mago debt in the database Adapter layer through stronger type documentation, safer failure handling, and focused refactoring, while leaving broader API changes for separate decisions.

Changes:

Adds API annotations, typed constants, array shapes, and exception documentation.
Guards unusable PDO connections and missing drivers, and fixes parameter lookup, nullable SQL, and empty-profile handling.
Expands regression tests and updates Mago configuration and baselines.

File	Description
`test/unit/Adapter/StatementContainerTest.php`	Tests null SQL normalization.
`test/unit/Adapter/Profiler/ProfilerTest.php`	Tests profile ordering, empty state, and timing.
`test/unit/Adapter/Platform/TestAsset/TestPlatform.php`	Allows configurable identifier quotes.
`test/unit/Adapter/Platform/Sql92Test.php`	Tests identifier escaping and asymmetric quotes.
`test/unit/Adapter/ParameterContainerTest.php`	Covers missing positions, metadata, and name mappings.
`test/unit/Adapter/Exception/RuntimeExceptionTest.php`	Tests new exception constructors.
`test/unit/Adapter/Driver/TestAsset/TestNonProviderFeatureDriver.php`	Adds an invalid feature-provider fixture.
`test/unit/Adapter/Driver/Pdo/TestAsset/UnconnectableConnection.php`	Adds a connection fixture without a PDO resource.
`test/unit/Adapter/Driver/Pdo/TestAsset/BindingObservableStatement.php`	Exposes binding state for tests.
`test/unit/Adapter/Driver/Pdo/StatementTest.php`	Tests binding types, state, and nullable SQL.
`test/unit/Adapter/Driver/Pdo/ResultTest.php`	Declares coverage for the row-count helper.
`test/unit/Adapter/Driver/Pdo/PdoTest.php`	Tests connection guards and statement creation.
`test/unit/Adapter/Driver/Pdo/ConnectionTransactionsTest.php`	Tests transactions against SQLite PDO.
`test/unit/Adapter/Driver/Pdo/ConnectionTest.php`	Tests connection resources and missing-driver guards.
`test/unit/Adapter/Driver/Feature/DriverFeatureProviderTraitTest.php`	Tests composition guards and fluent returns.
`test/unit/Adapter/AdapterTest.php`	Tests delegation, initial parameters, and helper selection.
`src/Adapter/StatementContainerInterface.php`	Marks the contract as public API.
`src/Adapter/StatementContainer.php`	Enables strict types and normalizes null SQL.
`src/Adapter/SchemaAwareInterface.php`	Adds API annotation and removes an obsolete comment.
`src/Adapter/Profiler/ProfilerInterface.php`	Marks the profiler contract as public API.
`src/Adapter/Profiler/ProfilerAwareInterface.php`	Marks profiler integration as public API.
`src/Adapter/Profiler/Profiler.php`	Handles empty profiles and clarifies profile types.
`src/Adapter/Platform/Sql92.php`	Types its constant and updates guard documentation.
`src/Adapter/Platform/PlatformInterface.php`	Documents API status and safe-word element types.
`src/Adapter/Platform/AbstractPlatform.php`	Types quote pairs and handles failed fragment splitting.
`src/Adapter/ParameterContainer.php`	Centralizes position resolution and guards metadata access.
`src/Adapter/Exception/RuntimeException.php`	Adds exceptions for unusable connection state.
`src/Adapter/Exception/InvalidQueryException.php`	Marks the exception as public API.
`src/Adapter/Exception/InvalidConnectionParametersException.php`	Documents rejected parameter types and API status.
`src/Adapter/Exception/InvalidArgumentException.php`	Marks the exception as public API.
`src/Adapter/Exception/ErrorException.php`	Marks the exception as public API.
`src/Adapter/Driver/StatementInterface.php`	Documents statement parameter element types.
`src/Adapter/Driver/ResultInterface.php`	Adds iterator generics and API annotation.
`src/Adapter/Driver/PdoDriverInterface.php`	Marks the PDO driver contract as public API.
`src/Adapter/Driver/PdoDriverAwareInterface.php`	Marks PDO driver integration as public API.
`src/Adapter/Driver/PdoConnectionInterface.php`	Marks the PDO connection contract as public API.
`src/Adapter/Driver/Pdo/Statement.php`	Extracts binding helpers and normalizes null SQL.
`src/Adapter/Driver/Pdo/Result.php`	Extracts row-count resolution and improves types.
`src/Adapter/Driver/Pdo/AbstractPdoConnection.php`	Guards missing resources and drivers.
`src/Adapter/Driver/Pdo/AbstractPdo.php`	Guards bare PDO connections and simplifies parameter formatting.
`src/Adapter/Driver/Feature/DriverFeatureProviderTrait.php`	Validates feature-provider composition.
`src/Adapter/Driver/Feature/DriverFeatureProviderInterface.php`	Clarifies feature collection types and API status.
`src/Adapter/Driver/Feature/DriverFeatureInterface.php`	Marks the feature contract as public API.
`src/Adapter/Driver/Feature/AbstractFeature.php`	Marks the feature base class as public API.
`src/Adapter/Driver/DriverInterface.php`	Types constants and explicitly declares mixed inputs.
`src/Adapter/Driver/DriverAwareInterface.php`	Marks driver integration as public API.
`src/Adapter/Driver/ConnectionInterface.php`	Defines a connection-parameter type alias.
`src/Adapter/Driver/AbstractConnection.php`	Applies connection-parameter and resource documentation.
`src/Adapter/AdapterInterface.php`	Types constants and documents parameter collections.
`src/Adapter/AdapterAwareTrait.php`	Adds strict types and API annotation.
`src/Adapter/AdapterAwareInterface.php`	Adds strict types and API annotation.
`src/Adapter/Adapter.php`	Uses strict helper selection and first-class callables.
`mago.toml`	Configures justified exclusions and statement initializers.
`lint-baseline.toml`	Removes resolved Adapter lint entries.
`analyzer-baseline.toml`	Removes resolved findings and retains deferred API issues.

_{Review effort: Balanced}

The first-class callable exposed quoteIdentifier()'s $identifier name, so calling the helper with value: threw an unknown named parameter error. Forward through a closure instead, and cover both helpers with a test that passes value by name.

simon-mundy added 6 commits November 4, 2026 17:36

simon-mundy mentioned this pull request Nov 4, 2026

Configure mago guard to enforce the library's layering #249

Open

simon-mundy requested a balanced review from Copilot November 4, 2026 10:27

simon-mundy self-assigned this Nov 4, 2026

Copilot started reviewing on behalf of simon-mundy November 4, 2026 10:28

View session

simon-mundy added enhancement New feature or request qa Improvements in quality assurance of the project labels Nov 4, 2026

simon-mundy added this to @phpdb Nov 4, 2026

github-project-automation Bot moved this to Todo in @phpdb Nov 4, 2026

Copilot AI reviewed Nov 4, 2026

View reviewed changes

Comment thread src/Adapter/Adapter.php Resolved

Navigation Menu

Repository navigation

Bring src/Adapter to Mago compliance - #248

Bring src/Adapter to Mago compliance#248
simon-mundy wants to merge 7 commits into
php-db:0.6.xfrom
simon-mundy:qa/mago-adapter

simon-mundy commented Nov 4, 2026

Uh oh!

codecov Bot commented Nov 4, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

simon-mundy commented Nov 4, 2026

Problem

Cause

Fix

Types and tags

Guards, with new named constructors on Adapter\Exception\RuntimeException

Refactors with no change in behaviour

Scoped expectations, each with a docblock reason

mago.toml

Behaviour changes

What stays the same

Still baselined: decisions needed

Tests

Result

Uh oh!

codecov Bot commented Nov 4, 2026

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Guards, with new named constructors on `Adapter\Exception\RuntimeException`

`mago.toml`