1 September 2026¶
65 changes:
- The exit code a scheduler reads from
spool:drain - The rest of the Adminer route, and a log line read back rather than copied
- One broken job does not stop the
workpass - The refusal in
auth:unlock - …and the rest of the day, below
The exit code a scheduler reads from spool:drain¶
50 of 65 statements never executed, on a command the framework schedule runs every minute.
The writing is WriteSpool's subject and has its own tests. What nothing covered is the part a
scheduler actually consumes — the exit code — and the distinction this command makes is one
somebody has already paid for:
| nothing buffered | exit 0, and silent |
| rows written | exit 0, counted per table |
| a row kept for the next run | exit 0, reported as a comment |
| a row parked | exit 1 |
A kept row is the retry budget being spent, which is the spool working. Reported as a failure, the scheduler recorded one every minute until the budget ran out — and one deployment read "3 errors in 200 seconds" as three tasks failing when it was one task failing three times. A parked row is data set aside with no further attempt, so somebody has to look: that is the one worth raising on, and it is only a usable signal because the other case is not.
Silence on the empty case is the same argument from the other end: a line a minute saying "nothing"
is a log nobody reads, and a log nobody reads is where the one line that mattered goes unnoticed.
-v says it, for somebody who is watching on purpose.
Sixteen tests on both backends, including the two options doing what they claim: --status reports
the driver and the depth and writes nothing (it exists for draining before a migration, so it
has to be safe at the moment somebody can least afford a surprise write), and --max-attempts
reaches the spool rather than being parsed and dropped — an option that parses and does nothing is
worse than no option, because the operator believes they have changed the budget.
And two lines in the wrong order cost the whole point of the test¶
The test gives the spool a private directory so a drain here cannot write another test's rows.
WriteSpool::reset() clears the driver, the attempt budget and the directory — so setting the
directory and then resetting put it back to var/spool, and the first run drained the
installation's own buffer: 648 undrained tokenactions rows from other tests, written against a
schema that has no such table.
Which is the hazard the private directory exists to prevent, arriving through the order of two
lines. Reset first, then point it somewhere — and the failure was loud only because the assertion
was assertSame('', …) on an empty spool. A looser assertion would have passed while draining
somebody else's data.
The rest of the Adminer route, and a log line read back rather than copied¶
The gate and the rewriting were covered in the last two rounds; what was left is what the route puts on the page and what it writes down.
The chrome. position: fixed rather than sticky, because Adminer's pages scroll sideways
whenever a table has many columns and sticky pins only the vertical axis — taking the Back link
off the screen exactly when somebody is lost in a wide table. And anything without a <body> is
returned untouched: Adminer answers redirects, downloads and JSON fragments, and injecting a div
and a stylesheet into one of those corrupts it. The site name is operator-typed text and is
escaped; the test puts <script> in it and checks.
The audit line. It is the only trace a refusal leaves, since the page says nothing on purpose, so it has to answer who, from where, for what — a run of refusals from one address is the shape of somebody trying the door, and that pattern is invisible if the line says only "refused".
Worth a note on how that one is tested. audit() composes and logs in a single step, so asserting
what it says means reading what it wrote — and the temptation is to reproduce the composition in
the test, which asserts the copy. Logger has a stream mode, so the test points it at
php://memory and reads the real line back. No log file touched, and no second copy of the format
to drift.
Suite: 13,402 → 13,409 tests, wall clock 2:54 on a busy host.
One broken job does not stop the work pass¶
59 of 84 statements never executed in Console\Commands\Work — the process an installation
without cron runs instead of a crontab, and the whole argument for having it is the property
nothing was checking.
On such an installation this is the only thing running the background. A pass that abandoned itself on the first exception would stop the buffered writes, the queue and the cleanup along with the broken job, and keep stopping them every minute for ever, on the strength of one bug in one task. So the pass reports, counts, and steps over.
Three outcomes, and only one is a failure:
✓ ran |
timed, and logged with its duration |
↷ declined |
the no-overlap lock is held by the previous minute's copy — the mechanism working |
✗ raised |
counted, with the message, and written to the schedule channel |
The middle one is the subtle one, and it is asserted by taking the lock before the pass runs:
counted as a failure, a job that legitimately takes three minutes would report two failures for
every success — and that count is what --once hands a cron line as its exit code.
Also asserted: a failure is logged, not only printed. work runs under systemd or as a
container command, where nothing is attached to the output.
Suite: 13,409 → 13,415 tests, wall clock 2:22.
The refusal in auth:unlock¶
78 of 138 statements never executed, on a command whose most important line is a refusal.
It exists for the developer who has mistyped a fixture password three times and cannot test the login flow they are working on — which makes it, by construction, a command that weakens a brute-force defence. So the assertions worth having are the limits, not the feature:
--allrefuses outside development, and the refusal names the narrow alternative. "Clear every lockout on this server" is what somebody working through a password list would ask for, and a refusal with no next step is a refusal somebody works around.- A scope narrows it. The same value can be locked twice —
10.0.0.5as an identifier and as an IP — and--scope=ipclears one. Clearing more than was asked is the--allmistake arriving quietly. - An unknown scope is refused with the valid ones named, because a typo that silently unlocked nothing reads as that account was not locked.
- An expired lockout is not listed, though its row stays: the row is the failure history the progressive backoff counts, and listing it would send somebody to unlock an account that is already usable.
Both backends, and here that is not a formality: authserver.loginlockouts is created by
hand-written DDL per engine in its migration, and lockoutuntil is TIMESTAMPTZ on
PostgreSQL against DATETIME on MySQL. Is this one expired and how much longer are claims
about two different comparisons, and the fixture has to write the timestamp in the form each
engine will read back.
Suite: 13,415 → 13,435 tests, wall clock 2:17.
A test ping that went to everybody¶
The webhook controller was at 46.5% — 84 of 157 statements — and it already had a thorough unit test. Eleven cases: https required, an unusable URL refused, an unknown event type named with the alternatives, credentials demanded, a GET refused, somebody else's endpoint a 404. All of it with doubles, and all of it correct.
Everything it covered sits before the first query. The half that was never executed was
everything after ->table(...): the registration upsert, the listing, the ownership WHERE, the
stats() aggregate over a join. So the new tests are the ones a double cannot express, and the
existing file keeps the validation — there is nothing to prove twice.
One of the doubles was hiding a fault. The unit test stubs the delivery service:
public function queueEvent(string $eventType, ?int $userId, array $payload, ...): int
{
$this->controller->queued[] = $eventType; // a test event was queued. Which one?
return 1;
}
It records that an event was queued, which is the whole of what the seam can see. Against the
real queue, POST /Webhook/test on your own endpoint queued an event for every endpoint
subscribed to that event type, including other applications'. queueEvent() fans out by
design — token_revoked concerns every application holding a token for that user — and test()
called it like a real event.
So any client could cause traffic against another client's URL by subscribing to the same type. The
payload is {"test": true} with no user attached, so nothing about a person leaked; what leaked was
the ability to make somebody else's server receive a signed POST on demand.
queueEvent() takes an optional endpoint id now, and test() passes it. Additive — every existing
caller keeps the fan-out — and the id is still matched against the event type and is_active, so
naming an endpoint that does not subscribe queues nothing rather than the wrong thing.
The test that found it reads the queue rather than a spy:
$this->assertSame(1, $this->queuedFor($mine));
$this->assertSame(0, $this->queuedFor($theirs),
"a test ping for one application's endpoint was fanned out to another's");
Both backends, and one assertion only the PostgreSQL lane can fail: webhook_type carries a
CHECK constraint listing the valid event types, written in the migration, while the controller
advertises its own list from a constant in supported_types. Two files, no link between them. The
test registers every type the controller advertises and asserts the database accepts it — on MySQL
there is no constraint to disagree with, so the MySQL run cannot fail it, which is the whole reason
the second lane exists. Registering the eight advertised types is also what proves the upsert: it
compiles to ON CONFLICT on one engine and a read-then-branch on the other, against a unique key
where a second insert is a constraint violation rather than a duplicate row.
The guide's event-type list was also short by one — permissions_changed has been deliverable
since 26 August and the section documenting it did not list it as a type you could subscribe to.
Suite: 13,435 → 13,451 tests, wall clock 2:16.
The two mails nobody had read¶
NewSignInNotification at 49%, SecurityChangeNotification at 54%. Both had tests — via(),
toPush(), the account resolution — and in both the uncovered half was toMail(): the longest
method in either class, and the only part a person ever sees.
No bug in the copy. What the tests pin down is the set of things these messages must not contain, which is most of their design and all of what a helpful change would undo:
- No link. Both arrive unbidden and describe what may be an intrusion. A "review this
sign-in" button is the phishing mail the message warns about, only larger and easier to press.
Both say to open the site directly, and both assertions are for the phrase as well as the
absence of
<aandhttp. - No IP address in the sign-in alert. Asserted as a regular expression rather than a literal, because the failure worth catching is somebody adding an address, not this particular one.
- Nothing the caller composed, unescaped.
detailon a security change is a passkey name the account holder typed.
One assertion I wrote and deleted: that the constant never appears in the copy. PASSWORD is
'password', and "change your password" is a sentence the mail is entitled to write. It survives
only for the unknown-kind case, where the constant is a snake_case token and its appearance in the
body would mean the match fell through to default and printed the argument at the reader.
The interesting find was in the guide, not the code. The alerts section still said "Mail only",
with the reason: a database notification would put a security warning in the panel of the session
that triggered it, which in the case worth warning about is the wrong person. That reason is still
right and the sentence is no longer true — via() has added push since, and push passes the same
test for the same reason: a browser is subscribed only because somebody granted permission in it
earlier, so the subscriptions on an account are the owner's devices, not the one the sign-in just
happened on. The section now says so, and names the unsubscribe list as well: this is the one
notification the framework sends with one, because it is the one that belongs to a list.
Suite: 13,451 → 13,468 tests. The 17 new tests are unit tests and cost 0.7s; the full-suite wall clock measured between 2:16 and 2:48 across three runs of the same commit today, which is host load rather than anything in the suite — worth writing down so the next entry's figure is not read as a regression.
The loop in php pramnos work¶
56% — and the covered half was the pass. runDuePass() had a round of its own earlier: one due
task, and what happens to the others when it raises. execute(), the loop around it, had never
run, and that is where the process's promises live.
Four of them, none previously executed, each one the kind of decision that reads as an oversight until you ask what the alternative does:
--once does not take the worker lock. It is the cron equivalent. A crontab line that refused
to run because a work daemon holds the lock would be a cron line that silently does nothing,
once a minute, for ever. The assertion is the whole lifecycle:
A second worker refuses to start, because two workers run every job twice — and
withoutOverlapping() on the individual tasks narrows that rather than preventing it, since most
tasks do not set it.
A lock taken over stops the loop. What a deploy looks like from inside: the replacement started before this one was told to stop.
--max-runtime exits 0. A supervisor restarts on exit, and a planned recycle reporting non-zero
becomes a crash in whatever reads the unit's history.
Plus the finally: runDuePass() catches everything a task can raise, so what escapes comes from
the loop's own machinery — a heartbeat against a database that has gone away. Without the finally
that leaves a lock nothing holds, and the next worker refuses to start until somebody deletes it by
hand.
Two of the eleven tests cost real time and are meant to. sleepUnlessStopping() exists so a SIGTERM
is noticed within a second rather than at the end of the interval, and the early return is only half
that claim — a version that never slept at all would satisfy it and turn the loop into a spin. So
one test times a stopping worker on a 60-second interval and requires it back inside a second, and
the other times a running worker on a one-second interval and requires it to have actually waited.
That second one costs the suite a second, once.
The systemd() seam records the call and then returns the real notifier. With no NOTIFY_SOCKET
in the environment — every environment but a systemd unit — ready() has nothing to write to and
says so by returning false. A double would only have hidden that, and ready() returning bool
rather than void is precisely the sort of thing a hand-written double gets wrong: mine did, and the
suite refused to load until it matched.
Guide: the three lock decisions and the interval clamp are written down now, including the cost of
--once skipping the lock — run a crontab line and a work daemon on the same installation and a
task without withoutOverlapping() can run twice. Pick one shape.
Suite: 13,468 → 13,479 tests, wall clock 2:24.
Two TTLs, and the one nobody was reading¶
AbstractAdapter at 57% — 45 of 105 statements — and the uncovered half was the entire
Redis-shaped API: hashSet(), listPush(), listTrim(), increment(), swap(), expire().
Non-atomic defaults that keep the whole structure under one key through load()/save(), which
RedisAdapter overrides with native commands. Two implementations of one contract, application
code written against whichever adapter the developer runs locally, and neither half ever compared
with the other.
They disagreed, in both directions, and both faults were live on the File adapter.
A permanent hash was readable for one hour. hashGet() and listRange() read with load()'s
default $timeout of 3600 — meaning nothing by it, they just did not pass one — and
FileAdapter::load() treated the reader's argument as the entry's expiry. So a structure saved
with no TTL disappeared an hour after it was written.
A one-second counter never expired. counter() reads with $timeout = 0, 0 > 0 is false,
and so nothing was checked at all. On the File adapter a rate-limit window never closed. That is
the direction that matters, because a rate limiter is the caller this API has.
FileAdapter::save() had been recording the TTL all along — getRemainingTtl() reads it, which is
how the cache browser shows an expiry — and load() ignored it. It is authoritative now, with the
reader's $timeout keeping the meaning Cache::load($id, $category, $timeout) has always given
it: an additional maximum age this reader will accept. Either limit exceeded expires the entry,
and the structured reads pass 0 because they have no opinion about age.
Then a second finding, from an assertion I wrote for the wrong reason. I asserted that
generateKey() strips a slash, expecting it to pass. It did not:
The prefix, the category and the extension are sanitised. The id is not — and neither is it at
the Cache level, where _generateCacheName() interpolates it as given. FileAdapter then
concatenated the key onto the directory path unchanged. So $cache->load('user_' . $x) with an x
from a request put whatever that was into a path: a/b writes silently outside its own category
directory, where clear('a') will never find it again, and ../../x writes outside the cache
directory altogether.
Fixed in the adapter rather than the key builder, which is where the class of bug actually lives:
this is the only place a key becomes a path, and on Redis a slash is just a character. Every
operation — save(), load(), delete(), getRemainingTtl() — resolves through getFilePath(),
so they all agree on the resulting filename.
The third thing the tests found was mine. swap() on Redis is GETSET, which stores the value
verbatim, while save() writes a {data,time} envelope — so a value written by swap() is not
readable by load(), and the guide says so: the counters and swap() are the raw-key family.
My test read a swapped value back with load() and passed on the File adapter, whose fallback goes
through save(). The permissive one is the trap: code that mixes the families works locally and
returns nothing on Redis. The assertion now stays inside the family and the docblock says why.
Every adapter rather than every backend — this is the cache layer, and the equivalent of covering four SQL backends is covering the three stores that implement the contract. The Redis rows skip when no server is reachable.
One note on cost, because the first version was not free: three TTL claims × three adapters ×
sleep(2) was 12.8 seconds. They all need the same clock to move, so they share one wait now —
twelve assertions for two seconds instead of twelve. A clock seam in three adapters would buy those
back and would also mean the thing under test is no longer the clock.
Two existing tests failed on the first full run, and were right to. Both backdate a cache file's
mtime to simulate expiry, and my version measured age from the time recorded inside the
entry — so touching the file no longer aged it. mtime is the better choice anyway: it is what
getRemainingTtl() and therefore the sampled cleanup() already measure from, so a sweep and a
read now agree about which entries are stale.
Suite: 13,479 → 13,511 tests, wall clock 2:24.
A test that asserted nothing, in the panel that shows codes¶
AuthCollector at 60.5%, and the uncovered 64 statements were all of twoFactor() — the factors
an account holds, the floors, the pending step-up, and the two reads behind
debug.reveal_factor_codes. Every line of it is a query, so with no database the method answers
['error' => …], because a panel that raises takes the page with it.
Which is what the existing unit test was asserting against:
public function testCodesAreNotRevealedByDefault(): void
{
$_SESSION['loginflow_pending_userid'] = 4242;
$data = (new AuthCollector())->collect();
$this->assertArrayNotHasKey('revealed', (array) $data['twofactor']);
}
LoginFlow::pending() reads loginflow_pending_time as the moment the step-up started and treats
a missing one as 0, so a session carrying only the user id is an expired step-up:
pendingUserId() answers null, twoFactor() returns null, (array) null is [], and an empty
array has no revealed key. The test passed against nothing at all — and it is the test standing
between a deployed installation and live six-digit codes in its network log.
The real assertion needs a database, and it is worth being exact about:
foreach ([null, false, 0, '', '0', 1, '1', 'true', 'yes'] as $value) {
$application->applicationInfo['debug'] = ['reveal_factor_codes' => $value];
$this->assertArrayNotHasKey('revealed', …);
}
'1', 1 and 'true' are what a flag looks like when it arrives from an environment variable or
a hand-edited config. All three are truthy, and the comparison is === true, so all three leave it
off. That is the line worth pinning: a loose comparison here means an installation that never asked
for live credentials gets them because somebody typed a string.
The rest is what the panel shows when it is on — a decrypted enrolment secret with a code that verifies against it, the live factor preferred over a half-finished one, and the mailed code taken from the newest mail that has one. Newest-first is not a detail: a code from last week is worse than no code, because somebody will type it and then look for the bug somewhere else. A newer mail with none in it — a password-change confirmation arriving after the step-up mail — must not hide an older one that has.
Three things went wrong in the writing, and each was mine rather than the framework's.
An unmarked value is not a failed decryption. I stored a plausible base64 blob and asserted the
panel hid it; it came back verbatim, because Encrypter::maybeDecrypt() returns an unmarked string
unchanged so that rows written before encryption came in still work. The scenario the docblock
actually names is a key rotation, so the test now encrypts properly and then changes APP_KEY.
User::load() reads through a cache. ->get(true, 10, 'userlist') — ten seconds, in a category
Model flushes on every write. A test writing usertype with the query builder tells neither that
cache nor the process-wide static, so the account came back with the usertype a previous test had
given it, and the floor assertions passed or failed depending on how fast the suite ran. One helper
does the update, the cacheflush('userlist') and the clearUserCache() together.
And a find-and-replace wrote $this->stepUpInFlight() into the body of stepUpInFlight(),
which is a stack overflow: PHPUnit printed its header and the process vanished with no output and
no failure. Worth writing down because the symptom looks like the runner breaking rather than the
test.
Both backends: authserver.twofactor_setup is a schema on one engine and a prefix on the other,
and the mail lookup is a LOWER(tomail) = ? with an ORDER BY and a LIMIT.
An intermittent run worth writing down rather than shrugging at¶
Twice today the first full run after a new test file appeared reported errors — four the first time, thirteen the second — and every run after it was clean. Three consecutive green runs follow this one, with identical assertion counts.
I do not know what those runs failed on: both times the error list scrolled past before I captured it, which is the actual mistake here. The pattern that the failing run is always the first after a new file points at state surviving between runs — the test databases and the file cache both do — but that is a hypothesis, not a finding, and nothing below it should be read as fixed. For the rest of this work the first run after a new file goes to a log file.
Suite: 13,511 → 13,536 tests, wall clock 2:24.
The API login, from the password to a token that works¶
ApiAccount at 61.7%. It has thorough unit tests — a non-POST refused, missing credentials, a
wrong password, a lockout, the second-factor step, the shape of the response — and every one of
them replaces verifyCredentials() and issueToken() with a double. Right for what they assert,
and it means the chain those two sit in had never run: 44 of 115 statements.
That chain is the endpoint's entire purpose. A password goes in, and what comes back has to be a
credential the API accepts on the next request — three separate pieces of work (JWT::encode(),
User::addToken(), and the response body) that agree on nothing unless somebody checks. So the new
test signs in with a real password, against a real users row, through the real login flow, and then
presents the token the way a client would:
$loaded = new User();
$this->assertNotFalse($loaded->loadByToken($token, 'auth', false),
'the token this endpoint just issued does not load its user');
The expiry is the part with two answers, and the reason this is worth an integration test rather
than another double. A TTL stamps the JWT's exp claim and the usertokens.expires column, and
they are enforced by different code — JWT::decode() for the first, loadByToken() for the second.
Stamping one and not the other leaves a token that is dead by one route and alive by the other, and
which of the two an installation notices depends on how it authenticates. The test reads both and
asserts they are the same number.
Around that: no expiry at all when auth.token_ttl is unset, because giving existing installations
one silently would sign every client out on upgrade day; a negative TTL read as no expiry rather
than as an already-dead token; nbf backdated twelve hours, which is deliberate tolerance for a
client clock behind the server's and looks like a bug until you have debugged one; and a 500 with
no token row when the installation cannot sign, since a row with no token is a row nobody can
revoke.
One assertion is about what is not there. userPayload() is built from a fully loaded User,
which holds every column of the row, the password hash among them:
$this->assertSame(['id', 'username', 'email'], array_keys($user),
'the profile grew a field; check what else is on a loaded User');
Naming three fields rather than serialising the object is the whole of the protection.
Writing it turned up one thing worth knowing: tokenTtl() reads $this->application, not
Application::currentInstance(). A controller constructed without an application gets a TTL of 0
— the never-expires branch — silently. The router always supplies one, so this is not a fault; it
is a test that would otherwise have asserted against the wrong branch and passed.
Guide: the endpoint, both its failure answers, auth.token_ttl and why expiry is opt-in, the two
places a TTL lands, and the twelve-hour nbf.
Suite: 13,536 → 13,556 tests, wall clock 2:23. The first full run after the new files was clean this time, which weakens the guess above about first runs rather than confirming it.
The menu's permission check, and the limit that is not a ladder¶
Two files whose uncovered halves were both the branch that runs on a real installation.
NavRegistry asks the application's own hasPermission() first, and its unit tests cover
that path well. The other branch — the item's permission name going to the framework's own
Pramnos\Auth\Permissions store — had never run in a test, and it is the branch that matters,
because of what stood there before it: a check that consulted an optional PermissionEngine
addon which exists nowhere, found it absent, and carried on. Every declared permission was
skipped on every installation, and every item was shown to every signed-in user.
So the three answers are now asserted against the real store, on both backends: a deny hides the
item, an allow shows it, and no rule at all shows it. That third one is what makes the check
safe to have switched on — an application that declares permission names and has granted none of
them would otherwise have emptied every menu on upgrade. Beside them, two failure modes that must
also be "no opinion": an application scheme whose hasPermission() raises, and a permission-gated
item asked about by something that is not an account.
Loginlockout::recordFailedAttemptWithin() was 43 of the class's 130 statements, and all of
the per-address limiter. The account ladder beside it is thoroughly tested; this is a different
method answering a different question, and the difference is the design:
- No ladder. An address is not an account — it can be a shared office NAT, and locking one out for a day over somebody else's typing is a denial of service delivered by the security feature.
- The deadline belongs to the window the first failure opened. Refusing until
now + windowon every attempt would let a slow attacker hold an address refused indefinitely, punishing everyone behind it while they move on to the next of their thousand addresses. max($now + 1, …), so a threshold reached on the last second of a window still refuses. Not a rounding detail: without it the one attempt that crossed the threshold is the one that goes unpunished.- A threshold or window below
1disables the limiter rather than tightening it, becauseattempts >= 0is true of every attempt and a mistyped0would refuse the first request from every address on the site.
Both on both backends, and here that is not a formality: every claim is about a timestamp written
and read back through strtotime(), and the column is TIMESTAMPTZ on one engine against
DATETIME on the other — the difference that once made lockoutuntil land in the past on a
non-UTC host, so the lockout never engaged at all.
Suite: 13,556 → 13,588 tests, wall clock 2:22.
The intermittent failure had a cause, and it was a migration¶
I have been reporting an intermittent run for most of today — four errors once, thirteen another time, clean on either side, and both times the list scrolled past before I captured it. It is captured now, and it turned out to be two faults of the same shape: shared state deciding a test's outcome by ordering.
The one that was mine to find¶
ForeignKeyGuardMigrationTest runs the whole AddMissingForeignKeysToExistingTables migration to
assert one guard, and it failed with:
ERROR: insert or update on table "user_activity_log" violates foreign key constraint
"fk_user_activity_log_userid"
DETAIL: Key (userid)=(2) is not present in table "users".
ALTER TABLE … ADD CONSTRAINT validates every existing row, so one audit row belonging to a user
somebody had since deleted aborted the statement — and with it the whole batch. Whether such a row
existed when this class ran depended on which other test had gone first.
The migration's guard already asked three questions before adding a key: does the child column exist, does the referenced table exist, does the referenced column exist. All three are about shape. Nothing asked whether the data satisfied the constraint, and that is the same question — the file's own docblock says the migration "must not assume the schema of tables it does not own", and the data is part of what it does not own.
It is also exactly backwards without the check. A database that has run for years without these
keys is precisely where a deleted user can have left an audit row behind; a fresh one has nothing
to orphan. So the migration failed on the installations it was written for and succeeded on the
ones that did not need it — reported, every later migrate, as a raw constraint error naming a
table the operator never touched.
canAddForeignKey() counts the orphans now and skips with a line saying which constraint, how
many rows, and that migrate will add it once they are dealt with. Skipped rather than
repaired: deleting rows is not a migration's decision to take on an operator's behalf, and an
orphaned user_activity_log row is an audit record — a trail losing entries because an upgrade
tidied up is worse than a missing constraint. A NULL child value is not an orphan; a nullable
foreign key means "no parent".
The one that was hiding a security limit¶
The other, found while writing the tests below: authserver_user_activity_log had two owners with
CREATE TABLE IF NOT EXISTS, and one of them — the Account characterization test — declared it
without a details column, because the screen it tests does not select one. Whichever ran first
decided the shape for the whole run.
When it was that one, on MySQL:
- every
ActivityLog::record()insert failed on the unknown column and was swallowed by the logger's own catch, so the audit log recorded nothing; EmailSecondFactor::recentSends()selectsdetailsto filter by purpose, so it threw, was caught, and returned "nothing recent" — which means the resend limit on emailed sign-in codes silently did not apply.
Both failures are the designed-in behaviour of code that is right to be forgiving: an audit logger must not break the request it is auditing, and a rate limit must not refuse every code because a log table is missing. Forgiving code needs the fixture to be exact, and the fixture was not. It is built from the migration now.
This is the second time today the same shape has cost an hour — tokenactions had four owners with
different shapes earlier. The rule is worth stating plainly: a test that needs a framework table
builds it from that table's migration. Hand-rolled DDL with IF NOT EXISTS is a shape that wins
a race, and the loser is whatever ran second.
Turning sign-in codes by email on and off¶
Account::emailfactor() — 59 statements, the largest unexecuted method in the account screen, and
the reason the two faults above surfaced today. {@see EmailSecondFactorTest} covers the service
underneath it; this is the screen, and screens are where the decisions about who may change what
live.
Three of them, each the sort that is only ever wrong once:
- Turning the factor off asks for the password. A second factor a stolen session can remove is not a second factor. Everything else on that screen protects the account; this is the one action that weakens it, so it re-authenticates.
- Nothing happens on a GET, or without the anti-CSRF token. A link in an email that turns off somebody's second factor is the whole reason the token exists.
- A refused send says which refusal it was. The rate limit and a broken mailer are the same
falsefromsend(), and the message used to be "we could not email you a code — check the address on your profile". Somebody told that presses the button again, sees it again, and concludes nothing is being sent, while the code is already in their inbox.
Both backends, and the enrolment and removal go through an upsert that compiles differently on each.
One note on cost. The store keeps an HMAC of the code and nothing else — by design, and the reason
it cannot be read back — so the first version of the "a correct code enrols" test searched all
million candidates. Honest, and a second of CPU on each lane for a fact the test does not need:
which six digits were generated is the service's subject and has its own tests. The hash of a
code this test chooses is written over the one that was sent, and verify() runs for real against
it. 4.1s to 1.5s.
Suite: 13,588 → 13,609 tests, wall clock 2:25, and two consecutive runs with identical assertion counts — which is the first thing today I would call evidence rather than absence.
The two screens that let somebody back in¶
forgotpassword() and resetpassword() — 43 statements between them, none executed. The token
below them is thoroughly covered: only its hash stored, resolving does not spend it, issuing again
invalidates the last, an expired one refused. The actions on top are where the decisions live, and
they had never run.
The one this is really about is anti-enumeration, and it is asserted by comparing rather than by reading:
$this->assertSame($known->rendered, $unknown->rendered,
'the answer differs, so the form says which addresses have accounts');
Reading one answer and checking it says "sent" would pass just as well against a version that answers differently for an address it recognises. The property is that the two are identical, so that is the assertion — and the difference that must exist is on the inside: a token written for the account that exists, nothing at all for the one that does not.
Then the refusals, and two of them are the same shape in opposite directions. A reset POST with no anti-CSRF token must change nothing and leave the link usable — the obvious failure is the reset going through, the quiet one is the token being spent on the way to refusing, which leaves the account holder with a link that stopped working and no explanation. Same for a password the policy rejects: the token is resolved before the policy is checked and cleared only on success, so one mistyped confirmation does not cost a second trip through the mailbox. Both now have a test, and the second one also checks the link still works afterwards rather than only that the row is still there.
The round trip is one test rather than two halves: the link is obtained by running the forgot screen and reading what it mailed, then handed to the reset screen. A fixture that wrote its own token would quietly replace the half of the boundary most likely to be wrong — a token issued in one shape and looked up in another passes every test on either side of it and none across.
The render methods are replaced by a recorder. What a theme does with a context is its business; what puts a value in that context is the subject, and a real render would need a view stack to assert one string.
Guide: the forgot form's identical answer and the two refusals before it, and a table of which refusals spend the link — the failure mode of getting those wrong being entirely invisible from the outside.
Suite: 13,609 → 13,637 tests, wall clock 2:23.
Changing a password, and what it does to the other browsers¶
Account::changepassword() — 18 statements, every one of them a refusal or a consequence. The two
worth the class are at opposite ends of the method.
The current password is asked for. A session is a bearer credential living in a browser somebody may have walked away from, and a password change is how an account is taken over permanently. This is the step that costs an attacker something they have to already know — the same re-authentication the second-factor screen does, for the same reason.
The other sessions end and this one does not. Both halves fail differently, so both are asserted:
$this->assertSame(1, $this->logoutFlag($theirs), 'the other device was left signed in');
$this->assertSame(0, $this->logoutFlag($mine), 'the change signed the owner out of this browser');
People change a password because they think somebody else has it. Leaving the others alive means the other person keeps the account while the owner believes they have just taken it back — worse than not offering the change, because it manufactures confidence. Signing this browser out is the opposite failure: it reads as the change not having worked, and the person cannot tell whether to try again. It is off by default, because for an application that treats a password change as routine hygiene, signing every device out is a support call.
Between them, the history, and the assertion is about ordering rather than about the feature:
$this->assertTrue($history->wasUsedBefore($this->uid, self::OLD));
$this->assertFalse($history->wasUsedBefore($this->uid, self::NEW),
'the password the account now has was remembered as a previous one');
remember() is given the hash the account is moving away from, and it is read before the
write for that reason. Remembering the new one would refuse the password the account has right now
on the next change, which reads as the history being broken.
One thing this cost me, twice today: ActivityLog::record() asks FeatureRegistry, which is
loaded from configuration rather than read off applicationInfo. Declaring the feature on the
application alone leaves the log a silent no-op — so a test asserting that something was recorded
is asserting that nothing was, and it passes on the day the assertion is wrong.
And a third fixture caught by the same net¶
The first full run after these failed four times in AccountExportTest: Field 'visitorid'
doesn't have a default value. Its sessions insert named neither visitorid nor history, both
NOT NULL with no default in the shipped table — so it had been depending on some other test
building sessions with a laxer shape first, and broke the moment one built it from the migration
instead.
Third time today. The pattern is now unmistakable: a fixture that omits a NOT NULL column, or declares a table without one, works for exactly as long as the run order keeps a permissive shape in front of it. Naming every NOT NULL column is not defensive verbosity — it is the difference between a test that passes and a test that passes because of something else.
Suite: 13,637 → 13,655 tests, wall clock 2:26, two runs with identical assertion counts.
Registration: what a form is allowed to decide about itself¶
Account::register() — 17 statements, the one screen that creates an account, and none of it had
run. Most of the tests are the refusals in front of a public write that inserts a row and sends a
mail: the setting that keeps it closed by default, the anti-CSRF token, the human check, the field
validation, the password policy, the two "already taken" cases.
The one that is not about spam is this:
$this->postWithToken($this->submission($username) + [
'usertype' => '90', 'validated' => '1', 'userid' => '1',
]);
…
$this->assertSame(0, (int) $row['usertype'], 'a registration form granted itself a usertype');
createUser() names the five fields it sets and usertype is not among them, so a submission
carrying one is ignored. That is the entire protection, and it is exactly the kind of line somebody
later "generalises" into a loop over the submitted fields — at which point a public form can hand
out administrator accounts and every existing test still passes.
Beside it, one about what comes back: a refused submission is echoed with the username and the address filled in, because retyping an address after a failed check is how people give up, and never with the password, because the form is rendered into HTML that ends up in a browser's history, a proxy log and the occasional bug report screenshot. Asserted by looking for the password in the whole rendered context rather than in the field it would most obviously be in.
One PHPUnit detail worth knowing, since it cost a run: a private helper called name() in a test
class is a fatal error — TestCase::name() is final. The message is clear once you see it, and the
suite refuses to load rather than failing a test, so a filtered run shows a stack trace with no
test names in it at all.
Suite: 13,655 → 13,679 tests, wall clock 2:26.
Two screens between a person and their session¶
Account::verify() — the step-up between the password and the session, 13 statements. A unit
test this time, deliberately: everything it decides comes from $_POST and the answer of a
LoginFlow, and the flow is the part that talks to a database and already has its own tests on
both backends. Doubling it leaves the branching, which is all that was uncovered, and spares a
class that would learn nothing from a connection.
Three branches carry weight:
- Sending a code is a POST. A GET that sends mail is one a crawler, a link preview or a back button can fire, and each firing invalidates the code already in the person's hand — which presents as codes that never work. There is a test that a GET carrying the send parameters still sends nothing, because that is the case a link would actually look like.
- The factor is named by the form. Every factor's code is six digits, so trying each in turn would spend one attempt of every other factor per press. Mistyping an emailed code three times would lock the authenticator too, and the person would be told their app was wrong. Asserted as "exactly one completion attempt, against exactly the named factor".
- A wrong code keeps the half-login. Dropping it sends somebody back to the password form for a typo, and the second password attempt looks to the lockout like a new sign-in — so three typos on a code become three failed logins.
Plus the older send_email_code field, still accepted because a form rendered before the
factor-by-name change is sitting in somebody's browser while the deploy happens; refusing it would
break their next press with no explanation and no way to know a reload was the fix.
TokensController::view() — 32 of that class's 42 uncovered statements, and the one method in
it with no test at all. Its siblings all have one. This is the page somebody opens when an
integration misbehaves, and the lines worth pinning are not about what it shows:
- a
limitis clamped, not obeyed. It arrives in a query string, a busy integration's token has tens of thousands of actions, and?limit=100000on an admin page either times out or exhausts memory — while the person who typed it was debugging something else; - a
pagebelow one is floored, because($page - 1) * $limitis otherwise a negative offset: PostgreSQL refuses it outright and MySQL is more forgiving, so without the floor the page works on one engine and 500s on the other. That is the assertion the second lane exists for here; - a token that is not there is said so. An operator following a link out of an old ticket gets a sentence and the list, not a page of empty labels that reads as the token existing and having done nothing.
The view is recorded through a __set() rather than a list of declared properties, so the test
does not have to be updated every time the page shows one more thing — and a property the method
stops assigning shows up as a missing key rather than as a stale value.
Suite: 13,679 → 13,714 tests, wall clock 2:27. Total coverage 91.49% → 91.79% over the day's work
so far; Account.php alone went 74.4% → 86.6%.
Where a class's design lives in its catch blocks¶
Email\Unsubscribe at 67.7%, and every one of the 50 uncovered statements was a catch block or
an empty-input guard. The happy path was thoroughly covered — the suppression row, the all list,
the case-insensitive match, the repeat, the undo, the consent trail. None of the failure paths had
ever run.
That is not a gap in the corners. This class's documented behaviour is its failure behaviour, and one branch is the opposite of how the rest of the framework fails:
Answers true when it cannot tell. Sending to somebody who unsubscribed is the one mistake a mailbox provider counts against every future message, including the transactional mail this method is never asked about.
isOptedOut() fails closed, and nothing had executed that branch — so nothing would have
noticed a later change making it fail open. The symptom of such a change is not an error anywhere:
it is mail going to people who asked us to stop, during an outage, followed by a deliverability
problem that outlives the outage by months.
The other catches are the mirror image, and each now has a test: a missing consent table, an
application handler that raises, a preference that cannot be written — none of them may stop the
unsubscribe, because the suppression row is written first and is what decides delivery. And the one
failure that is reported: when the row itself cannot be written, optOut() returns false. An
endpoint answering "done" while writing nothing leaves a person unsubscribing again, and again,
from mail that keeps arriving.
Writing the empty-address tests turned up a small inconsistency worth fixing. optOut(),
optIn() and isOptedOut() all refuse a blank address; token() was the one entry point that did
not — it signed one:
That token verifies. So the endpoint reads ['email' => '', 'list' => 'all'] out of it, calls
optOut(''), is refused, and shows the reader a failure for a link the framework generated. It
returns an empty string now, which url() and mailto() already turn into an omitted header —
the honest outcome: no address, no unsubscribe link.
The PostgreSQL lane is new here. The existing record test declares itself MySQL only
(markTestSkipped('… runs on MySQL only.')), so this service had never been exercised on the other
engine at all — and pramnos.emailoptouts is a real schema on one and a table prefix on the other.
One fixture note, learnt the hard way earlier today: several of these tests drop the opt-out table on purpose, because that is how an outage is arranged. The teardown rebuilds it rather than just deleting rows — a teardown that only cleaned would hand the next class the very failure this file is about, arriving where nobody asked for it.
And one more piece of static state, caught only by the full run. Settings::$database is a static
handle set once, so in a suite run it is whatever an earlier class left there: the PostgreSQL lane
migrated a settings table on PostgreSQL and then watched the write go to MySQL, reporting a
missing table with a MySQL error message. It passed under --filter and failed in the suite,
which is the signature of static state — and the reason a filtered green is never the last word.
Suite: 13,714 → 13,736 tests, wall clock 2:28.
The refusal that only exists in the log¶
/adminer answers a refused request with the site's own 404, and that is the design: a refusal has
to be indistinguishable from an address that does not exist. «Not found» in Courier is a page
nothing else on the site produces — it tells whoever is looking that something is here and that
they were turned away, which is exactly the thing not to say.
Which leaves the log as the only place a refusal exists at all. So the log line is not bookkeeping, it is the feature — a run of them from one address is the shape of somebody trying the door, and nothing else in the system would show it. It is written before the 404 for that reason, and none of it had ever been executed.
The tests read the log rather than the response, because the response is deliberately empty:
$this->assertStringContainsString('Adminer refused', $this->logged());
$this->assertSame(1, $probe->notFounds);
$this->assertSame([], $probe->served, 'a refused request reached Adminer anyway');
The second 404 — no Adminer package installed — is the same page and a different log line, and the test asserts it is not recorded as a refusal. "There is no package here" and "you are not allowed here" are different problems with different fixes, and the person reading is usually an administrator who cannot tell which one they have from the page.
The auto-login switch gets the treatment a hand-written config value deserves: false, 'false',
'0' and 0 all leave the login form. It is written into app.php and read back as whatever the
file made of it, and a switch that only understood the boolean would hand the application's
database credentials to Adminer on an installation that had asked it not to — with no way for the
installation to tell.
Driven through applicationInfo['devpanel'], which is the real path a configured value takes. My
first draft invented three seams on the controller for the test to override; they did not exist,
and adding them would have meant testing a shape that only exists under test.
A flake of my own, and what it was actually saying¶
The full run after this failed one assertion I had written the day's most confident-sounding docblock around: "a token edited by a single character does not verify". It flipped the token's last character.
Base64 carries six bits per character, and the last one holds padding bits base64_decode
discards — so flipping it frequently decodes to the identical bytes, the signature still matches,
and the token verifies. Under --filter the random address happened to encode to a length where it
did change; in the suite it did not. The code was right and the test was wrong, which is the
better way round but only because the suite ran.
Rewritten as the attack rather than as a mutation: decode the payload, put a different address in, keep the signature, re-encode. That is deterministic, and it is what somebody would actually try — which is what the docblock claimed to be testing all along.
Suite: 13,736 → 13,744 tests, wall clock 2:28, two runs with identical assertion counts.
Three shapes, two failures, and a count nobody was counting¶
ApiListQuery at 68.4%, and all 36 uncovered statements were in one method — run(), the single
entry point every listable object shares. Its parameters come off a URL, so fields and search
each arrive in three shapes and the caller does not choose which. None of the alternatives had ever
been executed, and neither had either of the failure answers.
A unit test with a fake source. The interface is nine methods and not one of the branches under test is about SQL: the fragment building has its own suite on both backends, and the schema, the row fetching and the counting are the source's business. What is left is the orchestration — exactly the part nothing was running.
The branch worth the most is the one that costs a query:
$recordsFiltered = (int) $result['total'];
$recordsTotal = $searchConditions !== '' ? $source->apiListRecordsTotal(…) : $recordsFiltered;
DataTables draws "showing 3 of 50 (filtered from 900)" from two numbers, and the engine only has
the filtered one. Recomputing the grand total only when a search is active is both halves of
one decision: reporting the filtered count as both would have the table claim a search matched
everything there is — and offer one page where there are ninety — while recomputing it
unconditionally buys a second COUNT(*) on a screen drawn at every page load. Both directions are
now asserted, including that the extra count is not run when there is no search.
Then the two failures, which are the same empty array from outside:
- a paginated query that raises answers with an error envelope naming the filter and the order, because the caller sent a page number and a search box and those fragments are the only way to see what was actually run;
- an unpaginated fetch returns
[]both when nothing matched and when the query failed.apiListLastError()is what tells them apart, and reporting a failure as an empty list is how a broken filter looks exactly like a table with nothing in it.
Three of my own assumptions were wrong and the tests corrected them, which is the useful direction.
The error envelope names error, not success. The builder quotes identifiers, so u.username
reaches the SELECT as u.`username`. And fields in the answer is the caller's list with
prefixes stripped — the primary key is added to the query and not to that list, so a caller asking
for username alone gets fields: ["username"] and a userid in every row.
One branch is deliberately not here: the structured filter escapes its values through a live connection, so it cannot be reached without one — and it belongs to the SQL builder's own suite, which runs on both backends.
Suite: 13,744 → 13,762 tests, wall clock 2:27.
A repair that left a warning behind¶
AdminerBridge::repairSession() — 25 of that class's 46 uncovered statements, and a routine whose
correctness is invisible by design. It runs before Adminer, touches one key in a session belonging
to another package, and leaves no trace when it works.
It exists because the first version of the /adminer route left our session open. Adminer starts
one only when none is active, so it used ours — and one of its keys is token. Ours is a hex
string; its is rand() ^ $_SESSION["token"], which gives «A non-numeric value encountered» twice a
page and a CSRF check that cannot work. Closing our session fixed it for new visitors and did
nothing for anybody who had already loaded the broken page: the bad value was sitting in their
adminer_sid session. So it is repaired rather than merely prevented — the alternative is telling
people to clear their cookies, which is what software says when it cannot fix itself.
Everything worth asserting about it is restraint: only token, only when it is not numeric, only
when no session is already open, only for a cookie shaped like a session id. And that it puts back
what it changed.
Which is where the tests found the same fault inside the fix. Beside use_cookies it was setting
and restoring session.use_only_cookies — doing nothing, because that setting governs whether the
id may come from the URL and the id here is set explicitly on the line below — while emitting
on PHP 8.5, from the restore, whenever the setting was already off. A repair that leaves a warning behind has not repaired anything; this one was leaving one of its own, in the same method whose docblock says so. It is gone.
The test that found it is the one asserting the ini settings are restored — it had to read them to compare, which is how the deprecation surfaced at all. Neither the route nor its existing tests would have shown it: it is a deprecation, not an error, on a development-only page.
And it had to move out of the shared process¶
The first version of this file took 91 unrelated tests down with it. session.save_path cannot
be changed once a session has been started in the request, and by the time this class runs in a
full suite, dozens of tests have started one — so setting a scratch save path either failed
silently or, worse, succeeded and pointed the rest of the suite at a directory this test then
deleted.
No teardown reaches that. The class runs #[RunTestsInSeparateProcesses] now, which is the only
honest way to test session internals: seven processes, one second. It is the same lesson three
fixtures taught earlier today — shared state decides a test's outcome — arriving through a door I
had not checked.
The assertion about the restored ini setting needed the same correction in miniature. It captured
the value before seeding the fixture, and seeding calls session_start(['use_cookies' => false]),
which disables the setting as a side effect — so the expectation was a value that no longer existed
by the time the repair ran. It now sets the state a real request is in, and asserts the repair
leaves it that way.
Suite: 13,762 → 13,769 tests, wall clock 2:27.
The alert that fired on every sign-in from a familiar device¶
NewSignInAlert::checkAndNotify() — the decision at the end of a sign-in, 17 statements never
executed. Writing the "a device the account has used before is not new" test made it fail, and it
was right to.
The history is read after the login lifecycle has logged the current sign-in, so the current fingerprint is already in it and would always match itself. It is therefore excluded — and the exclusion dropped every row carrying it:
foreach ((array) $rows as $row) {
$fingerprint = SignInFingerprint::fromUserAgent($row['user_agent'] ?? null);
if ($fingerprint !== $exclude) { // ← every occurrence, not one
$known[$fingerprint] = true;
}
}
That looks equivalent and is not. An account that uses a laptop and a phone has both in its history: signing in from the laptop removed all the laptop's rows and left the phone's — a non-empty history missing the current device, which is the definition of "new". Both devices alerted on every single sign-in, forever, which is precisely the alert-nobody-reads failure the feature exists to prevent, arriving through the mechanism meant to prevent it.
Only an account with exactly one device escaped, because then the set came back empty and an empty set is deliberately "not new".
One row is what the current sign-in contributes, so one is what comes out now. If it has not been logged yet, the row dropped is an earlier sign-in from the same device and any others still mark it familiar; only a device used for the second time ever can still look new, which is the conservative direction.
The fixture is the reason this was findable at all. My first version recorded one row for the familiar device, and under that history the old code and the new one agree — the exclusion removes it either way and the device looks new, which reads as correct until you notice the row it removed was supposed to be this sign-in rather than last week's. A real history at that moment has three rows: last week's from this machine, one from the other machine, and the one happening right now.
The rest of the method is written to be incapable of failing a login, so the assertions are mostly
negative and each covers an exception that would otherwise be thrown out of one: the account never
asked, the device is familiar, there is no deliverable address (not an error — an account can exist
without one), and the history cannot be read. That last is the one worth keeping: an empty set means
"everything is new", so a database hiccup would mail every user with the preference on, about a
sign-in they are performing right now, for a reason they will never hear. isNew() answers false
for an empty set, which covers a failed read and a genuinely empty history at once — and the
comment claiming the two were "distinguishable" is corrected, because they are not and do not need
to be.
And the FK guard's own test was asserting about the previous run¶
The full run then failed the orphan-guard test I added this morning: the constraint was added over
a row that violates it. It was not — the constraint was already there, added by an earlier clean
run, and canAddForeignKey() short-circuits when one exists. So the migration skipped, the
constraint stayed, and the assertion read that as a failure.
The test was asserting about the state a previous run had left rather than about this migration. It drops the constraint first now, which makes the precondition its own; the migration puts it back on the next run once the orphan is gone. Third time today that a test's outcome turned out to depend on what ran before it — and the first time it was one of mine from the same day.
Suite: 13,769 → 13,789 tests, wall clock 2:30, two runs with identical assertion counts.
Three assertions that were checking a query error¶
MassMessagesController was at 71.7%, and the existing suite already covered most of what I would
have written: the GET and CSRF refusals, criteria matching nobody, the missing message, the rule
that a sent message stays, the usertype floor. So the new tests went into that file rather than
beside it — the successful send, which no test had ever run because every existing one stops at a
refusal, and the one refusal missing from the list.
That one is worth the iteration on its own:
$this->assertSame(1, $this->recipientCount($id), 'the second send queued the whole list again');
$this->assertStringContainsString('already has recipients', $second->errors[0] ?? '');
A double press, a double-submitted form, a browser retrying a request it believes failed — any of them would reach every person on the list a second time, and there is no undo for delivered mail.
Writing the positive case is what exposed the rest. The send worked — «2 recipient(s) queued», an audience of two — and the assertion still failed, because the fixture's own helper was:
private function recipientCount(int $id): int
{
try {
return (int) $this->db->queryBuilder()->table('#PREFIX#massmessagerecipients')
->where('massid', $id)->count(); // ← no such column
} catch (\Throwable $exception) {
return 0; // ← every call, forever
}
}
The column is messageid. So every call threw and every call answered "none queued", which means
three existing assertions were passing against a query error rather than against an empty table
— including the one that checks a refused send queued nothing, and the one that checks the usertype
floor stops a send. Both are about the least recoverable action in the framework, and neither was
checking anything.
The teardown had the same wrong column, so the recipient rows were never removed either: a cleanup that threw, was swallowed, and looked exactly like a cleanup with nothing to do.
The catch is gone. A fixture that swallows a query error asserts nothing, quietly, for as long as
nobody writes the positive case — and this is the fourth time today that a test's silence turned out
to be the finding.
One more assertion came out of reading the audience resolver: naming an account explicitly does not
get it past the safety filters. only_ids is applied as a filter rather than instead of the rest,
so "send this to these three people" cannot become a way to mail an account somebody switched off
or one whose address was never validated — and a send to an unvalidated address is a bounce, which
is counted against the domain that produced it.
Suite: 13,789 → 13,803 tests, wall clock 2:30. Total coverage 91.79% → 92.05% before this file.
The two helpers every page depends on¶
helpers.php at 72.9%, and the uncovered lines were in the two functions with the widest reach in
the framework.
getUrl() is what sURL and URL are defined from at bootstrap, so it decides every absolute
URL the application ever writes — every link, asset, redirect and every URL in every mail. The
branches nothing had executed were the ones that pick the scheme and the port, which is exactly
where being wrong is invisible in development and total in production:
X-Forwarded-Proto, which is how almost every deployment is shaped — TLS ends at the load balancer and the application is reached over plain HTTP. Without that branch every absolute URL ishttp://, browsers block them as mixed content on anhttps://page, and the stylesheet, the script and the form action fail at once. The page looks broken rather than misconfigured.:443and:80omitted,:8080kept. A redundant:443is legal and wrong to write: it appears in canonical URLs and in OAuth redirect-URI comparisons, which are string comparisons — so it is a redirect URI that no longer matches the one the client registered. And a development port must survive, or every link onlocalhost:8080points at port 80.- No
SERVER_NAME, no URL. A CLI request has none, andhttp:///assets/style.cssis what the alternative produces: a string that looks like a URL, resolves to nothing, and gets written into mail sent from a queue worker.
humanCheckField() renders the proof-of-work fields on the public forms, and its uncovered
half was the CSP nonce. The failure mode there is the worst kind available: a strict policy drops
an un-nonced inline script silently, so no solution is ever computed and the check refuses
every submission — a public form nobody can send, presenting as the check doing its job. Both
script tags need it, and the test counts them rather than looking for one.
Writing the escaping test found something worth fixing. The token is escaped correctly into its
HTML attribute, but the challenge JSON is written inside a <script> element, and it was
encoded with JSON_UNESCAPED_SLASHES:
</script> closes a script element wherever it appears in one. HTML scans for those characters and
does not know it is inside a JavaScript string literal, so quoting is no defence at all. Nothing
reachable depended on it — the only caller passes HumanCheck::challenge(), whose token is
hex.int.int.hmac and cannot contain a < — but this is a global helper with a documented array
parameter, and a view is entitled to call it with its own. JSON_HEX_TAG now encodes < and >,
which every browser parses identically.
Suite: 13,803 → 13,814 tests, wall clock 2:29.
The fixture that could not find a class¶
api:docs at 73.8%, and 18 of the 38 uncovered statements were
warnIfASiblingWouldHaveFoundMore() — the whole reason that class was rewritten. An application
serving 72 endpoints got Wrote 1 path(s), 1 operation(s), and nothing in that line was false:
a document describing one endpoint of seventy-two is indistinguishable from an application that
genuinely has one, so it gets published and believed.
The warning had never fired in a test. Writing the case that fires it explained why:
private function writeControllers(string $relative, int $routes): void
{
…
file_put_contents($dir . '/DemoController.php', "<?php\n" . $methods);
}
A file with #[Route] methods and no class declaration, on the reasoning — written into the
docblock — that the generator "reflects over files". It reflects over classes:
fromClasses() skips anything class_exists() denies, and discoverClasses() builds the name from
the namespace plus the file name. So every fixture file was skipped and every run in that class
reported 0 path(s), 0 operation(s).
Nothing failed, because no assertion looked at a count. The sibling comparison is
$otherCount > $found, and 0 > 0 is false — so the test asserting that an explicit directory is
not second-guessed passed with no sibling to ignore. It now has one.
The fixture writes a real class, in the namespace the command will derive, and loads it. Deriving
that namespace by the same rule detectNamespace() uses means the fixture and the command agree by
construction rather than by coincidence — and the class name carries a random suffix, because a PHP
process may declare a class once and several tests write into the same relative directory.
With counts that exist, the new tests can say what the feature is for: the warning names the other
directory, both counts and the flag to re-run with, because "check your configuration" is not
something anybody can act on. It does not switch directories — the document written is still the
one that was asked for, since a command that quietly scanned somewhere else would be a worse
surprise than a thin document, with no way to tell which of the two you are reading. And an
equally-sized sibling gets no warning at all: the comparison is > rather than !=, because two
directories of equal size are two halves of an API and a line advising a re-run that would find
exactly as much trains people to ignore the one that matters.
And a TypeError out of every model save, seen once in twenty-one runs¶
The full run after this failed somewhere unrelated:
TypeError: count(): Argument #1 ($value) must be of type Countable|array, false given
src/Pramnos/Cache/Adapter/FileAdapter.php:318
src/Pramnos/Cache/Cache.php:728
src/Pramnos/Database/Database.php:2673
cleanEmptyDirectories() checks is_dir() and then calls scandir(). Between the two, another
flush can have removed the directory — and this sweep runs from Database::cacheflush(), which runs
on every model save, so two of them walking the same tree is ordinary. scandir() answers
false, and count(false) is a TypeError: an Error rather than an Exception, so it went
straight past the catch (\Exception) three lines below and out of the save that triggered the
flush.
Seen once, in the twenty-first full run of the day, and never before it — which is what a race looks
like from outside. It is the same shape as the vanishing directory cleanup() was taught about this
morning, in the one method that fix did not reach; the catch there is now \Throwable for the same
reason.
The read is a seam, because the interesting case is the read failing and a test cannot make
scandir() fail on a directory it owns — the suite runs as root, so permissions are no lever. Two
tests: the sweep survives an unreadable directory, and a genuinely empty one is still removed. The
second matters as much: every cache write creates a directory, and before this swept upward they
stayed for good — three thousand empty ones on one installation, each walked again by the next sweep.
Suite: 13,814 → 13,821 tests, wall clock 2:32, two runs with identical assertion counts.
The email log, and an id the fixture did not actually know¶
EmailsController at 74.3%, with 39 of its 43 uncovered statements in data() — the rows behind
the list an operator reads to answer "did that message go out?". Every cell in it is built by hand
from a database row, and four of those decisions are worth pinning.
The one that needs both engines is the address filter. It is written LOWER(tomail) = LOWER(%s),
and the reason is one word: = is case-sensitive on PostgreSQL and not on a default MySQL
collation, so a filter written the obvious way works in development and quietly matches nothing in
production — and an empty list is exactly what an operator reads as "nothing was ever sent to this
person". The value is also quoted by the driver rather than concatenated, because
Datasource::getList() takes a WHERE fragment rather than bindings and the address arrives in a
query string.
Then the cells: the id and the subject both open the message, because a row whose only way in is a button in the last cell makes the rest of the row a target people click at and nothing happens; everything a person typed is escaped, since a subject line is written by whoever composed the message and rendered into an admin page as HTML; the date is formatted, because the column is a Unix integer and printed raw it is a ten-digit number in a column headed "Date" — which is what this screen showed; and only an unsent message offers a resend, since offering "send again" on one that already went is how somebody sends it twice.
Two fixture problems, both mine, both worth writing down because they are the same mistake in different clothes: the test knew something it had not actually established.
The first was pagination. rowFor($id) scanned an unfiltered list for a seeded row, and mails
is shared with the whole suite — so "the first page of everything" is not a place a new row can be
relied on to appear. Each test seeds its own address now and filters by it, and the
"unfiltered" test became an assertion about exclusion, which pagination cannot affect.
The second was the id. seedMail() returned getInsertId(), and the row that came back had a
different number — auto-increment gaps from the deletes this fixture does, most likely. The
assertions did not need it: what they claim is that the id cell and the subject cell open the
same message and that the resend link points at this row, and both are checkable by reading
the id out of the markup under test. So seedMail() returns nothing now, and the invariant is
asserted instead of a number the fixture believed.
The fifth hand-rolled table, and this one contradicted the framework¶
The full run then produced thirteen errors in a file I had not touched:
mysqli_sql_exception: Data truncated for column 'date' at row 1
tests/Unit/Pramnos/Application/Controllers/EmailsControllerTest.php:112
That class builds mails by hand with CREATE TABLE IF NOT EXISTS and declares
date datetime NOT NULL. The shipped column is an integer holding a Unix timestamp. My new
test builds the table from its migration, so the migration's shape won the race and the
hand-rolled inserts of '2023-01-01 10:00:00' had nowhere to go.
The second consequence is the one worth the paragraph. That class was asserting against a schema
the framework does not have — and EmailsController::data() reads the column as (int) $row[4],
so a datetime of 2023-01-01 10:00:00 becomes the integer 2023, which is a date in January
1970. The test passed because nothing in it ever looked at the rendered date. Writing the test that
does look at it is what surfaced both halves.
Fifth time today, and the tally is worth stating plainly: tokenactions, authserver_user_activity_log,
sessions, massmessagerecipients, and now mails. Every one was a fixture that built a framework
table by hand, and every one either lost a race or asserted against a shape that does not ship. The
rule the changelog has repeated all day now has five instances behind it: a test that needs a
framework table builds it from that table's migration.
Suite: 13,821 → 13,839 tests, wall clock 2:34, two runs with identical assertion counts.
Three coverage numbers for one commit, and none of them a flake¶
I spent part of an earlier iteration on RedisAdapter measuring 222/328 in a full run, 0/328 in a
filtered one, and 90/328 in a filtered one with a single extra test — and reported it as an
unexplained instability, on the grounds that the code provably ran. It is not instability. It is
attribution, and I had written the rule down myself earlier in the same session before walking into
it.
#[CoversClass] does not merely label a report. PHPUnit restricts what a test contributes to
the classes its metadata names. The parity test declared this:
Its Redis rows run, pass, and reach a real server — a scratch test proved hashSet() records at
x1 under the same coverage run — and every line of RedisAdapter was discarded anyway. Adding the
missing attribute: 0/328 → 90/328, nothing else changed. The 222 in a full run is other tests,
which declare it, contributing their own lines.
The attribute belongs there on the merits: that class exists to assert the fallback and the native
implementation agree, so all three adapters are its subject. It does not belong on
PageCacheEdgesTest, which merely uses a Redis adapter as a collaborator — declaring it there
would misstate the test and credit RedisAdapter with lines nothing asserts on. A number inflated
that way is worse than one honestly low.
And with the report telling the truth, the real gap is one line repeated 21 times¶
Twenty-one methods open with it, and not one had been executed. It is the most important line in the file and the least interesting to write a test for, which is presumably why: a cache is the one dependency an application is supposed to survive losing, and an adapter that raises when Redis is unreachable turns a Redis restart into a 500 on every page.
The shapes are not interchangeable, so each is asserted on its own. counter() must answer 0 or a
rate limiter's arithmetic breaks on null. hashGetAll() and listRange() must answer [] or a
caller's foreach breaks. hashGet() must answer the caller's own $default, which is what
makes "no cache" indistinguishable from "not in the cache" — the whole contract of a cache. And
getStats()/getAllItems() must still answer, because they feed the panel somebody opens
because the cache looks wrong: raising there makes the diagnostic page the one page that cannot
load when there is something to diagnose.
No server needed, which is the point — this is the no-server case, reached by constructing the
adapter and never connecting, exactly the state a failed connect() leaves behind.
Suite: 13,839 → 13,846 tests, wall clock 2:32.
The changelog feature had never run, and the ranking was hiding it¶
Asked where the coverage stood, the answer included a nudge: "and the changelog looks like it has very low coverage". It did — three of its four files were at exactly 0%, and my own ranking had never shown them.
That is a fault in the method, not just the code. I was ranking by percentage over files of at least forty statements, which is a sensible way to keep three-statement classes out of the list and a very effective way to hide this: a reader, a writer and a service provider are twenty to forty statements each. The filter concealed eight files at 0% — 189 statements — and 462 uncovered statements in total, a quarter of the remaining distance to the target.
A file at 0% is not a low score. It means the machinery is written, shipped, and has never been executed — and every 0% file touched today has produced a real defect, for the obvious reason that nothing about it had ever been observed. Ranking by percentage puts those last, because they are small. The guide now says to rank by zero first, then by absolute gap, and to treat percentage as the least useful of the three.
What the feature promises, now asserted¶
ChangelogWriter gives any model with $emitChanges an audit trail with no further wiring, and
its design is entirely about not charging the request for it:
- It appends to the spool; it does not insert. The measured figures are in its own docblock — 2.807 ms for an insert into a hypertable with indexes against 0.003 ms for the file append, a factor of about nine hundred, paid on every save of every audited model.
- Nothing it does may fail the request. The write it describes has already committed, and there is nothing a caller could do about a queue failure. Asserted by pointing the spool at a path it cannot use and requiring the handler to answer normally.
- Registering is idempotent. Documented as safe to call repeatedly, and it has to be: providers boot per application, and a second registration writes every audit row twice — which is not a duplicate anybody notices until they are counting changes to answer a question.
- The trace is opt-in, because
getTraceAsString()on every save is not free, and it carries the feed row's natural key rather than a surrogate: a surrogate would have to exist before the row does, which means a database round trip per change, in the request, undoing the append the whole design rests on.
ChangelogServiceProvider teaches the spool how to store those rows, and both of its rules have a
failure mode that leaves no trace:
- the spool round-trips every row through JSON, so a nested array must be re-encoded before it
reaches
insert(). Without it the drain writes the literal string"Array"into ajsonbcolumn — once per row, with no error anywhere; - and a
nullmust stay null, becausejson_encode(null)is the string"null", which a database stores as a JSON null rather than SQL NULL — soWHERE details IS NULLquietly stops matching the rows that have no details.
ChangelogReader is what a screen calls, and its default is the decision worth the test:
events only. The automatic feed is one row per save and would bury the semantic rows — the
reference application writes AND logtype != 90 by hand in every listing that shows a person what
happened, and forgetting it once is what this default prevents. Beside it, the direction that
matters more: an origin list naming nothing returns nothing, because an empty whereIn would
either error or match everything, and a filter built from user input that came back empty would
otherwise hand over the whole trail.
Both backends for the reader, and not as a formality: the view is a UNION ALL over two tables,
and MySQL hands the JSON columns back as strings while PostgreSQL may hand jsonb back either way
depending on the driver. A caller that trusted one shape would work in development and raise in
production — or the reverse, and only ever on one engine.
Suite: 13,846 → 13,944 tests, wall clock 2:41 (up from 2:32 — the 98 new tests are 3s of it; the rest is host variance, and the figure is worth watching next iteration).
FW-052: a sweep with no way to ask for it¶
A consuming project deleted its legacy cache class and found the last thing it did had no
replacement: a scheduled task, every thirty-six hours, that called cleanup().
FileAdapter::cleanup() does exactly what such a task needs — walks the tree, removes what has
expired, and prunes the empty directories every cache write leaves behind (3,064 of them on one
container before that was fixed). But it was protected, and its only caller was
shouldCollectGarbage(): one call in a hundred, and never at all under PRAMNOS_TESTING. The
public getAdapter() was no help, because the method stayed protected behind it. And
flushEverything() is a different operation — it removes what is still valid, which on a warm
cache is an expensive thing to do on a schedule.
So the sampled design assumed a deterministic trigger it never exposed.
Cache::cleanup() now forwards to the adapter and returns the number of entries removed.
AbstractAdapter::cleanup() answers 0 — the honest default, since Redis, Memcached and the array
store all drop an entry when its TTL passes and have nothing to reclaim — and FileAdapter
overrides it.
Three decisions inside a small change:
It does not go through the sampling. The PRAMNOS_TESTING guard and the one-in-a-hundred
throw belong to the automatic sweep; a deterministic entry point that an environment flag could
silence would be no entry point at all — and this very test could not have been written.
It is not added to AdapterInterface. An application with its own adapter implements that
interface, and a method added to it breaks every one of them on upgrade. The facade asks whether
the adapter has cleanup() and answers 0 when it does not, so such an installation gets a
scheduled task that does nothing rather than a fatal on its first run. There is a test with an
interface-only adapter for exactly that.
It returns a count, because the caller is a cron task with somewhere to log it, and "how much
was there to reclaim" is the figure that says whether the schedule is frequent enough. That needed
deleteIfStillThere() to report whether it actually removed the file — it returned void, so the
first version of the counter counted every candidate including the ones another request had already
swept.
The part of the filing that was a warning¶
The report explains why the project did not simply keep its legacy sweeper for this one task: the
old _checkIfFileIsExpired() read the stored TTL as $details->timeout, an object property,
while this adapter serialises an array. Over a tree written by the modern adapter every read
gives null, filemtime < time() - null is true of every file, and the task would have deleted
the entire cache every thirty-six hours.
Worth checking rather than assuming, so I did: checkIfFileIsExpired() goes through
remainingTtl(), which reads $details['timeout'] as an array and — better — returns null for a
file it cannot parse, with "unreadable is not expired: a file this adapter cannot parse is not one
it may delete". The framework's sweep is safe from that, and the test asserts both directions in
one case, because a sweep that removed everything would satisfy a test that only checked the
expired entry was gone.
Suite: 13,944 → 13,951 tests, wall clock 2:38.
The two authentication mails somebody is waiting for¶
SecondFactorCodeNotification, NewDeviceAuthLinkNotification and PlainAddress were all at
0%: three classes on the sign-in path that had never been executed. They are the counterpart to
the alerts covered a few entries up, and the difference between the two kinds is the whole design
of both.
An alert arrives unbidden. The reader was not expecting it, so it carries no link — a person who learns to click links in authentication mail they did not ask for has been trained by their own provider to fall for the phish. These two are the reply to something the reader did seconds ago, with the form still open in front of them, and that changes what each may carry:
- The code mail carries six digits and nothing to click, because a code has to be typed into a
page the reader already has open. That makes it useless to anyone else reading the mailbox unless
they also hold the password — and it is why this notification is
mailonly. A database notification would put the code in the panel of whoever is holding the half-finished session, which is precisely the person the second factor exists to stop. - The link mail does carry one, and therefore has to supply its own context: it names the device that asked. A reader who recognises "Firefox on Windows" as themselves is not being trained to click anything that arrives, and one who does not recognise it has just been told, in the only message that could have told them.
Both say how long they last, because a reader coming back twenty minutes later needs to know that
is what happened rather than concluding the site is broken — and both get the singular right, since
"expires in 1 minutes" is the kind of detail that makes a person doubt the rest of the message. The
tests assert the plural in both directions, the sub-minute TTL that used to be able to say zero, and
the fallback when a misconfigured setting hands either of them 0: a mail announcing a code that is
already dead, about a code that works.
The escaping is asserted at both ends even though neither value is attacker-controlled today. The
code is generated digits and the device string comes from a User-Agent — but a single-use auth URL
carries a token, it is printed as visible text as well as linked so the reader can see where it
goes, and a quote in it would end the attribute. Carrying a value into a mail body is this object's
only job, which makes it the place where trusting the caller stops being free.
PlainAddress is two statements and exists for one mail: telling the previous address that the
account's email was changed. When a stolen session changes the address first, that message is the
only signal the real owner gets — so it cannot be routed through the user object, which now points
at the attacker's mailbox. It is deliberately not a User with its address overwritten, because a
mutation made to send one mail is exactly the kind that survives into a save(). Every channel
other than mail answers null: there is no account behind it, so a database notification has no
user id and a broadcast has no channel.
Suite: 13,951 → 13,967 tests, wall clock 2:38.
The database backplane, and a key order that is MySQL's to choose¶
DatabaseEventStore was at 0% — the store behind the database broadcasting driver, for
deployments with no Redis. The driver's own poll loop is unit-tested against an in-memory store,
which is what the interface is for, and it meant nothing had ever run against a real table. Three
things a poll backplane can get wrong are invisible to that double: the payload's shape coming
back, the boundary of "newer than" (id >= $lastId redelivers the last event on every poll, and
this driver polls in fractions of a second), and who the channel filter lets through — IN () is
not valid SQL, and both natural repairs fail open, handing a consumer with no subscriptions every
channel on the server.
Running it on both backends found something worth writing down.
MySQL does not preserve a payload's key order. The column is a native JSON type, and MySQL
normalises the document it stores — an object's members are re-sorted, by key length first. So a
payload published as ['title' => 'Νέα', 'url' => '/a/b'] reaches the consumer as
{"url":…,"title":…}. PostgreSQL keeps the text as written, so the same code preserves the order
there and the difference is invisible until a deployment changes backend. Keys and values always
round-trip, Greek text and slashes included — only the order is the backend's to choose. That is
now a section in the realtime guide, with the consequence spelled out: read a payload by key, and
never json_encode() one you received to compare it, hash it or sign it. The Redis drivers hand
back the bytes you published and would answer all three differently.
The test caught it as a passing assertion in one lane and a failure in the other, which is the only
way it could have been caught — and it also showed why the first test I wrote passed by luck:
['id' => …, 'name' => …] is already in MySQL's sort order.
A missing table fails loudly, and that is right. The table name is configurable, so a typo
reaches the store, and the tempting kindness is to catch the error and answer "no events" — the
caller is a poll loop and an exception out of a worker is noisy. Wrong kindness: append() shares
the table, so a missing one means every published event is being dropped as well as never
delivered, and a stream that quietly delivers nothing looks exactly like a system where nothing is
happening. subscribe() calls latestId() before its loop begins, so the failure lands where the
subscription is set up. The test asserts the error names the table, and does not assert its type:
the two backends raise different classes, which a caller wanting to handle this would need to know.
Suite: 13,967 → 13,989 tests, wall clock 2:38.
Two statements, and the form they exist to remove¶
adminer-object.php was the last file at 0% — two statements, and the reason it is a file at all is
the reason it was worth a test. Adminer's bootstrap asks function_exists('adminer_object') with an
unqualified string, which PHP resolves in the global namespace only, so the hook cannot be a
namespaced function, a closure or a method. A refactor that tidied it into any of those would break
nothing visibly: Adminer would build its own object and show its login form, which is a working
page. It is the page this whole arrangement exists to prevent.
So the test asserts the hook is global and that a namespaced adminer_object() does not exist,
which is the assertion that catches the tidy-up — and that including the file twice is not fatal,
since a redeclared function takes the request down with a blank page rather than an exception.
Behind the hook is the eval-ed subclass, which had never been built either, and each of its four
overrides is a refusal now pinned by a test:
credentials()answers from configuration with a hostile query string in place. Adminer's default reads the server and username out of the URL, which behind a single-purpose gate makes a link enough to aim an authorised session at any host this machine can reach, with credentials the visitor supplies. The empty-configuration branch defers to Adminer rather than attempting a login with a blank username — one line no configured installation ever runs.loginForm()is asserted by what is absent: no<input, no<form. That form takes a driver, a server, a username, a password and a database, which is what turns one permission into a general-purpose database client pointed at the rest of the network.login()passes on an empty password too, because the default's way of deciding whether one is required is to open a second connection and find out.name()names the installation and escapes it — a production database and a local copy are otherwise identical on screen.
And plugin() puts the password in $_SESSION['pwds'][driver][server][user] as a plain string: the
slot is exact, the array form is only for the permanent-login cookie, and either mistake ends in
Adminer showing a password prompt on a page that is supposed to have none.
Suite: 13,989 → 14,000 tests, wall clock 2:38.
«Created», about a file that was not there¶
Hunting the biggest remaining gaps turned up a defect worth more than the coverage. Three of the
generators in MakeCommandBase — create:api, create:controller, create:model — did this:
No recursion and no return check, against nineteen sibling calls in the same class that pass
true. Api/Controllers, Controllers and Models sit two levels below the application root, so
on a project that has not got the parent yet — a fresh one, or one adding its first API controller —
mkdir() failed with a warning, the file_put_contents() below it failed too, and the command
printed «created» and the path of a file that did not exist.
Which is the part that made it expensive rather than annoying: the developer goes looking for a bug
in generated code that was never written. It is now one ensureTargetDirectory() for all three,
recursive, and it throws with the path when the directory cannot be made. The check is
!mkdir() && !is_dir() rather than the return value alone, because two commands generating into the
same new tree is ordinary and losing that race is not a failure.
Alongside it, two more gaps in that class and one in the panel:
offerSearchRegistration() — 24 statements, and the reason none had run is that the method does
nothing at all unless app/search.php exists, which the framework's own repository has no reason
to have. So every existing test took the first return ''. What the tests now pin is a generator
being careful with a file the developer owns: it offers only when that file exists (the project
saying it uses the search registry at all), only once per entity — a second create:crud appending
a duplicate block would give every result twice, which reads as a bug in the search — and only when
asked, with a non-interactive run explaining how to do it by hand instead of writing. The test
creates the file and removes it, and skips rather than overwriting when a checkout has one:
appending to somebody's real registrations is precisely the mistake the duplicate guard exists to
prevent.
buildWizardFormFields() — 16 statements of pure function, column definitions in and HTML out,
which makes it the least defensible gap of the three. Three decisions now asserted: the primary key
gets no field (a form that posts an identity invites a save to target another row), nullable
decides required in both directions, and a Select2 foreign key loads over AJAX while still
pre-rendering the currently-selected option — eager rendering is fine on the developer's ten-row
table and is a megabyte of <option> on the customer's, and dropping the selected one makes an
edit form open blank. Also #PREFIX#users and users both reading userList, since the wizard
spells it either way and the wrong one gives an always-empty select on the field most likely to be
on the form.
The panel's MCP tab — 81 statements across the POST endpoint, the page and the server lookup.
This is the endpoint that runs whatever a project registered, so each of its refusals is asserted to
stop: terminate() is overridden to count instead of exiting, and a refusal that fell through to
the dispatch below it would be the CSRF check as decoration. The subtler ones: a JSON scalar is
refused as well as a parse failure ("5" parses, and params.arguments has to be an object); the
request comes back with "arguments":{} rather than [], because PHP decodes {} to an empty
array and a screen whose whole job is showing what was sent must not lie about it; and a tool that
threw is flagged, since dispatch() reports it as a successful response whose content is the
exception message. The container's server wins over a locally built one — a debugger describing a
different server than the one being debugged is worse than no debugger.
Total coverage 92.35% → 92.73%. Suite: 14,000 → 14,038 tests, wall clock 2:38.
The settings store, and ending somebody else's session¶
User was the largest remaining gap after Database, and its 175 uncovered statements fall into
two coherent families rather than scattering. Both are now covered on both backends.
Per-user settings — 56 statements¶
Four accessors, and every one of their decisions exists for the operator rather than for the code,
which is what made them worth specific assertions rather than a smoke test: the value is JSON so a
list stays a list and false stays false (a store that flattened to text hands back "0" for a
switch somebody turned off, and "0" is truthy — the classic way a switch ends up permanently on);
a write upserts, because checked-then-written races two requests into two rows and «the value» stops
having an answer any read can resolve; removing a setting deletes the row, because a null value is a
switch turned off and no row is a switch nobody touched, and only the second lets the application's
default apply again; and a value that is not valid JSON comes back raw, because rows get written by
hand in a database client and refusing to read one is the store deciding an operator's edit did not
happen.
Writing the degradation test exposed an asymmetry I had assumed was a bug and turned out to be the
design — with one real gap inside it. The readers degrade and the writers report failure.
getSetting() answers the default and listSettings() answers [], because a project that has not
migrated has no settings and a framework upgrade should not take down every page that consults a
preference. A write cannot degrade the same way: a caller told it succeeded will tell somebody the
switch was changed. So both writers answer false — including the delete, and «removed» about a row
that is still there is the one answer an operator will act on and be wrong about.
The gap: setSetting() logged its failure and deleteSetting() did not. It was the only writer
that reported a failure and left no trace of it, so an operator pressing «remove» got an error with
nothing anywhere to say why. It logs now, in the same shape as its sibling, and the guide says which
returns to check and which to ignore.
Ending other sessions — 54 statements¶
A session here is two things, and the reason this code exists is that ending one without the other
leaves the account reachable: the sessions row a browser is tracked by, and the web_session
token that actually authenticates its requests. Revoke only the row and a live bearer token remains;
revoke only the token and the tracker still believes in a session. Each half has its own try, and
there is now a test that drops the sessions table and asserts the tokens are still revoked —
a property no reading of the code confirms, and the half that still works is the one that
authenticates requests.
Two things pinned because both were bugs the code already carries the scar of. The device is matched
on the fingerprint alone, not the whole stored deviceinfo, which also holds the issuing
address: consumer addresses are dynamic, so a router reboot between two logins made the strings
differ, the match failed, and the older token stayed live forever. And the returned count is a count
— update() hands back a Result, and casting that to int raised while the sessions were ended
perfectly correctly, which is exactly why a wrong number survived. Both directions are asserted, and
so is the sparing of the caller's own session: people change a password because they think
somebody else has it, and signing them out of their own browser reads as the change not having
worked, while signing out only themselves leaves the other person holding the account and the owner
believing they have taken it back.
Three fixture mistakes of mine, all the same mistake. sessions calls the address host_addr and
the user agent agent, keeps one time rather than a first and last visit, and its primary key is
visitorid; usertokens spells the expiry expires. Hand-writing what a framework table
«obviously» looks like is how a fixture ends up asserting a shape that does not ship — the columns
came out of the migrations in the end, which is where they should have come from first.
The fourth failure was not a mistake but a property. Minting a second token while the first is still
in $_SESSION['usertoken'] revokes it — correctly, because a login arriving on a request that
already holds a token is a replacement. So the fixture cannot mint two tokens in one request and
call it two browsers: two logins are two requests, and the test now models that explicitly.
A table rebuilt from part of its history is not the shipped table¶
The wall clock went from 2:38 to 3:20, which is the kind of number the loop is supposed to stop on, and chasing it produced the most transferable finding of the batch.
These tests drop sessions and usertokens and rebuild them from their migrations — the rule that
has caught five hand-rolled fixtures asserting shapes the framework does not ship. But they rebuilt
from the creation migrations only, and idx_sessions_userid, idx_sessions_time and
idx_usertokens_token are not declared there: they come from AddMissingIndexesToExistingTables, a
retrofit. So the rebuild handed the rest of the suite two unindexed tables, and every later test
that looks a session up by user started scanning. Adding the retrofit to the list took it back to
2:56.
Two smaller costs came out of the same hunt. Dropping and re-migrating on every test, rather than
once per class, is a few hundred milliseconds ×62 in two lanes — the shape cannot change between
tests of one class, and the drop is there to defeat a shape left by a different one, so a flag
keyed by class is enough. And rebuilding the shipped usertokens exposed two fixtures inserting
token rows without deviceinfo, scope, notes and the rest: NOT NULL with no default, supplied
by addToken() on every real write. Those inserts were passing against whatever shape an earlier
test had left behind, which is the same failure mode as a hand-rolled table, arrived at from the
other direction.
The remaining gap turned out not to be a gap. Measured with the two new files removed, the suite takes 2:56.6; with them, 2:56.1. The machine has drifted since the 2:38 reading — so the new tests cost nothing, and the retrofit fix was a real 24-second saving against the same machine rather than a return to par.
Suite: 14,038 → 14,088 tests, wall clock 2:56 (2:56 without these tests — no attributable cost).
The query logs that only run on a developer's machine¶
Database is the largest single file gap at 191 uncovered statements, and the two families taken
here — the development-only query logs (39) and the SQL result cache (20) — were uncovered for the
same reason in opposite ways: the first only runs under DEVELOPMENT, and the second only matters
on the second request.
Three logs, and every bound in them is a scar¶
startLogs() runs from connect() under DEVELOPMENT and opens databaseQueries.log,
duplicateQueries.log and the slow-query log. Every limit in that code was added after a
developer's machine fell over: a 3,123-test suite died at the 2,394th with exit 255 and no message,
which is what PHP's memory limit looks like from the outside. The query log held the full SQL of
every statement for the life of the process; the duplicate detector held every distinct statement as
an array key; and the log was written in the destructor, so the run that died wrote nothing at
all — empty on exactly the run somebody needed it for.
So there are two kinds of assertion. The feature works: a statement and its timing reach the log,
the summary names the request and the count, a repeat reaches the duplicate log, a statement seen
once does not. And the bounds hold: the accumulated text is flushed past 256 KB and not before
(a flush per statement turns a diagnostic into a write per query), the fingerprint map forgets its
oldest half rather than clearing — keeping the newest, since a bound that dropped the recent end
would lose exactly the duplicates a developer is looking at — and a log over 512 KB rotates to one
generation of .old, replaced rather than accumulating.
The duplicate detector is the part worth documenting rather than just covering, so the logging guide now has a section on all three. It answers the question that is hardest to answer any other way: a page is slow and no individual query looks slow enough to explain it, because the shape is one request asking the same question in a loop and every individual answer is fast. That is why the file is separate from the query log rather than a column in it.
One assertion of mine was wrong in an instructive way: I counted = rules to check the duplicate
header is written once per request, and got two. The header draws its rule above the date line and
below it, so one header is two rules — the behaviour was right and the count was measuring the wrong
thing. It counts the date-and-URL line now, which is one per header.
A cache that returns almost the right thing¶
The SQL result cache has three ways to be silently useless rather than broken, and each is now
pinned. The round trip must be transparent: above ten kilobytes the payload is gzipped and
marked with a prefix, and the tests assert both the marker on the stored value and that what comes
back is what went in — including that an integer id does not return as a string, which is the worst
shape of cache bug because it only happens the second time. A miss and a cached nothing are
different answers: false means ask the database, [] means the database already said nothing,
and conflating them re-runs the expensive query that returns no rows on every single request — the
query most worth caching and the one nobody notices is uncached. A corrupt compressed entry is a
miss, not an exception, because a half-written cache entry is ordinary and the data is in the
database the whole time.
shouldCacheResult() is asserted in both directions and against configuration, since the row count
alone is not the cost: a thousand rows of two integers and a thousand rows of a text column are
three orders of magnitude apart, and it is the second that fills the store.
And the regression the code carries a comment about: an absent cache setting used to be read as
'memcached' here, so an installation that had configured nothing asked for a store that was not
running, the connection failed, and the SQL cache silently downgraded to a private file store —
working, slower, and with nothing anywhere to say it had happened. There is now a test that
configures nothing and asserts the round trip still works.
One of my own assertions could have passed vacuously — assertStringStartsNotWith is satisfied by an
empty string, so "a small payload is not compressed" would also have passed if nothing had been
cached at all. It asserts the stored value is non-empty first.
Passing under a filter, failing in the suite¶
The cache tests passed on their own and failed fourteen times in the full run, and the cause is worth recording because it will catch the next person writing a test that touches the cache.
Cache keeps a static record of which methods have connected. A test anywhere earlier in the
suite that reaches for a store which is not running leaves that record false, and every instance
built afterwards has caching false — so load() returns before consulting anything. My tests were
inheriting the ambient store and asserting a round trip through it, which meant they were really
asserting that nothing earlier in a 14,000-test suite had poked at an unreachable cache.
They now pin cache.method to file for their duration and restore it, which is also the honest
scope: these tests are about cacheStore() and cacheRead(), not about which store an installation
configures.
The one test that cannot be pinned that way is the one about an absent setting — its whole subject is
what happens when nothing is configured. It no longer insists on a hit, because on a machine where
the ambient default is unreachable a hit is not available, and that is precisely the situation the old
hard-coded 'memcached' turned into a silent downgrade. It asserts the call goes through and answers
one of the two things a cache may answer, a hit or a miss, rather than raising out of whatever page
read it.
Suite: 14,088 → 14,142 tests, wall clock 3:04 against a machine currently taking 2:56 for the previous count — the new tests run in under a second on their own.
A documented setting name that does not exist¶
Account is the largest untouched file at 126 uncovered statements, and covering two of its
clusters — the human check on the public forms (18) and the reset mail itself (21) — turned up a
documentation bug that would have cost somebody an afternoon.
The Authentication Guide named the setting auth.security.human_check_forms, in two places. The
policy reads auth.security.human_check. There is no human_check_forms anywhere in the framework,
so an installation that configured what that guide said got the default — checks off — with nothing
anywhere to say so. The Security Guide had it right, which is how the two drifted without anybody
noticing. Both lines are corrected.
I found it by making the same mistake. My first version of the test configured a setting called
security, and all six assertions passed vacuously: SecurityPolicy::value() reads
applicationInfo['auth']['security'] — app.php, not a row in the settings table — so the policy
answered its default and the check was never required at all. That is deliberate in the framework
and worth knowing: a check on a public form is part of how a deployment was built, not something a
screen can switch off.
The check, once it was actually on¶
Sign-in, registration and forgot-password are the three forms anybody on the internet may submit, and each is abused differently — credential stuffing, junk accounts, and using the site to deliver mail to an address somebody else typed. The asymmetry now pinned in both directions:
- it fails closed on verification. A form that requires a check and submits nothing is refused, including from a browser with no Web Worker. Letting that through would make the check bypassable by advertising an old user agent, which is the first thing anybody automating a form tries. All three shapes of "nothing" are asserted separately, because a single mistake collapses them into the not-required branch and the check silently stops existing.
- it degrades open at mint time, and that is not a contradiction: failing to offer a check locks legitimate people out of their own account, failing to demand one lets a script through, and only the second is recoverable by turning the switch off.
There is also a test that solves a real challenge, because every refusal above would be satisfied by
a check that refuses everything — which is a site nobody can sign in to. It solves it with
HumanCheck::meetsDifficulty(), the class's own public rule, rather than reimplementing the
leading-zero-bit count: a solver with its own copy of that rule can disagree with the one under
test, and then the test asserts the two agree with each other rather than that a real solution is
accepted.
The reset mail¶
Every existing test of this flow replaces sendResetEmail() with a recorder — correctly, for what
those tests are about — so the one thing the mail exists to deliver had never been composed. It is
read back through the mails audit row rather than a stubbed mailer, which is the same row an
operator reads when somebody says the mail never arrived, and it is written whether the transport
worked or not. That is what makes it usable here: the test asserts what was composed, not that this
machine can deliver mail.
The link is asserted to appear twice — once in an href and once as readable text. Mail clients
strip anchors, gateways rewrite them, and a plain-text reader sees no anchor at all, so either one
alone makes the message useless for somebody. The token is asserted to arrive URL-encoded, because
the whole message is one URL and a mangled token is a link that refuses a person who did everything
right. And a transport failure is asserted not to raise: an exception here becomes a 500, and a 500
on the forgot-password form tells whoever submitted it whether the address matched an account —
exactly what the identical-answer property elsewhere in that flow exists to hide.
Two of my own fixture guesses were wrong again, both the same way: mails keys on id, not
mailid. Read off the migration in the end.
A test suite that says a file has never run, about a file it runs eleven times¶
Coverage re-measured at 93.23% (from 92.73%), and the fresh report listed
src/Pramnos/DevPanel/adminer-object.php at 0% — a file this morning's
AdminerObjectHookTest exercises with eleven passing tests.
#[CoversClass] restricts what a test contributes to coverage, and adminer-object.php
declares a global function, not a class. So with only #[CoversClass(AdminerBridge::class)] on it,
none of those eleven tests contributed a line to that file and the report went on calling it
unexecuted. It is the same measurement puzzle as the RedisAdapter one from earlier in this session,
arrived at from the other direction — there a class was under-credited, here a file no attribute
names at all. #[CoversFunction('adminer_object')] fixes it, and the lesson generalises: a file
that no Covers* attribute names is a file the report says nobody has run.
Worth knowing when reading a coverage report: a 0% file is not always unexecuted machinery. Check what names it before writing a test for it.
The cache that must not outlive what it describes¶
Application is 124 uncovered statements, and the two clusters taken here are the auto-migration
fingerprint cache (24) and the framework's own navigation registrations (10).
The fingerprint cache is the kind of code where being uncovered is genuinely dangerous rather than untidy: a broken cache still returns an answer, and the wrong answer leaves the schema behind the code — the single failure the auto-migration check exists to prevent. Its whole design is one decision. The key is the fingerprint, derived from the migration files themselves: their count and the latest timestamp. A time-based cache would be wrong here, because after a deploy that adds a migration a stale «all applied» leaves the code ahead of the schema for however long the lifetime happens to be. Keying on a description of the files makes the cache invalidate itself — a deploy that adds a migration changes the key, the next request misses, and no lifetime has to be guessed.
So the tests assert the pair rather than either half: a remembered fingerprint is found, and a different one is not. Only together do they say the cache cannot outlive what it describes — «not found» alone is satisfied by a cache that stores nothing, and «found» alone by one that answers true for everything, which is a system that stops checking its migrations entirely. A newer timestamp at the same count also misses, because a migration can be replaced as well as added.
Two smaller properties, both of which only bite on a real machine. The key is namespaced by the
application root, because APCu is per FPM pool rather than per application — without it,
application A's «41 verified» answers application B's question and B's schema is behind its code with
the check reporting everything fine. And the marker file is written to a temporary name and renamed,
since rename() within a filesystem is atomic and file_put_contents() is not: a reader finding a
half-written marker would skip the migration check on the strength of a file nobody had finished
writing.
migrate() is asserted to swallow everything, which is the right behaviour and worth stating: it
runs on the way into a request, so an exception escaping it turns a migration problem — or a
database that is briefly unreachable — into a blank page on every route at once. That is a far worse
outage than a schema one migration behind.
Two integers and a URL, in the navigation nobody reads¶
Navigation is a registry filtered per visitor, not markup, which makes the registration the half worth testing: a link registered with the wrong gate is shown to somebody it should not be, and nothing about the page it points at will stop them — the screen's own check is a separate decision that may or may not exist.
Two of the framework's own registrations encode a judgement rather than a fact, and both are now
pinned. Mass messages need usertype 90 where every other admin screen needs 80, because that
screen mails everybody: the privilege to edit a record and the privilege to send a message to every
account on the installation are not the same privilege, and the entire difference is one integer in
one constructor call — exactly what a later tidy-up normalises away. And admin links go through
AdminArea::url(), not sURL . $path, because the public site header shows this same section and
its links have to lead into the area from outside; a bare link lands the visitor on the same screen
in the public theme, no sidebar, outside the area's usertype floor, with nothing to say anything
happened.
Feature-gated links are asserted absent as well as present, since «registered with a feature tag»
and «registered only when the feature is on» look identical from the inside and differ entirely for
an installation with the feature off. And every admin. item is asserted to require a sign-in and a
usertype floor as a sweep rather than one at a time, so a link added later inherits the assertion.
Suite: 14,164 → 14,181 tests, wall clock 2:36 — the machine that read 2:56–3:20 during the previous two entries is back to its usual pace, which retrospectively confirms those readings were load rather than test cost.
Two generators, one table, two different primary keys¶
MakeCommandBase has been the top-ranked file for four iterations and I had been avoiding it,
because its remaining methods write files into the repository and need a live table. Doing it
properly found a real bug — the kind that only a test with an awkward fixture can find.
create:api reads the primary key out of the column report. create:model derived it by
convention: singular table name plus id. For a table this toolchain generated those agree, and
the code says as much — "the generator derives it by convention" is a comment, not an oversight. For
a legacy table they do not. customers keyed on customer_id produced a model declaring
customerid, which is not a column: such a model loads nothing and inserts a new row on every save,
presenting as «the edit form does not work» rather than as a generator bug. And the API controller
generated from the same table addressed customer_id, so the two halves of one scaffold disagreed.
createModel() now asks the schema on the live-table path, through
SchemaBuilder::primaryKeyColumns() — which both backends already implement, so this adds no
driver-specific SQL. The convention remains the answer in the two cases where the schema cannot be
asked: the wizard path, where the migration is written but not run, and a composite key, where
Model addresses a row by one column and there is no honest single answer. The parameter added to
buildModelFromWizardColumns() is optional, so no existing caller changes behaviour. The console
guide now has a section saying which of the three applies when.
I found it only because the probe table deliberately breaks the convention — named
modelprobe_<hex> and keyed on probeid. A fixture that had followed the convention would have
passed against both behaviours and told me nothing, which is worth remembering: for a generator
whose job is inference, the fixture has to be the case the inference gets wrong.
A second, smaller inconsistency in the same method. createModel() has two branches — live table
and wizard — and the wizard one returned a summary that stopped after the file paths. So on the one
path where the developer has most reason to wonder whether it worked (there is no table behind it to
go and look at), the command confirmed nothing. It prints the same closing line now.
What the generators are now checked to produce¶
createApi was the largest single uncovered method left at 39 statements, and what had never been
checked is the only thing the command produces: a controller whose contents are derived column by
column from the live schema. A generated file is read once and edited afterwards, so a wrong guess is
not a crash — it is a line somebody keeps. Three now asserted:
- the primary key is found on both backends and is not writable. PostgreSQL reports a
PrimaryKeyflag and MySQL aKeycolumn readingPRI— two branches for the one derivation that matters most, since a controller without its key has no working single-record route. And no assignment of that key from request input exists anywhere in the file: aPOSTbody that can set the id is a request that can be pointed at an arbitrary row. - each column type reaches its own cast. An integer read through
strip_tagsstores 0 for anything non-numeric; a nullable number that coerces to 0 turns "not supplied" into a value, and the generated block goes out of its way to keep those apart. @apiBodybrackets match nullability, because that is what an integrator reads to know which fields are required — documented the wrong way round, their first request fails validation for a reason the documentation says cannot happen.
Both generator tests end by running php -l over what they produced. It is the assertion that makes
the others worth making: a syntax error in a generated file is a fatal on the first autoload, and the
developer's first sight of their new class is a parse error.
This also exercises this morning's ensureTargetDirectory() fix from the exact angle that produced
it — src/Api/Controllers is two levels below the root and neither level exists in this repository,
which is the case the old bare mkdir() failed at while reporting a file it had not written.
Suite: 14,181 → 14,217 tests, wall clock 2:42.
A stylesheet that still points at the CDN, and a table from an older schema¶
Two clusters this round, and both were uncovered for reasons worth naming.
Pulling a CDN stylesheet's own references local (51 statements)¶
init can install a front-end library, and a stylesheet is not one file: Font Awesome's CSS names
five webfont files. Download only the CSS and the project gets a stylesheet whose every url() still
points at the CDN — which works in development, and is exactly the outcome somebody chose init to
avoid. It also fails silently: the page looks right until the CDN is blocked by a content policy or
the machine is offline, and then the icons are empty boxes with nothing in any log.
The resolution is where this goes wrong, and one shape carries it: ../webfonts/fa-solid-900.woff2
is what a CDN stylesheet under /npm/pkg/css/ actually contains, and the parent segment has to be
resolved rather than pasted — pasted, it asks for /npm/pkg/css/../webfonts/..., which some
servers normalise and some reject, so the bug half-works depending on which CDN the project chose.
The rewriting has its own trap, and it is the one I would have got wrong: ?#iefix and
#fontawesome are how font stacks disambiguate formats, and neither belongs in a filename — but the
text replaced in the CSS has to be the original reference, fragment included. Strip it from both
and the rewrite matches nothing: the file is downloaded, the stylesheet still points at the CDN, and
nothing reports a problem. There is now a test for exactly that, and for the neighbouring cases —
data: URIs and url(#gradient) left alone, a repeated reference fetched once but rewritten at all
three occurrences, an unresolvable reference skipped without costing the other four.
No source change was needed: downloadFile() already answers from a PRAMNOS_TESTING seam, which is
what made the whole path testable without a network.
The organisation summary screen (39 statements in one action)¶
The existing tests of this controller run against a fake view whose layouts cover list, edit and
members but not view — a neat illustration of how a screen goes uncovered: not because anybody
decided to skip it, but because the harness grew around the screens that came first.
What the action does is summarise a membership, and the decisions are: active members only and only ten of them (a summary, with the full list one click away); the count is the whole membership, not the size of the sample — ten shown and «10 members» beside them on an organisation with four hundred is the kind of wrong number somebody plans against; and both the table and its FK column are configurable, so the screen has to follow the override.
Then the finding. The test database held an authserver_user_organizations with three columns —
user_id, organization_id, is_active — and not the ones the shipped migration declares (userid,
granted_by, granted_at, expires_at, is_active). A shape from an older schema, surviving
because this database persists between runs, and runMigrations() is a no-op for a table that
already exists. Every insert failed on userid, and the screen under test joins on uo.userid — so
it could never have been exercised against that table, which is the likeliest reason the action was
never covered at all. Dropping and re-migrating fixed it and broke nothing else in 14,247 tests.
Three of my own mistakes on the way there, all of the same family, and the guide now carries the rules they teach:
Request::staticGetOption()reads$_GET['_option']and nothing else. I setoption, so every test took the "id is not valid" branch and six of them passed for the wrong reason.- A MySQL schema qualifier is not a schema. The framework folds
authserver.user_organizationsdown toauthserver_user_organizations;hasTable()knows about the folding and a rawqueryBuilder()->table('authserver.user_organizations')does not — it asks MySQL for a database calledauthserver. UseRole::membershipTable(), orschema()->quoteTable()for raw SQL. getInsertId()after inserting into aSERIALcolumn is not portable between the two backends. Read the row back by a value you supplied.
The testing guide gained two rules from earlier today as well: include the retrofit migrations
when rebuilding a table, not only the creating one — several indexes come from
AddMissingIndexesToExistingTables, and a table rebuilt from part of its history makes other tests
scan — and #[CoversFunction] for a file that declares a function rather than a class, which is why
adminer-object.php was reported at 0% while eleven tests exercised it.
Suite: 14,217 → 14,247 tests, wall clock 2:32 — the fastest reading of the day.
The base class every model extends¶
Application\Model is what an application's whole data layer inherits, which makes its uncovered
parts unusually expensive: an off-by-one in the pagination is an off-by-one in every listing at once.
Two clusters covered here — the semantic change events (22 statements) and _getPaginated (23).
What a model says happened, as opposed to what changed¶
Two audit feeds exist and the distinction is the whole point. The automatic one is a row per save, derived from the columns that moved. This one is a model saying «approved», «revoked», «merged» — words that describe what a person did, which no diff of columns can recover, and it is the feed an operator reads when asking why an account is in the state it is in.
Three properties now pinned, each a specific way this goes wrong:
- recording must not be able to break the thing it records. The event goes through
WriteSpoolso the audit write is never inside the caller's transaction, and if spooling fails anyway the exception is swallowed and logged. A save that failed because the audit trail was unwritable is the worst available reading of the word «audit» — the operation the trail exists to record is the one it prevented. Asserted by pointing the spool at a path under a regular file, which can never be a directory. - empty details are
null, not[]. The column holds JSON and[]re-encodes as an empty array, so a reader distinguishing «no details» from «details that happen to be empty» gets the wrong answer and the panel renders an empty object where it should render nothing. - the entity can be named, falling back to the model name. Several classes can be facets of one record, and a timeline filtered by entity is only as good as the name the writer chose.
Plus withoutChangeEmission(), which exists for the one operation whose physical shape is not its
meaning: a soft delete is an UPDATE that means DELETED, so the automatic feed would file it as
«updated» and the caller silences that to emit the truthful event itself. Three tests, because the
finally and the saved-previous-value both earn their place: the flag is restored when the callback
throws — otherwise one failed soft delete silences the feed for every save after it in the same
request — and nesting restores the outer state rather than setting false, so an inner suppressed
operation finishing does not un-silence the one around it.
Pagination, and an arithmetic quirk I did not expect¶
The failures pagination has are all plausible-looking, which is why each is asserted separately: a page that repeats one row and skips another is a listing somebody scrolls past; a total taken from the page puts «10 results» under a table of ten on a set of four hundred; zero items divides by zero. The two pages either side of a boundary are compared for overlap rather than checked one at a time, because an offset one too small and one too large both still return a page of results.
The one that is invisible until much later: the primary key is forced into the select. A caller asking for one column gets two, deliberately — the rows become models, and a model without its key cannot be reloaded or saved. Omit it and the listing renders perfectly while every edit link on the page points at nothing, which is found by a person rather than by a test unless the test is this one.
Two of my own assumptions were wrong, and both are now documented rather than changed:
abs()is applied after the decrement. So?page=-1becomes offsetitems × 2— the third page, not the first. The guard's job is preventing a negativeOFFSET, which is a syntax error on both backends and would turn a public listing into a 500; it was never normalisation. Odd, harmless, and load-bearing for anyone relying on the arithmetic, so the test characterises it._isnewis not public. Reading$model->_isnewfrom outside the class creates a dynamic property and answersnull, so my assertion that a loaded row is not marked new was asserting nothing at all. It reads the property where the class keeps it now — and the property matters: a loaded row marked new would insert a duplicate on save.
Suite: 14,247 → 14,279 tests, wall clock 2:32.
(The coverage run scheduled alongside this batch was stopped at 83%, so the percentage is unmeasured since 93.23%; the next iteration re-measures before ranking.)
An open redirect with the panel's own appearance vouching for it¶
Coverage re-measured at 93.48% (from 93.23%). Going after DevPanelController::returnUrlFor —
fifteen uncovered statements deciding where the panel's «Back» button points — found a real
vulnerability, and it was found by a test asserting the property rather than by reading the code.
The check was a plain prefix match:
For a base of https://example.com, the URL https://example.com.evil.test/phish starts with it.
So: an attacker registers a host whose name begins with yours, sends a signed-in administrator to the
DevPanel with that Referer, and the panel remembers it and renders it as the destination of its own
Back button. An open redirect, on an administrative page, with the panel's appearance vouching for
the link — and nothing in the markup looks wrong.
There were four copies of the check: the referrer on the way in and the remembered value on the
way out, in each of the static and instance versions of the method. They are now one
isOnThisSite(), which requires what follows the base to be a boundary — /, ?, #, or the end of
the string.
The first version of that fix required a /, and the suite immediately said why that is wrong:
/adminer?db=x is the Adminer page and has no slash after it, and the same comparison decides
whether a referrer is part of the panel. So Back started bouncing between the panel and Adminer
again — the exact bug the exclusion had been added for. Both boundary cases now have their own test,
so a later simplification has to argue with them.
The security guide has the rule, the working helper, and the two rules that go with it: check on the
way out as well as in, because a session value outlives the request that validated it; and escape
it, because it arrived in a header and is going into an href.
I also found the bug's neighbour in my own test: I had written the site's base as a literal
http://localhost, and sURL here is https://pramnosframework.test. Four assertions were
therefore checking that a non-matching referrer is refused — passing, and testing nothing. The base
is taken from sURL now, with an assertion that it is not empty.
An upload check with no table behind it¶
MediaObject's content check is what stops an upload being code, and it had never run. An upload
arrives with two claims and both are the client's — the extension it chose and the Content-Type it
sent — so a PHP script named holiday.jpg and labelled image/jpeg satisfies every check above
this one. finfo says text/x-php, and that is the only thing between the file and a directory the
web server answers for. Covered now, along with the .htaccess the upload directory gets: php_flag
engine off and a rewrite refusing .php, .phtml, .phar, because php_flag is silently
ignored on a server without mod_php — which is most of them now, so either mechanism alone leaves a
deployment where an uploaded script runs.
The check being too strict is asserted from the other side, and deliberately so: a spreadsheet
exported as CSV and named .xls has always been accepted here, and a check that started refusing it
would be a worse bug than the one it guards against — reported as «the site stopped accepting my
file», with nothing in any log.
And a gap worth naming rather than quietly working around. Every successful uploadFile() ends by
querying #PREFIX#media for a duplicate by md5, and the framework ships no migration for the
media table. The model has been here for years with no schema behind it, which is why the existing
media tests hand-roll the DDL themselves — the same shape as the emailtracking gap found earlier
this session, where a feature's inserts had been failing into a catch for years. So the refusals are
covered here (they return before the file is moved) and the accepted path is left asserted only
indirectly, with the reason written into the test rather than papered over with a hand-rolled table.
A smaller thing found on the way: UNITTESTING was defined inside one test's setUp(). A constant is
process-global and cannot be undefined, so whether the framework's upload seam engaged depended on
which test ran first — a second test using the same seam passed after that one and failed when run
alone. It is defined with the rest of the environment now.
Suite: 14,279 → 14,298 tests, wall clock 2:32.
A sweep that deleted whatever happened to be in var/¶
DaemonOrchestrator::cleanupStaleLockFiles() is what makes a crashed daemon recoverable: the daemon
claims its slot with a lock file and touches it as a heartbeat, so a kill -9 leaves the file behind
and the orchestrator believes the process is still running. The sweep clears those at startup.
Twenty-one statements, never executed — and covering them surfaced how far the sweep reached.
getManagedLockFileGlobPattern() returned '*', meaning every file directly in var/ older
than the stale threshold. But var/ is not a lock directory. On this checkout alone it holds
junit.xml and the migrations-*.lock advisory locks that stop two migration runs overlapping, and
deleting one of those because six minutes passed is precisely how two concurrent migrations begin —
which is the thing the lock exists to prevent.
The method's own docblock had already described the right shape: a narrow pattern
('{QUEUE_PROCESSOR_*,KAFKA_CONSUMER_*}' is its example) with '' offered as the way to skip the
scan. The default is '' now: a base class does not know which files are its locks, and an
orchestrator that wants the sweep names its own. Nothing in the framework overrode it, so '*' is
what every installation was running.
An existing test asserted the old default, which is how a dangerous default stays put — it had a test, so it looked considered. Its expectation is updated with the reason written next to it.
What the sweep must not touch is where the rest of the assertions went, since this is code that
deletes things at startup: a fresh lock survives (it belongs to a process running right now, and
removing it starts a second copy of the work — the failure the lock prevents, caused by the mechanism
meant to repair it); a lock exactly at the threshold is not yet stale (the heartbeat interval and
this threshold are configured independently, so a daemon one slow cycle behind must not be declared
dead); a .stop file is never removed, however old, because it is an instruction rather than a claim
and an old one is the normal case for a daemon that has stopped; and a directory matching the
pattern is left alone, is_file() being the only thing between this sweep and @unlink() on
cache/.
Nothing is printed when nothing was stale, which is every ordinary start — a line on each one trains the reader to skip the startup output, and that is where the lines that matter appear.
Suite: 14,298 → 14,306 tests, wall clock 3:09 (the machine is loaded again; 2:32 earlier today for 14,298).
The two tables that were never on the list¶
Pramnos\Media\MediaObject arrived with this framework's original import in April 2020. The
migrations were written six years later, in May 2026, by reconstructing the schema of a consuming
application — and that application does not use MediaObject. So media and mediause were never
in the source that reconstruction read: not omitted from a list, never on one. The only trace left
behind is a comment on users.photo, which documents a reference into mediause while nothing
created it.
There is a migration now, CreateMediaTables, and its shape came out of SHOW CREATE TABLE on a
running installation rather than from reading the model.
Three written-down shapes, none of them the running one¶
Before this, the schema was written down in three places and they disagreed:
description |
shortcut |
order |
extrainfo |
|
|---|---|---|---|---|
| Production | varchar(255) |
varchar(128) |
order |
present |
| The framework's own test | text |
varchar(255) |
order |
present |
| The media guide | TEXT |
VARCHAR(50) |
order_field |
absent |
The guide's version is the instructive one: somebody writing it hit the fact that order is a
reserved word and renamed the column in the documentation to get around it. Both the test's DDL and
the guide's SQL are gone; the migration is the definition, and the guide now describes it and says so.
Both tables in one migration, and why the date is the users table's¶
mediause.mediaid has a cascading foreign key onto media.mediaid, so media has to exist first.
Across two files that is a dependencies entry somebody has to get right and the runner has to
honour; in one up() it is the order of the statements and cannot be misconfigured. The date matches
create_users_table because these tables are that vintage — they were always part of the original
schema, and users.photo refers to one of them.
Two foreign keys are deliberately absent. media.userid and media.medialink both use 0 as a
sentinel — «no signed-in user» and «not a duplicate» — and uploadFile() finds the original of a
re-upload with where md5 = %s and medialink = 0. A key on either would reject that zero on the
first insert. There is a test asserting the zero is still acceptable, so a well-meaning key cannot be
added later without the suite saying so.
MySQL made the signedness explicit: increments() emits UNSIGNED, mediause.mediaid is
integer() → signed, and the key was refused with «Referencing column and referenced column are
incompatible». Production has both as signed int(11), which is why it works there. The idiom for
this already existed in create_organizations_table, put there for the same reason.
The framework would have shipped a table its own query builder could not filter¶
order and specific are both reserved words, and this is where the migration stopped being just a
migration. Grammar::quoteIfCaseSensitive() quotes an identifier only if it contains an upper-case
letter — added for PostgreSQL's case folding, with a docblock explaining that all-lower-case
identifiers are left alone so that no existing generated SQL changes. A reserved word is the other
reason a bare identifier fails, and it is invisible to that rule because reserved words are written
in lower case. where('order', 5) compiled to WHERE order = ?, a syntax error on both backends.
MediaObject never hit it: its queries are hand-written SQL with the backticks already in place. My
test hit it on the second assertion it made against the new table.
The grammar now also quotes a lower-case identifier that is a reserved word — the set that turns up as column names in practice, not the full SQL list. It keeps the original rule's promise, because quoting a bare identifier is always valid and the only SQL that changes is SQL that was already broken. 14,324 tests confirm nothing else moved.
What the new shape improves, and the catch-up for an older installation¶
Four differences from what is running, each chosen to be a widening rather than a change, so an
existing installation catches up with four ALTERs and nothing that works stops working. The media
guide now carries that SQL. In short: utf8mb4 instead of utf8mb3, because utf8mb3 cannot hold
an emoji and description/tags are free text; one index on md5 rather than the duplicate pair
production has; a new (module, specific) index on mediause, because three queries filter on
those and every one is a full scan today; and filesize/date as bigint, since int(11) caps a
file at 2 GB and a Unix timestamp at 2038. A fifth needs no action: every column gets a default, which
only widens what an insert may omit.
There is deliberately no index on media.module — nothing queries it, and an index nobody reads is a
write on every insert.
Suite: 14,306 → 14,324 tests, wall clock 2:31.
What twenty years left in the media class¶
Three defects, all found by writing the migration and all fixed without changing a signature or a column name. Each was invisible for the same reason: the failing path is one nobody exercises until the data is old enough.
An unreadable file got the hash of nothing¶
createMd5() did md5(file_get_contents($this->filename)). On a missing file file_get_contents()
returns false and md5(false) is md5('') — d41d8cd98f00b204e9800998ecf8427e, the same value
for every missing file. A re-upload is found with where md5 = %s and medialink = 0, so every file
whose bytes had gone was a duplicate of every other one, and the next upload of an unreadable file
could be linked to any of them.
Measured, not imagined: a production library of 4,551 files holds 14 rows carrying exactly that
hash. The hash is left empty now, which matches nothing — the honest answer for a file nobody can
read. The two inline copies of the same computation go through createMd5() rather than repeating it.
A thumbnail the reading process cannot load was a fatal, not a missing thumbnail¶
thumbnails holds serialised objects, and the class name is part of the serialisation — so who can
read a row depends on which classes that process has.
I got this wrong on the first pass and it is worth recording why, because the mistake is the more
interesting half. Seeing production rows serialised under an application's own thumbnail class — one this
framework does not declare — I concluded those rows would fatal wherever they were displayed. They do
not: the application that owns them declares exactly that class, aliased to that exact global name,
with the same eight properties this framework's Thumbnail has. Its own pages read its own rows
perfectly well and always did. A serialised class name is not missing just because this codebase
does not contain it.
The failure is real but narrower: a reader that cannot load the class. This framework on its own, a
second application sharing the database, a CLI process that never boots the first application's
aliases. There unserialize() yields __PHP_Incomplete_Class and getThumb() reads $thumb->reason
on every entry — reading any property of an incomplete class is a fatal error rather than a missing
thumbnail.
Entries that cannot be read are dropped on load now, and getThumb() falls back to an empty
Thumbnail — which it already did for a file with no thumbnails, so nothing new had to be taught to
the caller. unserialize('') returning false, and the foreach (false) warning behind it, is
handled by the same normalisation. The filter keeps any object that carries a reason, which is why
an application's own thumbnail class passes through untouched.
unserialize() is deliberately not restricted with allowed_classes. It would be better
hardening and it would also discard an application's own thumbnail class that loads perfectly well
today, which is a behaviour change for installations that are working. The filter achieves the part
that matters, which is not crashing.
The type it detected and threw away¶
uploadFile() reads the real MIME type with finfo to decide whether the content matches the
extension — the check that refuses a PHP script named holiday.jpg — and discarded it. So the
security decision was unauditable, and anything serving the file later had to re-guess the type from
the extension, which is exactly the claim that check exists to distrust. mediatype is no
substitute: it is a display family the class branches on ten times over, and cannot tell a png from a
jpeg.
There is a mimetype column now, filled from the value already in hand — and from a detection of its
own in addImage(), so the column is populated whichever way a file arrived. No validation is added
to addImage(): it takes a file the application already has, not one a visitor sent.
And a docs test that could not see migrations¶
Adding the migration's name to a guide failed DocsNameResolutionTest, which checks that every
Pramnos\… name a guide mentions resolves to something the framework ships — by scanning src/.
database/migrations/ is a composer classmap, so a migration is every bit as real and as
autoloadable, and the test was reporting a correct reference as broken. It scans both roots now,
which also means a wrong migration name in a guide is caught, where before it would have passed
unnoticed.
Suite: 14,324 → 14,330 tests, wall clock 3:10 (a loaded machine; 2:31 for 14,324 earlier).
media.thumbnails holds JSON now, and the column stopped naming a class¶
The question that started it: is serialize() a bad format for a column, and should it hold data
rather than an object? Yes, and the reasons had all just been demonstrated in the same afternoon.
serialize() writes the class name into the data. Four consequences follow from that one fact:
- Who can read a row depends on which classes the reading process has. Real example, found
earlier the same day: rows serialised under an application's own thumbnail class. The application that owns
them declares exactly that class and reads them fine — but this framework standalone, a second
application on the same database, or a CLI script that skips that autoloader gets
__PHP_Incomplete_Class, andgetThumb()reads$thumb->reasonon every entry. Reading any property of an incomplete class is a fatal error. - Renaming a property silently drops every stored value, because property names are in the payload too. No error at read time; the field is simply empty from then on. PHP 8.2 deprecating dynamic properties makes that worse, not better.
unserialize()instantiates classes and runs their magic methods. Write access to the column becomes a step towards code execution. The framework already knew this —Helpers::checkUnserialize()exists and passesallowed_classes => false, and a consuming application uses it in five places.MediaObjectused it in none.- Nothing outside PHP can read it. No SQL, no
JSON_EXTRACT, no reporting tool, no index on «thumbnails wider than 500px».
Thumbnail is eight scalar properties, no methods, no nesting — nothing JSON cannot carry.
What changed, and what deliberately did not¶
Storage changed; memory did not. getThumb() still returns a Thumbnail and its callers still
write $thumb->url. Only the column's contents are different, so no template and no call site moves.
No migration. The reader tells the formats apart by the first character — JSON starts [, PHP's
serialised array starts a: — and the writer only produces JSON, so a row converts itself the next
time its media object is saved. Nothing has to be backfilled and nothing breaks in the meantime.
A legacy row's objects come back as they are, not recast into Thumbnail. An application with its
own thumbnail class has been getting its own type out of those rows for years; converting them on read
would change what getThumb() hands its callers, for rows nobody has touched. Encoding, on the other
hand, copies whatever of the eight fields a foreign object has — so the first save after this change
converts the row and the class name is gone from the data.
An unencodable payload falls back to serialize() for that row, and logs it. Losing the
thumbnails would be worse than writing the old format once, and the reader accepts both anyway.
A THUMBNAIL_FIELDS constant declares the list once, because three things have to agree about it —
what is written, what is read, and what the class actually has — and adding a property while
forgetting one of the three is how a value starts disappearing on save.
The same question, answered differently for userdetails.value¶
User::_save() also serialises, and the same argument seemed to apply until the data was measured:
of 23,485 rows, 21,325 are plain text, 2,096 are serialised arrays, and 64 are serialised
objects — three model classes cached in _country, _location, _nationality. At 0.3%, changing that
column's contract to «must be JSON-serialisable» costs more than it fixes. Left alone.
The measurement did turn up something else: 2,141 rows whose fieldname is \0*\0_data or
\0*\0_isnew — mangled names of Model's protected properties, written as if they were user
settings. The framework already refuses to write them ($fixname = substr($fieldname, 3) is exactly
the de-mangling, and the _ guard then rejects them), so they are historical debris rather than an
active leak — but they are still loaded on every user read, which is 9% of that table's traffic.
Cleaning them up is the application's call, not the framework's.
Suite: 14,330 → 14,338 tests, wall clock 3:02.
LIKE '%…%' was a fatal error¶
Database::prepareQuery() is sprintf underneath, and it ended with:
with no check for whether there was anything to substitute. So in an argument-less query every
literal % was read as a format directive, and the ordinary victim is LIKE:
To sprintf the trailing %' is «pad with the next character» and there is no next character, so
PHP 8 raises ValueError: Missing padding character. The @ does not help — it suppresses
warnings, not exceptions. A routine LIKE query was a fatal error, and the method this one replaced
had the guard: if (count($args) > 0) { return @vsprintf(…); } return $query;
Reported with the surface measured: 82 lines across 15 files containing LIKE '% in one consuming
application, several in production code, plus four internal callers inside Database itself.
The requested fix would have changed something else¶
The filing asked for the predecessor's guard — return the query untouched when there are no
arguments. That fixes the crash and quietly breaks %%.
vsprintf collapses %% to %, and this method documents %% as the way to write a literal
percent, so callers have been getting % out of it for years. Returning the query as-is would leave
DATE_FORMAT(\created`, '%%c')as'%%c'in the SQL — which MySQL reads as a literal%followed
byc` rather than the month number. A wrong answer instead of an error, which is worse.
So the guard collapses %% by hand instead:
Both halves are asserted, on both backends: a LIKE pattern with no arguments survives and runs
against a real table; %% still collapses; a query with arguments substitutes exactly as before; and
a LIKE pattern alongside an argument works, which is what the reported failures were actually made
of. A %s with no argument now reaches the database as a malformed query rather than raising — the
same thing the predecessor did, and a caller that writes a placeholder without a value has a bug
either way.
And a test of mine that passed for the wrong reason¶
testThePreparedQueryRuns counted rows in users, and passed under a filter while failing in the
full run on one backend: whether that table exists at that point depends on which test ran first. It
creates its own table now. The same lesson as three other times today — a test that needs a schema
builds it.
Suite: 14,338 → 14,354 tests, wall clock 2:36.
«Show password», and a static flag that was process state¶
The first recommendation of web.dev's sign-in form guidance, and the reason is mobile: on a phone the commonest cause of a failed sign-in is a typo in a field nobody can read. Someone who cannot see what they typed retries the same wrong thing and then resets a password they never forgot.
Pramnos\Html\PasswordToggle::render($inputId, $showLabel, $hideLabel, $class) renders the control
beside an existing password input. Two decisions in it are worth more than the markup.
The button ships hidden and its own script unhides it. A control that cannot do anything is
worse than no control — without JavaScript a visible «show» button is something a person presses
twice and then distrusts the rest of the form. This way a no-JS visitor sees exactly the form they saw
before, and the field never depended on the script.
Only type changes. name, id and autocomplete stay exactly as they were, because those
three are what a password manager matches on: a toggle that renamed the field would stop it offering
the saved password, which costs the visitor more than an unreadable field does. Focus and the caret
position are preserved too — toggling mid-word and losing your place is the same frustration in a
different shape. Both are asserted, the second by checking the script does not contain
field.name, field.id, field.autocomplete or removeAttribute.
The flag the test caught¶
The first version kept a static «script already emitted» flag, so the script went out once per page however many fields there were. That looked obviously right and was wrong: a static is process state, not request state.
The screen-level test found it immediately — the test client runs the application in-process, so the
first rendered page set the flag and the second came out with a button and no listener behind it.
A long-running worker, or anything else that produces two responses from one process, would do the
same. A visible control that does nothing is precisely what the hidden attribute exists to prevent,
reintroduced by the optimisation.
So there is no flag. Every button carries the script, and the script guards itself in the browser:
the first copy binds a delegated listener on document, the rest return immediately. A few hundred
repeated bytes against a class of bug that only appears outside a plain request — which is exactly
where nobody looks for it.
Two tests of mine that measured the wrong thing¶
Counting toggles by data-pramnos-password-toggle gave four on a two-field page: the attribute name
is in the script too, as a selector and in setAttribute. It counts aria-pressed="false" now, which
is one per button and appears nowhere in the script.
And the id is checked rather than escaped — it reaches an attribute and a getElementById call, and
an id is a developer's constant, so anything outside ^[A-Za-z][A-Za-z0-9_:.-]*$ raises rather than
being quietly encoded into a control that addresses nothing.
Suite: 14,354 → 14,368 tests, wall clock 3:04.
Every new project gets it, and every old one is told how¶
The show-password control existed and was wired into one installation's four screens, which left two holes: a project scaffolded tomorrow would not have it, and a project already running had no instructions.
The scaffolds¶
All three themes, every password field: 51 controls across 36 files — seventeen fields per theme, which is the whole set. Not only the sign-in screens. Change-password, delete-account confirmation, an administrator setting somebody else's password, an SMTP password on a settings page: all of them are passwords typed by hand and misread the same way.
Two pre-existing gaps surfaced doing it. Eight fields had no id — smtp_pass, the
OAuth2/security password, the users/edit password — so they could carry neither a toggle nor a
<label for>, which is a practice these same views keep everywhere else. Six of them had a label with
no for sitting directly above. Both are fixed in the same pass, because the id is what makes either
work.
The rule is enforced, not remembered¶
ScaffoldPasswordFieldsTest reads the view files and asserts four things per theme:
- there are password fields to find at all — otherwise a wrong path reads as «everything complies»;
- every field has an
id; - every field has a toggle addressed to its id;
- every toggle points at a field that exists.
That last one catches what a rename does: the field becomes new_password, the toggle still says
password, and the control renders perfectly while addressing nothing. Nothing about the page looks
wrong.
This is the mechanism rather than the documentation. A password field added to a scaffold next year cannot ship without a way to read what was typed, because the suite says so — which is precisely how the control came to be missing from every screen of a real installation in the first place.
And a section for projects that cannot upgrade yet¶
The upgrade guide now carries «Show password, for existing projects»: how to find every field
(grep -rn 'type="password"' src/Views/), the one line to add beside each, and — for a project whose
dependency cannot move — a self-contained copy of the class to keep locally and delete on upgrade.
It names the four properties a hand-rolled toggle usually gets wrong, because that is the actual value
of the section: the button must ship hidden; only type may change, since name/id/
autocomplete are what a password manager matches on; focus and the caret must survive; and the
script must guard itself in the browser rather than being emitted once from PHP — a static «already
emitted» flag is process state, not request state, and gives the second response from a process a
button with nothing behind it.
Suite: 14,368 → 14,380 tests, wall clock 2:38.
Three attributes nobody sees on a desktop¶
Findings #3, #4 and #7 of the sign-in form evaluation, applied to all three scaffold themes and enforced by a test. None of them changes what a form submits; all three are invisible unless you are holding a phone, which is why they were missing everywhere.
- The username field is no longer capitalised or corrected. iOS capitalises the first letter and
autocorrects a field it reads as prose. A username that changed silently is, to the person typing
it, a wrong password. Three attributes, because they are three behaviours:
autocapitalizeis the first letter,autocorrectis the word, andspellcheckis the red underline that invites somebody to «fix» a username that was right. - A one-time code opens the numeric keypad. These fields carried
pattern="[0-9]{6}", which validates the value and does nothing to the keyboard, so a phone offered the alphabetic one for a field that accepts only digits. - The last field of each form says
enterkeyhint="go", so the keyboard's action key submits instead of offering a newline or a next-field arrow.
The mistake worth writing down¶
The first attempt matched <input\b[^>]*?> and rewrote the attributes inside it. That is wrong for
a PHP template, and it broke about a hundred scaffold views in one pass: an attribute value here
routinely contains <?php echo … ?>, whose > ends the match early — so the «tag» is a fragment, and
the appended attribute lands in the middle of PHP code. Every one of those files then failed to parse.
git checkout and a rethink. The safe form needs no parsing at all: insert straight after an
attribute that is certainly inside the tag.
src.replace('autocomplete="username"',
'autocomplete="username" autocapitalize="none" autocorrect="off" spellcheck="false"')
No brackets to find, no PHP to trip over, and the worst case is a missed insertion rather than a corrupted file. 33 files, clean on the first run.
The same reasoning shapes ScaffoldSignInPracticesTest: it anchors on autocomplete="…" and looks
for the companion attributes within a few hundred characters either side — comfortably one tag, never
across two inputs. Less precise than parsing, and it cannot corrupt anything. Which input is last
in a form cannot be decided that way at all, so that one is asserted per screen instead: every
sign-in screen has the hint somewhere, and a form with none is one where the keyboard offers «next»
on the field that ends it.
The upgrade guide's «for existing projects» section now carries these three as well, with the regex warning stated plainly.
Suite: 14,380 → 14,389 tests, wall clock 3:11.
A passkey inside the username autofill¶
Finding #2 of the sign-in evaluation, and it called this the largest gain available from what was already built — correctly: the ceremony, the endpoints and the credentials all existed, and the passkey was sitting behind a button most people never press. Conditional mediation puts it in the username field's own autofill list, so signing in is one tap on a suggestion.
PramnosWebAuthn.conditional(optionsUrl, verifyUrl) is the new half, started on load by pf-auth.js
when the page has both a field declaring autocomplete="username webauthn" and a
[data-pf-passkey-login] button to read the URLs from. It is left waiting: the promise settles when
somebody picks a passkey, which may be never, and that is the normal case rather than a timeout.
The interaction that would have broken the button¶
A browser allows one outstanding credentials.get(), and a conditional request holds it for the
life of the page. So switching this on naively refuses the next ceremony — the «Sign in with a
passkey» button stops working, silently, and nothing on the page says why. It was there yesterday and
does nothing today.
authenticate() now cancels any pending conditional request before starting its own, and
cancelConditional() is public for anyone wiring their own button. That cancellation is the whole
reason the feature is safe to ship, so it is what the test is built around.
Tested by running the shipped bytes¶
ConditionalMediationClientTest loads scaffolding/assets/js/pf-webauthn.js under Node against a
stubbed navigator.credentials that records what it was asked for — the same approach
HumanCheckClientAgreementTest uses, and for the same reason: a test describing what the script
should do would agree with itself and prove nothing.
Six scenarios, and writing them corrected the harness twice. The first version of the
button-cancels-conditional case reported aborted: 0 and passed — because its stubbed get()
resolved immediately, so the conditional ceremony had already finished and there was nothing pending
to cancel. It proved the opposite of what it claimed. The stub now keeps the first ceremony pending,
and the assertion is real: aborted: 1, two get() calls (one conditional, one not), and exactly
one assertion posted for one sign-in.
The other three stand-down paths are asserted too, each for a concrete reason: an older browser
without isConditionalMediationAvailable is never asked, because calling it is a TypeError on the
sign-in page; a browser that answers false gets no ceremony at all, because starting one that failed
would hold the single outstanding get() and break the button for exactly the browsers that need it
most; and cancelling is not an error, because the person used the password form and an error message
about a ceremony nobody asked for is worse than none.
And an anchor that stopped matching¶
Adding the webauthn token changed autocomplete="username" to autocomplete="username webauthn",
and ScaffoldSignInPracticesTest anchors on the former as an exact literal — so it found no username
field at all and would have reported «no username field found in tailwind» rather than a missing
attribute. It matches both spellings now. A literal anchor is safe to write with and brittle to
search with, which is the trade the test's own docblock describes.
Suite: 14,389 → 14,395 tests, wall clock 2:34.
The toggle was in the wrong place, and only a keyboard noticed¶
Reported by somebody using the form: the tab order on the sign-in screen was wrong.
It was, and I had done it. These forms carry no tabindex at all, which is correct — so the tab
order is the DOM order. The show-password control was placed in the label row, above the input,
because that looks tidier:
Tabbing off the username landed on the button instead of the password field. The form worked, looked right, and every automated check passed, because nothing was asserting order.
Worth noting what the tempting fix would have been: tabindex="-1" on the button. It removes the
symptom by making the control unreachable without a mouse — which is the opposite of the point, since
this exists for people who cannot see what they typed. Moving it after the field costs nothing and
keeps it reachable.
The framework's scaffolds already had it after the input, so only the wired installation was affected.
Both now have a test for the property rather than for the markup: find the offset of id="password"
and of aria-controls="password" and assert the second is greater. It reads the rendered page in one
case and the view source in the other, and it is the kind of assertion that would have caught this
before a person had to.
Restoring a setting that was never there¶
Found while chasing why /register rendered no form in the full suite. Two test helpers snapshot a
setting, change it, and put it back — and for a setting with no row, «putting it back» wrote an
empty string:
$this->restoreSettings[$name] = ($existing && $existing->numRows > 0)
? (string) ($existing->fields['value'] ?? '')
: ''; // ← a row that was never there
Absent and empty are not the same answer. A reader with a non-empty default falls back to it for the
first and reads the second as a deliberate blank — so a test could leave the application in a state
no deploy would ever produce, and the next test to read that setting gets a different answer
depending on the order. The snapshot records null for absence now and the restore deletes the row.
For auth_allow_registration both readings happen to mean «closed», so this was not the cause of that
particular failure — the register screen renders no form when registration is off, which is correct
behaviour and a precondition the test should set for itself. It does now. The restore semantics were a
real bug standing behind it either way.
And the restore itself had the same shape of bug¶
The first version deleted the row and then called Settings::clearSettings() to drop the stale
in-memory copy. clearSettings() empties the store including everything loaded from
app/settings/settings.php, and loadSettings() only runs from the constructor when $loaded is
false — nothing reloads that file inside a running process. So a tearDown meant to forget one setting
handed the rest of the run an installation with no settings at all, and it surfaced two files away: a
mail wrapper that rendered its body with no template, and a cache dashboard listing a different
backend from the one the test had written to. Both passed in isolation.
Settings::deleteSetting() already does the right thing — the row, that one key, and the SQL-cache
entry, leaving the rest of the store standing. clearSettings() keeps its behaviour, because tests
across the suite rely on it clearing everything, and gained a docblock saying what it costs and what
to reach for instead.
The «show password» control is an eye inside the field¶
Two reports, one fix. It rendered as a text button — a bold «Show password» under the box, louder than the field it belongs to, for something most people never press. And it had been written into the label row above the input, which is where the tab-order complaint came from.
It renders an inline SVG eye now, and its own script moves it inside the field's right-hand edge at
runtime: the input gets wrapped in a position: relative span, the button is positioned absolutely,
and the field gains right padding so a long password does not run underneath the icon. No view and no
theme changes for it, and there is no stylesheet to ship.
Doing the placement in JavaScript is what lets both requirements hold at once. The button has to come
after the input in the document — with no tabindex on the form, tab order is document order — and
absolute positioning moves it visually without moving it in the document. tabindex="-1" would have
fixed the tab order too and is the wrong answer: it makes the control mouse-only, removing it for the
visitor most likely to need it.
The words are the accessible name now rather than the visible content: aria-label and title carry
them, swapped along with aria-pressed and the icon so a screen reader hears the new state. The icon
is sized in em and stroked in currentColor, so it fits a theme nobody told it about and needs no
icon set, no build step and no network.
Thirty-six scaffolded views dropped the btn btn-ghost btn-xs argument they were passing: a theme's
btn brings padding, a border and a background that fight the positioning. The $class argument
stays for a caller that wants to take over.
Suite: 14,395 → 14,400 tests, wall clock 2:35.
Two things a sign-in form never said: that it failed, and that it was working¶
Findings five and six of the eight, and the last two with any work left in them. Both are invisible on the machine of the person who wrote the screen, which is why neither had ever been noticed.
The errors were announced to nobody¶
An alert alert-error box is a red rectangle to anybody who can see it and nothing at all to anybody
who cannot: no role, no aria-invalid, no aria-describedby. A screen reader read out an unchanged
sign-in form and never said why the submission failed. Of the eight findings this was the one touching
accessibility.
190 alert boxes across 96 scaffold views got a role, split by severity: alert-error and
alert-danger are assertive and interrupt, alert-info, alert-success and alert-warning are
polite and wait for the next pause. A success message that interrupts whatever was being read is how a
well-meaning sweep makes a page worse.
But role="alert" alone would have been the comfortable half-fix. A live region is announced when it
changes, and a server-rendered error has been in the document since before the page existed — it
never changed, so most screen readers say nothing about it. What works with no JavaScript at all is the
description: the fifteen views that render a form error now give the box id="form-error" and point
their first field at it.
$errorFieldAttributes = $errorText !== ''
? ' aria-invalid="true" aria-describedby="form-error"'
: '';
The message is then read out as part of the field the moment focus lands on it, and focus lands there
on load because the first field carries autofocus.
The first field only, and the test asserts the count. These errors are form-level — «wrong username or password» is about the pair — and marking four fields invalid to report one failure tells a screen reader four things that are not true.
ScaffoldFormFeedbackTest found two real defects on its first run, both of the kind a review does not
see: a warning box in the mail viewer marked role="alert", interrupting to say something that could
have waited; and two boxes in the bootstrap settings screen carrying two role attributes, because
the sweep read backwards from class= to the opening < and could not see a role that came after it.
Nothing complains about a duplicate attribute — the first one silently wins.
Pressing submit changed nothing on the page¶
The human-check proof runs in a worker and the form waits for it. Meanwhile the form looks exactly as it did before the press, so somebody presses again — and a second sign-in attempt is not free, it is a failed attempt against a lockout counter.
pf-auth.js gained wireSubmitProgress(): a form marked data-pf-progress has its submit buttons
disabled on submit, gains a pf-busy class, gets … appended to whatever the button said, and the
form gets aria-busy="true". An ellipsis and not a spinner class, because this script ships with three
themes and is copied into projects with none of them — an indicator only one theme styles is an
invisible indicator.
The subtlety is the whole reason this is not three lines. A submit listener that called
preventDefault() did one of two opposite things:
- refused the submit — validation failed and the person stays on the page to fix it. Disabling the button here hands them a form that can never be submitted, which is a worse bug than the one being fixed and only appears when validation fails.
- held the submit — the proof is a moment from finishing and the form will go on its own. That hold is precisely when the second press happens, so it is the case this exists for.
Nothing in the event distinguishes them. So the indicator defers by a tick, returns if the default was
prevented, and markSubmitBusy is exported on window.PramnosAuth for whoever is holding to call —
which pf-humancheck.js now does while it waits.
Tested by running the shipped bytes under Node against a DOM small enough to submit a form in, five
scenarios. validation-refused and held-then-busy are the two halves of that distinction; if they
ever agree, the design has quietly broken and nothing else would say so.
Also: forgotpassword and register had every attribute the other screens had and no pf-auth.js
tag at all — correct-looking in a diff, inert in a browser. The test checks for the script as well as
the mark.
And the eighth finding was already satisfied¶
Filed as unverified, and measuring it was most of the work. Two of the three themes already had it:
plain-css declares font-size: 1rem on its shared field rule, Bootstrap's .form-control is 1rem,
and daisyUI raises --font-size to 1rem on focus for .input and .textarea.
.select keeps the smaller minimum, and the first instinct — add a rule «to be safe» — was written and
then deleted: tapping a <select> on iOS opens the picker wheel, not a keyboard, so there is nothing to
zoom for. The rule would have added a focus-time layout shift to fix nothing. What shipped instead is a
test pinning the one file the framework controls, with the measurement written down so the next person
does not repeat it.
Suite: 14,400 → 14,415 tests, wall clock 2:34.
Four faults in the media library, three of them silent¶
All four measured and filed against 5d7a37ee, all four reproduced here before being touched.
addRemoteImage() fetched any URL it was handed¶
Documented one line below addImage(), in the same tone, as though the two were the same operation
with a different source. They are not: one reads a file the application already has, the other makes
an outbound request from the server to an address that — for every caller shape the guide invites, an
importer, a «fetch the logo from their website» button, a picture-by-URL field — came from outside.
It did no host resolution, no private-range refusal, no scheme restriction beyond whatever the stream
wrapper allowed, no size cap, and no type check on what came back. $ext was taken from the URL's own
text and defaulted to jpg, so the extension said nothing about the bytes — and everything under
www/uploads/ is served back by the web server according to that extension.
The reachable-address half is the part that matters. 169.254.169.254 is not a private range, it is
link-local, and it is where every cloud provider serves credentials to whatever asks.
Pramnos\Security\OutboundUrl is new and is where this now goes. It resolves the host, refuses every
address inside this network across both address families, refuses a scheme that is not http/https,
refuses credentials in the URL, treats «does not resolve» as a refusal rather than as «nothing failed
the check», caps the body mid-stream, and — the part that is not obvious — dials the address it
approved with the name in Host:, because between an isPublic() that passed and a
file_get_contents() that follows sits a second DNS lookup that a hostile resolver can answer
differently. Redirects are off, since a 302 is a second address chosen by the server being fetched.
It is its own class because every feature of this shape needs the same check: a webhook target, a feed importer, an avatar by URL.
One test had to be rewritten to assert the opposite of what it said. testAddRemoteImageFromLocalFileUri
fed a file:// URL in to exercise the fetch without a network, and in doing so pinned the defect in
place — file:///etc/passwd is a perfectly well-formed URL, and the convenience and the vulnerability
were the same line.
0 meant «always», for four of the six ceilings¶
The guard was $startWidth > $this->medium, so medium = 0 was true for every picture that has ever
existed. It then resized to (0, 0), both dimensions fell to false, and ResizeTools substituted
its own defaultwidth — so the setting that reads as «off» produced a derivative for every upload,
at 120 pixels, a width the caller never named. Only max read the way it looked, because it alone was
wrapped in a != 0 check, and that inconsistency between the three was most of the trap.
All six now behave like max: an axis at 0 is not consulted, and both axes of a rendition at 0
means that rendition is not derived. $media->deriveNothing = true says the whole thing in one line —
it used to take six assignments, two of which meant the opposite of the other four, plus a number
chosen to sit above every real picture.
max = 0 silently capped every retrieval at 120 pixels¶
Found while proving the tripwire on the filing bit, which is the only reason it was found at all.
get() passed its own storage ceiling down as a bound on the request:
and ResizeTools reads maxsize as an upper bound on what may be asked for. With max = 0 every
requested width was over the ceiling, so every get() — at any size — came back at the 120-pixel
default. Measured: a 40×40 source asked for at 512 gives 512×512 with the defaults and 120×120 with
max = 0. So the one ceiling that read correctly for storage was the one that broke retrieval, and an
application that set it to protect its originals lost sized renditions without being told.
They are two questions and they have two settings now: max is «do not rewrite what I stored»,
maxRequest is «how large may a derived image be». maxRequest at 0 falls back to max and then to
ResizeTools' own ceiling, so nothing changes for a store that sets neither.
get() invented pixels, and an SVG became a picture of an error message¶
ResizeTools::resize() had no floor. Asking a 40×40 PNG for 512×512 wrote 512×512 of stretched blur —
on disk, recorded in thumbnails, served to a browser as a real rendition, and larger than the
original it came from, which is the opposite of what a thumbnail is for. Requests are clamped to the
source's own dimensions now, scaled by the limiting factor so the requested aspect survives: 512×256
of a 40×40 source gives 40×20, not 40×40. allowUpscale keeps the deliberate case possible.
The SVG is the same fault one layer down, and it is the one worth reading twice. MediaObject accepted
an SVG and recorded it correctly — mime=image/svg+xml, x=200, y=100, error=false, all right.
Then the first request for a size replaced it: GD cannot decode SVG on an ordinary build, so
loadImageByType() returned false, makeErrorImg() drew the source path onto a 500×100 white JPEG,
and that was stored as the rendition — at a URL ending .jpg, with thumbnails recording 128×64.
Nothing raised and nothing logged, because a JPEG of an error message is a perfectly valid JPEG and
every check downstream accepted it.
Three separate changes, in the order that matters:
makeErrorImg()is behinddebug, where it always belonged. Outside it,resize()writes nothing and returnsfalse, so the caller can decide.get()on a vector returns the original at its real dimensions. An SVG is already every size, so the original is the rendition.mediatypestays1— it is an image, and every application's ownmediatype == 1branch means «this is a picture»; a new number would route vectors into whatever those branches do with a type they do not know.get()on anything else GD cannot read returns the original, setserror, and records nothing. A row pointing at a file that was never created is worse than no row: the nextget()at that size finds it, fails thefile_exists()check, deletes it and saves — one write per request, for ever.
A Thumbnail's x/y are read back from the file that was written rather than left as what was
asked for. They diverge exactly when something has gone wrong, which is when a recorded size is worth
having.
And one the new tests found on their own¶
loadInfo() destructured getimagesize() straight into three properties. For a file that is not an
image that returns false, so the assignment warned three times — «Cannot use bool as array» — and
left width, height and type at whatever they held, after which the run continued on those values
and failed further in. It surfaced as an odd warning from a corrupt upload rather than as «that is not
an image».
Application::exec() got a seam, and four branches ran for the first time¶
Not part of the same work, but in the same commit because it is the same shape of finding. exec() had
eight hits across the suite and thirty-two statements that had never executed — and they were not error
handling, they were the declarations an application makes about itself in app.php and then never sees
applied: the scripts and css loops through which every globally registered asset passes, and
forcessl.
forcessl could not be reached from a test at all: it reads sURL, a constant the suite's bootstrap
defines as an https address, so a redirect whose entire purpose is to happen on every insecure
request had never run once. siteUrl() is a protected method returning that constant, which is the
whole change, and the branch is tested now — including that it is a 301, because a permanent
redirect is cached and the second visit never makes the insecure request, where a temporary one asks
for the password over HTTP again tomorrow.
Suite: 14,415 → 14,463 tests, wall clock 2:35.
The reconnect nobody could reach, on both engines¶
Database::execute() carries a re-prepare-and-retry path for a connection that has gone away.
Twenty-five of its statements had never executed once across 14,000 tests, and the reason turned out
not to be «nobody wrote a test» — it was that the path could not run, on either engine, for three
separate reasons.
This is the failure it exists to survive, and it is worth stating before the causes. Every connection
has an idle timeout, and the processes that hold a handle longest are the ones nobody watches: a queue
worker, a scheduled command, a daemon. They prepare a statement once and execute it for hours. A
restarted database, a failover, an operator's KILL clearing a lock, a pooler recycling a backend —
all the same thing. The symptom is not an exception in a request somebody sees; it is a worker that
stops doing its job quietly, and a queue that grows.
MySQL: the gate was unreachable code¶
if ($statement->execute()) {
…
} else {
if ($retry && (mysqli_errno($connection) == 2006 || … == 2013)) {
mysqli's default error mode has been MYSQLI_REPORT_ERROR | MYSQLI_REPORT_STRICT since PHP 8.1:
mysqli_stmt::execute() throws rather than returning false. So the else — and the 2006/2013
check, the re-prepare, the retry inside it — was dead code, and a lost connection came out of
execute() as an uncaught mysqli_sql_exception.
The exception is caught and turned back into the false the surrounding code was written against, so
one branch serves both error modes. isConnectionGone() reads the exception's code and its message,
because the code is 0 when the driver raises it from a state where errno was already cleared.
PostgreSQL: the gate was asked at the one moment it lies¶
Here pg_execute() returns false rather than throwing, so there was no unreachable branch. The gate
was !isConnectionAlive($connection), which asks pg_connection_status() — and that reports the last
known state rather than polling. Measured against a terminated backend:
| when | pg_connection_status() |
|---|---|
| before any operation | OK |
at the instant the failing pg_execute() returns |
OK |
| afterwards | BAD |
The gate sits in the second row. It reads the error text now — «server closed the connection unexpectedly» is there at exactly that point — with the status kept only as a secondary signal.
PostgreSQL, again: the statement cache handed back a dead plan¶
And with the gate fixed the retry still failed. The prepared-statement cache is keyed on
md5($query) alone:
Which is right for what it was written for — the same query prepared twice in one request costs one
PREPARE — and wrong the moment the connection changes underneath it. A prepared plan does not
outlive the session that made it, so after prepare() correctly reconnected, the cache handed back a
plan name PostgreSQL had already forgotten, and every execute of it failed for ever. The cache is
scoped to the connection that filled it.
How it is tested¶
By killing the connection from a second connection, with the server's own facility: KILL <id> on
MySQL, pg_terminate_backend(<pid>) on PostgreSQL. Not by closing the handle from PHP, which the
driver knows about and which is therefore a different code path — the point is a connection PHP still
believes in and the server has already forgotten, which is what production hands you.
Three tests per engine, and the second and third exist because the first is not enough:
- the statement executes and returns the right rows. A retry that reconnected and returned an empty result would read as «the table is empty» to every caller above it, which is worse than an error: a worker acting on nothing looks like a worker with nothing to do.
- the handle keeps working afterwards, for entirely new statements. The retry reassigns
$statementand$connectioninside the loop, and a test that stopped at the first execute would pass over an incomplete assignment. - a retried write lands exactly once, which is what pins the «only when the connection was already gone» rule against a well-meaning future «retry on any error».
The kill is waited out by asking the server whether the connection is still listed, rather than by sleeping: 20 polls at 10ms instead of a fixed 200ms guess. Those six tests take 0.96s rather than 2.3s because of it.
Suite: 14,463 → 14,469 tests, wall clock 2:35 — unchanged, which the poll is what buys: the first version of these tests slept 200ms per kill and put the whole suite eleven seconds over its band.
Three sign-in methods at zero hits, and the seam that was in the wrong place¶
Account::renderForgot(), renderReset() and authlink() — twenty-four statements, zero hits across
14,000 tests. Not obscure code: two of them draw the pages somebody uses when they cannot get in, and
the third is the passwordless sign-in.
The reason they were at zero is the part worth writing down. AccountPasswordResetScreenTest drives
forgotpassword() and resetpassword() thoroughly — the anti-enumeration property, the CSRF refusal,
the spent link — and it replaces the two render methods with a recorder, on the stated grounds that a
real render would need a view stack to assert one string. That is a fair trade for what that file is
about, and it is also exactly how a method ends up never running while the action above it is well
covered. A coverage report says «this line never ran»; it does not say «because the test that would
have run it stubbed it out one level too high».
So the seam moved down a level: getView() is stubbed instead, which is the one collaborator these
methods have, and everything they actually do then executes.
What is worth asserting about a renderer is not the HTML — that is a theme's business — but the
handful of values a view cannot work without and cannot ask for itself. routeBase, because every
link the page draws is built from it. humanCheck on the forgot page and deliberately not on the
reset page: the forgot form sends mail to an address the submitter chose, which makes it the cheapest
way to use somebody else's site to deliver one unwanted message at a time, while the reset form is
reached only by holding a link that was already mailed. And that the caller's context wins over the
fixed one — a renderer that assigned the fixed values after the copy loop would silently drop every
error key and flash, and look perfectly right on the happy path.
For authlink() the assertion is the redirect rather than the absence of an error: a link that
authenticated somebody and then left them on the sign-in page is, to them, indistinguishable from a
link that did not work, so they ask for another one.
And a blank page nobody had hit¶
Testing revokeapplication()'s refusals turned up a real one. The non-AJAX redirect sat at the end of
the action, after the try/catch — so the two early returns above it, «client_id is required» and
«Application not found», skipped it. A browser form hitting either got a flash message queued for a
page that was never rendered: a blank response, and the message surfacing later on whatever the
visitor opened next. The XHR caller was answered correctly in both, which is why nobody found it.
Ending the response is one decision, so it is made in one place now — sendRevokeResponse(), for all
four outcomes.
Suite: 14,469 → 14,479 tests, wall clock 2:35.
The same schema, two different generated APIs¶
create:api reads a table and writes a controller, column by column, through a type switch. Two arms
of that switch had never run, and covering them turned up an asymmetry worth knowing about.
The JSON arm. case "json": had never executed, which matters more than its seven statements
suggest: the string arm is the switch's default, and a JSON column falling through to it is written
with strip_tags. strip_tags on a JSON body eats every < in it, so {"a":"1<2"} is stored as
{"a":"12"} — valid JSON, different data, nothing raised. A missing case in a switch whose default
silently corrupts is not a loud kind of bug.
The boolean arm is unreachable on MySQL, and that is the finding. getColumns() reports a
PostgreSQL BOOLEAN as boolean and a MySQL TINYINT(1) as tinyint, and the type switch lists
tinyint among the integers. So the identical schema generates:
| PostgreSQL | MySQL | |
|---|---|---|
| apidoc | @apiBody {Boolean} [flag] |
@apiBody {Number} flag |
post block |
$tmpVar truthiness dance |
staticGet(…, 0, 'post', 'int') |
Documented rather than changed. TINYINT(1) in MySQL genuinely is a small integer that convention
treats as a flag; the column type does not record which was meant, and a generator that assumed «flag»
would mangle every TINYINT(1) somebody uses as a number. But the two differing is a real thing, and
the person who finds out is an integrator reading the apidoc for one deployment while calling the
other — so it is now a test that names both answers, in the class whose PostgreSQL lane already exists
because the primary key is detected differently on the two engines.
The PostgreSQL half pins something quieter too: that arm brackets every boolean as optional,
nullable or not, because it never consults Null. A NOT NULL flag is documented as something the
caller may leave out.
And appName. $application->appName is how one repository serves more than one application, and
the three statements that honour it in createApi() had never run. Both halves have to move together —
a namespace that gained the segment while the path did not produces a file the autoloader cannot find,
which surfaces as «class not found» about a file that plainly exists. Asserted as both, in one test.
Suite: 14,479 → 14,485 tests, wall clock 2:37.
Removing a retention policy had never been done¶
SchemaBuilder has two implementations of every policy behind one signature: a native TimescaleDB one
and a software one that writes a row in pramnos.framework_policies for the PolicyEngine daemon to
act on. The software half is what runs on MySQL and on plain PostgreSQL — most deployments — and
removeSoftwarePolicy() had never executed once.
That is the shape of gap that costs data rather than uptime. A retention policy is «delete rows older
than this», so a removal that silently did nothing leaves a daemon deleting from a table somebody has
decided to keep, and the operator's evidence that they stopped it is a method that returned true.
Both lanes run the same assertions, which is the point: the contract is that a caller cannot tell
which machinery answered. MySQL takes the software path, and the container with the extension takes
the native one — where policyInterval() has a detail no software store has, an interval living in a
JSON config under a key TimescaleDB has renamed across versions, so it tries drop_after,
compress_after and older_than in turn.
The duplicate-registration test skips itself on the native lane and says why in its own message.
Idempotence is the extension's problem there; the defect it guards was in the software store, where
the check meant to prevent a second row answered a flat false off TimescaleDB — so every run of the
ensure command added another policy, and N identical policies issued the same DELETE N times against
the same table.
Three helpers around it were covered on the way:
primaryKeyColumns()against a composite key, because a single-column fixture cannot tell «returns the primary key» from «returns the first column it found» — and the order is what every hypertable check depends on, since TimescaleDB requires the partitioning column to be part of the key. A table with no key returns an empty array rather than an array holding an empty string: a caller doingin_array()behaves the same either way and one doingcount()does not.withSchema(), which returns a scoped clone. The reason it is a clone is the assertion: a caller reaching into thepramnosschema for one statement must not silently move every later statement there too.withSchema('')clears the override rather than scoping to a schema called nothing, which would produce"".tableon PostgreSQL and_tableon MySQL — both failing at the server, one statement later, about a table nobody named.
Suite: 14,485 → 14,499 tests, wall clock 2:35.
The forty-two statements that run Adminer¶
Adminer::serveAdminer() is the largest single never-executed unit in the framework, and the reason
is mundane: locate() looks for vendor/vrana/adminer or vendor/dg/adminer-custom, neither of which
this repository installs, so the route always answered «not found» and the body was unreachable from
any test that went through the front door.
It takes its entry point as an argument, though — so it can be handed a script the test writes, and every decision it makes around the include becomes observable. Which is the interesting part, because two of those decisions are security decisions:
unset($_POST['auth'], $_POST['logout']). Adminer's auth.inc.php acts on $_POST['auth'] —
driver, server, username, password, database — before anything else. Removing its login form took away
the page that submits that, not the ability to submit it: a hand-made POST, or a form on another site
aimed at this URL, would have logged this Adminer into any host reachable from the server with any
credentials the sender knew. Asserted from inside the include, where Adminer would read it, because
a route that unset the keys and then restored them would pass a check made afterwards.
$_SESSION = array(), and not merely session_write_close(). Closing writes the data and releases
the handle; $_SESSION keeps its contents in memory, and Adminer reads them when it decides not to
start a session of its own. token is the collision that made this visible — its CSRF token is
rand() ^ $_SESSION["token"] and this framework's value is a hex string.
And the buffer callback, which is a fixed bug whose own comment says it was invisible. Adminer is a
script and several of its paths end with exit, so the ob_get_clean()-and-rewrite that used to sit
after the include never ran: PHP flushed at shutdown and the page went out with ./static/default.css
links, which resolve to /static/… from /adminer. No stylesheet, and it looked like a broken tool.
exit cannot be exercised from inside a test — it takes the runner with it, which this found out the
direct way. So what is asserted is the structural fact underneath: serveAdminer() returns with the
buffer still open, and the rewrite appears when the test flushes it. Under the old implementation
that same flush would produce the raw bytes, because the rewrite was code that had already been
skipped. The distinction survives without the exit.
Also covered: the open is audited (the only record of who opened the database tool — Adminer keeps
none, and the web server's log says a URL was fetched rather than which account fetched it), the
working directory is restored, a throwing script produces a 404 with no half-written page, and a
prepareLogin() that has already redirected includes nothing at all.
Suite: 14,499 → 14,505 tests, wall clock 2:36. Coverage 93.56% → 93.70%.
The DevPanel's refusals, and two detail screens nobody had opened by id¶
The panel's happy paths were already covered — the queries each card builds, the windows they respect, the empty states. What had never run was the other half.
raw()'s three refusals. It answers 404 when the devpanel feature is off, not 403: a 403
confirms the panel exists on this server and is merely closed to you, which is a fact worth not
publishing about a tool that browses the database. The test asserts that nothing follows it —
renderError() is declared never, and this is what establishes the declaration is honoured rather
than merely written.
The file-name refusal is the one worth reading. There is exactly one directory to read from and it is
not the caller's to choose, so the name from the query string is compared against the known list rather
than joined to a path — ../../.env fails an in_array() and never reaches a filesystem call. Which
is why the assertion is on the message rather than on the absence of a crash: a traversal attempt and
a typo get the same answer, and that is the design.
A detail screen for an id nobody has. renderTokenDetail() and renderUserLog() each check for
the row and stop; without that check the page would render with every field empty, which reads as «this
token exists and has no activity» rather than «there is no such token». Two tests rather than one data
provider over both, because they are separate methods with separate queries — a provider would assert
that one of them behaves correctly twice.
And the Adminer tab's position. It is inserted after the Database tab, because Adminer is the other way to look at the database and reads as noise anywhere else; and when there is no Database tab to sit beside it is appended rather than dropped, so an installation that hid that tab does not lose the tool to «type the URL yourself».
Two things this class does not cover, with the reason on record: guardAccess()'s 403 branch needs
isDevMode() to be false and that method is private and reads a constant, and renderLogViewer()'s
failing-requests block needs seeded RequestLog entries on disk. Both are reachable, neither is
reachable cheaply, and a comment saying so is worth more than a test that stubs its way to the line.
Suite: 14,505 → 14,512 tests, wall clock 2:33.
A redirect is a second address, and the API said so without letting anybody act on it¶
OutboundUrl::fetch() shipped this morning with redirects off and a docblock naming the correct
pattern: following redirects safely means re-running isPublic() on each hop, which is the caller's
decision to make with the Location header in hand. A caller could not make it. fetch() returned
string|false and a $reason, and ignore_errors => true meant a 302 came back as a successful
fetch of an empty body — no status, no Location, the header array living and dying inside the
method. The three things available were: refuse every redirect, follow them unchecked with your own
file_get_contents(), or reimplement fetch(). The option the docblock described was the one that was
not there.
That is a real gap and not a theoretical one. Refusing redirects outright is usually not open to a
caller: an address that has sat in a catalogue for years is very often an http:// that now redirects
to https://, or a path a CDN has since moved, and refusing those is safe and useless.
maxRedirects, with the check re-run per hop¶
Each Location is resolved against the address that sent it and passed through isPublic() before it
is dialled; a refusal on any hop fails the whole fetch with a reason saying which. Done inside rather
than handed out as headers, because that is what keeps the guarantee a caller cannot reconstruct: the
address that was checked is the address that was dialled.
And an unfollowed redirect is now a failure rather than an empty success, which is the trap
underneath the original report. false and a reason is the only honest answer available at
maxRedirects = 0.
The fiddly part is a pure function, and that is why it is tested properly¶
nextHop($from, $responseHeaders, $reason) returns the next checked address, null when the response
is not a redirect, or false when it is one that must not be followed. null and false are
deliberately different: a loop turns on the difference, and a falsy check collapses them and makes
every ordinary 200 a failure.
resolveLocation($from, $location) is the one worth not writing twice. Four shapes arrive in the wild
and only one of them is a URL:
Location |
resolves to |
|---|---|
https://b.test/logo.png |
itself |
//b.test/logo.png |
inherits the scheme, not the host |
/logo.png |
same host, absolute path |
logo.png |
relative to the directory: /a/b + c is /a/c |
The protocol-relative row is the one that bites: read as a path, it turns somebody else's host into a
directory on yours and the fetch goes somewhere nobody checked. .. and . are collapsed, because the
address that gets checked has to be the string that gets dialled.
Being pure is what makes this testable at all. Every address the class would follow to is by
definition outside this network, so a live redirect cannot be reached from a suite that makes no
network calls — the loopback server that would serve one is exactly what isPublic() refuses. Nine
resolution cases, the last-Location-wins rule, a redirect with no Location, a redirect to another
scheme, and the finished-chain case where the status must be read from the last status line rather
than the first.
MediaObject::addRemoteImage() follows three hops by default ($remoteMaxRedirects), for the reason
above: an image address in a catalogue is usually not exact any more, and every hop is checked.
Suite: 14,512 → 14,528 tests, wall clock 2:37.