Fix mago analyze findings (WIP) - #60
Merged
Merged
Conversation
Added #[Override] to all methods that override a parent class or interface method but were missing the attribute: - src/Driver.php (8 methods) - src/Metadata/Source.php (7 methods) - src/Sql/Ddl/CreateTableDecorator.php (2 methods) - src/Sql/Ddl/AlterTableDecorator.php (3 methods) - src/Connection.php (2 methods) Applied manually (rather than via `mago analyze --fix`) to keep the existing use Override; + bare #[Override] convention already used elsewhere in the codebase; the auto-fix instead inserts fully-qualified #[\Override], which would trip lint's no-fully-qualified-global-class-like. mago analyze: 433 -> 411 remaining issues.
Added @throws docblocks for every method that can propagate an exception without declaring it, following two conventions: - In Container/*Factory.php __invoke() methods: prefer PSR interfaces for container-related exceptions (Psr\Container\ContainerExceptionInterface, Psr\Container\NotFoundExceptionInterface, Laminas\ServiceManager\ Exception\ExceptionInterface), since these are factories consumed through laminas-servicemanager. - Everywhere else: use the base PhpDb\*\Exception\ExceptionInterface for the relevant namespace (PhpDb\Exception\ExceptionInterface or PhpDb\Adapter\Exception\ExceptionInterface) rather than concrete exception classes, since callers should catch the interface, not a specific implementation. - Native PDOException documented separately in Pdo/Connection where relevant, since it has no PhpDb interface equivalent. Fixed across: 6 Container/*Factory.php files, Driver.php, Statement.php, Connection.php, Pdo/Connection.php, Result.php (23 findings, plus one cascading finding in DriverInterfaceFactory after Driver::__construct() was documented). mago analyze: 388 -> 365 remaining issues.
- src/Metadata/Source.php: added /** @var ResultSetInterface $results */ before each of the 8 $this->adapter->query(..., QUERY_MODE_EXECUTE) calls. AdapterInterface::query() declares a 3-way union return type (StatementInterface|ResultSetInterface|ResultInterface), but toArray() only exists on ResultSetInterface. Since these queries are always SELECT statements, the runtime type is always ResultSetInterface; the annotation documents that guarantee for the analyzer. - src/Result.php: 3 findings, 2 different fixes: - loadDataFromMysqliStatement(): added a real instanceof mysqli_stmt guard. This method is only ever called from a branch that already guarantees this, but the analyzer can't see across the method-call boundary, so the guard makes the precondition explicit. - rewind() and loadFromMysqliResult(): added instanceof guards before data_seek()/fetch_assoc(). These calls previously assumed $this->resource is never a bare mysqli connection object, but Connection::execute() can pass the raw connection through to Driver::createResult() for non-SELECT (write) queries. Iterating a Result wrapping a write-query outcome was a real latent crash (undefined method on mysqli). Rather than suppress the finding or silently patch around it, added explicit guards that throw a clear RuntimeException if this path is ever hit, so the gap stays visible instead of being buried. Filing a follow-up issue in this repo with the exact rationale. mago analyze: 365 -> 352 remaining issues. non-existent-method: 23 -> 0. Note: possibly-undefined-string-array-index jumped 4 -> 34 as a side effect - toArray()'s return type is now resolvable, so mago can finally analyze the array shape of $row inside the Source.php foreach loops instead of treating it as opaque mixed. Not a regression, just newly visible precise feedback for future work.
Added precise array docblock types in place of bare `array` type hints,
matching each array's actual literal shape rather than defaulting to
array<array-key, mixed>:
- ConfigProvider::getDependencies()/__invoke(): full array-shape
docblocks (aliases/factories are always class-string => class-string
maps, per Laminas ServiceManager config conventions)
- Config/option bags ($options, $features, $connectionParameters,
$connectionInfo) across Driver.php, Pdo/Driver.php, Connection.php,
Pdo/Connection.php, and 3 Container/*Factory.php files:
array<string, mixed>
- Statement::execute($parameters): array<array-key, mixed> (bind params
can be positional int or named string keys)
- Result::$statementBindValues: array{keys: string[]|null, values:
array<int, mixed>} (actual fixed shape)
- AlterTableDecorator::getSqlInsertOffsets(): array<int, int>
- AlterTableDecorator::processAddColumns()/processChangeColumns():
array<int, array<int|string, string>>
- SelectDecorator::processOffset(): string[]|null (matches the sibling
processLimit()'s existing identical-shape docblock)
mago analyze: 352 -> 338 remaining issues. imprecise-type: 20 -> 0.
- Sql/SelectDecorator.php, Sql/Ddl/CreateTableDecorator.php, Sql/Ddl/AlterTableDecorator.php: $subject given a null default; the property's type already allows null, only set later via setSubject(). No behavior change. - Pdo/Driver.php: $profiler (inherited from AbstractPdo, declared ?ProfilerInterface with no default) redeclared with a null default. No behavior change, same type. - Pdo/Connection.php: $dsn and $driverName (inherited, non-nullable) redeclared as nullable with null defaults, matching their real lazy-set lifecycle (set inside connect()/setResource()). - Connection.php (mysqli): $driverName given a null default. $driver made nullable, with an explicit guard added at its one real call site in execute() that throws PhpDb\Adapter\Exception\RuntimeException if execute() is somehow called without setDriver() ever being called. This replaces a previous @mago-expect suppression with a real, visible runtime check, verified empirically that the two early constructor `return;` statements each need their own suppression/fix since mago tracks them as separate finding instances from the property declaration itself. - Statement.php: reverted an earlier suppression attempt for $mysqli/$driver/$resource. These 3 properties have multiple independent read sites across execute()/prepare()/getResource()/ bindParametersFromContainer(), and mago does not retain null-narrowing of a property across separate statements/methods the way it does for local variables - a real fix requires capturing each property into a local variable after a guard clause, which is a larger, more invasive rewrite than we want to do as a side effect of this analyze pass. Left unsuppressed (visible in `mago analyze` output) rather than adding another @mago-expect - tracked in a follow-up issue and intended to be captured by a mago baseline (not inline suppression) once this PR's analyze work is otherwise done. mago analyze: 338 -> 326 remaining issues.
Missed adding @throws Exception\RuntimeException when the guard clause was added for the non-existent-method fix earlier in this branch. mago analyze: 326 -> 325 remaining issues.
This was
linked to
issues
Aug 10, 2026
Prefixed unused $container/$requestedName parameters with underscore across 8 Container/*Factory.php __invoke() methods, per mago's own suggested remediation. These factories deliberately do not implement any Laminas ServiceManager FactoryInterface: PSR-11's v1 -> v2 jump added parameter and return types to ContainerInterface, so implementing a formal FactoryInterface would lock this library to SMv4 only, breaking SMv3 support. Since __invoke() has no enforceable interface contract, PHP does not require unused leading/middle parameters to be removed or renamed - but they also can't just be deleted, since Laminas ServiceManager calls factories positionally ($container, $requestedName, $options), and $options is used in most of these, so removing an earlier unused parameter would shift $options into the wrong position. Underscore-prefixing keeps the signature and calling convention intact while marking the parameters as intentionally unused. mago analyze: 325 -> 312 remaining issues. unused-parameter: 13 -> 0.
…oops Documents the actual SELECT column shape per query loop, reducing this file's mixed-*/non-existent-method finding count from 77 to 39.
tyrsson
force-pushed
the
mago-analyze-fixes
branch
from
August 10, 2026 19:07
53b0b5a to
9371a24
Compare
This was referenced Aug 10, 2026
- count(): guard against bare mysqli (no num_rows), cast num_rows to int - getAffectedRows(): cast affected_rows/num_rows to int - getGeneratedValue(): tighten $generatedValue to string|int|false|null - initialize(): remove unreachable instanceof guard, drop redundant instanceof check already proven by prior elimination - loadDataFromMysqliStatement(): guard result_metadata() possibly returning false; type $col via object shape docblock - loadFromMysqliResult(): narrow fetch_assoc()'s type to what PHP actually documents (array|null, not array|false|null per stub) - $currentData: type as ?array, add Iterator<int, array<array-key, mixed>|null> generics matching key()/current() - $isBuffered: default to null (matches ResultInterface::isBuffered(): ?bool contract), not false Reduces this file's mago analyze findings from 23 to 6; the rest are tracked in #66 for baseline.
This was
unlinked from
issues
Aug 10, 2026
- getResource(): add local @return docblock overriding the interface's legacy resource|false|null contract; suppress the resulting incompatible-return-type check (native mysqli_stmt is always accurate for this class, matching the existing @PHPStan-Ignore rationale) - prepare(): assign mysqli::prepare()'s result to a local variable and guard before storing into $resource, instead of assigning the possibly-false result directly into a strictly-typed property (which would throw an uncontrolled TypeError before the intended InvalidQueryException could run) - setDriver(): guard that $driver is the concrete Driver class before assignment, since $this->driver is used with a mysqli-Driver-specific createResult($resource, $buffered) signature not part of DriverInterface - setSql(): coalesce null to '' before assignment, matching the existing empty-string-as-unset convention used in prepare() Reduces this file's mago analyze findings from 10 to 3 (the 3 uninitialized-property findings already tracked in #61).
- constructor: remove unreachable final branch (connectionInfo's type is array|mysqli|null, exhaustively handled by the two prior branches) - setDriver()/$driver: widen to ?DriverInterface (unlike Statement.php, the sole call site 'createResult($resource)' passes only one arg, fully compatible with the interface contract; no concrete Driver features are actually used) - getCurrentSchema(): guard query()'s bool|mysqli_result return before calling fetch_row(), guard fetch_row()'s real false-on-failure return (per PHP docs), and narrow its single-column shape via docblock - createResource(): add native mysqli return type Reduces this file's mago analyze findings from 46 to 38.
….php DriverInterface::createResult($resource) is documented with a generic resource type to stay valid across every RDBMS platform (mysqli has been object-oriented since PHP 5.0 and was never part of PHP's resource-to-object migration, so this mismatch isn't fixable locally). Proposed upstream fix tracked at php-db/phpdb#170 (@template generics). Reduces this file's mago analyze findings from 38 to 37; the remaining findings are tracked in #68 for baseline.
- connect(): remove dead is_string($dsn) guard ($dsn is always a string by this point on every code path); cast getAttribute()'s genuinely mixed return before strtolower() - getCurrentSchema(): remove incorrect @var PDOStatement docblock that hid query()'s real PDOStatement|false return; guard $resource nullability before use; narrow fetchColumn()'s mixed return via contextual @var docblock (single-column query) - getLastGeneratedValue(): guard $resource nullability before use - $dsn/$driverName: suppress write-only-property false positives (confirmed via grep: read by inherited AbstractPdoConnection::getDsn() and AbstractConnection::getDriverName(), which mago's per-class analysis doesn't see) Reduces this file's mago analyze findings from 18 to 8; the remaining 8 are the $connectionParameters cluster already tracked in #65.
- $subject: suppress write-only-property false positive (read by the
inherited AbstractSql::$subject handling via get_object_vars())
- getSqlInsertOffsets(): tighten return shape to array{0,1,2,3: int}
(the trailing range(0,3) fill loop guarantees all four keys); narrow
via @var at the return point since mago's own flow-tracing through
the switch/foreach can't prove the shape on its own
- remove dead $j ??= 0 (both $insert and $j are always set together
in the same switch case each iteration; $insert resets to '' at the
top of the loop, so the coalesce can never fire)
- compareColumnOptions()/normalizeColumnOption(): promote existing
docblock types to native type hints
- processAddColumns()/processChangeColumns(): guard against the
genuinely-nullable (per inherited parent signature) $adapterPlatform
parameter; the real dynamic-dispatch call path always passes a real
platform, but the signature itself still legally permits null
Reduces this file's mago analyze findings from 43 to 33; 21 are the
$connectionParameters/getOptions()/untyped-array cluster tracked in
#65, and 12 are a mago loop-bound limitation tracked in #69.
simon-mundy
reviewed
Aug 11, 2026
simon-mundy
reviewed
Aug 11, 2026
simon-mundy
reviewed
Aug 11, 2026
simon-mundy
reviewed
Aug 11, 2026
simon-mundy
reviewed
Aug 11, 2026
simon-mundy
reviewed
Aug 11, 2026
simon-mundy
reviewed
Aug 11, 2026
simon-mundy
reviewed
Aug 11, 2026
simon-mundy
reviewed
Aug 11, 2026
simon-mundy
reviewed
Aug 11, 2026
simon-mundy
reviewed
Aug 11, 2026
simon-mundy
reviewed
Aug 11, 2026
simon-mundy
reviewed
Aug 11, 2026
simon-mundy
reviewed
Aug 11, 2026
simon-mundy
reviewed
Aug 11, 2026
simon-mundy
reviewed
Aug 11, 2026
simon-mundy
reviewed
Aug 11, 2026
simon-mundy
reviewed
Aug 11, 2026
simon-mundy
reviewed
Aug 11, 2026
simon-mundy
reviewed
Aug 11, 2026
| : " AND {$p->quoteIdentifierChain(['T', 'TABLE_SCHEMA'])} = {$p->quoteTrustedValue($schema)}"; | ||
|
|
||
| /** @var ResultSetInterface $results */ | ||
| $results = $this->adapter->query($sql, AdapterInterface::QUERY_MODE_EXECUTE); |
Member
There was a problem hiding this comment.
Why doesn't this already infer ResultSetInterface?
Member
Author
There was a problem hiding this comment.
Only thing I can figure is due to something upstream. if that is the case then once its fixed it will show redundant here and can be removed. We will probably face this a lot.
simon-mundy
reviewed
Aug 11, 2026
| $results = $this->adapter->query($sql, AdapterInterface::QUERY_MODE_EXECUTE); | ||
|
|
||
| $data = []; | ||
| /** @var array{TRIGGER_NAME: string, EVENT_MANIPULATION: string, EVENT_OBJECT_CATALOG: string, EVENT_OBJECT_SCHEMA: string, EVENT_OBJECT_TABLE: string, ACTION_ORDER: string, ACTION_CONDITION: ?string, ACTION_STATEMENT: string, ACTION_ORIENTATION: string, ACTION_TIMING: string, ACTION_REFERENCE_OLD_TABLE: ?string, ACTION_REFERENCE_NEW_TABLE: ?string, ACTION_REFERENCE_OLD_ROW: ?string, ACTION_REFERENCE_NEW_ROW: ?string, CREATED: ?string} $row */ |
Member
There was a problem hiding this comment.
Yech. Can we put this in a shape at the top of the class and refer to it by name?
…ories Per Simon's preference, revert the $_container/$_requestedName underscore-prefix rename (commit 5bf22d7) back to $container/ $requestedName, and suppress the unused-parameter finding directly with // @mago-expect analysis:unused-parameter instead. Laminas ServiceManager still calls factories positionally, so the parameters must remain in place regardless of naming convention.
…factories Fold the analysis:unused-parameter suppression into the last line of each factory's existing docblock (all 8 have one, since all document @throws) instead of a separate line comment above the method.
Signed-off-by: Joey Smith <jsmith@webinertia.net>
Signed-off-by: Joey Smith <jsmith@webinertia.net>
…nvention Per the mago baseline docs (one file per tool), the analyzer baseline should be named analysis-baseline.toml rather than mago-baseline.toml.
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.
Stacked on #58 (mago-lint-fixes). Working through mago analyze findings rule-by-rule. Fully resolved so far: missing-override-attribute, class-must-be-final, unhandled-thrown-type, imprecise-type, non-existent-method, redundant-docblock-type, most redundant-comparison/redundant-condition. In progress: uninitialized-property.
Related: #59, #61, #62, #63, #64, #65, #66, #67, #68, #69, #70, #71, #72, #73, #74