Skip to content

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

Merged
simon-mundy merged 6 commits into
php-db:0.6.xfrom
simon-mundy:refactor/dead-code-cleanup
Jul 1, 2026
Merged

Remove dead code in Profiler, add static analysis annotations#142
simon-mundy merged 6 commits into
php-db:0.6.xfrom
simon-mundy:refactor/dead-code-cleanup

Conversation

@simon-mundy

@simon-mundy simon-mundy commented Mar 25, 2026 •
edited
Loading

Copy link
Copy Markdown
Member
Q A
Documentation no
Bugfix no
BC Break no
New Feature no
RFC no
QA yes
House Keeping yes

Summary

Dead-code removal in src/ plus expansion of unit-test coverage. No behavioural changes.

src/ changes

src/Sql/AbstractSql.php — Remove dead code

  • Removed processValuesArgument() and its Values match arm
  • Removed the unreachable is_string/is_callable join-name branch and its InvalidArgumentException throw
  • Dropped the now-unused imports.

src/ResultSet/AbstractResultSet.php — Remove dead code

  • Collapsed the Iterator/else branches in initialize(), valid(), and rewind(). A Traversable that is neither Iterator nor IteratorAggregate is not constructible in PHP, so the key()/reset() fallbacks and the InvalidArgumentException throw were unreachable. The narrowed assignment is annotated with @phpstan-ignore.

src/Adapter/Driver/Pdo/AbstractPdo.php — Mark unreachable throw

  • Added @codeCoverageIgnoreStart/End around the extension_loaded('PDO') throw in checkEnvironment(). The throw cannot fire in practice: AbstractPdo requires the PDO extension to even be loadable.

Test coverage

  • Added unit tests across Adapter, Sql, ResultSet, Metadata, RowGateway, and TableGateway suites, substantially raising coverage.
  • Renamed ~30 test methods to describe behaviour rather than implementation.

@simon-mundy
Improve test coverage from 84% to 99%, remove dead code, fix test inf…
07062c3 
…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>
@simon-mundy
Remove dead code in Profiler, add static analysis annotations
72da3e8 
- 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.
@simon-mundy
Rename test methods to describe behaviour not implementation
cf1f623 
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 mentioned this pull request Mar 25, 2026
Merged
@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
@simon-mundy
Fix phpcs violations: alignment, FQCN imports, unused imports
530a55e
@simon-mundy
Delete REFACTOR.md
c8936ad 
Signed-off-by: Simon Mundy <46739456+simon-mundy@users.noreply.github.com>
@tyrsson

tyrsson commented Jun 30, 2026

Copy link
Copy Markdown
Member

There's still merge conflicts. I'm assuming this PR needs merged before #143 ?

@simon-mundy
Merge remote-tracking branch 'upstream/0.6.x' into refactor/dead-code…
1cdf008 
…-cleanup

# Conflicts:
#	src/Adapter/Profiler/Profiler.php
#	test/unit/Adapter/Container/AdapterInterfaceDelegatorTest.php
#	test/unit/Adapter/Driver/Pdo/ConnectionTest.php
#	test/unit/Adapter/Driver/Pdo/PdoTest.php
@github-project-automation github-project-automation Bot moved this from Todo to In Progress in @phpdb Jul 1, 2026
@simon-mundy simon-mundy merged commit 865a19c into php-db:0.6.x Jul 1, 2026
13 checks passed
@github-project-automation github-project-automation Bot moved this from In Progress to Done in @phpdb Jul 1, 2026
simon-mundy added a commit to simon-mundy/phpdb that referenced this pull request Jul 1, 2026
@simon-mundy
Merge upstream/0.6.x (PR php-db#142) into test/adapter-coverage-100
984631b 
Resolves 7-file conflict caused by PR php-db#142's test-method renames and
attribute cleanup overlapping with this branch's coverage work.

- AdapterAwareTraitTest: adopt CoversMethod over CoversTrait to match
  the rest of the suite (78 other files use CoversMethod; this was the
  only CoversTrait usage)
- AbstractConnectionTest/TestConnection: kept this branch's TestConnection
  fixture (extends AbstractConnection directly) over upstream's
  ConnectionWrapper, which extends AbstractPdoConnection and would have
  reintroduced the wrong-hierarchy coverage gap this PR fixes. Also gave
  TestConnection a constructor param for driverName so
  testGetDriverNameReturnsValueWhenSet genuinely exercises a non-null
  value instead of asserting null
- AdapterInterfaceDelegatorTest: both branches independently added the
  same three tests at different positions; deduplicated to one copy
- PdoTest: adopted upstream's `$unused = $pdo->getProfiler()` over a
  phpstan-ignore comment to silence the unused-result warning
- ProfilerTest: kept this branch's split of testProfilerStart into
  separate string/container tests (one logical assertion per test)
  over upstream's combined version
- Dropped upstream's `CoversMethod(..., '__construct')` additions on
  AbstractPdo/AbstractPdoConnection in PdoTest/ConnectionTest since
  neither class declares its own constructor

Full unit suite (1451 tests), phpcs, and phpstan all pass post-merge.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tyrsson tyrsson tyrsson approved these changes

Assignees

@simon-mundy simon-mundy

Labels

qa Improvements in quality assurance of the project

Projects

@phpdb
Status: Done

Milestone

0.6.0

Development

Successfully merging this pull request may close these issues.

2 participants

@simon-mundy @tyrsson