CONTRIBUTING.md
Rama main · réplica de lectura del repositorio original.
# Contributing to Prismarr
> Definition of Done - every feature, fix or refactor must pass this checklist
> before being considered complete and pushed to `main`.
Prismarr is a self-hosted app published publicly on Docker Hub. Quality and
security directly affect real users who run this on their homelab. Respect
the process - "we'll fix it later" becomes a user-reported bug in v1.x.
---
## 🔧 1. Code
- [ ] The feature works end-to-end, tested manually in a running container.
- [ ] No leftover `console.log`, `dd()`, `var_dump`, `dump()`, `TODO`, `FIXME`.
- [ ] Comments are written in **English**. User-visible strings go through the i18n layer (`messages+intl-icu.{en,fr}.yaml`), with English as the source of truth.
- [ ] **Zero credentials** in code, commits, logs, docs (sacred rule).
- [ ] No unused `use` / imports.
## 🧪 2. Tests
- [ ] At least **one unit test** for any new business logic.
- [ ] For a bug fix: **a regression test that first reproduces the bug**, then
the fix turns it green.
- [ ] `make test` passes at 100% - no `markTestSkipped()` or `markTestIncomplete()`.
- [ ] Edge cases covered: `null`, empty strings, unicode, boundaries.
## 💾 3. Schema / Migrations
- [ ] If you touched an Entity: ran `make migrations-diff` and committed the
generated file.
- [ ] Reviewed the SQL in `migrations/Version*.php` - especially `DROP` and
`RENAME` operations (they lose data).
- [ ] Tested with a clean DB: `docker compose down -v && make restart`.
- [ ] **Never modify a migration once pushed to `main`.** If a released
migration is wrong, create a new migration that corrects it.
## 🔒 4. Security
- [ ] `#[IsGranted('ROLE_ADMIN')]` on any destructive action (delete, reset,
bulk operations, config changes).
- [ ] `$e->getMessage()` is never leaked in a JSON response - log it internally
and return a generic message.
- [ ] All user input is validated (email format, URL scheme, length, type).
- [ ] No concatenated SQL - use Doctrine parameter binding.
- [ ] CSRF token on every sensitive form.
- [ ] If you fetch a user-supplied URL: SSRF guard (protocol whitelist +
`CURLOPT_REDIR_PROTOCOLS`).
## 🎨 5. UI / UX
- [ ] Tested in a real browser: happy path + 1–2 edge cases.
- [ ] Turbo-safe if there's inline JS:
- IIFE wrapper `(function(){ ... })()`
- `var` instead of `const`/`let`
- `.then()` instead of `async/await`
- Declarative `data-bs-toggle` for modals (never `new bootstrap.Modal()`)
- Helpers `bindDoc(event, key, handler)` for global listeners
- `window._prismarr*Timer` + `turbo:before-render` cleanup for polling
- [ ] User-facing error messages go through the translator (`trans()`), in plain language, no technical jargon.
- [ ] Any dynamic value spliced into an `innerHTML` template runs through `window.escHtml()` (escapes `& < > " '`).
- [ ] Dark **and** light theme both checked.
- [ ] Responsive verified on at least one mobile viewport.
### 5.1 Multi-instance Radarr / Sonarr
Since v1.1.0, `RadarrClient` and `SonarrClient` are autowired *unbound* —
they only point at a real instance once `bindInstance($i)` (mutating, used
by `MultiInstanceBinderSubscriber` for slug-aware routes) or
`withInstance($i)` (immutable clone, used in dashboard / calendar / search
where the same request iterates over every instance) has been called.
- [ ] Inside a slug-aware route (`/medias/{slug}/…`), the autowired client
is already bound — call its methods directly. The subscriber 404s
unknown slugs before the controller runs.
- [ ] Outside a slug-aware route, **never** call the autowired client
directly: it points at whichever instance was last bound and is
effectively undefined. Iterate `ServiceInstanceProvider::getEnabled()`
and call `withInstance($i)->method()` per loop iteration.
- [ ] Items present on multiple instances are deduped by `tmdbId` (movies)
or `tvdbId` then `tmdbId` (series) — same key Phase D uses
everywhere (dashboard, calendar, search, qBit resolver).
- [ ] If the data needs to know which instance it came from (badge,
"open in Radarr" link, owner status) carry the slug alongside the
record — the slug is the primary key the frontend uses to round-trip.
- [ ] iCal `UID`s and any persistent identifier surfaced to the user must
be rooted on `tmdbId` / `tvdbId`, not on the per-instance internal
Radarr/Sonarr `id`, so the same item stays stable across instance
swaps.
## 📚 6. Docs & Git
- [ ] Commit message is clear and self-contained (one or two lines describing
the change and its motivation).
- [ ] `README.md` updated if the feature changes user-facing behaviour.
- [ ] `CHANGELOG.md` updated under `[Unreleased]` with a one-liner entry.
- [ ] `git push` immediately after the commit (no local-only commits).
---
## When you hit any of these situations, stop
- You catch yourself saying *"I'll add the test later"* → **no**, add it now.
- You want to tweak an already-pushed migration → **no**, write a new one.
- A user-supplied input is going into a URL request → **no**, SSRF-guard it.
- A JSON error response is about to contain an exception message → **no**,
strip it and log internally.
---
## The workflow in one sentence
> Feature works → tests green → migration reviewed → security checked →
> UI tested → docs updated → commit message in English or French → push.
---
## The single command before every commit
```bash
make check
```
Runs PHP lint + Twig lint + full PHPUnit suite in ~2 seconds. If it fails,
do not commit. Fix first.
A GitHub Actions CI workflow runs `make check` on every PR and blocks merging
if it fails (`.github/workflows/ci.yml`) - the same contract, enforced.
---
## Evolving this workflow
This document is not frozen. As the project grows (new tooling, new types
of tests, new security concerns), amend it:
1. Propose the change in a PR, motivated by a real failure or a new risk.
2. Update `CONTRIBUTING.md` + `Makefile` + `.github/workflows/ci.yml` together.
3. Announce the change in `CHANGELOG.md` under a `### Contributor`
heading so existing contributors notice it.
4. Do not quietly lower the bar - always raise it. If a check becomes
redundant, replace it with something stronger, not nothing.
---
## 🔒 Absolute golden rules (non-negotiable, no exceptions)
These five rules override every other concern, including urgency. Breaking them
is what kills a public project. If a contribution requires violating any of
them, reject it and propose an alternative.
1. **No credentials** in code, commits, logs, docs, or messages. Prismarr's
repo is public; a leaked secret is exploited by a bot within minutes.
Credentials live in `.env.local` (gitignored) or the `setting` DB table.
2. **No modified migrations after push.** Once a `migrations/Version*.php`
is on `main`, it is immutable. To fix a bad schema, write a NEW migration.
Editing a released migration corrupts every user's database on upgrade.
3. **No destructive or breaking changes for production users.** Covers:
renaming a public env var, renaming a `setting` DB key (users lose their
config), changing the default port, breaking the `.env.local` /
`prismarr.db` format without a migration path, removing a feature without
a deprecation cycle, hard-deleting entities instead of soft-deleting.
**If `docker compose pull` can break an existing install, the change
needs a documented migration plan or must be rethought.**
4. **`make check` must be green before every commit.** Runs lint + full
PHPUnit suite. Completes the Definition of Done checklist above - no
test for new logic = no commit. No regression test for a bug fix = no
commit.
5. **No silent weakening of existing security checks.** CSP, CSRF,
rate-limiter login, SSRF guard, ProfilerGuard, CURLOPT_REDIR_PROTOCOLS,
`ROLE_ADMIN` attributes - each one has a documented reason (see commit
history for Sessions 7a–7d + 8a). If a check blocks a legitimate feature,
refine it (e.g. extend the CSP with justification) - do not disable it.
Violating any of these, even if `make check` passes, makes the commit invalid.