Best practices: should-fix deviations #14

New Issue

2026-09-15T09:21:42-04:00

lyra commented

2026-09-15 09:21:42 -04:00

Should Fix

Not broken, but non-idiomatic and likely to trip up contributors or tooling.

1. .dockerignore uses *.md to exclude docs. This is a footgun: the moment the project adds a non-doc markdown file it gets silently dropped from the image, and CONTRIBUTING.md/CHANGELOG.md are already inconsistently excluded while readme.md is not (only because it's needed at runtime? unclear). Prefer an explicit whitelist (see "must fix" #3) or list files by name.
2. Committed .env.test with fixed test secret/TOTP seed. Standard Symfony practice is to commit .env (defaults) and .env.test with non-sensitive test values — which is what's done — but the fixed APP_SECRET=test_secret_key_change_me and fixed TOTP secret JBSWY3DPEHPK3PXP live in both phpunit.dist.xml and .env.test, i.e. duplicated in two places. Pick one source of truth (.env.test) and drop the <server> duplicates, or they will drift.
3. bin/phpunit shim bypasses Composer's bin proxy. It hard-requires vendor/phpunit/phpunit/phpunit, skipping the vendor/bin/phpunit proxy Composer already generates, and doesn't forward a -c config. Either delete it and use vendor/bin/phpunit, or have it pass -c phpunit.xml.dist. (Tied directly to must-fix #1.)
4. No Composer scripts for common tasks. composer.json only has the Symfony auto-scripts. Standard practice is to expose composer test, composer cs-check, composer cs-fix so contributors/CI don't have to know the bespoke tooling paths. CI currently calls vendor/bin/php-cs-fixer + vendor/bin/phpunit by hand.
5. readme.md / license.txt lowercase + non-standard extension. Convention is README.md and LICENSE (or LICENSE.md). GitHub/Gitea auto-detect the standard names; license.txt is likely not surfaced in the repo UI/license API. Low-risk renames.
6. phpunit config sets no requireCoverageMetadata / coverage target, and CI runs --coverage-text with no threshold. Not a defect, but best practice for a security tool is an enforced minimum coverage (--fail-on-risky, requireCoverageMetadata="true", or a min % in CI). Currently coverage is informational only.

(Related: most of #3/#4 disappear once must-fix #1 renames the phpunit config, since vendor/bin/phpunit will then auto-discover it.)

## Should Fix Not broken, but non-idiomatic and likely to trip up contributors or tooling. - [ ] **1. `.dockerignore` uses `*.md` to exclude docs.** This is a footgun: the moment the project adds a non-doc markdown file it gets silently dropped from the image, and `CONTRIBUTING.md`/`CHANGELOG.md` are already inconsistently excluded while `readme.md` is not (only because it's needed at runtime? unclear). Prefer an explicit whitelist (see "must fix" #3) or list files by name. - [ ] **2. Committed `.env.test` with fixed test secret/TOTP seed.** Standard Symfony practice is to commit `.env` (defaults) and `.env.test` with *non-sensitive* test values — which is what's done — but the fixed `APP_SECRET=test_secret_key_change_me` and fixed TOTP secret `JBSWY3DPEHPK3PXP` live in **both** `phpunit.dist.xml` and `.env.test`, i.e. duplicated in two places. Pick one source of truth (`.env.test`) and drop the `<server>` duplicates, or they will drift. - [ ] **3. `bin/phpunit` shim bypasses Composer's bin proxy.** It hard-requires `vendor/phpunit/phpunit/phpunit`, skipping the `vendor/bin/phpunit` proxy Composer already generates, and doesn't forward a `-c` config. Either delete it and use `vendor/bin/phpunit`, or have it pass `-c phpunit.xml.dist`. (Tied directly to must-fix #1.) - [ ] **4. No Composer scripts for common tasks.** `composer.json` only has the Symfony auto-scripts. Standard practice is to expose `composer test`, `composer cs-check`, `composer cs-fix` so contributors/CI don't have to know the bespoke tooling paths. CI currently calls `vendor/bin/php-cs-fixer` + `vendor/bin/phpunit` by hand. - [ ] **5. `readme.md` / `license.txt` lowercase + non-standard extension.** Convention is `README.md` and `LICENSE` (or `LICENSE.md`). GitHub/Gitea auto-detect the standard names; `license.txt` is likely not surfaced in the repo UI/license API. Low-risk renames. - [ ] **6. `phpunit` config sets no `requireCoverageMetadata` / coverage target, and CI runs `--coverage-text` with no threshold.** Not a defect, but best practice for a security tool is an enforced minimum coverage (`--fail-on-risky`, `requireCoverageMetadata="true"`, or a min % in CI). Currently coverage is informational only. (Related: most of #3/#4 disappear once must-fix #1 renames the phpunit config, since `vendor/bin/phpunit` will then auto-discover it.)

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: public/preauth#14