Skip to content

CONTRIBUTING.md

Latest commit

 

History

History
179 lines (137 loc) · 8.04 KB

CONTRIBUTING.md

File metadata and controls

179 lines (137 loc) · 8.04 KB
Raw

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 UIDs 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

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.