Mago migration - format, lint, analysis, coverage, mutation improvements - #57
Open
tyrsson wants to merge 44 commits into
Open
Mago migration - format, lint, analysis, coverage, mutation improvements#57tyrsson wants to merge 44 commits into
tyrsson wants to merge 44 commits into
Conversation
…-literal-password, no-isset) - Convert PHPUnit Test attribute to short form with import across test files (mago's auto-fix left it fully-qualified) - string-style: convert SQL-building concatenation to interpolation/heredoc in Metadata/Source.php and Statement.php - no-multi-assignments: split chained null assignment in Pdo/Connection.php - no-literal-password: extract fake test password to a named const + suppress - no-isset: replace ambiguous isset() checks with explicit null comparisons (yoda-style) or array_key_exists, across factories, Ddl decorators, Pdo/Connection, Connection, and Metadata/Source - Revert an unsafe mago auto-fix in ResultTest that changed a discard-arguments closure into a first-class callable, breaking the test - Fix a misplaced use-import (landed inside class body instead of before it) in AbstractAdapterTestCase from the earlier Test-attribute rename - Fix a stale #[Depends] reference to a pre-rename test method name in Pdo/TableGatewayTest
connectionFails() previously made a real (unmocked) mysqli connection attempt to verify connect() throws on failure. That real connection attempt left the mysqli extension in a state that caused the *next* test's mocked real_connect() to fall through to the real C-level implementation instead of the stub, breaking nonSecureConnection, sslConnection, and sslConnectionNoVerify whenever they ran after it in the same process (order-dependent, not previously visible when run standalone). Now uses a proper mock. Because connect()'s catch block discards the caught exception and reconstructs an ErrorException from resource properties (connect_error/connect_errno) that only a real connection populates, this currently surfaces as a TypeError rather than the RuntimeException the method is meant to throw. Asserting the exact TypeError message documents this as a known, pre-existing gap (inherited from laminas-db) rather than silently working around it.
- Metadata/Source.php: 9 else-branches building SQL fragments converted to ternaries (single assignment target in both branches); renamed a hardcoded "_laminas_" constraint-name prefix to "_phpdb_" while in there - Driver.php, Pdo/Connection.php, Connection.php: converted asymmetric if/else and elseif chains to guard clauses (early returns) - Result.php: converted nested if-inside-else to a match expression, preserving the "leave isBuffered untouched" no-op case explicitly - AdapterPlatform.php: ternary for resource assignment - Statement.php (bind-param type building): merged the else branch into the existing switch's default case, since they were identical - Statement.php (Standard ParameterContainer Merging Block): real bug fix, not just a restyle. $parameterContainer is a non-nullable, always- initialized property, so the outer "if (! $this->parameterContainer instanceof ParameterContainer)" guard could never be true - the branch that lets a caller-supplied ParameterContainer replace the instance one was dead code. Removed the dead guard; a passed-in ParameterContainer now actually replaces $this->parameterContainer as intended. - Connection.php constructor: flagging (not removing) that the third branch is unreachable given the array|mysqli|null parameter type - left as-is pending a decision on removing it separately. Full suite (349 tests, unit + integration) passes.
Added named arguments to builtin function calls flagged by mago:
range(), substr_replace(), str_replace(), array_fill(), print_r()
across src/Sql/Ddl/{Create,Alter}TableDecorator.php, src/Result.php,
src/Metadata/Source.php, src/AdapterPlatform.php, and two integration
test fixtures.
Swap-and-invert the negated ternaries introduced by the earlier no-else-clause fixes in Metadata/Source.php (7x) and Connection.php (1x).
…-methods
These findings require actual refactoring (splitting classes/methods),
which is out of scope for this mago-migration lint pass. Suppressed with
@mago-expect on the affected classes/methods:
- src/Connection.php, src/Result.php: cyclomatic-complexity, kan-defect,
too-many-methods
- src/Pdo/Connection.php: cyclomatic-complexity
- src/Metadata/Source.php, src/Sql/Ddl/{Create,Alter}TableDecorator.php:
cyclomatic-complexity and/or kan-defect
- connect() in Connection.php/Pdo/Connection.php, loadColumnData()/
loadConstraintData() in Metadata/Source.php: halstead
- ambiguous-function-call: import array_key_exists in the two DriverInterfaceFactory classes - no-shorthand-ternary: explicit null/empty checks in Statement::prepare() and SetupTrait::getAdapter() (preserves prior "treat '' as unset" behavior) - no-redundant-variable: ConnectionTransactionsTest no longer tracks a $nested counter whose final value was never read; asserts against the known literal expected counts directly - no-error-control-operator: removed unnecessary @ suppression from AdapterPlatformTest::quoteValue() - confirmed empirically the mocked driver setup never raises the notice being guarded against - no-assign-in-argument: hoisted mock assignment out of the initialize() call in StatementIntegrationTest - no-empty: explicit [] / '' comparisons in the two Extension listeners and Connection::connect() - assert-description: added description to assert() in ResultTest - no-negated-ternary: fixed ternary introduced by the shorthand-ternary fix in SetupTrait 109 -> 4 remaining issues (empty-catch-clause / fully-qualified-class-like in Pdo/Connection.php and unit ConnectionTest.php, pending discussion).
- Pdo/Connection::getLastGeneratedValue() now catches PDOException (the only exception lastInsertId() can throw), resolving no-fully-qualified-global-class-like without needing an alias - unit/Pdo/ConnectionTest now catches InvalidConnectionParametersException and RuntimeException (the only exceptions connect() declares via @throws) instead of the generic Exception import These concrete imports will also be needed once we get to the analyze PR, since checked exceptions must be annotated per-method there anyway. Remaining no-empty-catch-clause findings (3) are intentional swallows (best-effort connection attempts / fallback to false) and are suppressed with @mago-expect plus a rationale comment. mago lint: 0 issues remaining.
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.
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.
- 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.
- 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.
Same fixes applied as AlterTableDecorator.php (commit 4e2c1fc): - $subject: suppress write-only-property false positive - $columnOptionSortOrder: native array type hint - getSqlInsertOffsets(): native types, tightened array{0,1,2,3: int} return shape via @var at the return point - processColumns(): rename $platform -> $adapterPlatform to match parent CreateTable::processColumns(), guard nullability, add @throws - remove dead $j ??= 0 - compareColumnOptions()/normalizeColumnOption(): promote docblock types to native type hints Reduces this file's mago analyze findings from 26 to 15; all 15 are already tracked (getOptions()/untyped-array cluster in #65, mago loop-bound limitation in #69).
Covers all 179 remaining findings, each traced to an upstream root cause, a mago flow-analysis limitation, or a false positive, and each documented in a tracking issue (#61, #64-#74) tied to milestone 0.5.0. Also: - fix mago.toml's pinned schema version (1.45.0 -> 1.46.0, matching the installed mago version) - reference the baseline via [analyzer].baseline in mago.toml so mago analyze applies it automatically without --baseline
…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.
tyrsson
added
CI/CD/SA
HouseKeeping
Fix mago analyze findings (WIP)
Fix mago lint findings (WIP)
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Signed-off-by: Joey Smith <jsmith@webinertia.net>
Signed-off-by: Joey Smith <jsmith@webinertia.net>
Signed-off-by: Joey Smith <jsmith@webinertia.net>
Signed-off-by: Joey Smith <jsmith@webinertia.net>
Signed-off-by: Joey Smith <jsmith@webinertia.net>
Signed-off-by: Joey Smith <jsmith@webinertia.net>
tyrsson
changed the title
Member
Author
|
All related PR's have been merged into this PR. Mago analysis baseline currently contains 179 items CodeCov is 89.16409% Mutation MSI currently stands @ 1086 mutations were generated: Metrics: |
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
1 participant
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.
First PR in the Mago tool-by-tool stack. Mechanical reformat only, no manual edits.
mago format, isolated in its own commit.git-blame-ignore-revsrecording that commit's SHAmago format --checkpasses cleanNext in stack: lint, then analyze, then guard.