Skip to content

M1I09: SocketFactory (fallback chain)#66

Merged
s2x merged 2 commits into
mainfrom
feature/m1i09-socket-factory
May 2, 2026
Merged

M1I09: SocketFactory (fallback chain)#66
s2x merged 2 commits into
mainfrom
feature/m1i09-socket-factory

Conversation

@s2x
Copy link
Copy Markdown
Contributor

@s2x s2x commented May 2, 2026

Summary

Component Description
Issue #11 — SocketFactory with fallback chain
Files src/Server/Socket/SocketFactory.php, tests/Server/Socket/SocketFactoryTest.php
Changes Static factory with fallback: REUSE_PORT → ForkSharedProxy if unsupported

Acceptance criteria

  • create(MASTER) returns MasterProxy
  • create(FORK_SHARED) returns ForkSharedProxy
  • create(REUSE_PORT) returns ReusePortProxy if supported, else ForkSharedProxy
  • Lint + tests pass

Closes #11

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
1 MEDIUM tests/Server/Socket/SocketFactoryTest.php:35 Fallback test assertion is too weak — doesn't validate fallback logic

Follow-up Observations

  • SocketFactory::create() accepts $port and $protocol but never passes them to proxy constructors — this is a known decision per watpliwosci.md #6 ("params not needed in proxy constructors — createSocket() receives them at call time"). Consider documenting this in a PHPDoc @param tag to clarify intent.
  • Static factory with hardcoded new calls prevents mocking/stubbing in unit tests — consider making the factory instantiable with overridable methods for future testability.
  • Consider adding a @throws docblock to document potential \UnhandledMatchError if new ProxyType enum values are added without corresponding factory cases.

What's Fine ✔️

  • All acceptance criteria satisfied: MASTERMasterProxy, FORK_SHAREDForkSharedProxy, REUSE_PORTReusePortProxy/ForkSharedProxy
  • All tests pass (3/3, 3 assertions)
  • No security issues: no leaked credentials, PII, injection vectors, or insecure crypto
  • match expression is exhaustive for current ProxyType enum values
  • Code follows the milestone spec and plan exactly

Comment thread tests/Server/Socket/SocketFactoryTest.php Outdated
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 2

# Severity Finding Status
MEDIUM Unused $port/$protocol params in create() Known decisionwatpliwosci.md #6, accepted by design

Previous Finding Resolved

  • ✅ Fallback test split into testCreateReusePortReturnsReusePortWhenSupported + testCreateReusePortFallsBackToForkSharedWhenNotSupported — each path now explicitly tested with markTestSkipped for the inverse condition.

Follow-up Observations

  • Missing class-level PHPDoc on SocketFactoryMasterProxy has one but the main entry point for proxy selection doesn't. Consider adding a short description of the fallback chain. (effort: low)

What's Fine ✅

  • All 4 acceptance criteria met — MASTERMasterProxy, FORK_SHAREDForkSharedProxy, REUSE_PORTReusePortProxy (supported) / ForkSharedProxy (unsupported)
  • 4 tests (3 pass + 1 skipped per platform), lint + CI green on PHP 8.2–8.5
  • No security issues, no secrets, no PII
  • Signature and fallback logic match the milestone doc exactly
  • match expression is exhaustive — PHP enforces all ProxyType cases handled

@s2x s2x merged commit 2d487ed 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.

M1I09: SocketFactory (fallback chain)

1 participant