Contributing

Contributing to FreeITSM

Pull requests are welcome, and several of FreeITSM's features started as one. This page is the checklist I review against. Every point on it is here because a real pull request (or my own code) got it wrong, so it is worth a read before you start - and worth giving to your AI coding assistant.

Each review is written up in full, so you can see the reasoning: Long forms in a task's Work view (#198) · REST custom fields, evidence and inventory (#195-#197) · Asset reconciliation, tags and labels (#164) · SSO JIT roles and profile sync (#155) · Checklists closure gating.

How a pull request is handled

Every pull request gets one of three verdicts:

• Merge - it goes in as it is.
• Merge with fixes - your commit goes in under your name, and my fixes follow in a commit of my own. A review page like the ones above says what changed and why.
• Rewrite - the idea is right but the code would be risky to adapt. I write it myself, credit you, and close the PR.

Nothing is merged without being read line by line and run. That includes tests, which run against a real database.

Before you write code

1. Agree the design first. For anything bigger than a fix, open an issue. I write a design on the wiki, and the code follows it - including the names of fields, parameters and permissions. If the code needs to differ, say so in the PR so the design can be updated.
2. Base it on current main, and keep each PR to one feature. Three small PRs are far easier to review than one big one.
3. Upgrades must be invisible. Somebody upgrading should see no change in behaviour until they choose a new option. Real installs - including MSPs running many client companies - depend on this.

The rules

Security

• Nothing runs from a URL unless it is meant to. Every file in tests/ and scripts/ starts with if (PHP_SAPI !== 'cli') { http_response_code(404); exit; }. CI fails without it (tests/web-exposure-guard.php). Files in includes/ only define functions and classes - opening one in a browser must do nothing.
• Check access before you write anything - module access, write access (Read only analysts change nothing), the company (multi-company installs), and for the REST API the key's permission. Before a file reaches the disk, not after.
• A REST permission does one job. Add a narrow resource or action (change_attachments.create) rather than borrowing a broad one (changes.update). The verbs are read, create, update, delete (and manage for configuration).
• Session endpoints check the CSRF token, and error messages never include database or exception text.

The database

• Schema goes in two places by hand, and one by script: database/freeitsm.sql (new installs) and includes/db_verify_schema.php (Database Verification, existing installs). Then run php scripts/gen_db_verify_indexes.php - includes/db_verify_indexes.php is generated, and CI fails if it is edited by hand.
• Verification adds missing tables, columns and indexes - and nothing else. It does not widen a column, change a unique key or move data. If an existing install needs that, write an upgrade step that is safe to run twice and keeps every row.
• Every column that names an analyst goes in analystReferenceRegistry() (includes/analyst_lifecycle.php), with its kind (history, setup, live, ...). tests/analyst-lifecycle.php fails otherwise.

Writing data

• One write path. If the UI already writes it, move that code into the service in includes/services/ and call it from both - never a second copy in the API.
• Own the transaction, or join the caller's. Side effects - workflows, emails, calendar sync, notifications - run after the commit, never inside it.
• A failed lookup is not an answer. Never turn an error into "no", "empty" or "not needed". Let it fail, or report it as unknown.
• Never swallow an error to make a test pass, and never return 200 for something you ignored - say what was ignored.
• Other people's rows are not yours. An importer touches only the rows its own source wrote. Before narrowing or widening a DELETE, find every writer of that table and check who relied on the old behaviour.

Tests

The tests run against the developer's own database, which is usually a copy of real data. So:

• Create what you need, with a ZZ/zz_ prefix, and delete it by the ids you created - never by name pattern or by "everything matching". A name can be truncated by a narrow column and never found again.
• Never go through a path that sends email, fires workflows or notifies people unless that is the thing being tested. Write the fixture row directly.
• Never change install settings. If you must, snapshot them and put them back in finally.
• Prove the test can fail. A control that fails on the old code is worth more than ten that pass.
• PHP and Chrome only - no Node.js, no npm. Every test runs as php tests/<name>.php. A test that needs a real browser drives Chrome over its DevTools socket from PHP: copy cdpEvaluate() from tests/task-form-embed.php (PR #198).
• Clean up API keys' rate-limit rows (api_key_rate_limits) along with the keys.

Text and translation

• New strings go in lang/en only. The other locales are translated together before each release.
• t() has no fallback argument, and existing English wording stays as it is - changing it breaks every translation of it.
• Buttons are one word where possible ("Save", "New", "Delete").

The REST API

• Every operation is in api/v1/spec.json, and php api/v1/lib/openapi_check.php passes.
• Errors use apiError() with a stable code (device_ref_conflict), and the message says what to do next.
• Retries are safe: a client_reference (or similar) returns the original for the same content and a 409 for different content.

Code style

• Match the file you are in. Keep the comments that explain why, and add a TRAP: comment wherever the obvious change would be wrong (grep -rn "TRAP:" lists them all).
• Documentation goes on this wiki, not in a docs/ folder in the repo.

Checks to run before sending it

php -l <every changed PHP file>
php scripts/gen_db_verify_indexes.php --check
php tests/web-exposure-guard.php
php api/v1/lib/openapi_check.php          # if you touched the REST API
php tests/analyst-lifecycle.php           # if you added a column that names an analyst
FREEITSM_TEST_URL=http://localhost/your-site php tests/<your-suite>.php

Then run Database Verification (System → Database) on an install that already had data, and check that nothing changed that should not have.