CalendarState was the string "fixed", and its docblock explained that this
was a confession rather than a design: events are written from the sync
engine, from extraction, from the web editor and from CalendarEvent/set,
and a log recording a quarter of the writes "is not a weaker version of
it, it is a lie with a number on it". So canCalculateChanges was false,
there was no CalendarEvent/changes, and every client re-ran its query.
calendar_change_log closes that. The rows are written by a Doctrine
listener rather than by a recorder the writers call, and that is the whole
point — CalendarEvent exposes public mutable properties, so fields move
wherever somebody assigns one and lets the flush carry it. An RSVP in
InviteResponder, a SEQUENCE bump in EventReconciler, the recurrence flags
in RecurrenceMaterialiser: none pass through CalendarEventWriter, so a
recorder wired into write() would have missed all three and every deletion
besides. Doctrine already knows what changed; asking it is the only way to
be complete without asking every future caller to remember.
CalendarChangeLog the log. Its own table rather than new
JmapObjectType cases on jmap_change_log, which
CalendarState suggested: that table is keyed by
account_id, and CalendarAccountResolver says a
user can delete their last account and keep a
calendar. CalDAV also counts per collection, and
there is nowhere to put the collection.
the listener onFlush collects, postFlush writes. An ignore
list, not an allow list, so a field added later
defaults to being logged — over-recording costs a
wasted conditional GET, under-recording is the
silent failure.
CalendarChangeReader tokens and deltas, protocol-neutral, in both
scopes: per user for JMAP, per calendar for the
CalDAV server this is the groundwork for.
Two bugs fell out while building it.
The first was mine and is why postFlush writes SQL rather than persisting
entities. Flushing from inside postFlush re-runs the whole unit of work,
and the second pass sees what the first left behind: EventDismisser
removes an event while an EventSourceLink still points at it, which is
legal once and throws "a new entity was found through the relationship"
twice. It broke every calendar deletion in the suite and five demo-flow
tests. A plain INSERT stays out of the UnitOfWork entirely, so this
listener cannot make somebody else's flush fail.
The second is subtler: a flush that throws never reaches postFlush, so the
queued rows survived to be written by the next flush that succeeded —
logging changes the database had rolled back. onFlush clears the queue as
it starts. A worker whose EntityManager is reset and carries on is exactly
where that would have bitten.
CalendarEvent/get, /query and /set now return real sequences. Calendar/get
still does not: the log records events, not the collections holding them,
so a rename or a recolour moves nothing. That is a smaller gap than it
sounds — a user has a handful of calendars, Calendar/get returns all of
them by default, and CalDAV discovers the collection set with a PROPFIND
that holds no token. CalendarState now says so instead of describing a
world that no longer exists.
Also marks config/reference.php optional in phpstan.dist.neon. It is
gitignored and generated, every sibling entry in that exclude list already
carries (?), and without it PHPStan refuses to start on a clean checkout.
No deployment change. One new table, created on boot.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`paths: ./` walks the tree with a RecursiveDirectoryIterator *before*
excludePaths are applied, so a directory it cannot read is a hard crash rather
than a skip — and no exclusion can prevent it, because the crash happens during
enumeration. `.claude/worktrees` holds entire git worktrees of this repository,
37 of them here, several carrying a root-owned `var/secrets` left behind by a
container. `composer stan` therefore died on a directory belonging to a
different checkout of itself, and adding the path to excludePaths did nothing
at all.
The top-level directories that hold PHP are named instead. CI on a fresh clone
was always fine, which is exactly why this could sit here: it only breaks for
somebody running the gate on the machine they work on.
The existing excludePaths are left as they are. Most are no longer reachable
from `paths` and are belt and braces now, but they still document what is not
project code, and they come back into force the moment anyone widens `paths`
again.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Controller/Admin/User/DefaultController was unreachable dead code: it
referenced App\Form\Admin\UserFormType and User::setName(), neither of
which exists, used a ParamConverter attribute that is neither imported
nor installed, and both its templates were missing. Nothing linked to it,
which is why every route fatalling on load went unnoticed.
Removing it was the other option. Implementing won because the gap is
real: plMail's data model is multi-user throughout — accounts hang off a
user, and the Gmail webhook already reasons about two users connecting
the same address — but there was no way to create the second user.
InstallController makes the first administrator and InstallGuard then
404s it forever; app:setup returns early once any user exists. There was
no path, from the web or a terminal, to a second account.
Three things an administrator deliberately cannot do here, all for the
reason CONTRIBUTING.md gives for keeping 2FA removal out of the web UI —
an admin session must not become a second way into every mailbox:
- change an existing user's password. The field is on the create form
and not the edit form, and posting it anyway is refused twice over:
it is unmapped, and Symfony rejects the whole submission.
- remove anyone's second factor. Still console-only.
- read anyone's mail. Nothing here touches Account or Message.
Removal is a soft delete, because the rows hanging off a user are their
mail and a cascade from a misclick in an admin panel is not a recoverable
mistake. The address is freed so the same person can be re-added. Neither
your own account nor the last remaining administrator can be removed or
demoted — the second is the one that looks fine at the time and is only
noticed when nobody can reach the panel.
Two bugs in UserRepository fell out of using it, both the same mistake of
building a Doctrine expression and never passing it to andWhere():
createUndeletedQueryBuilder's soft-delete filter was a no-op, so every
caller including findOneByEmailExcept matched deleted rows; and
createSearchQueryBuilder ignored its argument entirely, so searching
returned the unfiltered list. Its LIKE pattern was wrong too —
sprintf('%%s%', $search) never interpolates anything.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
55k lines of PHP with no static analysis, and a dependency tree holding
four OAuth clients, a JWT library and an IMAP parser with nothing
watching it for advisories.
PHPStan at level 5 with a baseline. Level 5 is where the class of defect
this codebase actually has starts showing up — nullable entity getters
flowing into non-nullable parameters — without a fortnight of triage
first. 208 findings are baselined as a debt ledger, not a licence; the
level goes up when the ledger is small enough that raising it is a day's
work.
Two of the 217 were real bugs, both fixed here rather than baselined:
"Test connection" in account settings constructed ConnectionTestResult
with four arguments where six are required, and passed null for a string
parameter. Both call sites are the early returns for an incomplete form
and a blank password — that is, the two cases the button exists to
report, each of which raised an ArgumentCountError instead.
GraphCategorySyncer caught App\Service\Mail\GraphApiException, which does
not exist; the client throws App\Domain\Exception\GraphApiException. The
catch never matched, so the 403 that means "this account needs
reconnecting with MailboxSettings.ReadWrite" propagated as an unhandled
error instead of degrading quietly, which is what the comment inside it
says it is there to do.
One more it found is left alone deliberately, because it needs a decision
rather than a fix: Controller/Admin/User/DefaultController references
App\Form\Admin\UserFormType and User::setName(), neither of which exists,
so that admin page fatals on load.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
