The review my tests could not do
TetherPHP had just finished a hygiene pass. declare(strict_types=1) in every file, PHPStan clean at level 8, 51
tests passing, CI running on every push. By the numbers it looked like a healthy young codebase.
So I sat down and read both repositories properly, top to bottom, and found seven things that would have embarrassed me in front of the first person to actually use it.
The two that mattered most
Every HEAD and OPTIONS request returned a 500.
The router stores routes keyed by method, and only get() and post() existed to register them. So the table only
ever had two keys:
if (array_key_exists($request->uri, $this->routes[$request->method])) {
A HEAD request indexes $this->routes['HEAD'], which does not exist. PHP warns about the undefined key, hands back
null, and the next line dies:
array_key_exists(): Argument #2 ($array) must be of type array, null given
HEAD is not exotic. Browsers send it. Link checkers send it. Every uptime monitor I have ever configured sends it. OPTIONS is the CORS preflight, so a cross-origin API built on this framework would have failed before the real request was ever attempted.
Any URL with a query string returned a 404.
The kernel built the request from $_SERVER['REQUEST_URI'], which includes the query string. So the router was
asked to match /devlog?page=2 against a table containing /devlog, and quite correctly did not.
/devlog -> 200
/devlog?page=2 -> 404
Pagination. Filters. A UTM tag on a shared link. All 404.
Why nothing caught them
This is the part I keep turning over.
The tests were real tests. They covered route registration, group prefixing, static-versus-dynamic precedence, segment-count matching, the CSRF paths, the path helpers. They were also all unit tests, and both of these bugs live in the seam between two units that were individually correct. The router is fine if you hand it a method it knows about. The kernel is fine if you hand it a path.
PHPStan could not see them either, and that is not a criticism of PHPStan. $this->routes[$request->method] is a
perfectly well-typed expression; the analyser has no way to know the array only ever has two of the keys that string
could hold. REQUEST_URI is a string, and a string is what the constructor wants.
Neither tool was wrong. They were answering questions I had not thought to ask.
The rest of the list
| What | Effect |
|---|---|
CSRF token read only from $_POST |
PUT, PATCH and DELETE could never present a token, so they could never be authorised |
| Route parameters captured, never passed | The documented /docs/{page} syntax did not work |
make:feature generated a Responder with its only return commented out |
Every generated feature fataled on first request |
boilerplate:clear protected 'Action.txt' |
A stub filename that cannot match a .php glob, so it deleted app/Actions/Action.php — the base class every Action extends |
boilerplate:clear globbed app/**/*.php |
** is not recursive in PHP, so nested views survived |
| Sessions started with no cookie flags | No HttpOnly, no SameSite |
That fourth row is my favourite. The protected-files list was written with stub names in it, and the glob only
matches .php, so the one thing it was supposed to protect was the one thing guaranteed to be deleted.
What changed
All of it is fixed in v0.3.4. The router defaults to an empty table for verbs it does not know and answers HEAD
from the GET table, because HTTP defines HEAD as GET without a body. The kernel routes on parse_url($uri, PHP_URL_PATH). The CSRF check also reads X-CSRF-Token, which is how a request without a form body sends one. A
rejected write is now a 403 rather than an uncaught exception reported as a 500.
More usefully, each fix arrived with a test that fails against the old code. There is a fixture application inside the test suite now, so "what does this framework do with an OPTIONS request" is a question the suite can answer.
The actual lesson
I do not think the answer is "write more unit tests". The answer is that a web framework's real contract is request in, response out, and I had no test that exercised that contract. Every test operated one layer below it.
The uncomfortable part is that the green suite and the clean analyser made me more confident, not less. They were measuring the things I had already thought about. Reading the code was the only thing that surfaced the things I had not.