mirror of
https://github.com/karatektus/pl_mail.git
synced 2026-10-25 01:54:10 +00:00
* Run the end-to-end suite on pull requests into milestone and wip branches
A release's work is now collected on a milestone branch (milestone/v0.4.0)
before it reaches main, and the suite only ran on pull requests into main,
so nothing proposed for the milestone was tested. `milestone/**` joins
`main`, and `wip/**` comes along from wip/28, where it already runs the
suite on the phases of #28 — the same block on both, so they merge
without a conflict.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit d009ff72ad)
* linked-issue: read the description of a pull request into a milestone branch (#254)
GitHub links an issue from "Closes #N" only in a pull request into the
default branch. Into milestone/v0.3.8 or a wip/ branch the closing
references stay empty, so the check failed on every such pull request
however its description named the issue (#252 was the first). For a pull
request into another branch, the description is now read for the same
keywords GitHub accepts.
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
* The release pull request is the suite run behind a tag (#252)
Releasing asked for the E2E workflow to be started by hand on main before
a tag. Since main became protected, the Release vX.Y.Z pull request cannot
merge until that workflow is green on it, and its commit is what is tagged,
so a manual run only ran the same jobs twice. Releasing now says so, and to
update the branch if main moved after its checks ran, since the protection
does not ask for up-to-date branches.
The paragraph in CONTRIBUTING and the header of e2e.yml that said main is
not verified commit by commit describe main before it was protected; both
now describe it as it is.
Closes #251
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
* Calendar on a phone: a readable month, a toolbar and top bar that fit, a live now line (#256)
The month chip is its title first, on up to two lines, then its time range
and place; a narrower day drops the end and the place for a third line of
title, long words are hyphenated, and an unanswered invitation has a
dashed bar. Below 39rem, where a day is bars, the grid shrinks and the
chosen day is listed in full under it: today at first, any tapped day
after, with a + for that day that the toolbar's + follows too. On a wider
touch screen the first tap opens a day's card and the second its entry
(calendar--month-tap, and a day-open: variant that the hover and the tap
share).
The calendar toolbar fits below 37rem with icons, a short date label and
smaller buttons, and under 22rem today's date in a box for Today. English
writes a same-month week as "Oct 20-26" (range_short_month). The top bar
tightens below 640px and moves Sync now, What's new and Send feedback into
the user menu. Dialogs are the whole screen below 768px, as compose is.
calendar--now moves the now line every minute on the view's clock and
redraws a view that shows the old day or the new one when the date
changes, which also moves the month's circle on today.
And a + on a day that has gone opened the editor on today:
DefaultEventStart::onDay pulled any past day to the next hour. A past day
now keeps its date.
Closes #253
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
* Close an issue when its pull request is merged into a milestone branch (#260)
GitHub acts on "Closes #N" only for merges into the default branch, so
issues fixed on a milestone branch stayed open until the release reached
main. A workflow on merged pull requests into milestone/** now closes the
issues the description names, with a comment linking the pull request.
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
* Preview the reply, not the quote, and send replies with their quote visible (#258)
The list preview flattened the whole body and skipped short blocks to find
the first long one, so a one-line reply previewed as its attribution line or
the quoted mail. The quoted history (same seam as the reading view's "···")
and a "-- " signature are cut off first now, in HTML and plain text, and the
chrome skip only applies to bulk mail. The JMAP preview uses the same code.
The composer folded the quote with an inline display: none and its clean-up
removed only the toggle, so replies went out with an invisible quote. The
fold is removed in the editor and again on the server, and the reading view
reveals a folded quote in mail already received.
Closes #257
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
* Move a thread's unread count when JMAP marks a message read or unread (#267)
EmailPatchApplier wrote seenAt for $seen and left MessageThread::$unreadCount alone, so the web list (which draws bold from that counter) disagreed with the phone app. $seen now goes through ThreadStatusUpdater::markRead(), the web UI's own path, and markRead() counts the whole thread rather than the messages passed in.
Closes #266
Co-authored-by: Philippe Delodder <7259755+phdelodder@users.noreply.github.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
* Leave a message that is already in the destination folder where it is (#270)
Trashing a conversation with a message already in the bin queued that message as a move from Trash into Trash. webklex's move() then left the connection in EXAMINE and the server refused it ("NO Not permitted in EXAMINE state"), and the row had already lost its UID. ApplyImapFlagsHandler no longer sends a move into the folder a message is in (and gives the row its UID back), and ThreadStatusUpdater's trash(), archive() and restore() leave such messages out of the move.
Closes #269
Co-authored-by: Philippe Delodder <7259755+phdelodder@users.noreply.github.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
* Hide a conversation from the lists only when all of it is in the bin (#272)
A thread's labels are the union of its messages', so one deleted message put Trash on the thread and took the whole conversation out of the Inbox, its tab, label lists, All Mail, Unread, the badges and search, along with any reply threaded into it afterwards. The bin rule is now "carries Trash and has no message outside it", written once (binnedThreads()) for every list and badge and spelled out the same way in the search SQL. Option (b) of the issue.
Closes #268
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
* Let a long message scroll in the expanded compose window (#273)
expandedValueChanged() used closest('div') to reach the editor's wrapper, but closest() starts at the element itself and the editor is a div, so the wrapper never became a flex container and the editor stayed 170px tall, clipped by [contain:paint]. The wrapper is now the flex column and the editor grows (1 0 auto) to fill the window or its content; the plain-text field fills the expanded window too.
Closes #262
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
* Write each changelog entry as a file of its own, collected at release (#275)
Every pull request added its paragraph under ## Unreleased in CHANGELOG.md at the same place, so the second of any two conflicted with the first once it merged, and each conflict fix re-ran the whole E2E suite. A pull request now adds changelog.d/<issue>-<words>.md; bin/changelog-release.sh, run in the Release vX.Y.Z pull request, collects the fragments (highlights first) and anything still under ## Unreleased into the release's section and deletes them. CHANGELOG.md stays the one file What's new, bin/release-notes.sh and GitHub read.
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
* Say which step failed when notifications can't be turned on (#278)
Every failure in the enable path (registering the service worker, the
browser's push subscription, the server's answer) ended in the same
"Something went wrong" and the exception was dropped, so a report from a
Pixel 8 could not say which. The pane now names the step and the
DOMException name or HTTP status, and logs the exception to the console.
Closes #277
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
* Contacts: one address book, with Google, Outlook, Nextcloud and CardDAV contacts in it (#282)
A contacts page and pane for plMail's own contacts and the ones on a server:
Google and Outlook on the mail account's own sign-in, Nextcloud through the
Nextcloud connection, any CardDAV server through Settings -> Contacts. Every
field a phone keeps, the photo included, syncs both ways. Duplicates merge
with 90 days to undo; vCard and CSV import and export; contacts suggested
while composing; JMAP Contact/get, /query and /set. Nextcloud calendars come
with the Nextcloud connection too.
Replaces #237 and #109 by @jo2904 and #244 by @DanversKara.
Closes #157
Closes #236
Co-authored-by: jo2904 <jo2904@users.noreply.github.com>
Co-authored-by: DanversKara <DanversKara@users.noreply.github.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
* Install APCu and Xdebug with PIE instead of from PECL (#286)
PECL is deprecated in favour of PIE and was down for most of an evening on 2026-10-21, failing every image build. PIE 1.5.2 now installs APCu (and Xdebug in the dev image) from the extensions' own releases; it is mounted for that one step and not left in the image. Nothing changes at runtime.
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
* Keep contact photos where the web container can serve them (#287)
Contact photos were stored in var/contacts, outside the volumes the web and worker containers share, so synced photos answered 404. They now live under APP_STORAGE_DIR/uploads/contacts; a migration clears the lost paths and has synced books read again so the photos come back.
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
* Match a contact's email addresses in any case (#288)
Addresses are stored lower-cased but were matched as the provider spelled them, so a pull removed and re-inserted the same address and broke the unique key (prod sync failure). They are now matched in their stored form.
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
* Open a contact's reference email in the mail pane (#294)
A click on one of a contact's reference emails in the docked contacts
pane was a Turbo visit inside the contacts frame, which blanked the mail
pane. The click is now offered to the mail pane as a mail-pane:open
event; the pane opens the message beside the contact. Without a mail
pane (the full contacts page) it stays an ordinary visit.
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
* Open mail beside the full Contacts page without a reload (#297)
The full Contacts page was a page of its own, so a reference email
clicked there was a visit: the contact was gone, and on a notebook the
mailbox came back on the calendar it remembered.
The full page is now the mailbox with data-contacts-full on the shell:
the mail column put away and the contacts dock given the row, with the
page's own content in the dock's frame. Opening a message from it loads
the message into the existing mail pane, then contacts--pane#reveal
drops the state in a view transition: the mail slides in from the left
and the dock narrows to its width, the same DOM throughout. On a
notebook the calendar gives way; on a wide screen it stays.
The reference links lose data-turbo-frame="_top", which made ui--split
stash a calendar position for a visit that never came.
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
* Preview image and video attachments on click, like PDFs (#295) (#298)
Clicking an image or video attachment chip opens a viewer in the modal
instead of downloading. Previewable chips (PDF included) get a download
half on the right. Images load from the existing attachment route; videos
from a new /mail/attachment/{id}/video route that serves a fixed video
type inline under the sandbox CSP and answers Range requests.
Closes #295
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
* Erase a message's parts with it, and delete mail one way (#300)
MessageEraser loaded a message's parts to delete their files, then removed
only the message. After the delete Doctrine forgot the message but kept the
parts managed, so the next flush in the same unit of work failed with "a new
entity was found through the relationship MessagePart#message". Every sync
that removed server-deleted mail with an attachment failed that way.
The eraser now removes the parts, removes a thread it empties (announced to
JMAP as destroyed), and logs file-delete failures instead of keeping the row.
MessagePurger hands its local delete to the eraser, so "delete forever" gains
the thread-label re-sync it lacked. ImapAccountSyncer clears an entity manager
a failed folder left open, so one folder's refused flush no longer fails the
rest.
Closes #299
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
* Rework the contact editor: quick add, paste details and Improve with AI (#305) (#306)
New contacts start in quick add: paste a name, signature, address line or
vCard and the details it finds are listed, each with a one-click type. The
editor shows the name as one field whose parts fill themselves (titles and
degrees included), rows that appear as you type, Paste details for existing
contacts and a Save to picker. Contact help is a new AI feature, off by
default, that rereads pasted text and suggests updates from the signature
of a contact's newest mail. Phones are read with libphonenumber.
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
* Keep spam off the radar and out of search; add a Not spam button (#308)
Cards read out of mail in Spam are left off the Happening Soon panel and
the mailbox strip (read-time filter, so moving the mail out brings them
back), and a spam conversation no longer shows its cards above the
message. The bin keeps its cards. Search and the type-ahead leave Spam
out unless asked with in:spam, as they already did for the bin. A spam
conversation's toolbar gets a Not spam button that moves it to the inbox.
Closes #304
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
* Add to contacts asks new or existing; contacts button gets the calendar's three positions (#312)
Closes #309
Closes #311
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* Show the pictures a review request hid, dropped or never asked for (#274)
* Show the pictures a review request hid, dropped or never asked for
A BAUHAUS review request, sent through Bazaarvoice, showed neither its
logo nor its product photos, with images allowed. The image proxy was
fine; three steps around it were not.
- The logo is linked as http://. The sanitizer keeps http sources and
leaves them to render time, the fetcher only connects over TLS, and
nothing in between upgraded the URL, so every http image became the
placeholder. RemoteContentBlocker now signs the https:// form. The
fetcher's own rules also say in the log which one refused a target;
until now that refusal was the one silent path.
- The first product card sits inside a <form>, and dropElement('form')
took the photo, the name and the star links with it. The form is
unwrapped now; textarea and select still go with their content.
- Each card is class="d" style="display:none", switched on with
.d {display:block !important}. CssToInlineStyles keeps an inline
property whatever the rule says, so the card stayed hidden next to its
Outlook twin. Html5CssInliner now lets a stylesheet !important beat a
plain inline style, and matches selectors against forms as the divs
the reader will see, so `form .a` rules take the sender's fallback
for clients without forms.
app:backfill safe-html-hidden-cards
re-sanitizes stored mail with a form or a stylesheet !important. One
task for both, because neither fix brings the card back alone.
Closes #271
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* Mark the German mail page as translated from the current English one
Both pages gained the same two paragraphs in the previous commit, but
the translated-from hash still named the English page before them, so
DocumentationCoverageTest reported the translation as stale.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* Credit the image proxy entry and keep it as a changelog fragment
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Paul Lützner <karatektus@gmail.com>
* Swipe a mail row, choose what each swipe does, and long-press for its actions on a phone (#314)
On a phone, below md and by touch only: a row swipes right for read/unread and left for archive, each direction set under Settings → Mail → Swipe actions (read/unread, archive, star, nothing; never trash), and holding a row opens an action sheet with every action its strip offers. Mouse, trackpad and wider windows are unchanged.
Takes over #117, #118 and #119, whose fork was archived. Also gives the Contacts settings' two cards their gap.
Co-authored-by: Flouss <15911342+Flouss@users.noreply.github.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
* Admin → Integrations lists the calendar services, so a pinned CalDAV address can be changed later (#279)
CalDAV and Calendar feed get their own group below the file services, with the shared provider row and Configure form; Nextcloud stays with the file services. Calendar services are offered to everyone, so their rows say so and the form has no switch. The server address accepts only http or https, up to 512 characters.
Once people have connected, the CalDAV form says how many connections a changed or cleared pin reaches. A calendar feed offers its address only while one is stored, so it can be cleared.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* Offer an address book another connection already mirrors locked, not as new (#319)
Two connections to the same server, a Nextcloud one and a CardDAV one made by hand, reach the same address book under the same remote id. Settings → Contacts now offers such a book locked, saying which connection has it, and refuses a posted tick, as it does for calendars. Otherwise every contact in it showed twice.
Closes #317
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* Gmailify: the IMAP folder places a sibling's mail, Gmail deletions detach, and a switch to turn it off (#204)
Closes #203
Closes #205
- A sibling's IMAP copy keeps the placement its folder gives it; Gmail's INBOX/SENT/DRAFT/TRASH/SPAM no longer move it (by @phdelodder).
- Gmail deleting its copy detaches the sibling's IMAP copy instead of erasing it.
- app:backfill gmailify-placement repairs copies the old merge had moved.
- Settings → Aliases: a Gmailify switch per Gmail account. Off detaches carried copies in a background job and declines mail Gmail still collects for another account (Bcc'd mail excepted, as documented).
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
* Gmail sync: look up only the listed ids, not every stored one (#321)
Part of #223: newGmailIds() asks only about the ids Gmail just listed, and nothing when none were.
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
* Store whether a conversation is in the bin, kept by triggers (#322)
Part of #223: message_thread.binned holds #268's rule, kept by triggers on message_label, thread_label and message; lists, badges and search read the column. Identical results on 41 compared queries, most 5-100x faster on a 67k-thread mailbox.
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
* Account sync: wait out a provider that can't be reached, quietly (#333)
When the worker couldn't connect to the Gmail or Graph API at all, the
account sync went up Messenger's retry ladder: a WARNING with three nested
stack traces per attempt, then CRITICAL and a parked job if the outage
lasted. That job resumes from a cursor and is asked for again every
quarter hour, so nothing was ever lost.
WaitOutConnectionFailures now puts a SyncAccountMessage that failed only
with an HttpClient TransportException back on its own queue, on the
ingest ladder's delays, with one info line per attempt. After five waits
it is dropped with a single warning. Message batches and every other
failure keep the normal path.
Closes #332
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
---------
Co-authored-by: Sven Joder <sj@pmdevelopment.de>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Philippe Delodder <7259755+phdelodder@users.noreply.github.com>
Co-authored-by: jo2904 <jo2904@users.noreply.github.com>
Co-authored-by: DanversKara <DanversKara@users.noreply.github.com>
Co-authored-by: Flouss <15911342+Flouss@users.noreply.github.com>
1366 lines
53 KiB
Markdown
1366 lines
53 KiB
Markdown
# Code style and practice
|
||
|
||
The conventions this codebase actually follows, written down so another project can adopt them
|
||
without reverse-engineering them from the source. Everything here is extracted from working code —
|
||
the examples are real, and where a rule exists because something went wrong, the guide says what.
|
||
|
||
Drop this file into a new project, delete the sections whose stack it does not use, and keep the
|
||
first three. Those are the ones that make code look like this; the rest is Symfony-shaped detail.
|
||
|
||
**Contents**
|
||
|
||
1. [The governing idea](#1-the-governing-idea)
|
||
2. [Comments](#2-comments)
|
||
3. [Naming](#3-naming)
|
||
4. [PHP: language level and syntax](#4-php-language-level-and-syntax)
|
||
5. [Architecture and layering](#5-architecture-and-layering)
|
||
6. [Doctrine, entities and migrations](#6-doctrine-entities-and-migrations)
|
||
7. [Failure, errors and logging](#7-failure-errors-and-logging)
|
||
8. [Templates, JavaScript and CSS](#8-templates-javascript-and-css)
|
||
9. [Tests](#9-tests)
|
||
10. [Static analysis and tooling](#10-static-analysis-and-tooling)
|
||
11. [Documentation, changelog and commits](#11-documentation-changelog-and-commits)
|
||
12. [The condensed rules](#12-the-condensed-rules)
|
||
|
||
---
|
||
|
||
## 1. The governing idea
|
||
|
||
**Code records decisions, not just behaviour.** Anyone can read a function and learn what it does.
|
||
What they cannot recover is why it does that rather than the obvious alternative — and the obvious
|
||
alternative is usually what someone tries next, six months later, when they "simplify" it.
|
||
|
||
Three consequences run through everything below:
|
||
|
||
- **Write down the road not taken.** Not "this sets the label", but "through the service, not the
|
||
column, because snoozing has to move the Inbox label off and propagate that outward".
|
||
- **Prefer one rule over a set of exceptions.** Every entity carries `updated_at`, including the
|
||
tables written once — "one rule for every entity is worth more than the bytes an exception saves,
|
||
and nothing has to decide which kind of entity it is looking at."
|
||
- **Make the invariant structural where you can, and documented where you cannot.** A unique
|
||
constraint beats a comment saying not to insert duplicates. When the guard has to live in prose,
|
||
the prose says what breaks without it.
|
||
|
||
Everything else here is downstream of that.
|
||
|
||
---
|
||
|
||
## 2. Comments
|
||
|
||
This is the most visible thing about the style, so it comes before syntax.
|
||
|
||
### 2.1 The class docblock states the subject, in a sentence
|
||
|
||
Open with what the thing *is*, in plain language, then the reasoning that is not visible from the
|
||
signatures.
|
||
|
||
```php
|
||
/**
|
||
* Snoozing a conversation, and bringing it back.
|
||
*
|
||
* Snooze is archive-with-a-timer: the thread leaves the Inbox now and returns
|
||
* to it later. Expressed in labels like everything else — Inbox off, Snoozed
|
||
* on — so the "a message carries at least one label" invariant holds without a
|
||
* second mechanism, and so the snoozed pile is reachable by every path that
|
||
* already works on labels: the sidebar, a query, the unified feed.
|
||
*
|
||
* One service rather than one implementation per caller. The JMAP method and
|
||
* the web UI both go through here, because a snooze that meant something
|
||
* different depending on which client did it is worse than no snooze.
|
||
*/
|
||
final class ThreadSnoozeService
|
||
```
|
||
|
||
Note the shape: **what it is** (one line), **how it is modelled and why that model** , **what was
|
||
rejected**. A class whose docblock is `/** Service for snoozing threads. */` has not earned one.
|
||
|
||
### 2.2 Comments answer "why", never "what"
|
||
|
||
Bad, and not written here:
|
||
|
||
```php
|
||
// Loop over the messages and remove the inbox label
|
||
foreach ($messages as $message) {
|
||
```
|
||
|
||
Written here:
|
||
|
||
```php
|
||
// Before the labels move, so an IMAP job still sees the source folder.
|
||
// Archiving is what snoozing does to the provider; the difference is
|
||
// only that plMail remembers to undo it.
|
||
$this->propagator->archive($messages);
|
||
```
|
||
|
||
### 2.3 Name the bug the code prevents
|
||
|
||
When a line exists because of a defect, the comment says which one, concretely enough that a reader
|
||
can tell whether their change re-introduces it.
|
||
|
||
```php
|
||
{# `latest` comes from |last, which yields FALSE (not null) on an empty
|
||
collection — so `is not null` passes and .isDraft blows up on a bool.
|
||
A thread can legitimately end up with no messages (every message
|
||
expunged provider-side), and this row is rendered for the whole inbox,
|
||
so one such thread would 500 the entire list. #}
|
||
```
|
||
|
||
```js
|
||
/**
|
||
* Absent, this clears the snooze. That used to be the *only* behaviour:
|
||
* the signature took `until = null` as a second argument, and Stimulus
|
||
* calls actions with the event alone, so the row's snooze button silently
|
||
* unsnoozed instead of snoozing.
|
||
*/
|
||
```
|
||
|
||
### 2.4 Document deliberate absences
|
||
|
||
The strongest comments are the ones on things that are *not* there — an empty config block, a
|
||
missing feature, a check that was removed.
|
||
|
||
```neon
|
||
# ignoreErrors is deliberately empty.
|
||
#
|
||
# The rule usually wanted here — silencing "Property …::$id (int|null) is
|
||
# never assigned int" on Doctrine identifiers — is not needed: the
|
||
# phpstan-doctrine extension included above already understands that the
|
||
# ORM assigns identifiers by reflection, so the error never fires.
|
||
```
|
||
|
||
Without that, the next person adds the ignore, PHPStan fails the build on an unmatched pattern, and
|
||
half an hour goes somewhere.
|
||
|
||
### 2.5 Justify shape decisions in place
|
||
|
||
When something stays a method rather than becoming a property, or stays private rather than public,
|
||
say so where it lives:
|
||
|
||
```php
|
||
/**
|
||
* Stays a method: it takes an argument and asks the label list about it, so
|
||
* there is no single piece of state here to expose as a property.
|
||
*/
|
||
public function hasGmailLabel(string $label): bool
|
||
```
|
||
|
||
```php
|
||
/**
|
||
* The only field here that is not public, and deliberately: Postgres writes
|
||
* it, PHP never does, and no caller reads it either — the searches that use
|
||
* it match on the column in SQL. It exists so the generated column is part
|
||
* of the mapping and stays in the schema.
|
||
*/
|
||
private ?string $searchVector = null;
|
||
```
|
||
|
||
### 2.6 Config files carry essays
|
||
|
||
`phpstan.dist.neon`, `phpunit.dist.xml` and `.env` are code. They get the same treatment — every
|
||
non-obvious value explains itself:
|
||
|
||
```xml
|
||
<!-- A file of its own: InstallEmptyInstallTest writes APP_PUBLIC_URL
|
||
through the real service, and the generated secrets the test
|
||
environment actually runs on must not be the thing it overwrites. -->
|
||
<server name="APP_SECRETS_FILE" value="var/secrets/phpunit.env" force="true" />
|
||
```
|
||
|
||
Compose files are the exception: no comments except a header of at most three lines saying what the
|
||
file is for. The reasoning behind a setting goes in `docs/install/`.
|
||
|
||
### 2.7 Section separators inside long classes
|
||
|
||
Two forms, both used sparingly and only in files long enough to need navigation:
|
||
|
||
```php
|
||
// ---------------------------------------------------------------- helpers
|
||
```
|
||
|
||
```js
|
||
// ── Private ───────────────────────────────────────────────────────────
|
||
```
|
||
|
||
### 2.8 Voice
|
||
|
||
Prose, in complete sentences, with an em dash where a clause needs it. No "TODO: fix this", no
|
||
"HACK", no jokes, no first person. Emphasis is `**bold**` in docblocks and used for the one clause
|
||
that carries the paragraph. Uppercase is reserved for the rare rule that must not be missed:
|
||
|
||
```php
|
||
/**
|
||
* CREATION DEFAULT for Label::$isVisible — used only when the label
|
||
* row is first created (LabelResolver::systemLabel). After creation,
|
||
* visibility belongs to the user via the label settings and this
|
||
* method must not be consulted again.
|
||
*/
|
||
```
|
||
|
||
---
|
||
|
||
## 3. Naming
|
||
|
||
### 3.1 Classes
|
||
|
||
Named for what they *are*, not for the pattern they implement. `ThreadSnoozeService`,
|
||
`MessageCategorizer`, `SenderResolver`, `LabelChangePropagator`, `EncryptionKeyProbe`,
|
||
`RawMessageResolver`. Suffixes carry meaning and are consistent:
|
||
|
||
| Suffix | Means |
|
||
|---|---|
|
||
| `…Repository` | Every query against one entity. Nothing else. |
|
||
| `…Service` | Owns what an operation *means*; shared by every caller. |
|
||
| `…Resolver` | Turns an ambiguous input into the one right object. |
|
||
| `…Builder` | Assembles a structure from parts. |
|
||
| `…Factory` | Constructs a configured object. |
|
||
| `…Propagator` | Pushes a local change outward to a provider. |
|
||
| `…Handler` | Messenger handler; pairs 1:1 with a `…Message`. |
|
||
| `…Message` | Messenger payload; immutable, ids only. |
|
||
| `…Command` | Console command. |
|
||
| `…Helper` | Stateless functions over a value type. |
|
||
| `…Probe` | Checks a precondition at boot and refuses to continue. |
|
||
| `…Model` | Behaviour base class an entity extends. |
|
||
|
||
Interfaces end in `Interface` (`AccountSyncerInterface`, `MailSenderInterface`), traits in `Trait`
|
||
(`TimestampableTrait`), enums are bare nouns (`LabelRole`, `MessageFlag`, `ThreadingMethod`).
|
||
|
||
### 3.2 Methods
|
||
|
||
Verb phrases that read as instructions: `snooze()`, `wake()`, `attachLabel()`, `matchingIds()`,
|
||
`extractionCandidates()`, `hasProviderFolder()`. Predicates start `is`/`has`/`can`. A method that
|
||
answers a question about a collaborator keeps the question shape: `accountOf($message)`.
|
||
|
||
Private helpers at the bottom, in call order, after the public surface.
|
||
|
||
### 3.3 Variables
|
||
|
||
Full words. `$messages`, `$account`, `$inboxMailbox`, `$snoozed`, `$until`. Not `$msg`, `$acc`,
|
||
`$i` (outside a genuine index loop), `$data`, `$result`, `$tmp`. A variable named `$body` holds a
|
||
body; if it holds a decoded JSON body, it is `$body` and the line above says what shape.
|
||
|
||
### 3.4 Files and directories
|
||
|
||
One class per file, PSR-4, path mirrors namespace. Directories are grouped by *domain area first,
|
||
then kind* under each top-level layer: `src/Service/Mail/`, `src/Service/Graph/`,
|
||
`src/Repository/Label/`, `src/Entity/Monitoring/`, `src/Domain/Enum/Mail/`. This scales: a new
|
||
provider adds `src/Service/Fastmail/` rather than eleven files into one flat directory.
|
||
|
||
Twig partials are prefixed with an underscore (`_thread_row.html.twig`) and full pages are not
|
||
(`inbox.html.twig`). Turbo Stream templates carry `.stream.html.twig`.
|
||
|
||
---
|
||
|
||
## 4. PHP: language level and syntax
|
||
|
||
Targets PHP 8.4+ and uses what that buys. 460 of 505 source files declare strict types; the
|
||
exceptions are legacy and being closed.
|
||
|
||
### 4.1 File preamble
|
||
|
||
```php
|
||
<?php
|
||
|
||
declare(strict_types=1);
|
||
|
||
namespace App\Service\Mail;
|
||
|
||
use App\Domain\Enum\Mail\LabelRole;
|
||
use App\Entity\Mail\MessageThread;
|
||
use Doctrine\ORM\EntityManagerInterface;
|
||
```
|
||
|
||
`declare(strict_types=1);` on every new file. Imports for everything including `DateTimeImmutable`,
|
||
alphabetical, no grouping by vendor, no trailing blank line games. Fully-qualified inline names
|
||
(`\LogicException`, `\Exception`) appear only for throwaway catches.
|
||
|
||
### 4.2 `final` by default, `readonly` where it fits
|
||
|
||
349 of 505 source files declare a `final` class. The default is final; non-final is a decision you
|
||
make when a subclass exists, and it is usually an entity extending its `…Model` base.
|
||
|
||
Value objects and Messenger payloads are `final readonly`:
|
||
|
||
```php
|
||
final readonly class IngestedMessage
|
||
{
|
||
public function __construct(
|
||
public Message $message,
|
||
public Account $account,
|
||
public ?string $rawSource = null,
|
||
) {
|
||
}
|
||
}
|
||
```
|
||
|
||
`final` has a real payoff here and the tests lean on it: when every collaborator is final, mocking
|
||
is impossible, so tests are written against the real container instead. See §9.3.
|
||
|
||
### 4.3 Constructor injection, promoted, aligned, `readonly`
|
||
|
||
```php
|
||
public function __construct(
|
||
private readonly MessageRepository $messageRepository,
|
||
private readonly MessageThreadRepository $threadRepository,
|
||
private readonly LabelRepository $labelRepository,
|
||
private readonly ThreadStatusUpdater $status,
|
||
private readonly ThreadSnoozeService $snoozeService,
|
||
) {}
|
||
```
|
||
|
||
Always promoted, always `private readonly` for services and `public` for DTO fields. One parameter
|
||
per line with a trailing comma. **Variable names aligned in a column** when there are three or
|
||
more — the same alignment rule applies to array literals and to consecutive assignments:
|
||
|
||
```php
|
||
return [
|
||
'extAfterId' => $afterId,
|
||
'extCalendarTypes' => self::CALENDAR_TYPES,
|
||
'extMeetingHeader' => GraphMessageBuilder::MEETING_TYPE_HEADER,
|
||
'extJsonLd' => '%application/ld+json%',
|
||
];
|
||
```
|
||
|
||
```php
|
||
$io = new SymfonyStyle($input, $output);
|
||
$accountId = $input->getArgument('account-id');
|
||
```
|
||
|
||
Empty constructor bodies are `) {}` on one line (86 files do this). A constructor that calls
|
||
`parent::__construct()` obviously gets a real body.
|
||
|
||
### 4.4 Public properties, asymmetric visibility, no getters
|
||
|
||
Getters and setters are not written. There are eight `public function get…` methods in the entire
|
||
`src/Entity` tree, and each is doing something a property cannot.
|
||
|
||
State is public. State the outside must not write uses PHP 8.4's asymmetric visibility (88 sites):
|
||
|
||
```php
|
||
#[ORM\Id]
|
||
#[ORM\GeneratedValue]
|
||
#[ORM\Column]
|
||
public private(set) ?int $id = null;
|
||
|
||
#[ORM\Column]
|
||
public private(set) DateTimeImmutable $createdAt;
|
||
|
||
/** @var Collection<int, Label> */
|
||
public private(set) Collection $labels;
|
||
```
|
||
|
||
Mutation that has to stay coherent goes through a named method (`addLabel()`, `removeFlag()`), not
|
||
through a setter. The rule for whether something is a property or a method:
|
||
|
||
> A method that takes an argument and asks the state a question stays a method. A method that
|
||
> returns one piece of state and takes nothing becomes a property.
|
||
|
||
### 4.5 Explicit, Yoda-side comparisons
|
||
|
||
Comparison against a literal puts the literal first, and comparisons are always identity-strict and
|
||
always spelled out. 540 `null === …` against 16 the other way; 511 `true === …`.
|
||
|
||
```php
|
||
if (null === $label || $label->usr !== $this->getUser()) {
|
||
if (true === $label->isSystem) {
|
||
if (false === $this->labels->contains($label)) {
|
||
if ([] === $messages) {
|
||
if (0 === count($ids)) {
|
||
$attach = (true === array_key_exists('attach', $body) && true === $body['attach']);
|
||
```
|
||
|
||
`if ($label->isSystem)` is not written even though the property is a `bool`. The point is not the
|
||
type — it is that every condition reads as an explicit claim about a specific value, and a truthiness
|
||
bug (`"0"`, `[]`, `0.0`) can never hide in one.
|
||
|
||
### 4.6 Typed constants
|
||
|
||
Every constant declares its type — 47 `private const array`, 112 `private const int`, 62
|
||
`private const string`, and the public counterparts.
|
||
|
||
```php
|
||
/**
|
||
* Content types that mean "there is an invite in here".
|
||
*
|
||
* @var list<string>
|
||
*/
|
||
private const array CALENDAR_TYPES = ['text/calendar', 'application/ics'];
|
||
```
|
||
|
||
Magic numbers and repeated literals become named constants at the top of the class, with a docblock
|
||
saying what the value means, not what it is.
|
||
|
||
### 4.7 Enums carry behaviour
|
||
|
||
An enum is not a list of strings; it is where the per-case rules live, as `match` tables (69 in the
|
||
codebase). Sorting, display names, capability predicates and mappings from other enums all belong on
|
||
the enum rather than in a `switch` scattered through services.
|
||
|
||
```php
|
||
enum LabelRole: string
|
||
{
|
||
case Inbox = 'inbox';
|
||
// …
|
||
|
||
public function hasProviderFolder(): bool
|
||
{
|
||
return self::Snoozed !== $this;
|
||
}
|
||
|
||
public static function fromSpecialUse(MailboxSpecialUse $specialUse): self
|
||
{
|
||
return match ($specialUse) {
|
||
MailboxSpecialUse::INBOX => self::Inbox,
|
||
// …
|
||
};
|
||
}
|
||
|
||
public function sortOrder(): int
|
||
{
|
||
return match ($this) {
|
||
self::Inbox => 0,
|
||
self::Sent => 10,
|
||
// After Archive, before custom labels: it is a system place, but
|
||
// the least-visited one.
|
||
self::Snoozed => 60,
|
||
};
|
||
}
|
||
}
|
||
```
|
||
|
||
`match ($this)` over every case with no `default`, so adding a case is a compile-time-ish error
|
||
rather than a silent fallthrough.
|
||
|
||
Individual cases get their own docblock when they are exceptional — `Snoozed` above has one
|
||
explaining that it is the only role with no provider counterpart.
|
||
|
||
### 4.8 Strings and formatting
|
||
|
||
`sprintf()` for anything with a placeholder (332 uses), not interpolation:
|
||
|
||
```php
|
||
$io->error(sprintf('Account %s not found.', $accountId));
|
||
$io->text(sprintf('→ dispatched sync for %s (#%d)', $account->email, $account->id));
|
||
```
|
||
|
||
Nowdoc (`<<<'SQL'`) for embedded SQL, indented to the code and closed at the same indent. Never
|
||
built by editing a finished query:
|
||
|
||
> Built by concatenation rather than by editing a finished query — the first version pulled the
|
||
> LIMIT off with `str_replace` and silently stopped matching the moment the heredoc's indentation
|
||
> changed.
|
||
|
||
### 4.9 Control flow
|
||
|
||
Early return over nesting. A guard clause, a blank line, then the work:
|
||
|
||
```php
|
||
public function wake(MessageThread $thread): void
|
||
{
|
||
$messages = $thread->messages->toArray();
|
||
|
||
if ([] === $messages) {
|
||
$thread->snoozedUntil = null;
|
||
|
||
return;
|
||
}
|
||
|
||
// …
|
||
}
|
||
```
|
||
|
||
A `return` is preceded by a blank line whenever it follows a statement. Blank lines separate
|
||
paragraphs of logic — setup, the loop, the finish — and each paragraph usually has a comment or is
|
||
short enough not to need one.
|
||
|
||
### 4.10 Docblocks for types PHP cannot express
|
||
|
||
PHPDoc is for generics and array shapes, never to restate the signature:
|
||
|
||
```php
|
||
/** @return list<Message> */
|
||
/** @param iterable<Message> $messages */
|
||
/** @var array<string,string|list<string>>|null */
|
||
/** @var Collection<int, MessagePart> */
|
||
```
|
||
|
||
`list<T>` when the array is a packed list — it is, and the code keeps it so with `array_values()`.
|
||
|
||
### 4.11 Formatting mechanics
|
||
|
||
From `.editorconfig`, which is authoritative:
|
||
|
||
```ini
|
||
root = true
|
||
|
||
[*]
|
||
charset = utf-8
|
||
end_of_line = lf
|
||
indent_size = 4
|
||
indent_style = space
|
||
insert_final_newline = true
|
||
trim_trailing_whitespace = true
|
||
|
||
[{compose.yaml,compose.*.yaml}]
|
||
indent_size = 2
|
||
|
||
[*.md]
|
||
trim_trailing_whitespace = false
|
||
```
|
||
|
||
Four spaces everywhere except YAML compose files. Prose in comments and Markdown wraps at ~80–100
|
||
columns; code does not get hard-wrapped mid-expression to satisfy a limit.
|
||
|
||
---
|
||
|
||
## 5. Architecture and layering
|
||
|
||
### 5.1 The tree
|
||
|
||
```
|
||
src/
|
||
Command/ Console commands, grouped by area (Mail/, Imap/, Push/, Maintenance/…)
|
||
Controller/ HTTP actions, grouped by area (Mail/, Admin/, Settings/, Webhook/…)
|
||
Domain/ The vocabulary: Enum/, DTO/, Interface/, Exception/, Model/, Trait/, Helper/, Filter/
|
||
Entity/ Doctrine entities, grouped by area (Mail/, Label/, User/, Calendar/, Monitoring/)
|
||
Form/ Symfony form types
|
||
Infrastructure/ Framework-facing wiring: Doctrine/, Messaging/, Event/, Scheduler/, Setup/, Encryption/
|
||
Jmap/ A protocol implementation, self-contained, with its own Method/, Mapper/, Protocol/
|
||
Repository/ One per entity, mirroring Entity/'s grouping
|
||
Security/ Authenticators, user provider, two-factor
|
||
Service/ Everything that decides something. The largest layer, by design.
|
||
Twig/ Extensions and runtime helpers (Vendor/ for lifted upstream code)
|
||
```
|
||
|
||
`Domain/` holds no framework types. `Infrastructure/` holds the framework glue. `Service/` is where
|
||
meaning lives and is deliberately the biggest directory — 151 of 505 files.
|
||
|
||
### 5.2 Controllers hold actions and little else
|
||
|
||
A controller resolves input, authorises, delegates, and renders. It does not decide what an
|
||
operation means. The commit that established this is literally titled *"Leave controllers with their
|
||
actions and little else"*.
|
||
|
||
The pattern, in full:
|
||
|
||
```php
|
||
#[Route('/status/{type}/{id}', name: 'app_status_', requirements: ['type' => 'message|thread', 'id' => '\d+'])]
|
||
#[IsGranted('IS_AUTHENTICATED')]
|
||
class ThreadStatusController extends AbstractController
|
||
{
|
||
use ChecksCsrf;
|
||
use RendersTurboStreams;
|
||
|
||
#[Route('/archive', name: 'archive', methods: ['POST'])]
|
||
public function archive(Request $request, string $type, int $id): Response
|
||
{
|
||
$messages = $this->resolveMessages($request, $type, $id);
|
||
|
||
$this->status->archive($messages);
|
||
|
||
return $this->renderTurboStream('thread/status/_archive.stream.html.twig', [
|
||
$type => 'message' === $type ? $messages[0] : $messages[0]->thread,
|
||
]);
|
||
}
|
||
```
|
||
|
||
- Route attributes on the class for the shared prefix and name prefix, on the method for the rest.
|
||
- `methods:` always declared, and `requirements:` for anything a route segment must never be — an
|
||
unconstrained `{type}` is a `default` arm nobody wrote.
|
||
- `#[IsGranted]` at class level for the coarse question (is this a signed-in user, an admin);
|
||
per-action overrides only when they differ.
|
||
- **Ownership is a voter, never a comparison in the controller.** `OwnershipVoter` answers
|
||
`own` for every entity a user owns, and controllers ask it — `$this->denyAccessUnlessGranted(
|
||
OwnershipVoter::OWN, $subject)` where refusing is the answer, `$this->isGranted(...)` where the
|
||
action skips or returns instead. This replaced nineteen private helpers across eighteen files,
|
||
under six different names; all of them were correct, and that was the problem — each was a fresh
|
||
derivation of one sentence, so the twentieth was a fresh chance to get it wrong.
|
||
- Where several actions share a resolver, the guards go *in the resolver*, not in each action —
|
||
that part of the old pattern was right and is kept. `resolveMessages()` above checks CSRF and
|
||
ownership for all six of its callers, so an action cannot forget either.
|
||
- Rules that are not ownership stay in the controller, because they are a different question:
|
||
LabelController still refuses a *system* label, ComposeController still refuses an *already sent*
|
||
draft. Those are about what the thing is, not whose it is.
|
||
- CSRF as `ChecksCsrf::assertCsrf()`, one copy, taking the token from `_token` or `X-CSRF-Token`.
|
||
- Shared response mechanics as one private method (`renderTurboStream()`).
|
||
|
||
### 5.3 Every query lives in a repository
|
||
|
||
No `createQueryBuilder` outside `src/Repository`, no `EntityManager::getRepository()->findBy()`
|
||
inline in a service when the query means something. If a query is used twice, it is a named
|
||
repository method; if it is used once but has a reason, it is a named repository method with a
|
||
docblock explaining the reason.
|
||
|
||
Raw DBAL is allowed, and when it appears it says why it had to be raw:
|
||
|
||
```php
|
||
/**
|
||
* Raw DBAL because none of the three is expressible otherwise: jsonb key
|
||
* existence has no DQL operator and no registered function, and the rest
|
||
* is an EXISTS correlated to a second entity. Written as jsonb_exists()
|
||
* rather than the `?` operator that means the same thing — DBAL reads a
|
||
* bare `?` as a positional placeholder and refuses the query …
|
||
*/
|
||
```
|
||
|
||
Shared SQL fragments become class constants so two call sites cannot drift:
|
||
|
||
> This is the WHERE both candidate queries share, so a change to what counts as a candidate cannot
|
||
> land in one and not the other.
|
||
|
||
### 5.4 One service per concept, shared by every caller
|
||
|
||
When two entry points (a web controller and an API method, a command and a message handler) do the
|
||
same thing, they call the same service. This is stated as a rule and enforced by comments at both
|
||
ends:
|
||
|
||
```php
|
||
// Through the service, not $thread->snoozedUntil: snoozing has to move the
|
||
// Inbox label off and propagate that outward. Writing the column here
|
||
// is what this endpoint used to do, and it left the conversation
|
||
// sitting in the inbox … Thread/set goes through the same service for the
|
||
// same reason. The one deliberate difference is above: a form post gets the
|
||
// "in 1 day" fallback on an unparseable date, where the JMAP method
|
||
// refuses it — see ThreadSetMethod::snoozeDate().
|
||
```
|
||
|
||
Where the callers genuinely must differ, the difference is named and both sides point at each other.
|
||
|
||
### 5.5 Interfaces for the axis that varies
|
||
|
||
`src/Domain/Interface/` holds one interface per pluggable axis — `AccountSyncerInterface`,
|
||
`MailSenderInterface`, `IntegrationDriverInterface`, `PostIngestStepInterface`,
|
||
`EventExtractorInterface`. Implementations live under the provider's own directory
|
||
(`Service/Gmail/`, `Service/Graph/`, `Service/Imap/`), and a registry resolves them
|
||
(`MailSenderRegistry`). Adding a provider means adding a directory, not editing a `switch`.
|
||
|
||
Pipelines are built from a tagged-interface list (`PostIngestPipeline` over `PostIngestStepInterface`)
|
||
so a step is a class, not a branch.
|
||
|
||
### 5.6 DTOs cross boundaries
|
||
|
||
Anything passing between layers with more than two fields is a `final readonly` DTO under
|
||
`Domain/DTO/`, not an array. The docblock explains what it carries that its members do not obviously
|
||
imply:
|
||
|
||
```php
|
||
/**
|
||
* Carries the owning account rather than letting the pipeline read it off the
|
||
* message, because the two are not always the same one: under Gmailify a Gmail
|
||
* account fetches mail addressed to a sibling …
|
||
*/
|
||
```
|
||
|
||
The commit for this is *"Send the compose context whole instead of unpacking it four times"*.
|
||
|
||
### 5.7 Async work
|
||
|
||
Message and handler pair 1:1 by name — `SyncAccountMessage` / `SyncAccountMessageHandler`. Messages
|
||
are `readonly`, carry **ids and scalars only** (never entities), and have a docblock saying who
|
||
dispatches them and what the handler resolves:
|
||
|
||
```php
|
||
/**
|
||
* General/scheduled/push-driven sync of an entire account.
|
||
* The handler resolves the right AccountSyncerInterface for the provider.
|
||
*/
|
||
readonly class SyncAccountMessage
|
||
{
|
||
public function __construct(
|
||
public int $accountId,
|
||
) {}
|
||
}
|
||
```
|
||
|
||
Recurring work is declared in one place (`Infrastructure/Scheduler/MaintenanceSchedule`), and the
|
||
commands it fires are also runnable by hand.
|
||
|
||
### 5.8 Console commands
|
||
|
||
```php
|
||
#[AsCommand(
|
||
name: 'app:mail:sync',
|
||
description: 'Dispatch an account-level sync (IMAP or Gmail) for one or all active accounts',
|
||
)]
|
||
final class MailSyncCommand extends Command
|
||
```
|
||
|
||
- Names are `app:<area>:<verb>`, colon-separated, consistently grouped.
|
||
- `SymfonyStyle` for all output; `$io->error()`, `$io->text()`, `$io->success()`.
|
||
- Return `Command::SUCCESS` / `Command::FAILURE`, never a bare int.
|
||
- Arguments optional where a sensible "do all of them" default exists.
|
||
- Every command is listed in `CONTRIBUTING.md` with a one-line description. That table is part of
|
||
the definition of done.
|
||
|
||
---
|
||
|
||
## 6. Doctrine, entities and migrations
|
||
|
||
### 6.1 Entities
|
||
|
||
Attributes only, no YAML/XML mapping. Column types via `Types::` constants, enums via `enumType:`.
|
||
Relations declare `onDelete` at the join column so the database enforces what the code assumes.
|
||
|
||
```php
|
||
#[ORM\ManyToOne(inversedBy: 'messages')]
|
||
#[ORM\JoinColumn(nullable: false, onDelete: 'CASCADE')]
|
||
public Account $account;
|
||
|
||
#[ORM\Column(nullable: true, enumType: MessageCategory::class)]
|
||
public ?MessageCategory $category = null;
|
||
```
|
||
|
||
Collections are initialised in the constructor and exposed `public private(set)`, with add/remove
|
||
methods keeping both sides of the relation in step:
|
||
|
||
```php
|
||
public function addMessagePart(MessagePart $messagePart): static
|
||
{
|
||
if (!$this->messageParts->contains($messagePart)) {
|
||
$this->messageParts->add($messagePart);
|
||
$messagePart->message = $this;
|
||
}
|
||
|
||
return $this;
|
||
}
|
||
```
|
||
|
||
Mutators return `static` so they chain.
|
||
|
||
### 6.2 Indexes and constraints are commented
|
||
|
||
Every index and unique constraint carries a comment saying what it protects and **why the columns
|
||
are in that order**:
|
||
|
||
```php
|
||
// Remote-id uniqueness per account: one row per Gmail/Graph message. The
|
||
// batch handlers dedup in PHP before inserting, but that check is a read on
|
||
// stale data — this is the guard that actually holds when batches overlap
|
||
// across runs or retries. Provider id leads the column list so the indexes
|
||
// also serve the id-only lookups (findOneBy(['gmailId'|'graphId'])).
|
||
#[ORM\UniqueConstraint(name: 'uniq_message_gmail_id_account', columns: ['gmail_id', 'account_id'])]
|
||
```
|
||
|
||
Constraint names are `uniq_<table>_<columns>` and `idx_<table>_<columns>`, snake_case, spelled out.
|
||
|
||
### 6.3 Timestamps are a trait, and there are no exceptions
|
||
|
||
```php
|
||
trait TimestampableTrait
|
||
{
|
||
#[ORM\Column]
|
||
public private(set) DateTimeImmutable $createdAt;
|
||
|
||
#[ORM\Column]
|
||
public private(set) DateTimeImmutable $updatedAt;
|
||
|
||
#[ORM\PrePersist]
|
||
public function initTimestamps(): void
|
||
{
|
||
$now = new DateTimeImmutable();
|
||
|
||
$this->createdAt ??= $now;
|
||
$this->updatedAt = $now;
|
||
}
|
||
|
||
#[ORM\PreUpdate]
|
||
public function bumpUpdatedAt(): void
|
||
{
|
||
$this->updatedAt = new DateTimeImmutable();
|
||
}
|
||
}
|
||
```
|
||
|
||
Three things worth copying wholesale:
|
||
|
||
- **Non-nullable, no default.** "A row that exists has both, and a reader should not be made to
|
||
check for a null the database cannot contain. Reading one before the entity is persisted throws,
|
||
which is the right answer to a genuine mistake."
|
||
- **`??=` on `createdAt`**, so a backfill or a test can supply its own instant — and the docblock
|
||
explains that `??=` reads through `isset()`, which answers false for an uninitialised typed
|
||
property rather than throwing.
|
||
- **A test enforces the requirement the trait cannot.** The trait needs
|
||
`#[ORM\HasLifecycleCallbacks]` on the class; without it Doctrine silently does nothing, so
|
||
`TimestampableTest` checks every adopting entity for the attribute.
|
||
|
||
Dates are `DateTimeImmutable`, always. Never `DateTime`.
|
||
|
||
### 6.4 Migrations
|
||
|
||
Generated, then edited to carry a docblock that is a paragraph of reasoning, plus a one-line
|
||
`getDescription()` written in the same imperative voice as a commit subject:
|
||
|
||
```php
|
||
/**
|
||
* The last five entities that tracked their own timestamps take the shared
|
||
* trait, which means the five tables that only had created_at gain updated_at.
|
||
*
|
||
* Added nullable, backfilled from created_at, then made NOT NULL. Adding a NOT
|
||
* NULL column with no default fails outright on a table that already has rows,
|
||
* and log_entry and jmap_change_log always do — jmap_change_log is the busiest
|
||
* table in the schema, because every mail mutation writes one.
|
||
*/
|
||
final class Version20260803113000 extends AbstractMigration
|
||
{
|
||
private const array TABLES = [/* … */];
|
||
|
||
public function getDescription(): string
|
||
{
|
||
return 'Give the remaining five timestamped tables an updated_at';
|
||
}
|
||
```
|
||
|
||
Migrations run automatically on boot, so **anything irreversible is called out in the changelog**.
|
||
Backfills follow the nullable → populate → NOT NULL sequence rather than assuming empty tables.
|
||
|
||
---
|
||
|
||
## 7. Failure, errors and logging
|
||
|
||
### 7.1 Exceptions model the recovery, not the cause
|
||
|
||
The exception hierarchy is shaped by what the caller should *do*:
|
||
|
||
```
|
||
GmailApiException
|
||
├── GmailPermanentException — stop, this will never work
|
||
└── GmailThrottledException — back off and retry
|
||
GraphApiException
|
||
├── GraphResyncRequiredException
|
||
└── GraphThrottledException
|
||
```
|
||
|
||
A caller catching `GmailThrottledException` knows to retry; a caller catching
|
||
`GmailPermanentException` knows not to. That distinction is worth two classes.
|
||
|
||
### 7.2 Refuse loudly where refusing is cheap, warn where it is not
|
||
|
||
The clearest instance in the codebase, and a pattern worth stealing: `EncryptionKeyProbe` runs at
|
||
container start and **refuses to boot the web server** when the encryption key does not match stored
|
||
data — rather than writing data half the fleet cannot read. But the same probe only *warns* under
|
||
the console:
|
||
|
||
> The probe is deliberately fatal only when starting the server — a console invocation warns and
|
||
> continues, because refusing it would block the very command that repairs the situation.
|
||
|
||
Fail-fast is not a blanket rule. It is applied where the failure is worse than the stop, and the
|
||
exception is documented where it lives.
|
||
|
||
### 7.3 Health endpoints report verdicts, not data
|
||
|
||
```
|
||
GET /healthz — unauthenticated, because Docker healthchecks and uptime monitors hold no session.
|
||
```
|
||
|
||
- 503 only for the failure where serving is genuinely impossible (database down). A backed-up queue
|
||
stays 200: "mail is late, not gone, and restarting the container would not help."
|
||
- **No counts, no addresses, no version.** Anyone who can reach the port can read it. Numbers live
|
||
behind the admin role.
|
||
- A test asserts that shape, so an addition that leaks something fails the suite.
|
||
|
||
### 7.4 Logging
|
||
|
||
A constructor-injected `LoggerInterface $logger`, never a static call or a facade. Messages are
|
||
prefixed with the subsystem and written as statements of what happened:
|
||
|
||
```php
|
||
$this->logger->warning('GmailPush: rejected notification with bad or missing token');
|
||
$this->logger->warning('GmailPush: no emailAddress in payload');
|
||
$this->logger->warning('GmailPush: unparseable envelope');
|
||
```
|
||
|
||
Levels mean something: `warning` for "this request could not be honoured and here is why",
|
||
`error` for "something is broken". A log line that fires per-event on a hot path gets rate-limited
|
||
rather than deleted — an unactionable message repeated several times a second buries everything
|
||
else.
|
||
|
||
---
|
||
|
||
## 8. Templates, JavaScript and CSS
|
||
|
||
### 8.1 Twig
|
||
|
||
**Header comment first**, describing what the template renders and what variables it accepts:
|
||
|
||
```twig
|
||
{#
|
||
Renders one row in the message list.
|
||
|
||
Accepts either a MessageThread (`thread`) or a Message (`message`);
|
||
everything below normalises onto the same set of variables so the markup
|
||
is shared.
|
||
|
||
Draft handling: a row opens the compose dock instead of the reading pane
|
||
only when the row IS the draft …
|
||
#}
|
||
```
|
||
|
||
**Normalise, then render.** A partial that accepts more than one shape resolves everything into a
|
||
common set of `{% set %}` variables at the top, then has exactly one block of markup. Branching in
|
||
the markup is the thing this avoids.
|
||
|
||
Other rules:
|
||
|
||
- Defaults declared explicitly at the top: `{% set draft_scope = draft_scope|default(false) %}`.
|
||
- All user-facing text through `|trans` with dotted keys (`'thread_row.no_subject'|trans`). No
|
||
literal English in a template.
|
||
- Comments inside the logic block explain the same class of thing PHP comments do — see §2.3, whose
|
||
example is from this file.
|
||
- Partials underscore-prefixed; Turbo Stream responses in their own `.stream.html.twig`.
|
||
|
||
### 8.2 Translations
|
||
|
||
Two domains, and the distinction has bitten:
|
||
|
||
```yaml
|
||
# Constraint messages. Symfony translates validation output in the `validators`
|
||
# domain, not `messages` — a key put in messages.*.yaml renders as the raw key,
|
||
# which is how "setup.install.password_too_short" ended up on screen.
|
||
```
|
||
|
||
`messages.<locale>.yaml` is nested by area; `validators.<locale>.yaml` is flat with dotted keys.
|
||
Message text is a complete sentence with an em dash where it explains itself:
|
||
|
||
> `Enter the client secret too — a client ID on its own cannot sign anyone in.`
|
||
|
||
### 8.3 Stimulus controllers
|
||
|
||
```js
|
||
import { Controller } from "@hotwired/stimulus";
|
||
|
||
/**
|
||
* Handles per-row status actions in the message list.
|
||
*
|
||
* Values:
|
||
* id — entity ID (thread or message)
|
||
* type — "thread" (default) | "message"
|
||
*
|
||
* Routes used:
|
||
* thread → /thread/{id}/status/{action}
|
||
* message → /message/{id}/status/{action}
|
||
*/
|
||
export default class extends Controller {
|
||
static values = {
|
||
id: Number,
|
||
type: { type: String, default: "thread" },
|
||
};
|
||
```
|
||
|
||
- Anonymous `export default class extends Controller` — the filename is the identity.
|
||
- Docblock lists **values, targets and routes** as an aligned table. This is the controller's
|
||
contract with the template.
|
||
- Action parameters read via destructuring with defaults:
|
||
`const { snoozeUrl, until = null } = event.params;`
|
||
- Private methods use real `#private` syntax and live below a `// ── Private ──` separator.
|
||
- 4-space indent, double quotes, semicolons, trailing commas — same as the PHP side where the
|
||
language allows.
|
||
|
||
### 8.4 CSS
|
||
|
||
Design tokens, not literal colours, and the token file explains its own mechanics:
|
||
|
||
```css
|
||
/* ═══ Palette channels + runtime knobs ══════════════════════════════════════
|
||
Unlayered on purpose: these are variable definitions, not utilities, and
|
||
unlayered declarations outrank Tailwind's cascade layers.
|
||
Channels are stored as space-separated RGB triplets so alpha stays
|
||
composable via rgb(var(--x) / <alpha>).
|
||
═══════════════════════════════════════════════════════════════════════════ */
|
||
:root {
|
||
--rgb-surface: 255 255 255;
|
||
--rgb-ink: 39 39 42;
|
||
--rgb-accent: 37 99 235;
|
||
```
|
||
|
||
- Values aligned in columns, numerically right-aligned.
|
||
- Semantic names (`--rgb-ink-muted`, `--rgb-danger`), never `--gray-3`.
|
||
- A token that is unusual gets a paragraph — the mail-body sheet colour has one explaining that
|
||
senders authored their HTML against a light background, so rendering it dark misrepresents what
|
||
was sent.
|
||
- Dark mode as an explicit variant (`@variant dark (&:where(.dark, .dark *))`), tokens re-declared
|
||
rather than colours overridden per component.
|
||
|
||
---
|
||
|
||
## 9. Tests
|
||
|
||
### 9.1 Layout and naming
|
||
|
||
PHPUnit mirrors `src/` exactly: `src/Service/Mail/ThreadSnoozeService.php` →
|
||
`tests/Service/Mail/ThreadSnoozeServiceTest.php`. Namespace `App\Tests\…`. Classes are `final`.
|
||
|
||
**Test method names are sentences about behaviour**, not about the method under test:
|
||
|
||
```php
|
||
public function testSnoozeMovesTheThreadOutOfTheInbox(): void
|
||
public function testAbsentWorkersAreUnknownRatherThanUnhealthy(): void
|
||
public function testUnreadFollowsSeenAtRatherThanTheImapSeenFlag(): void
|
||
public function testAnEmptyTransportReportsNothingRatherThanZero(): void
|
||
public function testATokenlessPostDoesNotWrite(string $path, array $payload): void
|
||
public function testItLeaksNothingAboutTheInstance(): void
|
||
```
|
||
|
||
The `X rather than Y` form is used deliberately: it names the wrong behaviour the test exists to
|
||
prevent. Assertions carry messages where the failure would otherwise be cryptic:
|
||
|
||
```php
|
||
self::assertFalse($message->labels->contains($inbox), 'Inbox label should be gone');
|
||
```
|
||
|
||
### 9.2 The test class docblock states the claim
|
||
|
||
```php
|
||
/**
|
||
* Snooze is a label move, not a column write.
|
||
*
|
||
* That distinction is the whole subject here. The web endpoint used to set
|
||
* snoozedUntil directly and nothing else, which left the conversation sitting
|
||
* in the Inbox — locally and at the provider — while the row disappeared from
|
||
* the list, until the sweep "woke" a thread that had never left.
|
||
*
|
||
* Against a real container and a real database rather than doubles, for two
|
||
* reasons. Every collaborator this service takes is `final`, so none of them
|
||
* can be doubled anyway; and the behaviour worth pinning is the one that
|
||
* emerges from them together — labels moved, an outward propagation queued —
|
||
* which a set of mocks would assert into existence rather than observe.
|
||
*/
|
||
```
|
||
|
||
One claim, the bug it guards, and the reason for the testing strategy.
|
||
|
||
### 9.3 Real container over mocks
|
||
|
||
The default is `KernelTestCase` with the real services and a real database, wrapped in a transaction
|
||
that is never committed:
|
||
|
||
```php
|
||
protected function setUp(): void
|
||
{
|
||
self::bootKernel();
|
||
|
||
$container = self::getContainer();
|
||
$this->em = $container->get(EntityManagerInterface::class);
|
||
$this->service = $container->get(ThreadSnoozeService::class);
|
||
|
||
// Never committed, so the suite leaves nothing behind and can be run
|
||
// repeatedly against the same database.
|
||
$this->connection->beginTransaction();
|
||
}
|
||
|
||
protected function tearDown(): void
|
||
{
|
||
if (true === $this->connection->isTransactionActive()) {
|
||
$this->connection->rollBack();
|
||
}
|
||
|
||
parent::tearDown();
|
||
}
|
||
```
|
||
|
||
Seeding is done by private helpers on the test class (`seedAccount()`, `inboxThread(2)`), named for
|
||
what they produce.
|
||
|
||
Data providers via `#[DataProvider('someCases')]`, used for table-driven cases (header parsing,
|
||
address parsing, charset handling, endpoint lists).
|
||
|
||
### 9.4 A test must be able to fail
|
||
|
||
Stated as a rule and practised:
|
||
|
||
> Each of these was mutation-checked: the fix reverted, the suite run, the failure confirmed. A test
|
||
> that passes against the bug it names is worse than no test, because it is also a claim.
|
||
|
||
Coverage is chosen by risk, not by line count — the criterion used here is "a bug in this is
|
||
invisible until somebody notices mail is in the wrong place."
|
||
|
||
### 9.5 An assertion must survive the fixture growing
|
||
|
||
Count what the action *changed*, not what the database happens to hold.
|
||
|
||
```php
|
||
// Brittle: has to be edited every time the seeded mailbox gains a thread,
|
||
// and says nothing about the behaviour under test.
|
||
self::assertCount(13, $threads);
|
||
|
||
// Durable: a baseline, then the delta the action caused.
|
||
$before = count($repository->findBy(['account' => $account]));
|
||
$this->deliver($scenario);
|
||
self::assertCount($before + 1, $repository->findBy(['account' => $account]));
|
||
```
|
||
|
||
Where there is no "before" — a freshly provisioned user, say — assert the **relation** that holds
|
||
whatever the fixture's size:
|
||
|
||
```php
|
||
// Every message is its own thread EXCEPT the conversation, whose turns collapse.
|
||
self::assertCount(
|
||
count($messages) - (count(DemoMailbox::CONVERSATION) - 1),
|
||
$threads,
|
||
);
|
||
```
|
||
|
||
This applies to anything a shared fixture owns — messages, threads, labels, mailboxes, filters,
|
||
accounts, calendars. `seed-mail` seeds four threads today; a test asserting `toHaveCount(4)` is
|
||
asserting that number, not the feature, and it will fail the day somebody seeds a fifth for an
|
||
unrelated reason. The failure will be in a spec whose subject has nothing to do with the change,
|
||
which is the expensive kind.
|
||
|
||
Two absolutes remain correct and should not be rewritten:
|
||
|
||
- **Absence.** `assertCount(0, $crawler->filter('form'))` — zero *is* the claim, and it does not
|
||
drift.
|
||
- **A fixture the test owns.** A test that creates two events and asserts two is self-contained; a
|
||
baseline there would be ceremony.
|
||
|
||
The distinction is ownership: count freely what your own test made, never what a shared seeder did.
|
||
|
||
### 9.6 Suite configuration is strict
|
||
|
||
```xml
|
||
failOnDeprecation="true"
|
||
failOnNotice="true"
|
||
failOnWarning="true"
|
||
```
|
||
|
||
Plus `restrictNotices` / `restrictWarnings` on the source block. A warning is a failure.
|
||
|
||
### 9.7 Browser tests
|
||
|
||
Separate directory (`tests/e2e/`), excluded from the PHPUnit suite explicitly rather than by relying
|
||
on file extensions. The conventions that matter:
|
||
|
||
- **Specs import `test` from a local support module**, not from `@playwright/test`, and that module
|
||
re-exports everything else so a spec has one import line. The wrapper exists to give each parallel
|
||
worker its own user and its own signed-in session.
|
||
- **Per-worker fixtures over a global setup file**, because a single storage-state file only works
|
||
with one worker.
|
||
- **Locators by role and accessible name**
|
||
(`getByRole("button", { name: "Create label" })`), scoped to a container locator. Not CSS
|
||
selectors, except for structural ids that are part of the contract (`#modal-backdrop`).
|
||
- **Each spec reseeds its own fixtures** in `beforeEach`, so tests are independent and re-runnable.
|
||
- **Regression guards are labelled as such**, with the bug they hold:
|
||
|
||
```js
|
||
// Regression guard: the form must actually render inside the modal
|
||
// frame. Without the <turbo-frame id="modal"> wrapper Turbo finds
|
||
// nothing to swap and the dialog stays on the spinner.
|
||
```
|
||
- Anything that makes the suite slow is measured before it is accepted, and the reasoning is written
|
||
down — the CONTRIBUTING section on this names the change that took the run from 255s to 138s and
|
||
why it is easy to undo by accident.
|
||
|
||
---
|
||
|
||
## 10. Static analysis and tooling
|
||
|
||
### 10.1 PHPStan: a level you can hold, plus a shrinking baseline
|
||
|
||
```neon
|
||
parameters:
|
||
level: 5
|
||
paths:
|
||
- ./
|
||
```
|
||
|
||
The config's own comment is the policy:
|
||
|
||
> Level 5 with everything already failing captured in a baseline. The alternative — level 0 with no
|
||
> baseline — reports nothing interesting, and level 9 buys a fortnight of triage before it finds its
|
||
> first real bug. […] **The baseline is a debt ledger, not a licence.** It shrinks; raise the level
|
||
> when it is small enough that doing so is a day's work rather than a fortnight's.
|
||
|
||
Further rules that generalise:
|
||
|
||
- **Analyse the whole tree**, not just `src/`. `bin/`, `config/` and `public/` hold real code that
|
||
breaks the same way. Exclusions are enumerated with a reason each.
|
||
- Paths that may legitimately be absent are marked `(?)`, because PHPStan treats an
|
||
excludePath matching nothing as a config error.
|
||
- **`ignoreErrors` stays empty.** Suppression is a decision that needs a paragraph; if it has one,
|
||
it usually turns out the fix was cheaper.
|
||
- `treatPhpDocTypesAsCertain: false`, because docblocks here are prose as much as types and a stale
|
||
`@var` otherwise produces a stream of false "always false" reports.
|
||
|
||
### 10.2 Composer scripts are documented
|
||
|
||
```json
|
||
"scripts-descriptions": {
|
||
"test": "Run the PHPUnit suite (unit tests). Browser end-to-end tests live in tests/e2e and run via `npm run test:e2e`.",
|
||
"stan": "Run PHPStan at level 5 against src/ and tests/, ignoring what phpstan-baseline.neon already records."
|
||
}
|
||
```
|
||
|
||
Every task a contributor might run has a named script and a description, and both suites are listed
|
||
in `CONTRIBUTING.md` with the exact commands.
|
||
|
||
### 10.3 Version pinning
|
||
|
||
Framework packages pinned to one minor (`8.1.*`) via `extra.symfony.require`; the Node version lives
|
||
in `.nvmrc` and CI reads *that file* rather than repeating the number. One place per fact.
|
||
|
||
---
|
||
|
||
## 11. Documentation, changelog and commits
|
||
|
||
### 11.1 The document split
|
||
|
||
| File | Audience | Contains |
|
||
|---|---|---|
|
||
| `README.md` | Someone deciding whether to run it | What it is, what it can do, how to install |
|
||
| `CONTRIBUTING.md` | Someone changing it | Setup, tests, architecture notes, command reference, roadmap |
|
||
| `CHANGELOG.md` | Someone upgrading | What changed per release, and what it costs them |
|
||
| `CODESTYLE.md` | Someone writing in it | This file |
|
||
|
||
`README` never explains internals; `CONTRIBUTING` never explains what the product is.
|
||
|
||
### 11.2 Documentation explains failure modes, not just steps
|
||
|
||
The distinguishing feature of `CONTRIBUTING.md` here is that each section ends with what goes wrong.
|
||
Not "run this command", but:
|
||
|
||
> They drifted apart once before and it cost real data: the `.dist` was missing the
|
||
> `app_attachments`, `app_raw` and `app_uploads` mounts, so anyone starting from a fresh clone got
|
||
> attachment downloads that 404'd and blob data that vanished on container recreate.
|
||
|
||
Sections literally titled **"Things that bite"** collect the traps. A roadmap item that is done gets
|
||
ticked, with an explicit note that under-reporting is worse than no roadmap.
|
||
|
||
### 11.3 Changelog entries lead with the user's symptom
|
||
|
||
Bold the symptom in the user's words, then explain the cause and what changed:
|
||
|
||
```markdown
|
||
### Fixed
|
||
|
||
- **An Outlook account could stop syncing entirely.** `meetingMessageType` is
|
||
declared on Graph's event-message type, not on the base message, and naming it
|
||
unqualified does not get ignored — the whole `$select` is rejected, every
|
||
message in the batch answers 400, and nothing arrives. It is asked for through
|
||
the cast now, and a mailbox that refuses even that gets one retry without it
|
||
and is remembered, rather than never syncing again.
|
||
```
|
||
|
||
Each release states its migration and deployment impact up front —
|
||
`No schema change, no deployment change.` — because migrations run automatically on boot.
|
||
|
||
A pull request writes its entry as a file of its own (`changelog.d/266-jmap-read-state.md`), and
|
||
the release collects them into the changelog with a script. Pull requests that each add a paragraph
|
||
at the top of one file conflict with each other in turn, and each resolution re-runs CI.
|
||
|
||
### 11.4 Commit messages
|
||
|
||
**Subject: an imperative sentence describing the change's effect, no prefix, no scope, no ticket.**
|
||
|
||
```
|
||
Make a Graph folder move replace the location, not add to it
|
||
Stop believing a charset label the bytes disprove
|
||
Say why a message is in the tab it is in
|
||
Leave controllers with their actions and little else
|
||
Put every query in a repository, and make the rest say why they exist
|
||
Cover the decisions that fail without saying anything
|
||
```
|
||
|
||
Not `fix(graph): folder move`. Not `Fixed bug`. The subject is what the codebase can now do, or
|
||
stop doing, written as an instruction to it. `Stop …`, `Make …`, `Say …`, `Put …`, `Give …`,
|
||
`Leave …` are the recurring openers.
|
||
|
||
**Body: the reasoning, and what fell out.** Wrapped at ~72 columns, prose, with an aligned table when
|
||
the change touches a set of files:
|
||
|
||
```
|
||
Ninety-two tests over five classes, chosen on one criterion: a bug here
|
||
is invisible until somebody notices mail is in the wrong place.
|
||
|
||
MessageCategorizer the cascade, and the orderings that matter — a
|
||
mailing list carrying List-Unsubscribe is a forum,
|
||
a known correspondent beats every bulk header.
|
||
SearchQueryParser what happens to operators that cannot be honoured,
|
||
since every way of getting that wrong silently
|
||
widens the search to the whole mailbox.
|
||
|
||
Writing them turned up one more bug. A word ending in a colon — "Re:" —
|
||
was parsed as a half-typed operator and dropped …
|
||
```
|
||
|
||
Incidental discoveries go in the body — "Two bugs fell out" — rather than being silently included or
|
||
split into commits nobody can review.
|
||
|
||
Release commits are `Record vX.Y.Z`.
|
||
|
||
---
|
||
|
||
## 12. The condensed rules
|
||
|
||
Paste this into `CLAUDE.md`, `AGENTS.md`, or a PR template.
|
||
|
||
**Comments**
|
||
1. Every class gets a docblock: what it is, how it is modelled, what was rejected.
|
||
2. Comments say *why*. If a comment restates the code, delete it.
|
||
3. When code exists because of a bug, name the bug.
|
||
4. Document deliberate absences — the empty config, the check that is not there.
|
||
5. No TODO/HACK/FIXME. Either fix it or write down why it stays.
|
||
|
||
**Structure**
|
||
6. Controllers resolve, authorise, delegate, render. Nothing else.
|
||
7. Every query lives in a repository. Raw SQL says why it had to be raw.
|
||
8. One service per concept; every caller goes through it. Differences between callers are named at
|
||
both ends.
|
||
9. Interfaces for the axis that varies; implementations under the provider's own directory.
|
||
10. DTOs (`final readonly`) cross boundaries, not arrays.
|
||
11. Messenger payloads are readonly and carry ids, never entities.
|
||
|
||
**PHP**
|
||
12. `declare(strict_types=1);` on every file. PSR-4, one class per file.
|
||
13. `final` by default; `final readonly` for value objects.
|
||
14. Constructor injection, promoted, `private readonly`, one per line, aligned, trailing comma.
|
||
15. Public properties with `public private(set)` where the outside must not write. No getters or
|
||
setters.
|
||
16. Comparisons are explicit and literal-first: `null === $x`, `true === $flag`, `[] === $list`.
|
||
17. Constants are typed (`private const array`) and named for meaning.
|
||
18. Enums own their per-case rules as exhaustive `match` tables with no `default`.
|
||
19. `sprintf()` over interpolation. `DateTimeImmutable` over `DateTime`.
|
||
20. Guard clauses and early returns; blank line before every `return` that follows a statement.
|
||
21. Docblocks only for what the signature cannot say: `list<T>`, `array<K,V>`, `Collection<int,T>`.
|
||
22. Four spaces, LF, UTF-8, final newline, no trailing whitespace.
|
||
|
||
**Data**
|
||
23. Timestamps come from one trait, on every entity, with no exceptions.
|
||
24. Every index and unique constraint is commented, including why the columns are in that order.
|
||
25. Migrations carry a reasoning docblock and a one-line imperative description; backfills go
|
||
nullable → populate → NOT NULL.
|
||
|
||
**Failure**
|
||
26. Exception classes model what the caller should do (permanent vs. throttled), not what went wrong.
|
||
27. Refuse loudly where refusing is cheaper than the corruption; warn where refusing would block the
|
||
repair.
|
||
28. Unauthenticated endpoints report verdicts, never data. A test asserts the shape.
|
||
|
||
**Frontend**
|
||
29. Twig partials open with a header comment listing their inputs, normalise to common variables,
|
||
then render one block of markup.
|
||
30. All user-facing text goes through translation keys. Validation messages live in the `validators`
|
||
domain.
|
||
31. Stimulus controllers document values, targets and routes in the class docblock; private methods
|
||
use `#`.
|
||
32. Colours are semantic tokens, declared once; components never carry literals.
|
||
|
||
**Tests**
|
||
33. `tests/` mirrors `src/`. Test classes are `final`, methods are sentences —
|
||
`testUnreadFollowsSeenAtRatherThanTheImapSeenFlag`.
|
||
34. The class docblock states the claim and the bug it guards.
|
||
35. Real container and real database over mocks; roll back in `tearDown`.
|
||
36. Every test must be shown to fail against the bug it names.
|
||
37. Warnings, notices and deprecations fail the suite.
|
||
38. Browser specs import `test` from the local support module, reseed their own fixtures, and locate
|
||
by role and accessible name.
|
||
|
||
**Tooling and docs**
|
||
39. Static analysis runs over the whole tree with a baseline that shrinks. `ignoreErrors` stays empty.
|
||
40. Every fact lives in one place — the Node version in `.nvmrc`, the framework minor in one
|
||
constraint.
|
||
41. Commit subjects are imperative sentences about effect, with no prefix; bodies explain the
|
||
reasoning and disclose what fell out.
|
||
42. Changelog entries lead with the user's symptom in bold, then the cause, then the fix. Every
|
||
release states its schema and deployment impact.
|
||
43. A change to what a user does, an operator sets, or a provider needs, updates the handbook in the
|
||
same commit. The inventory — commands, environment variables, links, pages — is asserted by a
|
||
test, so the review is spent on whether the prose is still true.
|