Skip to content

M1I10: ProtocolHandlerInterface + TcpHandler#67

Merged
s2x merged 1 commit into
mainfrom
feature/m1i10-protocol-interface-tcp
May 2, 2026
Merged

M1I10: ProtocolHandlerInterface + TcpHandler#67
s2x merged 1 commit into
mainfrom
feature/m1i10-protocol-interface-tcp

Conversation

@s2x
Copy link
Copy Markdown
Contributor

@s2x s2x commented May 2, 2026

Summary

What Where
Create src/Server/Protocol/ProtocolHandlerInterface.php
Create src/Server/Protocol/TcpHandler.php
Create tests/Server/Protocol/TcpHandlerTest.php
  • ProtocolHandlerInterface — contract with single handle(Connection): void method
  • TcpHandler — implements the interface; accepts optional Closure callback; no-op without callback; does NOT close the connection

Acceptance criteria

  • Interface has single method: handle(Connection): void
  • TcpHandler accepts optional Closure callback
  • No callback = no-op (connection accepted, nothing happens)
  • Handler does NOT close the connection (worker will auto-close)

Closes #12

Copy link
Copy Markdown
Contributor Author

@s2x s2x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary — Round 1

# Severity File:Line Finding
No inline findings to report

Acceptance Criteria Check ✅

  • Interface has single method: handle(Connection): voidProtocolHandlerInterface defines exactly one method with correct signature
  • TcpHandler accepts optional Closure callback — constructor takes ?\Closure $callback = null, correctly nullable
  • No callback = no-ophandle() checks for closure before invoking; no side effects when callback is null
  • Handler does NOT close the connection — no $connection->close() call anywhere in TcpHandler

What's Fine ✔️

  • PSR-4 autoloading: CrazyGoat\Forklift\Server\Protocol maps correctly to src/Server/Protocol/
  • declare(strict_types=1) on all new files
  • PHPStan level (neon.dist): 0 errors
  • PHPUnit: 3/3 tests pass, 7 assertions
  • Constructor uses readonly property promotion — clean PHP 8.2+ pattern
  • Closure doc annotation @var \Closure(Connection): void|null aids IDE/static analysis
  • Test covers all three scenarios: null callback, callback invocation, and no-close verification
  • Matches issue #12 and milestone doc docs/superpowers/milestone-1/10-protocol-interface-tcp.md requirements exactly

Follow-up Observations

  1. Tests use real socketssocket_create() couples tests to the socket extension and OS networking. Consider extracting a ConnectionInterface + mock pattern in a future milestone. (Low effort, good isolation payoff.)
  2. instanceof \Closure check — Could be simplified to $this->callback !== null since the type hint already guarantees ?\Closure. Current approach is defensively safer (protects against edge cases like unserialize corruption), so no action needed — just noting for awareness.
  3. Callback exception propagationhandle() doesn't catch exceptions from the callback. When the worker loop is implemented, ensure it wraps handler invocations in try/catch to prevent a single misbehaving callback from terminating the worker process.
  4. No test for ProtocolHandlerInterface in isolation — Covered implicitly by TcpHandlerTest, but a dedicated interface compliance test (e.g., ProtocolHandlerInterfaceTest with a mock implementation) could catch future regressions if the interface changes.

@s2x s2x merged commit e8add68 into main May 2, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

M1I10: ProtocolHandlerInterface + TcpHandler

1 participant