Skip to content

Remove dead code in Profiler, add static analysis annotations#142

Open
simon-mundy wants to merge 5 commits intophp-db:0.6.xfrom
simon-mundy:refactor/dead-code-cleanup
Open

Remove dead code in Profiler, add static analysis annotations#142
simon-mundy wants to merge 5 commits intophp-db:0.6.xfrom
simon-mundy:refactor/dead-code-cleanup

Conversation

@simon-mundy
Copy link
Member

@simon-mundy simon-mundy commented Mar 25, 2026

Q A
Documentation no
Bugfix yes
BC Break no
New Feature no
RFC no
QA yes
House Keeping yes

Summary

Minimal src/-only changes that remove provably dead code and add static analysis annotations. No behavioural changes.

Changes

src/Adapter/Profiler/Profiler.php — Remove unreachable else-branch in profilerStart()

The method signature is string|StatementContainerInterface $target. The if handles StatementContainerInterface, the elseif handles string. PHP's union type enforcement means no other type can reach the method body — the else branch with its InvalidArgumentException throw is dead code. Removed the branch and cleaned up the now-unused InvalidArgumentException import and is_string() import.

src/Adapter/Driver/Pdo/AbstractPdo.php — Add @codeCoverageIgnore to checkEnvironment() throw

The throw fires when extension_loaded('PDO') returns false. But AbstractPdo itself requires the PDO extension to be loaded (it imports PDO classes). Any environment where this class is instantiable necessarily has PDO loaded, making the throw unreachable in practice.

src/ResultSet/AbstractResultSet.php — Add @phpstan-ignore for Traversable assignment

In initialize(), after the array and IteratorAggregate branches are handled, the remaining $dataSource is a Traversable that is necessarily an Iterator. PHPStan cannot narrow Traversable to Iterator here, so the ignore annotation is appropriate.

Test method renames

Renamed ~30 test methods across the Adapter test suite to describe behaviour rather than echo method names (Rule XII). Pattern: testFluent<Method> for chaining, test<Method>Returns<What> for getters, test<Method>ThrowsOn<Condition> for exceptions.

…rastructure

- Add ~270 new test methods across 60 files, bringing line coverage
  from 84% (2809/3354) to 99.13% (3289/3318)
- Achieve 100% coverage for Sql/*, ResultSet/*, Metadata/*, Container/*
- Remove dead code in AbstractSql (unreachable processValuesArgument,
  defensive throw in processJoin) and AbstractResultSet (unreachable
  non-Iterator branches in valid/rewind, unreachable throw in initialize)
- Remove 9 permanently-skipped tests that were redundant, had
  contradictory logic, or tested removed functionality
- Replace all anonymous classes in tests with proper TestAsset classes
  for reusability and consistency
- Fix invalid #[CoversMethod] attributes causing PHPUnit warnings
- Fix #[CoversMethod] parentheses in method name strings
- Add REFACTOR.md documenting completed work and remaining Adapter/* gaps

Signed-off-by: Simon Mundy <simon.mundy@peptolab.com>
- Profiler::profilerStart(): remove unreachable else-branch. The union
  type `string|StatementContainerInterface` guarantees only those two
  types can be passed; the if/elseif handles both exhaustively, making
  the else+throw dead code. Clean up unused imports (InvalidArgumentException,
  is_string).

- AbstractPdo::checkEnvironment(): mark the throw inside
  `if (!extension_loaded('PDO'))` with @codeCoverageIgnore. PDO is a
  hard requirement — this branch cannot execute in any environment where
  the class is loadable.

- AbstractResultSet::initialize(): add @phpstan-ignore for Traversable
  assignment. After array and IteratorAggregate branches are handled, the
  remaining Traversable is necessarily an Iterator, but PHPStan cannot
  narrow it.
Rename ~30 test methods across Adapter test suite to follow the
convention of naming tests after what they verify rather than which
method they call.

Pattern applied:
- Fluent/chaining tests: testFluent<Method>
- Getter tests: test<Method>Returns<What>
- Throw tests: test<Method>ThrowsOn<Condition>
- Delegation tests: test<Method>DelegatesToX

Simple getter/setter tests (testSetSql, testOffsetGet, etc.) left
as-is where the method name already describes the behaviour.
@simon-mundy simon-mundy requested a review from tyrsson March 25, 2026 22:49
@simon-mundy simon-mundy self-assigned this Mar 25, 2026
@simon-mundy simon-mundy added the qa Improvements in quality assurance of the project label Mar 25, 2026
@github-project-automation github-project-automation bot moved this to Todo in @phpdb Mar 25, 2026
@simon-mundy simon-mundy added this to the 0.6.0 milestone Mar 25, 2026
simon-mundy and others added 2 commits March 26, 2026 09:52
Signed-off-by: Simon Mundy <46739456+simon-mundy@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

qa Improvements in quality assurance of the project

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

1 participant