M1I03: Socket wrapper#58
Conversation
s2x
left a comment
There was a problem hiding this comment.
Dzięki za dodanie wrappera socketów. Kilka uwag — szczególnie getPeerName() wymaga poprawki przed mergem.
Ogólne: niespójna obsługa błędów
Klasa Socket konsekwentnie rzuca wyjątki przy operacjach na zamkniętym sockecie. Connection jest niespójna: read()/write() zwracają false, setOption() i getPeerName() nie robią nic. Dobrze byłoby ujednolicić na rzucanie wyjątków — wtedy caller zawsze wie, czego się spodziewać.
Brak testów dla Connection
78 linii kodu w Connection.php nie ma żadnego dedykowanego testu poza assertInstanceOf() i close() użytych w testAcceptReturnsConnection. Brakuje pokrycia dla read(), write(), getPeerName(), setOption(), getLastActivity() i ścieżek "po zamknięciu".
s2x
left a comment
There was a problem hiding this comment.
Fixes applied in 71e8fa2
All review comments have been addressed:
| # | Finding | Fix |
|---|---|---|
| 1 | getPeerName() missing $closed guard + ignored return value |
Added both: throws RuntimeException on closed socket or failed lookup |
| 2 | lastActivity updated on failed read()/write() |
Now only updates on success |
| 3 | Connection::setOption() ignores return value |
Now checks === false and throws RuntimeException with error message |
| 4 | Socket::setOption() ignores return value |
Now checks return value; kept SocketCreationException to match existing exception hierarchy |
| 5 | testCloseIsIdempotent has no assertion |
Added reflection-based null assertion after second close |
| 6 | testAcceptReturnsConnection uses raw PHP functions |
Changed to use $socket->setOption(), $socket->bind(), $socket->listen() API |
Summary
Acceptance criteria
Closes #5