diff --git a/.docker/Caddyfile b/.docker/Caddyfile new file mode 100644 index 00000000..e0a7f7f0 --- /dev/null +++ b/.docker/Caddyfile @@ -0,0 +1,120 @@ +# FrankenPHP configuration for the POC. Ported from .docker/nginx.conf and +# .docker/templates/default.conf.template. +# +# Mounted over the image default at /etc/frankenphp/Caddyfile. +{ + # Traefik terminates TLS and Caddy is only ever reached over plain HTTP on + # the app network, so Caddy must neither request nor serve certificates. + auto_https off + skip_install_trust + + # Request metrics, exposed at /metrics below. + metrics + + servers { + # set_real_ip_from / real_ip_recursive / real_ip_header X-Forwarded-For. + # + # private_ranges, not the template's 172.16.0.0/16: compose puts the + # frontend network on 172.18.0.0/16 and the client on 172.22.0.0/16, + # neither of which that /16 covers, so real-IP resolution never + # happened. private_ranges is 10/8, 172.16/12, 192.168/16 and + # localhost, which is what a /12 in the template would have meant. + trusted_proxies static private_ranges + client_ip_headers X-Forwarded-For + } + + frankenphp { + {$FRANKENPHP_CONFIG} + } +} + +# A bare port means no hostname, and so no certificate handling. +{$SERVER_NAME::8080} { + root {$SERVER_ROOT:/app/public} + + # gzip on + encode zstd br gzip + + # access_log /dev/stdout main + # + # JSON rather than nginx's `main` layout. Every field that format carried is + # here — client_ip, user_id, ts, method/uri/proto, status, size, and the + # Referer, User-Agent and X-Forwarded-For headers — plus duration, which + # nginx did not log. Reproducing the text layout byte for byte needs the + # transform encoder, which is not in the published image: this build has + # console, json, append, filter and journald only. It matches supercronic, + # which the fpm image already runs with -json. + log { + output stdout + format json + } + + # client_max_body_size + request_body { + max_size {$PHP_MAX_BODY_SIZE:5MB} + } + + # Prometheus metrics, behind ITKMetricsAuth on its own Traefik router, the + # way /cron-metrics was. + # + # This replaces the nginx `location = /cron-metrics` proxy to + # supercronic. That proxy pointed at ${NGINX_CRON_METRICS}, and the fpm + # entrypoint only starts supercronic when /app/crontab exists — this + # project has no crontab, so nothing ever listened and the route answered + # 502. nginx itself exported nothing: stub_status is compiled into the + # image but the template never enabled it. + # + # Caddy does export, so the endpoint finally has something behind it: + # request counts, durations and sizes by code, method and handler, requests + # in flight, plus Go runtime and process metrics. A supercronic sidecar, if + # one is ever added, needs a route of its own. + handle /metrics { + metrics + } + + # Protect files and directories from prying eyes. + # + # The nginx version leans on a negative lookahead to let /.well-known + # through. RE2, which Caddy uses, has no lookaheads, so the exception is a + # matcher of its own instead. + # + # Note that the extension list covers yml but not yaml: public/ serves + # api-spec-v1.yaml. + @hidden { + path_regexp hidden /\. + not path /.well-known/* + } + respond @hidden 404 + + @protected path_regexp protected (?i)(\.(engine|inc|install|make|module|profile|po|sh|.*sql|tar|gz|bz2|theme|twig|tpl(\.php)?|xtmpl|yml)(~|\.sw[op]|\.bak|\.orig|\.save)?|/(Entries.*|Repository|Root|Tag|Template|composer\.(json|lock)|web\.config)|/#[^/]*#|\.php(~|\.sw[op]|\.bak|\.orig|\.save))$ + respond @protected 404 + + # location ~ \.php$ { return 404; } plus the `internal` on the front + # controller: no .php path is reachable from outside, /index.php and + # /index.php/… included. The matcher sees the request as it arrived, so the + # rewrite php_server does further down is unaffected. + route { + @directPhp path *.php *.php/* + respond @directPhp 404 + + # try_files $uri /index.php$is_args$args + # + # Uncommenting the worker line is all worker mode needs: FrankenPHP sets + # FRANKENPHP_WORKER=1, and symfony/runtime has picked its own + # FrankenPhpWorkerRunner off that since 7.4 — no PHP package, no + # APP_RUNTIME override. + # + # Symfony 8.1 adds FRANKENPHP_RESET_KERNEL=1, which clones the kernel + # after each request. That does mean a kernel boot per request — + # AbstractKernel::__clone() nulls the container and clears `booted`, so + # the next handle() runs initializeBundles() and instantiates the + # compiled container again — but it keeps the PHP runtime, OPcache and + # autoloader warm, so it is not the same as no worker at all. Measured on + # /admin in prod: 1319 rps and a 9.0 ms median without a worker, 1494 and + # 4.3 ms with one, 1395 and 6.1 ms with one plus the reset. Roughly half + # the gain, and immune to state leaking between requests. + php_server { + #worker /app/public/index.php + } + } +} diff --git a/.docker/php-dev.ini b/.docker/php-dev.ini new file mode 100644 index 00000000..77fc5a23 --- /dev/null +++ b/.docker/php-dev.ini @@ -0,0 +1,15 @@ +; Xdebug settings, mounted only by docker-compose.override.yml. +; +; The extension is installed in the Dockerfile's `dev` stage and absent from +; `prod`, so neither this file nor the variables it reads reach a server. +; +; A port of mods-available/xdebug.ini from itkdev/php8.5-fpm, keeping its +; variable names: PHP_XDEBUG_MODE and PHP_XDEBUG_START_WITH_REQUEST are what +; itkdev-docker-compose sets when starting with a debugger attached. Xdebug also +; reads the XDEBUG_MODE environment variable directly, and that takes precedence +; — which is how CI turns on coverage without touching this file. +xdebug.mode = ${PHP_XDEBUG_MODE} +xdebug.client_host = ${PHP_XDEBUG_CLIENT_HOST} +xdebug.start_with_request = ${PHP_XDEBUG_START_WITH_REQUEST} +xdebug.max_nesting_level = ${PHP_XDEBUG_MAX_NESTING_LEVEL} +xdebug.output_dir = ${PHP_XDEBUG_OUTPUT_DIR} diff --git a/.docker/php.ini b/.docker/php.ini new file mode 100644 index 00000000..77ef19dc --- /dev/null +++ b/.docker/php.ini @@ -0,0 +1,47 @@ +; PHP settings for the FrankenPHP POC, mounted into +; /usr/local/etc/php/conf.d/. +; +; A port of what itkdev/php8.5-fpm configures and the published FrankenPHP image +; does not: the env-driven ini templates `fpm/conf.d/90-php.ini`, +; and `mods-available/opcache.ini`, plus the error +; logging from `fpm/pool.d/zz-fpm-docker.conf`. Xdebug lives in php-dev.ini, +; which production does not mount. The variable names are the fpm +; image's own, so the same overrides work; the Dockerfile defaults every one of +; them, because an unset variable expands to the empty string and PHP warns. + +; fpm/conf.d/90-php.ini +realpath_cache_size = 4096k +realpath_cache_ttl = 600 +expose_php = Off +max_execution_time = ${PHP_MAX_EXECUTION_TIME} +memory_limit = ${PHP_MEMORY_LIMIT} +post_max_size = ${PHP_POST_MAX_SIZE} +upload_max_filesize = ${PHP_UPLOAD_MAX_FILESIZE} +date.timezone = ${PHP_TIMEZONE} +sendmail_path = ${PHP_SENDMAIL_PATH} +max_input_vars = ${PHP_MAX_INPUT_VARS} + +; Logging. php-fpm sent its error log, its slowlog and — through +; catch_workers_output — everything a worker wrote to stderr to ${PHP_LOGS}, +; which is /dev/stderr. There is no fpm master here to collect worker output, so +; PHP writes to the same place directly. +; +; php-fpm configured no access log at all: the nginx access log was the only +; per-request record. Caddy's access log replaces it, see .docker/Caddyfile. +error_log = ${PHP_LOGS} +log_errors = On +display_errors = Off +display_startup_errors = Off +error_reporting = E_ALL & ~E_DEPRECATED + +; mods-available/opcache.ini +opcache.enable = ${PHP_OPCACHE_ENABLED} +opcache.jit = ${PHP_OPCACHE_JIT} +opcache.memory_consumption = ${PHP_OPCACHE_MEMORY_CONSUMPTION} +opcache.max_accelerated_files = ${PHP_OPCACHE_MAX_ACCELERATED_FILES} +opcache.max_wasted_percentage = ${PHP_OPCACHE_MAX_WASTED_PERCENTAGE} +opcache.revalidate_freq = ${PHP_OPCACHE_REVALIDATE_FREQ} +opcache.validate_timestamps = ${PHP_OPCACHE_VALIDATE_TIMESTAMPS} +opcache.interned_strings_buffer = 16 +opcache.fast_shutdown = 1 +opcache.optimization_level = 0xFFFFFFEF diff --git a/.github/workflows/api-spec.yaml b/.github/workflows/api-spec.yaml index 4b39553d..8e09a99a 100644 --- a/.github/workflows/api-spec.yaml +++ b/.github/workflows/api-spec.yaml @@ -53,7 +53,7 @@ jobs: Please run the following command, then commit and push the changes: ```shell - docker compose exec phpfpm composer update-api-spec + docker compose exec frankenphp composer update-api-spec ``` EOF )" \ diff --git a/.github/workflows/composer.yaml b/.github/workflows/composer.yaml index 26a728a3..dadd99a2 100644 --- a/.github/workflows/composer.yaml +++ b/.github/workflows/composer.yaml @@ -8,19 +8,19 @@ ### ### #### Assumptions ### -### 1. A docker compose service named `phpfpm` can be run and `composer` can be -### run inside the `phpfpm` service. +### 1. A docker compose service named `frankenphp` can be run and `composer` can be +### run inside the `frankenphp` service. ### 2. [ergebnis/composer-normalize](https://github.com/ergebnis/composer-normalize) ### is a dev requirement in `composer.json`: ### ### ``` shell -### docker compose run --rm phpfpm composer require --dev ergebnis/composer-normalize +### docker compose run --rm frankenphp composer require --dev ergebnis/composer-normalize ### ``` ### ### Normalize `composer.json` by running ### ### ``` shell -### docker compose run --rm phpfpm composer normalize +### docker compose run --rm frankenphp composer normalize ### ``` name: Composer @@ -51,7 +51,7 @@ jobs: docker network create frontend - run: | - docker compose run --rm phpfpm composer validate --strict + docker compose run --rm frankenphp composer validate --strict composer-normalized: runs-on: ubuntu-latest @@ -63,8 +63,8 @@ jobs: docker network create frontend - run: | - docker compose run --rm phpfpm composer install - docker compose run --rm phpfpm composer normalize --dry-run + docker compose run --rm frankenphp composer install + docker compose run --rm frankenphp composer normalize --dry-run composer-audit: runs-on: ubuntu-latest @@ -76,4 +76,4 @@ jobs: docker network create frontend - run: | - docker compose run --rm phpfpm composer audit --locked + docker compose run --rm frankenphp composer audit --locked diff --git a/.github/workflows/doctrine.yaml b/.github/workflows/doctrine.yaml index 8eb28dd9..49ee9691 100644 --- a/.github/workflows/doctrine.yaml +++ b/.github/workflows/doctrine.yaml @@ -26,19 +26,19 @@ jobs: - name: Run Composer Install run: | - docker compose run --rm phpfpm composer install + docker compose run --rm frankenphp composer install - name: Run Doctrine Migrations run: | - docker compose run --rm phpfpm bin/console doctrine:migrations:migrate --no-interaction + docker compose run --rm frankenphp bin/console doctrine:migrations:migrate --no-interaction - name: Setup messenger "failed" doctrine transport to ensure db schema is updated run: | - docker compose run --rm phpfpm bin/console messenger:setup-transports failed + docker compose run --rm frankenphp bin/console messenger:setup-transports failed - name: Validate Doctrine schema run: | - docker compose run --rm phpfpm bin/console doctrine:schema:validate + docker compose run --rm frankenphp bin/console doctrine:schema:validate load-fixtures: name: Load Doctrine fixtures @@ -53,15 +53,15 @@ jobs: - name: Run Composer Install run: | - docker compose run --rm phpfpm composer install --no-interaction + docker compose run --rm frankenphp composer install --no-interaction - name: Run Doctrine Migrations run: | - docker compose run --rm phpfpm bin/console doctrine:migrations:migrate --no-interaction + docker compose run --rm frankenphp bin/console doctrine:migrations:migrate --no-interaction - name: Load fixtures run: | - docker compose run --rm phpfpm composer fixtures + docker compose run --rm frankenphp composer fixtures # The jobs above migrate an empty database. A deployment migrates a database # that already holds rows, so a migration that cannot cope with existing @@ -90,26 +90,48 @@ jobs: run: | git checkout ${{ github.event.pull_request.base.sha }} + - name: Resolve the PHP service name + run: | + # This job straddles two revisions of docker-compose.yml: one + # of them may still call the PHP service phpfpm while the + # other calls it frankenphp. + if docker compose config --services | grep -qx frankenphp; then + echo "PHP_SERVICE=frankenphp" >> "$GITHUB_ENV" + else + echo "PHP_SERVICE=phpfpm" >> "$GITHUB_ENV" + fi + - name: Run Composer Install run: | - docker compose run --rm phpfpm composer install --no-interaction + docker compose run --rm "$PHP_SERVICE" composer install --no-interaction - name: Run Doctrine Migrations run: | - docker compose run --rm phpfpm bin/console doctrine:migrations:migrate --no-interaction + docker compose run --rm "$PHP_SERVICE" bin/console doctrine:migrations:migrate --no-interaction - name: Load fixtures run: | - docker compose run --rm phpfpm composer fixtures + docker compose run --rm "$PHP_SERVICE" composer fixtures - name: Check out the pull request run: | git checkout ${{ github.event.pull_request.head.sha }} + - name: Resolve the PHP service name + run: | + # This job straddles two revisions of docker-compose.yml: one + # of them may still call the PHP service phpfpm while the + # other calls it frankenphp. + if docker compose config --services | grep -qx frankenphp; then + echo "PHP_SERVICE=frankenphp" >> "$GITHUB_ENV" + else + echo "PHP_SERVICE=phpfpm" >> "$GITHUB_ENV" + fi + - name: Run Composer Install run: | - docker compose run --rm phpfpm composer install --no-interaction + docker compose run --rm "$PHP_SERVICE" composer install --no-interaction - name: Run Doctrine Migrations on the populated database run: | - docker compose run --rm phpfpm bin/console doctrine:migrations:migrate --no-interaction + docker compose run --rm "$PHP_SERVICE" bin/console doctrine:migrations:migrate --no-interaction diff --git a/.github/workflows/github_build_release.yml b/.github/workflows/github_build_release.yml index ba366729..9a6dac83 100644 --- a/.github/workflows/github_build_release.yml +++ b/.github/workflows/github_build_release.yml @@ -21,8 +21,8 @@ jobs: - name: Composer install run: | docker network create frontend - docker compose run --rm --user=root --env APP_ENV=prod phpfpm composer install --no-dev -o --classmap-authoritative - docker compose run --rm --user=root --env APP_ENV=prod phpfpm composer clear-cache + docker compose run --rm --user=root --env APP_ENV=prod frankenphp composer install --no-dev -o --classmap-authoritative + docker compose run --rm --user=root --env APP_ENV=prod frankenphp composer clear-cache docker compose run --rm node yarn install docker compose run --rm node yarn build diff --git a/.github/workflows/php.yaml b/.github/workflows/php.yaml index 2b958dc6..7bc829d1 100644 --- a/.github/workflows/php.yaml +++ b/.github/workflows/php.yaml @@ -9,20 +9,20 @@ ### ### #### Assumptions ### -### 1. A docker compose service named `phpfpm` can be run and `composer` can be -### run inside the `phpfpm` service. 2. +### 1. A docker compose service named `frankenphp` can be run and `composer` can be +### run inside the `frankenphp` service. 2. ### [friendsofphp/php-cs-fixer](https://github.com/PHP-CS-Fixer/PHP-CS-Fixer) ### is a dev requirement in `composer.json`: ### ### ``` shell -### docker compose run --rm phpfpm composer require --dev friendsofphp/php-cs-fixer +### docker compose run --rm frankenphp composer require --dev friendsofphp/php-cs-fixer ### ``` ### ### Clean up and check code by running ### ### ``` shell -### docker compose run --rm phpfpm vendor/bin/php-cs-fixer fix -### docker compose run --rm phpfpm vendor/bin/php-cs-fixer fix --dry-run --diff +### docker compose run --rm frankenphp vendor/bin/php-cs-fixer fix +### docker compose run --rm frankenphp vendor/bin/php-cs-fixer fix --dry-run --diff ### ``` ### ### > [!NOTE] The template adds `.php-cs-fixer.dist.php` as [a configuration @@ -61,6 +61,6 @@ jobs: docker network create frontend - run: | - docker compose run --rm phpfpm composer install + docker compose run --rm frankenphp composer install # https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/blob/master/doc/usage.rst#the-check-command - docker compose run --rm phpfpm vendor/bin/php-cs-fixer fix --dry-run --diff + docker compose run --rm frankenphp vendor/bin/php-cs-fixer fix --dry-run --diff diff --git a/.github/workflows/pr.yaml b/.github/workflows/pr.yaml index a726d00f..5e8040ff 100644 --- a/.github/workflows/pr.yaml +++ b/.github/workflows/pr.yaml @@ -15,8 +15,29 @@ jobs: - name: Run PHPStan run: | - docker compose run --rm phpfpm composer install --no-interaction - docker compose run --rm phpfpm vendor/bin/phpstan analyse + docker compose run --rm frankenphp composer install --no-interaction + docker compose run --rm frankenphp vendor/bin/phpstan analyse + + worker-state: + runs-on: ubuntu-latest + name: Worker state audit + steps: + - name: Checkout + uses: actions/checkout@v6 + + - name: Create docker network + run: docker network create frontend + + # igor-php audits every shared service in the compiled container for + # state that would leak between requests in a worker. It reads the + # service map IgorPhpBundle writes during cache:clear, and fails only + # on findings absent from igor-baseline.json — every entry in which + # carries a reason. Regenerate with `composer worker-state-baseline`. + - name: Audit shared services for worker-mode state leaks + run: | + docker compose run --rm frankenphp composer install --no-interaction + docker compose run --rm frankenphp bin/console cache:clear + docker compose run --rm frankenphp composer worker-state-check phpunit: runs-on: ubuntu-latest @@ -31,11 +52,11 @@ jobs: - name: Run tests with coverage run: | docker compose up --detach - docker compose exec -e XDEBUG_MODE=coverage phpfpm composer install --no-interaction - docker compose exec -e XDEBUG_MODE=coverage phpfpm bin/console --env=test doctrine:database:drop --if-exists --force --quiet - docker compose exec -e XDEBUG_MODE=coverage phpfpm bin/console --env=test doctrine:database:create --no-interaction --if-not-exists --quiet - docker compose exec -e XDEBUG_MODE=coverage phpfpm bin/console --env=test doctrine:migrations:migrate --no-interaction --quiet - docker compose exec -e XDEBUG_MODE=coverage phpfpm vendor/bin/phpunit --coverage-clover=coverage/unit.xml + docker compose exec -e XDEBUG_MODE=coverage frankenphp composer install --no-interaction + docker compose exec -e XDEBUG_MODE=coverage frankenphp bin/console --env=test doctrine:database:drop --if-exists --force --quiet + docker compose exec -e XDEBUG_MODE=coverage frankenphp bin/console --env=test doctrine:database:create --no-interaction --if-not-exists --quiet + docker compose exec -e XDEBUG_MODE=coverage frankenphp bin/console --env=test doctrine:migrations:migrate --no-interaction --quiet + docker compose exec -e XDEBUG_MODE=coverage frankenphp vendor/bin/phpunit --coverage-clover=coverage/unit.xml - name: Upload coverage to Codecov uses: codecov/codecov-action@v7 diff --git a/.github/workflows/twig.yaml b/.github/workflows/twig.yaml index 1d7cd59a..6000faae 100644 --- a/.github/workflows/twig.yaml +++ b/.github/workflows/twig.yaml @@ -8,13 +8,13 @@ ### ### #### Assumptions ### -### 1. A docker compose service named `phpfpm` can be run and `composer` can be -### run inside the `phpfpm` service. +### 1. A docker compose service named `frankenphp` can be run and `composer` can be +### run inside the `frankenphp` service. ### 2. [vincentlanglet/twig-cs-fixer](https://github.com/VincentLanglet/Twig-CS-Fixer) ### is a dev requirement in `composer.json`: ### ### ``` shell -### docker compose run --rm phpfpm composer require --dev vincentlanglet/twig-cs-fixer +### docker compose run --rm frankenphp composer require --dev vincentlanglet/twig-cs-fixer ### ``` ### ### 3. A [Configuration @@ -51,5 +51,5 @@ jobs: docker network create frontend - run: | - docker compose run --rm phpfpm composer install - docker compose run --rm phpfpm vendor/bin/twig-cs-fixer lint + docker compose run --rm frankenphp composer install + docker compose run --rm frankenphp vendor/bin/twig-cs-fixer lint diff --git a/.woodpecker/prod.yml b/.woodpecker/prod.yml index e38d7e78..077b0bf6 100644 --- a/.woodpecker/prod.yml +++ b/.woodpecker/prod.yml @@ -24,8 +24,8 @@ steps: keep: 4 playbook: "release" pre_up: - - itkdev-docker-compose-server run --rm phpfpm bin/console doctrine:migrations:migrate --no-interaction - - itkdev-docker-compose-server run --rm phpfpm bin/console messenger:setup-transports + - itkdev-docker-compose-server run --rm frankenphp bin/console doctrine:migrations:migrate --no-interaction + - itkdev-docker-compose-server run --rm frankenphp bin/console messenger:setup-transports - name: Run post deploy image: itkdev/ansible-plugin:1 @@ -42,4 +42,4 @@ steps: user: from_secret: user actions: - - itkdev-docker-compose-server exec phpfpm bin/console cache:clear + - itkdev-docker-compose-server exec frankenphp bin/console cache:clear diff --git a/.woodpecker/stg.yml b/.woodpecker/stg.yml index 0a5749a4..9df99156 100644 --- a/.woodpecker/stg.yml +++ b/.woodpecker/stg.yml @@ -31,7 +31,7 @@ steps: - git checkout ${CI_COMMIT_BRANCH} - git pull - itkdev-docker-compose-server up -d --force-recreate --remove-orphans - - itkdev-docker-compose-server exec phpfpm composer install -no-dev -o --classmap-authoritative - - itkdev-docker-compose-server exec phpfpm bin/console doctrine:migrations:migrate --no-interaction - - itkdev-docker-compose-server exec phpfpm bin/console messenger:setup-transports - - itkdev-docker-compose-server exec phpfpm bin/console cache:clear + - itkdev-docker-compose-server exec frankenphp composer install -no-dev -o --classmap-authoritative + - itkdev-docker-compose-server exec frankenphp bin/console doctrine:migrations:migrate --no-interaction + - itkdev-docker-compose-server exec frankenphp bin/console messenger:setup-transports + - itkdev-docker-compose-server exec frankenphp bin/console cache:clear diff --git a/CHANGELOG.md b/CHANGELOG.md index c4efbaf9..1c1ded17 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,67 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +- [#59](https://github.com/itk-dev/devops_itksites/pull/59) + 4544: POC for using FrankenPHP behind Traefik + - Serve the site from a single FrankenPHP container. The `frankenphp` service + is added in `docker-compose.override.yml` and + `docker-compose.server.override.yml`; `phpfpm` and `nginx` move into a + profile that is never enabled + - Port the nginx configuration to `.docker/Caddyfile` and the PHP settings the + fpm image took from `PHP_*` environment variables to `.docker/php.ini` + - Traefik keeps terminating TLS: `auto_https` is off and Caddy serves plain + HTTP on 8080 + - Move the whole stack to PHP 8.5, `itkdev/php8.5-fpm` and + `itkdev/supervisor-php8.5` included + - Point Taskfile, workflows, Woodpecker and the README at the `frankenphp` + service + - Serve Prometheus metrics from Caddy at `/metrics`, behind the same + `ITKMetricsAuth` middleware `/cron-metrics` used. nginx exported nothing, + and the supercronic it proxied to never started without an `/app/crontab` + - Log requests as JSON from Caddy, carrying every field nginx's `log_format + main` had plus `duration`. The published image has no transform encoder, so + the text layout cannot be reproduced exactly + - Trust `private_ranges` rather than the template's `172.16.0.0/16`, which + covered neither the `frontend` network nor the client, so real-IP resolution + never happened + - Leave worker mode off for now, but not for want of a runtime: `symfony/runtime` + has shipped `FrankenPhpWorkerRunner` since 7.4, so enabling it is one line in + `.docker/Caddyfile` and needs no package + - Make `PackageVersionFactory` and `ModuleVersionFactory` stateless: their + deduplication buffers are locals rather than properties, so a failing flush + can no longer leave entities from a closed EntityManager for the next call. + This was a live bug in the messenger consumer, which is already long-running + - Implement `ResetInterface` on `LeantimeService`, whose memoised user + directory has to stay a cache — `resolveUserName()` runs in a loop over + tickets — but must not outlive the request + - Call `unsetAll()` before `setController()` on the injected + `AdminUrlGenerator` in `DashboardController` and + `SecurityContractCrudController`, which `AppExtension` and + `RepoAdvisoryService` already did. The instance is held for as long as its + consumer, which in a worker outlives the request + - Cover all of it with tests: the factories had none, and neither the + dashboard nor the Security Contract CRUD was in the admin smoke test + - Run the container as `deploy` rather than root, dropping Caddy's + `cap_net_bind_service` since port 8080 needs none. `DEPLOY_UID` is a build + argument defaulting to 1042, the id the alpine images the servers run give + `deploy` + - Give the container a health check on `/health/live`, so `up --wait` waits for + the application rather than the process, and reorder `task site:update` to + install before waiting + - Split the image into `dev` and `prod` stages. Production drops Xdebug and + sets `opcache.validate_timestamps=0`, so it no longer loads a debugger it + never uses or stats every file on every request; Xdebug's ini moves to + `.docker/php-dev.ini`, which only development mounts + - Gate pull requests on `igor-php`, which audits every shared service in the + compiled container for state that would leak between requests. + `igor-baseline.json` records the 33 existing findings with a reason each, so + the job fails only on new ones; vendor code is out of scope + - Document worker mode and the statelessness it requires in `README.md` and + `claude.md`. It stays off. Measured on `/admin` in prod: 1319 requests per + second without a worker, 1494 with one, 1395 with one plus + `FRANKENPHP_RESET_KERNEL=1` — so the reset keeps about half the gain rather + than erasing it. `/health/live` inverts the ranking, and the numbers come + from a laptop sharing CPU with other containers - [#96](https://github.com/itk-dev/devops_itksites/pull/96) Show the Service Agreements monthly price as Danish kroner, `12.500,50 kr.`, on index and detail diff --git a/Dockerfile b/Dockerfile new file mode 100644 index 00000000..c0740fa8 --- /dev/null +++ b/Dockerfile @@ -0,0 +1,105 @@ +# FrankenPHP image for this application, in two flavours. +# +# The published dunglas/frankenphp images are deliberately minimal, so the +# extensions this application cannot boot without are added on top. Everything +# else it needs — ctype, iconv, dom, mbstring, opcache, … — is already there. +# +# Pinned to PHP 8.5 to match itkdev/php8.5-fpm and itkdev/supervisor-php8.5: the +# messenger worker and the web container share vendor/ and var/cache over the +# same bind mount, so they have to agree on the PHP version. +# +# Pick a stage explicitly. Compose does, through `target:` in the two override +# files; a bare `docker build .` gets `prod`, the last stage, which is the safer +# of the two to end up with by accident. +FROM dunglas/frankenphp:1.12-php8.5 AS base + +RUN install-php-extensions \ + pdo_mysql \ + amqp \ + intl \ + gd \ + zip + +# msmtp keeps sendmail_path working the way it does in itkdev/php8.5-fpm. +RUN apt-get update \ + && apt-get install --no-install-recommends --yes msmtp \ + && rm -rf /var/lib/apt/lists/* + +COPY --from=composer/composer:2-bin /composer /usr/bin/composer + +# Run as a normal user rather than root. +# +# DEPLOY_UID has to match whoever owns the checkout this container bind-mounts, +# or the application cannot write var/. In devops_docker-images that id depends +# on the base distro, consistently across 8.3, 8.4 and 8.5: the ubuntu tags put +# deploy at 1000 (roles/ubuntu/tasks/main.yml) and the alpine ones at 1042 +# (roles/alpine/templates/Dockerfile.j2). The servers run the alpine tags — +# php8.5-fpm:alpine here, supervisor-php8.5:alpine for the messenger consumer — +# so 1042 is the id that owns /app there, and the default. +# +# Local development runs the ubuntu tag at 1000, which does not matter: Docker +# Desktop virtualises bind-mount ownership. CI selects runner through +# COMPOSE_USER, at the id GitHub's runner account uses; the alpine images have no +# runner user, but nothing runs CI against those. +ARG DEPLOY_UID=1042 +ARG DEPLOY_GID=1042 +ARG RUNNER_UID=1001 + +# Caddy listens on 8080, which needs no capability to bind, so the one the image +# ships with goes. Both users need Caddy's state directories: it writes its +# instance id and an autosaved config there even with auto_https off. +RUN groupadd --gid ${DEPLOY_GID} deploy \ + && useradd --uid ${DEPLOY_UID} --gid ${DEPLOY_GID} --create-home deploy \ + && groupadd --gid ${RUNNER_UID} runner \ + && useradd --uid ${RUNNER_UID} --gid ${RUNNER_UID} --create-home runner \ + && usermod --append --groups deploy runner \ + && setcap -r /usr/local/bin/frankenphp \ + && chown -R deploy:deploy /data/caddy /config/caddy \ + && chmod -R g+w /data/caddy /config/caddy + +# The itkdev/php8.5-fpm image turns these into ini settings and defaults them on +# the image rather than in compose. `.docker/php.ini` reads the same names, so +# the same overrides work and no variable is ever unset. +ENV PHP_LOGS=/dev/stderr \ + PHP_TIMEZONE=Europe/Copenhagen \ + PHP_MEMORY_LIMIT=128M \ + PHP_MAX_EXECUTION_TIME=30 \ + PHP_MAX_INPUT_VARS=1000 \ + PHP_POST_MAX_SIZE=8M \ + PHP_UPLOAD_MAX_FILESIZE=2M \ + PHP_SENDMAIL_PATH="/usr/sbin/sendmail -S host.docker.internal -t -i" \ + PHP_OPCACHE_ENABLED=1 \ + PHP_OPCACHE_JIT=off \ + PHP_OPCACHE_MEMORY_CONSUMPTION=64 \ + PHP_OPCACHE_MAX_ACCELERATED_FILES=20000 \ + PHP_OPCACHE_MAX_WASTED_PERCENTAGE=10 \ + PHP_OPCACHE_REVALIDATE_FREQ=0 \ + PHP_OPCACHE_VALIDATE_TIMESTAMPS=1 + +# Development: Xdebug, and OPcache rechecking files so an edit takes effect. +FROM base AS dev + +RUN install-php-extensions xdebug + +# Read by .docker/php-dev.ini, which only development mounts. +ENV PHP_XDEBUG_MODE=off \ + PHP_XDEBUG_CLIENT_HOST=host.docker.internal \ + PHP_XDEBUG_START_WITH_REQUEST=yes \ + PHP_XDEBUG_MAX_NESTING_LEVEL=256 \ + PHP_XDEBUG_OUTPUT_DIR=/app + +USER deploy + +# Production: no Xdebug, and OPcache trusting what it compiled. +# +# validate_timestamps=0 stops PHP stat-ing every file on every request, which +# validate_timestamps=1 with revalidate_freq=0 made it do. The cost is that a +# code change needs a new container — both deployment paths give it one, since +# staging runs `up -d --force-recreate` and the release playbook brings the stack +# up again, and a fresh container starts with an empty OPcache. It also makes +# PHP_OPCACHE_REVALIDATE_FREQ moot. +FROM base AS prod + +ENV PHP_OPCACHE_VALIDATE_TIMESTAMPS=0 + +USER deploy diff --git a/README.md b/README.md index d8e51b32..f5fffe64 100644 --- a/README.md +++ b/README.md @@ -54,7 +54,7 @@ Authenticated users can access a simple read-only API – see the API documentat Run the `app:user:set-api-key` console command to set the API for a user: ``` shell -docker compose exec phpfpm php bin/console app:user:set-api-key +docker compose exec frankenphp php bin/console app:user:set-api-key ``` Use the API key to make an authenticated request, e.g. @@ -112,12 +112,193 @@ that the database is down. ```sh docker compose pull docker compose up --detach -docker compose exec phpfpm composer install -docker compose exec phpfpm bin/console doctrine:migrations:migrate --no-interaction +docker compose exec frankenphp composer install +docker compose exec frankenphp bin/console doctrine:migrations:migrate --no-interaction ``` Then create a `.env.local` file to set secrets for your local setup. +### Web server + +The site is served by a single [FrankenPHP](https://frankenphp.dev) container +instead of the usual phpfpm and nginx pair. `docker-compose.override.yml` +locally, and `docker-compose.server.override.yml` on the servers, add the +`frankenphp` service and park `phpfpm` and `nginx` in a profile that is never +enabled, so neither starts. Commands that used to run against `phpfpm` run +against `frankenphp`. + +Traefik still terminates TLS. Caddy listens on plain HTTP on port 8080 and +`auto_https` is off, so it neither requests nor serves certificates. + +The image is built in two flavours from one multi-stage `Dockerfile`, selected +by `target:` in the override files: + +| | `dev` | `prod` | +| --- | --- | --- | +| Xdebug | installed | **absent** | +| `opcache.validate_timestamps` | `1` | **`0`** | + +Development keeps Xdebug and lets OPcache recheck files so an edit takes effect. +Production has neither: the extension is not in the image, and OPcache trusts +what it compiled rather than stat-ing every file on every request, which +`validate_timestamps=1` with `revalidate_freq=0` made it do. The cost is that a +code change needs a new container, which both deployment paths already give it – +staging runs `up -d --force-recreate` and the release playbook brings the stack +up again, and a fresh container starts with an empty OPcache. A bare +`docker build .` resolves to `prod`, the last stage. + +Three files carry the configuration that used to live on the phpfpm and nginx +images: + +- `.docker/Caddyfile` – a port of `.docker/nginx.conf` and + `.docker/templates/default.conf.template`. +- `.docker/php.ini` – the PHP settings the `itkdev/php8.5-fpm` image derives + from its `PHP_*` environment variables, plus its tuned baseline. The compose + files still set the same variables; the ini file interpolates them. +- `.docker/php-dev.ini` – the Xdebug half of that, mounted only in development. + +Coming from the phpfpm stack, remove the containers it left behind once: + +```sh +docker compose rm --stop --force phpfpm nginx +``` + +#### Container user and health check + +The container runs as `deploy`, not root, and Caddy's `cap_net_bind_service` is +removed – nothing needs it on port 8080. `DEPLOY_UID` is a build argument +because the id has to match whoever owns the checkout being bind-mounted, and in +`devops_docker-images` that depends on the base distro: the ubuntu tags put +`deploy` at 1000, the alpine ones at 1042. The servers run the alpine tags, so +1042 is the default. Local development runs the ubuntu tag at 1000, which does +not matter because Docker Desktop virtualises bind-mount ownership, and CI picks +`runner` through `COMPOSE_USER` as it always did. + +Coming from the root-run container, hand the files it wrote to `deploy` once: + +```sh +docker compose run --rm --user root frankenphp chown -R deploy:deploy /app/var +``` + +`/health/live` backs a container health check, so `docker compose up --wait` +waits for the application to answer rather than merely for the process to exist. +That check calls into the application, which cannot answer before its +dependencies are installed – which is why `task site:update` starts the stack, +installs, and only then waits. + +#### Logging + +php-fpm sent its error log, its slowlog and everything a worker wrote to stderr +to `/dev/stderr`, and configured no access log at all – the nginx access log was +the only per-request record. `.docker/php.ini` keeps `error_log` pointed at +`${PHP_LOGS}` and Caddy's access log replaces nginx's. + +Caddy logs JSON rather than nginx's `log_format main` text. Every field that +format carried is in it – `client_ip`, `user_id`, `ts`, method, uri, proto, +`status`, `size` and the `Referer`, `User-Agent` and `X-Forwarded-For` headers – +plus `duration`, which nginx did not log. The text layout cannot be reproduced +byte for byte without the Caddy transform encoder, which the published image +does not carry: this build has `console`, `json`, `append`, `filter` and +`journald`. JSON also matches supercronic, which the fpm image already runs with +`-json`. + +#### Worker mode + +Worker mode is off. Turning it on needs no PHP package and no code change: +`symfony/runtime` has shipped `FrankenPhpWorkerRunner` since 7.4, FrankenPHP +sets `FRANKENPHP_WORKER=1` for a worker script, and `SymfonyRuntime::getRunner()` +switches on that. `.docker/Caddyfile` reads `{$FRANKENPHP_CONFIG}`, so the switch +is an environment variable on the `frankenphp` service: + +```yaml +environment: + FRANKENPHP_CONFIG: worker /app/public/index.php +``` + +Add a count – `worker /app/public/index.php 8` – to override the default, which +is twice the number of CPU cores. Keep `num_threads` × `memory_limit` below the +memory available to the container. + +What it bought here, measured in `prod` with a warm OPcache, 40 seconds at 20 +concurrent on `/admin`: + +| | requests/sec | median | +| --- | --- | --- | +| no worker | 1319 | 9.0 ms | +| worker | 1494 | 4.3 ms | +| worker + `FRANKENPHP_RESET_KERNEL=1` | 1395 | 6.1 ms | + +On `/health/live` the ranking inverts – roughly 40% *fewer* requests per second +with a worker. That endpoint returns a constant, which is worker mode's worst +case: there is no per-request work for the saved kernel boot to be weighed +against, and the runner's `gc_collect_cycles()` on every request is not free. +Worth knowing, since the health endpoints are the polled ones. + +The numbers come from a laptop sharing CPU with other containers and running the +application over a bind mount, so treat them as a shape rather than a figure. +Short runs on that machine varied by more than tenfold; only 40-second runs were +reproducible. Measure again on a server before adopting. + +**Services must not carry request state.** Under php-fpm a service instance died +with the request; in a worker it does not, so anything a service remembers leaks +into the next request. Prefer keeping services stateless. Where state is +deliberate, implement `Symfony\Contracts\Service\ResetInterface` – +`autoconfigure` tags it `kernel.reset` and Symfony calls it between requests. +For an object you do not own, clear it at the call site, the way every +`AdminUrlGenerator` chain here opens with `unsetAll()`. + +That rule is enforced. [igor-php](https://github.com/igor-php/igor-php) audits +every shared service in the compiled container for state that would leak between +requests, and runs on every pull request: + +```sh +docker compose exec frankenphp composer worker-state-check +``` + +Existing findings live in `igor-baseline.json`, so the job fails only on new +ones. Every entry there carries a reason – most are Doctrine entities returned +from a repository, which igor reads as shared services, and `AdminUrlGenerator` +chains it cannot see are already cleared by `unsetAll()`. Read the reasons before +adding to them; if a finding is genuine, fix it rather than baseline it. After a +deliberate change, regenerate with `composer worker-state-baseline` and write a +reason for each new entry. + +The audit needs the service map that `IgorPhpBundle` writes during +`cache:clear`, so run that first if the cache is cold. Vendor code is out of +scope (`ignore_vendors` in `igor.json`): it reported 341 findings there, none of +them ours to fix. + +`FRANKENPHP_RESET_KERNEL=1`, on Symfony 8.1 and later, clones the kernel after +each request instead, which makes this class of bug harmless. +`AbstractKernel::__clone()` nulls the container and clears `booted`, so the next +request runs `initializeBundles()` and instantiates the compiled container again +– a kernel boot, though not a recompile. It is not as expensive as it sounds: +the PHP runtime, OPcache and autoloader stay warm, and it kept about half the +worker-mode gain in the table above while still beating no worker on both +throughput and latency. + +That makes it a reasonable first configuration to deploy rather than only a +diagnostic – most of the latency win, immune to the leaks the audit below +guards against – with the reset turned off later once there is confidence. + +#### Metrics + +`/metrics` serves Prometheus metrics from Caddy, behind the `ITKMetricsAuth@file` +middleware on its own Traefik router. + +This is where `/cron-metrics` used to point. That route proxied to supercronic +on `${NGINX_CRON_METRICS}`, and the fpm entrypoint only starts supercronic when +`/app/crontab` exists – this project has no crontab, so nothing ever listened +and the route answered `502`. nginx exported nothing itself: `stub_status` is +compiled into the image but the template never enabled it, and php-fpm's +`pm.status_path = /status` was never routed. + +Caddy does export, so the endpoint has something behind it: request counts, +durations and sizes by code, method and handler, requests in flight, and Go +runtime and process metrics. FrankenPHP's own thread metrics only appear in +worker mode, which is off. A supercronic sidecar, if one is added, needs a route +of its own. + ### OpenID Connect All user access is controlled by OpenID Connect. Locally the login runs against a @@ -171,13 +352,13 @@ all the above data. #### Load fixtures ```sh -docker compose exec phpfpm composer fixtures +docker compose exec frankenphp composer fixtures ``` After loading fixtures you can sign in as an admin user: ```sh -docker compose exec phpfpm bin/console itk-dev:openid-connect:login admin@example.com +docker compose exec frankenphp bin/console itk-dev:openid-connect:login admin@example.com ``` ### Job queues and handlers @@ -186,13 +367,13 @@ All processing of Detctionresults is done in a series of message handlers. To run these do either: ```shell -docker compose exec phpfpm composer queues +docker compose exec frankenphp composer queues ``` or ```shell -docker compose exec phpfpm bin/console messenger:consume async --failure-limit=1 -vvv +docker compose exec frankenphp bin/console messenger:consume async --failure-limit=1 -vvv ``` ### Assets diff --git a/Taskfile.yml b/Taskfile.yml index 108d803a..47a25b4b 100644 --- a/Taskfile.yml +++ b/Taskfile.yml @@ -16,12 +16,18 @@ tasks: - task: compose vars: COMPOSE_ARGS: pull + # Start before installing, then wait: the frankenphp health check + # calls into the application, which cannot answer until its + # dependencies are in place. - task: compose vars: - COMPOSE_ARGS: up --detach --wait + COMPOSE_ARGS: up --detach - task: composer vars: COMPOSER_ARGS: install + - task: compose + vars: + COMPOSE_ARGS: up --detach --wait site:migrate: desc: Run database migrations @@ -52,7 +58,7 @@ tasks: CONSOLE_ARGS: --env=test doctrine:migrations:migrate --no-interaction --quiet - task: compose vars: - COMPOSE_ARGS: exec phpfpm vendor/bin/phpunit --stop-on-failure + COMPOSE_ARGS: exec frankenphp vendor/bin/phpunit --stop-on-failure queues: desc: Consume async messenger queue @@ -66,7 +72,7 @@ tasks: cmds: - task: compose vars: - COMPOSE_ARGS: exec phpfpm composer {{.COMPOSER_ARGS}} + COMPOSE_ARGS: exec frankenphp composer {{.COMPOSER_ARGS}} compose: desc: "Run `docker compose` command. Example: task compose -- ps" @@ -78,7 +84,7 @@ tasks: cmds: - task: compose vars: - COMPOSE_ARGS: exec phpfpm bin/console {{.CONSOLE_ARGS}} + COMPOSE_ARGS: exec frankenphp bin/console {{.CONSOLE_ARGS}} coding-standards:apply: aliases: [cs:apply] @@ -123,7 +129,7 @@ tasks: cmds: - task: compose vars: - COMPOSE_ARGS: exec phpfpm vendor/bin/php-cs-fixer fix + COMPOSE_ARGS: exec frankenphp vendor/bin/php-cs-fixer fix silent: true coding-standards:php:check: @@ -133,7 +139,7 @@ tasks: - task: coding-standards:php:apply - task: compose vars: - COMPOSE_ARGS: exec phpfpm vendor/bin/php-cs-fixer check + COMPOSE_ARGS: exec frankenphp vendor/bin/php-cs-fixer check silent: true coding-standards:twig:apply: @@ -142,7 +148,7 @@ tasks: cmds: - task: compose vars: - COMPOSE_ARGS: exec phpfpm vendor/bin/twig-cs-fixer lint --fix + COMPOSE_ARGS: exec frankenphp vendor/bin/twig-cs-fixer lint --fix silent: true coding-standards:twig:check: @@ -152,7 +158,7 @@ tasks: - task: coding-standards:twig:apply - task: compose vars: - COMPOSE_ARGS: exec phpfpm vendor/bin/twig-cs-fixer lint + COMPOSE_ARGS: exec frankenphp vendor/bin/twig-cs-fixer lint silent: true coding-standards:yaml:apply: @@ -181,13 +187,13 @@ tasks: cmds: - task: compose vars: - COMPOSE_ARGS: exec phpfpm composer validate --strict + COMPOSE_ARGS: exec frankenphp composer validate --strict - task: compose vars: - COMPOSE_ARGS: exec phpfpm composer normalize --dry-run + COMPOSE_ARGS: exec frankenphp composer normalize --dry-run - task: compose vars: - COMPOSE_ARGS: exec phpfpm composer audit + COMPOSE_ARGS: exec frankenphp composer audit # Analysis @@ -197,7 +203,7 @@ tasks: cmds: - task: compose vars: - COMPOSE_ARGS: exec phpfpm vendor/bin/phpstan + COMPOSE_ARGS: exec frankenphp vendor/bin/phpstan silent: true # Test matrix (mirrors pr.yaml CI jobs) diff --git a/claude.md b/claude.md index a83a1af7..647d5270 100644 --- a/claude.md +++ b/claude.md @@ -63,45 +63,100 @@ truncated and rebuilt by replaying DetectionResults. Manually maintained data ## Development Environment ```sh -# Start services (MariaDB, PHP-FPM 8.5, Nginx, Mailpit) +# Start services (MariaDB, FrankenPHP 8.5, Mailpit) docker compose pull && docker compose up --detach # Install dependencies -docker compose exec phpfpm composer install +docker compose exec frankenphp composer install # Run migrations -docker compose exec phpfpm bin/console doctrine:migrations:migrate --no-interaction +docker compose exec frankenphp bin/console doctrine:migrations:migrate --no-interaction # Load fixtures -docker compose exec phpfpm composer fixtures +docker compose exec frankenphp composer fixtures # Login as admin (after fixtures) -docker compose exec phpfpm bin/console itk-dev:openid-connect:login admin@example.com +docker compose exec frankenphp bin/console itk-dev:openid-connect:login admin@example.com # Process message queues -docker compose exec phpfpm composer queues +docker compose exec frankenphp composer queues # Build frontend assets docker compose run --rm node yarn install && docker compose run --rm node yarn build ``` +## Long-running processes + +The site is served by a single FrankenPHP container in place of phpfpm and +nginx. Worker mode is off but available — `symfony/runtime` has shipped +`FrankenPhpWorkerRunner` since 7.4, and `.docker/Caddyfile` reads +`{$FRANKENPHP_CONFIG}`, so it is an environment variable, not a code change: + +```yaml +FRANKENPHP_CONFIG: worker /app/public/index.php +``` + +The `Dockerfile` is multi-stage: `target: dev` adds Xdebug and lets OPcache +recheck files, `target: prod` has neither. The override files pick the target, so +build through compose rather than a bare `docker build`. + +The container runs as `deploy`, not root. `DEPLOY_UID` is a build argument +because the id must match the owner of the bind-mounted checkout — 1042 on the +servers, which run the alpine tags. `/health/live` backs a container health +check, so `up --wait` waits for the application to answer. + +`messenger:consume` is already long-running in production regardless, so the +rules below apply whether or not worker mode is on. + +**Writing a service that has to remember something:** + +1. Prefer statelessness. If the state only needs to live for one method call, + make it a local and thread it through the private helpers — see + `PackageVersionFactory`, whose deduplication buffers work this way. The + `ResetInterface` docblock advises this over the interface where possible. +2. Where the state is deliberate, implement + `Symfony\Contracts\Service\ResetInterface` and clear everything in + `reset()`. `autoconfigure` tags it `kernel.reset` with no manual tagging — + see `LeantimeService`, which caches the Leantime user directory because + `resolveUserName()` runs in a loop. +3. For an object you do not own, clear it where you use it. Every + `AdminUrlGenerator` chain in this codebase opens with `unsetAll()` for this + reason: EasyAdmin registers it `shared: no`, but the services holding it are + shared, so the instance outlives the request. + +Things that break a worker and have no place here: `exit()`/`die()`, writes to +superglobals, `__destruct()` on a shared service, and mutable `static` +properties. + +`composer worker-state-check` audits this with igor-php and runs on every pull +request. `igor-baseline.json` holds the known findings, each with a reason, so +the job fails only on new ones — fix a genuine finding rather than baselining +it, and regenerate with `composer worker-state-baseline` only after a deliberate +change. + +`FRANKENPHP_RESET_KERNEL=1` (Symfony 8.1+) clones the kernel after each request, +which makes all of this harmless. It costs a kernel boot per request but keeps +the PHP runtime warm, and measured on `/admin` it held about half the worker-mode +gain while still beating no worker — so it is a usable configuration, not just a +baseline. It is not a licence to write stateful services: the audit still runs. + ## Quality Checks All commands run inside Docker containers: ```sh # PHP coding standards (PHP-CS-Fixer) -docker compose exec phpfpm composer coding-standards-check -docker compose exec phpfpm composer coding-standards-apply +docker compose exec frankenphp composer coding-standards-check +docker compose exec frankenphp composer coding-standards-apply # PHPUnit tests (creates test DB, runs migrations, executes tests) -docker compose exec phpfpm composer tests +docker compose exec frankenphp composer tests # Frontend coding standards docker compose run --rm node yarn coding-standards-check # API spec export (must be committed) -docker compose exec phpfpm composer update-api-spec +docker compose exec frankenphp composer update-api-spec ``` ## CI/CD @@ -146,3 +201,6 @@ Pull requests run these checks: - Async processing uses Symfony Messenger with AMQP transport - Environment-specific config goes in `.env.local` (not committed) - API specs (`public/api-spec-v1.yaml` and `.json`) must be regenerated and committed when API changes +- Services must not carry request state. The web container and the messenger + consumer are both long-running, so anything a service remembers outlives the + request that put it there diff --git a/composer.json b/composer.json index fdfdcc75..1e371073 100644 --- a/composer.json +++ b/composer.json @@ -4,7 +4,7 @@ "license": "MIT", "type": "project", "require": { - "php": ">=8.4", + "php": ">=8.5", "ext-ctype": "*", "ext-iconv": "*", "api-platform/core": "^4.0", @@ -47,6 +47,7 @@ "ergebnis/composer-normalize": "^2.23", "friendsofphp/php-cs-fixer": "^3.6", "hautelook/alice-bundle": "^2.14", + "igor-php/igor-php": "^0.9.5", "justinrainbow/json-schema": "^6.0", "phpstan/extension-installer": "^1.4", "phpstan/phpstan": "^2.1", @@ -136,6 +137,12 @@ "update-api-spec": [ "bin/console api:openapi:export --output=public/api-spec-v1.yaml --yaml --no-interaction", "bin/console api:openapi:export --output=public/api-spec-v1.json --no-interaction" + ], + "worker-state-baseline": [ + "vendor/bin/igor-php --generate-baseline ." + ], + "worker-state-check": [ + "vendor/bin/igor-php ." ] } } diff --git a/composer.lock b/composer.lock index 4c17a85c..9169c4cb 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "64cb1b11b5e4c3da1bdc1f83e64b414f", + "content-hash": "2b3a00bb0d55568f74746b225af242aa", "packages": [ { "name": "api-platform/core", @@ -10498,6 +10498,56 @@ }, "time": "2026-03-21T21:21:40+00:00" }, + { + "name": "igor-php/igor-php", + "version": "v0.9.5", + "source": { + "type": "git", + "url": "https://github.com/igor-php/igor-php.git", + "reference": "35673901813ca9f2092fe062cbfd274e7a3aba89" + }, + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/igor-php/igor-php/zipball/35673901813ca9f2092fe062cbfd274e7a3aba89", + "reference": "35673901813ca9f2092fe062cbfd274e7a3aba89", + "shasum": "" + }, + "require": { + "php": ">=8.1" + }, + "bin": [ + "bin/igor-php" + ], + "type": "library", + "autoload": { + "psr-4": { + "IgorPhp\\IgorBundle\\": "src/php/" + } + }, + "notification-url": "https://packagist.org/downloads/", + "license": [ + "MIT" + ], + "authors": [ + { + "name": "Kevin MARTINS", + "email": "kevin.martins@me.com" + } + ], + "description": "The faithful assistant for your FrankenPHP Workers.", + "keywords": [ + "frankenphp", + "linter", + "static-analysis", + "symfony", + "worker" + ], + "support": { + "issues": "https://github.com/igor-php/igor-php/issues", + "source": "https://github.com/igor-php/igor-php/tree/v0.9.5" + }, + "time": "2026-08-12T10:38:29+00:00" + }, { "name": "justinrainbow/json-schema", "version": "6.11.0", @@ -14333,7 +14383,7 @@ "prefer-stable": true, "prefer-lowest": false, "platform": { - "php": ">=8.4", + "php": ">=8.5", "ext-ctype": "*", "ext-iconv": "*" }, diff --git a/config/bundles.php b/config/bundles.php index 2f78f3cd..264fa38a 100644 --- a/config/bundles.php +++ b/config/bundles.php @@ -23,4 +23,5 @@ Twig\Extra\TwigExtraBundle\TwigExtraBundle::class => ['all' => true], Symfony\UX\TwigComponent\TwigComponentBundle::class => ['all' => true], Doctrine\Bundle\MigrationsBundle\DoctrineMigrationsBundle::class => ['all' => true], + IgorPhp\IgorBundle\IgorPhpBundle::class => ['dev' => true], ]; diff --git a/docker-compose.dev.yml b/docker-compose.dev.yml index 71a88b42..cbfd97f9 100644 --- a/docker-compose.dev.yml +++ b/docker-compose.dev.yml @@ -1,10 +1,8 @@ # itk-version: 3.2.4 services: - phpfpm: + frankenphp: environment: - - PHP_SENDMAIL_PATH=/usr/sbin/sendmail -S mail:1025 - - nginx: + PHP_SENDMAIL_PATH: /usr/sbin/sendmail -S mail:1025 labels: - "traefik.http.routers.${COMPOSE_PROJECT_NAME:?}.middlewares=ITKBasicAuth@file" diff --git a/docker-compose.override.yml b/docker-compose.override.yml index be2dcc55..0e346b69 100644 --- a/docker-compose.override.yml +++ b/docker-compose.override.yml @@ -1,4 +1,91 @@ services: + # FrankenPHP POC: a single container replaces the phpfpm and nginx pair. + # Compose cannot delete an inherited service, so both are moved into a + # profile that is never enabled. Nothing depends on them once nginx is gone, + # so neither is started. + # + # Commands documented against phpfpm run against frankenphp instead, e.g. + # `docker compose exec frankenphp composer install`. + phpfpm: + profiles: + - replaced-by-frankenphp + + nginx: + profiles: + - replaced-by-frankenphp + + frankenphp: + build: + context: . + # Xdebug, and OPcache rechecking files so an edit takes effect. + target: dev + user: ${COMPOSE_USER:-deploy} + networks: + - app + - frontend + extra_hosts: + - "host.docker.internal:host-gateway" + environment: + # A bare port keeps Caddy on plain HTTP; Traefik terminates TLS. + SERVER_NAME: ":8080" + SERVER_ROOT: /app/public + PHP_MAX_BODY_SIZE: 5MB + # Read by .docker/php.ini, which uses the itkdev/php8.5-fpm image's own + # variable names. Everything else is defaulted on the image, so only the + # values that differ from the fpm image are set here. + PHP_MEMORY_LIMIT: 256M + # Depending on the setup, you may have to remove --read-envelope-from from msmtp (cf. https://marlam.de/msmtp/msmtp.html) or use SMTP to send mail + PHP_SENDMAIL_PATH: /usr/bin/msmtp --host=mail --port=1025 --read-recipients --read-envelope-from + PHP_XDEBUG_MODE: ${PHP_XDEBUG_MODE:-off} + PHP_XDEBUG_START_WITH_REQUEST: ${PHP_XDEBUG_WITH_REQUEST:-yes} + DOCKER_HOST_DOMAIN: ${COMPOSE_DOMAIN:?} + PHP_IDE_CONFIG: serverName=localhost + depends_on: + mariadb: + condition: service_healthy + # /health/live is public and touches nothing, so it reports whether this + # container is serving without depending on the database or the broker. + healthcheck: + test: + [ + "CMD", + "curl", + "--fail", + "--silent", + "http://localhost:8080/health/live", + ] + start_period: 30s + interval: 10s + timeout: 5s + retries: 3 + ports: + - "8080" + volumes: + - .:/app + - ./.docker/Caddyfile:/etc/frankenphp/Caddyfile:ro + - ./.docker/php.ini:/usr/local/etc/php/conf.d/zz-app.ini:ro + - ./.docker/php-dev.ini:/usr/local/etc/php/conf.d/zz-app-dev.ini:ro + - caddy_data:/data + - caddy_config:/config + labels: + - "traefik.enable=true" + - "traefik.docker.network=frontend" + # The image exposes 80, 443 and 2019 as well, so the port Traefik should + # talk to has to be spelled out. + - "traefik.http.services.${COMPOSE_PROJECT_NAME:?}.loadbalancer.server.port=8080" + - "traefik.http.routers.${COMPOSE_PROJECT_NAME:?}.rule=Host(`${COMPOSE_DOMAIN:?}`)" + # HTTPS config - uncomment to enable redirect from :80 to :443 + # - "traefik.http.routers.${COMPOSE_PROJECT_NAME:?}.middlewares=redirect-to-https" + # - "traefik.http.middlewares.redirect-to-https.redirectscheme.scheme=https" + # Metrics protection. Caddy's Prometheus endpoint, where nginx proxied + # /cron-metrics to a supercronic that this project never starts. + - "traefik.http.routers.${COMPOSE_PROJECT_NAME:?}-metrics.rule=Host(`${COMPOSE_DOMAIN:?}`) && PathPrefix(`/metrics`)" + - "traefik.http.routers.${COMPOSE_PROJECT_NAME:?}-metrics.middlewares=ITKMetricsAuth@file" + # Detailed health check protection. /health/live and /health/ready stay + # public; only /health/detail discloses internals. + - "traefik.http.routers.${COMPOSE_PROJECT_NAME:?}-health.rule=Host(`${COMPOSE_DOMAIN:?}`) && PathPrefix(`/health/detail`)" + - "traefik.http.routers.${COMPOSE_PROJECT_NAME:?}-health.middlewares=ITKBasicAuth@file" + # A mock identity provider, so the OpenID Connect login can be exercised locally. # The real one has no redirect URI registered for a developer machine. # @@ -54,3 +141,8 @@ services: volumes: - .:/app working_dir: /app + +# Caddy keeps its instance identity and last known config here. +volumes: + caddy_data: + caddy_config: diff --git a/docker-compose.redirect.yml b/docker-compose.redirect.yml index 2e7ac332..c649c6dc 100644 --- a/docker-compose.redirect.yml +++ b/docker-compose.redirect.yml @@ -1,6 +1,6 @@ # itk-version: 3.2.4 services: - nginx: + frankenphp: labels: # Add www before domain and set redirect to non-www - "traefik.http.routers.www_${COMPOSE_PROJECT_NAME:?}-http.rule=Host(`www.${COMPOSE_SERVER_DOMAIN:?}`)" diff --git a/docker-compose.server.override.yml b/docker-compose.server.override.yml index cbfa1813..ac302893 100644 --- a/docker-compose.server.override.yml +++ b/docker-compose.server.override.yml @@ -1,4 +1,82 @@ services: + # FrankenPHP POC: a single container replaces the phpfpm and nginx pair. + # Compose cannot delete an inherited service, so both are moved into a + # profile that is never enabled. Nothing depends on them once nginx is gone, + # so neither is started. + phpfpm: + profiles: + - replaced-by-frankenphp + + nginx: + profiles: + - replaced-by-frankenphp + + frankenphp: + build: + context: . + # No Xdebug, and OPcache trusting what it compiled. + target: prod + restart: unless-stopped + networks: + - app + - frontend + extra_hosts: + - "host.docker.internal:host-gateway" + environment: + # A bare port keeps Caddy on plain HTTP; Traefik terminates TLS. + SERVER_NAME: ":8080" + SERVER_ROOT: /app/public + PHP_MAX_BODY_SIZE: 5MB + # Read by .docker/php.ini, which uses the itkdev/php8.5-fpm image's own + # variable names. The image defaults match the fpm image's, so nothing + # needs restating here. + depends_on: + rabbit: + condition: service_healthy + # /health/live is public and touches nothing, so it reports whether this + # container is serving without depending on the database or the broker. + healthcheck: + test: + [ + "CMD", + "curl", + "--fail", + "--silent", + "http://localhost:8080/health/live", + ] + start_period: 30s + interval: 10s + timeout: 5s + retries: 3 + volumes: + - .:/app + - ./.docker/Caddyfile:/etc/frankenphp/Caddyfile:ro + - ./.docker/php.ini:/usr/local/etc/php/conf.d/zz-app.ini:ro + - caddy_data:/data + - caddy_config:/config + labels: + - "traefik.enable=true" + - "traefik.docker.network=frontend" + # The image exposes 80, 443 and 2019 as well, so the port Traefik should + # talk to has to be spelled out. + - "traefik.http.services.${COMPOSE_PROJECT_NAME:?}.loadbalancer.server.port=8080" + - "traefik.http.routers.${COMPOSE_PROJECT_NAME:?}-http.rule=Host(`${COMPOSE_SERVER_DOMAIN:?}`)" + - "traefik.http.routers.${COMPOSE_PROJECT_NAME:?}-http.entrypoints=web" + - "traefik.http.routers.${COMPOSE_PROJECT_NAME:?}-http.middlewares=redirect-to-https" + - "traefik.http.middlewares.redirect-to-https.redirectscheme.scheme=https" + - "traefik.http.routers.${COMPOSE_PROJECT_NAME:?}.rule=Host(`${COMPOSE_SERVER_DOMAIN:?}`)" + - "traefik.http.routers.${COMPOSE_PROJECT_NAME:?}.entrypoints=websecure" + # Metrics protection. Caddy's Prometheus endpoint, where nginx proxied + # /cron-metrics to a supercronic that this project never starts. + - "traefik.http.routers.${COMPOSE_PROJECT_NAME:?}-metrics.rule=Host(`${COMPOSE_SERVER_DOMAIN:?}`) && PathPrefix(`/metrics`)" + - "traefik.http.routers.${COMPOSE_PROJECT_NAME:?}-metrics.entrypoints=websecure" + - "traefik.http.routers.${COMPOSE_PROJECT_NAME:?}-metrics.middlewares=ITKMetricsAuth@file" + # Detailed health check protection. /health/live and /health/ready stay + # public; only /health/detail discloses internals. + - "traefik.http.routers.${COMPOSE_PROJECT_NAME:?}-health.rule=Host(`${COMPOSE_SERVER_DOMAIN:?}`) && PathPrefix(`/health/detail`)" + - "traefik.http.routers.${COMPOSE_PROJECT_NAME:?}-health.entrypoints=websecure" + - "traefik.http.routers.${COMPOSE_PROJECT_NAME:?}-health.middlewares=ITKBasicAuth@file" + rabbit: image: rabbitmq:4-management-alpine restart: unless-stopped @@ -18,7 +96,7 @@ services: retries: 30 supervisor: - image: itkdev/supervisor-php8.4:alpine + image: itkdev/supervisor-php8.5:alpine restart: unless-stopped stop_grace_period: 20s environment: @@ -36,7 +114,7 @@ services: rabbit: condition: service_healthy - phpfpm: - depends_on: - rabbit: - condition: service_healthy +# Caddy keeps its instance identity and last known config here. +volumes: + caddy_data: + caddy_config: diff --git a/docker-compose.server.prod.yml b/docker-compose.server.prod.yml index 8db1cf8a..cd85e345 100644 --- a/docker-compose.server.prod.yml +++ b/docker-compose.server.prod.yml @@ -3,6 +3,6 @@ services: volumes: - ../../shared/.env.local:/app/.env.local - phpfpm: + frankenphp: volumes: - ../../shared/.env.local:/app/.env.local diff --git a/docker-compose.server.yml b/docker-compose.server.yml index 45f2720c..acfc9fca 100644 --- a/docker-compose.server.yml +++ b/docker-compose.server.yml @@ -8,7 +8,7 @@ networks: services: phpfpm: - image: itkdev/php8.4-fpm:alpine + image: itkdev/php8.5-fpm:alpine restart: unless-stopped networks: - app diff --git a/docker-compose.yml b/docker-compose.yml index 444ce64d..4498a61e 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -27,7 +27,7 @@ services: #- ENCRYPT=1 # Uncomment to enable database encryption. phpfpm: - image: itkdev/php8.4-fpm:latest + image: itkdev/php8.5-fpm:latest user: ${COMPOSE_USER:-deploy} networks: - app diff --git a/igor-baseline.json b/igor-baseline.json new file mode 100644 index 00000000..5d15c2e7 --- /dev/null +++ b/igor-baseline.json @@ -0,0 +1,166 @@ +{ + "files": { + "src/Command/PurgeCommand.php": [ + { + "message": "Mutation detected on an injected dependency ($this->detectionResultRepository). Risk of State Leak in a worker.", + "reason": "A repository method call, not a property assignment. Console commands also run in their own process, never inside a worker request." + } + ], + "src/Command/ReplayDetectionResultsCommand.php": [ + { + "message": "Mutation detected on an injected dependency ($this->entityManager). Risk of State Leak in a worker.", + "reason": "Sets a Doctrine connection middleware for the duration of a console command. Commands run in their own process, never inside a worker request." + } + ], + "src/Controller/Admin/DashboardController.php": [ + { + "message": "Mutation detected on an injected dependency ($this->adminUrlGenerator). Risk of State Leak in a worker.", + "reason": "The chain opens with unsetAll(), which clears the accumulated route parameters before anything is set. Igor does not recognise unsetAll() as a reset. DashboardControllerTest asserts where the redirect lands, so a misplaced unsetAll() fails the build." + }, + { + "message": "Mutation detected on an injected dependency ($this->adminUrlGenerator). Risk of State Leak in a worker.", + "reason": "The chain opens with unsetAll(), which clears the accumulated route parameters before anything is set. Igor does not recognise unsetAll() as a reset. DashboardControllerTest asserts where the redirect lands, so a misplaced unsetAll() fails the build." + } + ], + "src/Controller/Admin/SecurityContractCrudController.php": [ + { + "message": "Mutation detected on an injected dependency ($this->adminUrlGenerator). Risk of State Leak in a worker.", + "reason": "The chain opens with unsetAll(), which clears the accumulated route parameters before anything is set. Igor does not recognise unsetAll() as a reset." + }, + { + "message": "Mutation detected on an injected dependency ($this->adminUrlGenerator). Risk of State Leak in a worker.", + "reason": "The chain opens with unsetAll(), which clears the accumulated route parameters before anything is set. Igor does not recognise unsetAll() as a reset." + } + ], + "src/Doctrine/Functions/SemverNumeric.php": [ + { + "message": "Mutation of state 'versionExpression' in SemverNumeric::parse()", + "reason": "A Doctrine DQL AST function node, not a service. Doctrine builds one per query while parsing, so the assignment in parse() cannot outlive it." + } + ], + "src/Handler/DockerImageHandler.php": [ + { + "message": "Mutation detected on an injected dependency ($this->dockerImageTagFactory). Risk of State Leak in a worker.", + "reason": "Calls a command method on a factory whose name begins with \"set\", which Igor treats as a setter. setPackageVersions() and friends do work and return void; they assign nothing on the factory, which is stateless." + }, + { + "message": "Mutation detected on an injected dependency ($this->packageVersionFactory). Risk of State Leak in a worker.", + "reason": "Calls a command method on a factory whose name begins with \"set\", which Igor treats as a setter. setPackageVersions() and friends do work and return void; they assign nothing on the factory, which is stateless." + }, + { + "message": "Mutation detected on an injected dependency ($this->advisoryFactory). Risk of State Leak in a worker.", + "reason": "Calls a command method on a factory whose name begins with \"set\", which Igor treats as a setter. setPackageVersions() and friends do work and return void; they assign nothing on the factory, which is stateless." + }, + { + "message": "Mutation detected on an injected dependency ($this->moduleVersionFactory). Risk of State Leak in a worker.", + "reason": "Calls a command method on a factory whose name begins with \"set\", which Igor treats as a setter. setPackageVersions() and friends do work and return void; they assign nothing on the factory, which is stateless." + } + ], + "src/Handler/DrupalHandler.php": [ + { + "message": "Mutation detected on an injected dependency ($this->packageVersionFactory). Risk of State Leak in a worker.", + "reason": "Calls a command method on a factory whose name begins with \"set\", which Igor treats as a setter. setPackageVersions() and friends do work and return void; they assign nothing on the factory, which is stateless." + }, + { + "message": "Mutation detected on an injected dependency ($this->moduleVersionFactory). Risk of State Leak in a worker.", + "reason": "Calls a command method on a factory whose name begins with \"set\", which Igor treats as a setter. setPackageVersions() and friends do work and return void; they assign nothing on the factory, which is stateless." + } + ], + "src/Handler/GitHandler.php": [ + { + "message": "Mutation detected on an injected dependency ($this->gitCloneFactory). Risk of State Leak in a worker.", + "reason": "Calls a command method on a factory whose name begins with \"set\", which Igor treats as a setter. setPackageVersions() and friends do work and return void; they assign nothing on the factory, which is stateless." + } + ], + "src/Handler/SymfonyHandler.php": [ + { + "message": "Mutation detected on an injected dependency ($this->packageVersionFactory). Risk of State Leak in a worker.", + "reason": "Calls a command method on a factory whose name begins with \"set\", which Igor treats as a setter. setPackageVersions() and friends do work and return void; they assign nothing on the factory, which is stateless." + }, + { + "message": "Mutation detected on an injected dependency ($this->packageVersionFactory). Risk of State Leak in a worker.", + "reason": "Calls a command method on a factory whose name begins with \"set\", which Igor treats as a setter. setPackageVersions() and friends do work and return void; they assign nothing on the factory, which is stateless." + } + ], + "src/Repository/DetectionResultRepository.php": [ + { + "message": "Mutation detected on a local reference to a shared service ($em). Risk of State Leak in a worker.", + "reason": "EntityManager::remove() is ordinary Doctrine usage, not a mutation of the manager. Whether the unit of work is flushed is the caller's business." + } + ], + "src/Service/ModuleVersionFactory.php": [ + { + "message": "Mutation detected on a local reference to a shared service ($module). Risk of State Leak in a worker.", + "reason": "Mutations on Doctrine entities returned from a repository, which Igor reads as shared services because the repository is one. The entities are rows, not services. The factory itself holds no state: its deduplication buffers are locals, covered by ModuleVersionFactoryTest." + }, + { + "message": "Mutation detected on a local reference to a shared service ($module). Risk of State Leak in a worker.", + "reason": "Mutations on Doctrine entities returned from a repository, which Igor reads as shared services because the repository is one. The entities are rows, not services. The factory itself holds no state: its deduplication buffers are locals, covered by ModuleVersionFactoryTest." + } + ], + "src/Service/PackageVersionFactory.php": [ + { + "message": "Mutation detected on a local reference to a shared service ($package). Risk of State Leak in a worker.", + "reason": "Mutations on Doctrine entities returned from a repository, which Igor reads as shared services because the repository is one. The entities are rows, not services. The factory itself holds no state: its deduplication buffers are locals, covered by PackageVersionFactoryTest." + }, + { + "message": "Mutation detected on a local reference to a shared service ($package). Risk of State Leak in a worker.", + "reason": "Mutations on Doctrine entities returned from a repository, which Igor reads as shared services because the repository is one. The entities are rows, not services. The factory itself holds no state: its deduplication buffers are locals, covered by PackageVersionFactoryTest." + }, + { + "message": "Mutation detected on a local reference to a shared service ($package). Risk of State Leak in a worker.", + "reason": "Mutations on Doctrine entities returned from a repository, which Igor reads as shared services because the repository is one. The entities are rows, not services. The factory itself holds no state: its deduplication buffers are locals, covered by PackageVersionFactoryTest." + }, + { + "message": "Mutation detected on a local reference to a shared service ($packageVersion). Risk of State Leak in a worker.", + "reason": "Mutations on Doctrine entities returned from a repository, which Igor reads as shared services because the repository is one. The entities are rows, not services. The factory itself holds no state: its deduplication buffers are locals, covered by PackageVersionFactoryTest." + }, + { + "message": "Mutation detected on a local reference to a shared service ($packageVersion). Risk of State Leak in a worker.", + "reason": "Mutations on Doctrine entities returned from a repository, which Igor reads as shared services because the repository is one. The entities are rows, not services. The factory itself holds no state: its deduplication buffers are locals, covered by PackageVersionFactoryTest." + }, + { + "message": "Mutation detected on a local reference to a shared service ($packageVersion). Risk of State Leak in a worker.", + "reason": "Mutations on Doctrine entities returned from a repository, which Igor reads as shared services because the repository is one. The entities are rows, not services. The factory itself holds no state: its deduplication buffers are locals, covered by PackageVersionFactoryTest." + } + ], + "src/Service/RepoAdvisoryService.php": [ + { + "message": "Mutation detected on an injected dependency ($this->adminUrlGenerator). Risk of State Leak in a worker.", + "reason": "The chain opens with unsetAll(), which clears the accumulated route parameters before anything is set. Igor does not recognise unsetAll() as a reset." + }, + { + "message": "Mutation detected on an injected dependency ($this->adminUrlGenerator). Risk of State Leak in a worker.", + "reason": "The chain opens with unsetAll(), which clears the accumulated route parameters before anything is set. Igor does not recognise unsetAll() as a reset." + }, + { + "message": "Mutation detected on an injected dependency ($this->adminUrlGenerator). Risk of State Leak in a worker.", + "reason": "The chain opens with unsetAll(), which clears the accumulated route parameters before anything is set. Igor does not recognise unsetAll() as a reset." + } + ], + "src/Trait/ExportCrudControllerTrait.php": [ + { + "message": "Mutation of state 'filterFactory' in ExportCrudControllerTrait::setFilterFactory()", + "reason": "Setter injection via #[Required]. The container calls these once while building the service, not at request time." + }, + { + "message": "Mutation of state 'exporter' in ExportCrudControllerTrait::setExporter()", + "reason": "Setter injection via #[Required]. The container calls these once while building the service, not at request time." + } + ], + "src/Twig/AppExtension.php": [ + { + "message": "Mutation detected on an injected dependency ($this->adminUrlGenerator). Risk of State Leak in a worker.", + "reason": "The chain opens with unsetAll(), which clears the accumulated route parameters before anything is set. Igor does not recognise unsetAll() as a reset." + }, + { + "message": "Mutation detected on an injected dependency ($this->adminUrlGenerator). Risk of State Leak in a worker.", + "reason": "The chain opens with unsetAll(), which clears the accumulated route parameters before anything is set. Igor does not recognise unsetAll() as a reset." + }, + { + "message": "Mutation detected on an injected dependency ($this->adminUrlGenerator). Risk of State Leak in a worker.", + "reason": "The chain opens with unsetAll(), which clears the accumulated route parameters before anything is set. Igor does not recognise unsetAll() as a reset." + } + ] + } +} diff --git a/igor.json b/igor.json new file mode 100644 index 00000000..5b58ed98 --- /dev/null +++ b/igor.json @@ -0,0 +1,13 @@ +{ + "exclude": ["migrations", "tests", "var"], + "safe_namespaces": [ + "Symfony\\", + "Doctrine\\", + "Psr\\", + "IgorPhp\\IgorBundle\\" + ], + "ignore_vendors": true, + "console_path": "bin/console", + "env": "dev", + "baseline": "igor-baseline.json" +} diff --git a/src/Controller/Admin/DashboardController.php b/src/Controller/Admin/DashboardController.php index e1edae7f..f7ee4426 100644 --- a/src/Controller/Admin/DashboardController.php +++ b/src/Controller/Admin/DashboardController.php @@ -28,6 +28,7 @@ public function __construct( public function index(): Response { $d = $this->adminUrlGenerator + ->unsetAll() ->setController(ServerCrudController::class)->setAction(Crud::PAGE_INDEX) ->generateUrl(); diff --git a/src/Controller/Admin/SecurityContractCrudController.php b/src/Controller/Admin/SecurityContractCrudController.php index d0e0cbb4..4ee8ecf0 100644 --- a/src/Controller/Admin/SecurityContractCrudController.php +++ b/src/Controller/Admin/SecurityContractCrudController.php @@ -132,6 +132,7 @@ public function syncAll(): RedirectResponse return $this->redirect( $this->adminUrlGenerator + ->unsetAll() ->setController(static::class) ->setAction(Crud::PAGE_INDEX) ->generateUrl() diff --git a/src/Service/LeantimeService.php b/src/Service/LeantimeService.php index ed1374a0..cdec2679 100644 --- a/src/Service/LeantimeService.php +++ b/src/Service/LeantimeService.php @@ -6,6 +6,7 @@ use Symfony\Contracts\HttpClient\Exception\ExceptionInterface; use Symfony\Contracts\HttpClient\HttpClientInterface; +use Symfony\Contracts\Service\ResetInterface; /** * Minimal JSON-RPC 2.0 client for the Leantime API. @@ -16,7 +17,7 @@ * the x-api-key header; this class only assembles the JSON-RPC body and * unwraps responses. */ -class LeantimeService +class LeantimeService implements ResetInterface { private const string JSONRPC_VERSION = '2.0'; private const string API_PATH = '/api/jsonrpc/'; @@ -281,4 +282,28 @@ private function loadUsers(): void } } } + + /** + * Drop the memoised user directory. + * + * loadUsers() fetches the Leantime directory once and every later lookup + * reads these two maps. Under php-fpm the instance died with the request, + * so "once per service instance" also meant "once per request". In a + * long-running worker the instance outlives the request and the directory + * would never be refetched, leaving new users, renames and changed + * addresses invisible until the worker recycled. + * + * Unlike the package and module factories, this cache cannot simply become + * a local: resolveUserName() is called inside a loop over tickets, so + * dropping it would cost one API round trip per ticket. Resetting it + * restores the per-request lifetime loadUsers() already documents. + * + * autoconfigure tags this kernel.reset, and services_resetter calls it + * between requests. + */ + public function reset(): void + { + $this->userNamesById = null; + $this->userIdsByEmail = null; + } } diff --git a/src/Service/ModuleVersionFactory.php b/src/Service/ModuleVersionFactory.php index 9772e20e..a591ee25 100644 --- a/src/Service/ModuleVersionFactory.php +++ b/src/Service/ModuleVersionFactory.php @@ -14,9 +14,6 @@ class ModuleVersionFactory { - private array $createdModules = []; - private array $createdModuleVersions = []; - public function __construct( private readonly EntityManagerInterface $entityManager, private readonly ModuleRepository $moduleRepository, @@ -26,41 +23,42 @@ public function __construct( public function setModuleVersions(Installation $installation, object $installedModules): void { + // Locals, not properties: see PackageVersionFactory for why. Entities + // persisted below are not flushed until the end of the call, so these + // maps stand in for the repositories until then, and nothing outlives + // the method. + $createdModules = []; + $createdModuleVersions = []; + $moduleVersions = new ArrayCollection(); foreach ($installedModules as $name => $installed) { - $module = $this->getModule($name, $installed->package); + $module = $this->getModule($name, $installed->package, $createdModules); if (isset($installed->display_name)) { $module->setDisplayName($installed->display_name); } $module->setEnabled('Enabled' === $installed->status); - $moduleVersion = $this->getModuleVersion($module, $installed->version); + $moduleVersion = $this->getModuleVersion($module, $installed->version, $createdModuleVersions); $moduleVersions->add($moduleVersion); } $installation->setModuleVersions($moduleVersions); $this->entityManager->flush(); - $this->createdModules = []; - $this->createdModuleVersions = []; } - private function getModule(string $name, string $package): Module + /** + * @param array $createdModules modules persisted in this call but not yet flushed, keyed by name and package + */ + private function getModule(string $name, string $package, array &$createdModules): Module { + $key = $name."\0".$package; + $module = $this->moduleRepository->findOneBy([ 'name' => $name, 'package' => $package, - ]); - - if (null === $module) { - /** @var Module $createdModule */ - foreach ($this->createdModules as $createdModule) { - if ($name === $createdModule->getName() && $package === $createdModule->getPackage()) { - $module = $createdModule; - } - } - } + ]) ?? $createdModules[$key] ?? null; if (null === $module) { $module = new Module(); @@ -69,13 +67,16 @@ private function getModule(string $name, string $package): Module $module->setName($name); $module->setPackage($package); - $this->createdModules[] = $module; + $createdModules[$key] = $module; } return $module; } - private function getModuleVersion(Module $module, string|int|float|null $version): ModuleVersion + /** + * @param array $createdModuleVersions versions persisted in this call but not yet flushed, keyed by module identity and version + */ + private function getModuleVersion(Module $module, string|int|float|null $version, array &$createdModuleVersions): ModuleVersion { if (is_int($version) || is_float($version)) { $version = (string) $version; @@ -86,13 +87,15 @@ private function getModuleVersion(Module $module, string|int|float|null $version 'version' => $version, ]); - if (null === $moduleVersion) { - /** @var ModuleVersion $createdModuleVersion */ - foreach ($this->createdModuleVersions as $createdModuleVersion) { - if ($module->getId() === $createdModuleVersion->getModule()->getId() && $version === $createdModuleVersion->getVersion()) { - $moduleVersion = $createdModuleVersion; - } - } + // A null version is deliberately left out of the buffer. + // ModuleVersion::getVersion() reports 'Unknown' for null, so the scan + // this replaced never matched a null-versioned module either. Keeping + // that quirk keeps this change about state and nothing else; see the + // note in the pull request. + $key = null === $version ? null : spl_object_id($module)."\0".$version; + + if (null === $moduleVersion && null !== $key) { + $moduleVersion = $createdModuleVersions[$key] ?? null; } if (null === $moduleVersion) { @@ -102,7 +105,9 @@ private function getModuleVersion(Module $module, string|int|float|null $version $module->addModuleVersion($moduleVersion); $moduleVersion->setVersion($version); - $this->createdModuleVersions[] = $moduleVersion; + if (null !== $key) { + $createdModuleVersions[$key] = $moduleVersion; + } } return $moduleVersion; diff --git a/src/Service/PackageVersionFactory.php b/src/Service/PackageVersionFactory.php index dff3bee5..ebd3cba5 100644 --- a/src/Service/PackageVersionFactory.php +++ b/src/Service/PackageVersionFactory.php @@ -14,9 +14,6 @@ class PackageVersionFactory { - private array $createdPackages = []; - private array $createdPackageVersions = []; - public function __construct( private readonly EntityManagerInterface $entityManager, private readonly PackageRepository $packageRepository, @@ -26,11 +23,22 @@ public function __construct( public function setPackageVersions(Installation $installation, array $installedPackages): void { + // Entities are persisted below but not flushed until the end of this + // method, so the repositories cannot find them yet. These two maps + // stand in for the repositories for the rest of the call, keeping the + // same package from being created twice. + // + // They are locals, not properties. Nothing survives the method, so the + // service holds no state between calls — which is what makes it safe in + // a long-running process, the messenger consumer included. + $createdPackages = []; + $createdPackageVersions = []; + $packageVersions = new ArrayCollection(); foreach ($installedPackages as $installed) { [$vendor, $name] = explode('/', (string) $installed->name); - $package = $this->getPackage($vendor, $name); + $package = $this->getPackage($vendor, $name, $createdPackages); $package->setDescription($installed->description); if (isset($installed->warning)) { @@ -40,7 +48,7 @@ public function setPackageVersions(Installation $installation, array $installedP $package->setAbandoned($installed->abandoned); } - $packageVersion = $this->getPackageVersion($package, $installed->version); + $packageVersion = $this->getPackageVersion($package, $installed->version, $createdPackageVersions); $installation->addPackageVersion($packageVersion); $packageVersion->setVersion($installed->version); @@ -50,9 +58,6 @@ public function setPackageVersions(Installation $installation, array $installedP if (isset($installed->{'latest-status'})) { $packageVersion->setLatestStatus($installed->{'latest-status'}); } - if (isset($installed->{'latest-status'})) { - $packageVersion->setLatestStatus($installed->{'latest-status'}); - } $packageVersions->add($packageVersion); } @@ -60,25 +65,19 @@ public function setPackageVersions(Installation $installation, array $installedP $installation->setPackageVersions($packageVersions); $this->entityManager->flush(); - $this->createdPackages = []; - $this->createdPackageVersions = []; } - private function getPackage(string $vendor, string $name): Package + /** + * @param array $createdPackages packages persisted in this call but not yet flushed, keyed by vendor and name + */ + private function getPackage(string $vendor, string $name, array &$createdPackages): Package { + $key = $vendor."\0".$name; + $package = $this->packageRepository->findOneBy([ 'vendor' => $vendor, 'name' => $name, - ]); - - if (null === $package) { - /** @var Package $createdPackage */ - foreach ($this->createdPackages as $createdPackage) { - if ($vendor === $createdPackage->getVendor() && $name === $createdPackage->getName()) { - $package = $createdPackage; - } - } - } + ]) ?? $createdPackages[$key] ?? null; if (null === $package) { $package = new Package(); @@ -87,27 +86,26 @@ private function getPackage(string $vendor, string $name): Package $package->setVendor($vendor); $package->setName($name); - $this->createdPackages[] = $package; + $createdPackages[$key] = $package; } return $package; } - private function getPackageVersion(Package $package, string $version): PackageVersion + /** + * @param array $createdPackageVersions versions persisted in this call but not yet flushed, keyed by package identity and version + */ + private function getPackageVersion(Package $package, string $version, array &$createdPackageVersions): PackageVersion { + // Keyed on object identity rather than on the id: within one call the + // package may have been created moments ago, and object identity holds + // whether or not Doctrine has assigned an id yet. + $key = spl_object_id($package)."\0".$version; + $packageVersion = $this->packageVersionRepository->findOneBy([ 'package' => $package, 'version' => $version, - ]); - - if (null === $packageVersion) { - /* @var PackageVersion $packageVersion */ - foreach ($this->createdPackageVersions as $createdPackageVersion) { - if ($package->getId() === $createdPackageVersion->getPackage()->getId() && $version === $createdPackageVersion->getVersion()) { - $packageVersion = $createdPackageVersion; - } - } - } + ]) ?? $createdPackageVersions[$key] ?? null; if (null === $packageVersion) { $packageVersion = new PackageVersion(); @@ -116,7 +114,7 @@ private function getPackageVersion(Package $package, string $version): PackageVe $package->addPackageVersion($packageVersion); $packageVersion->setVersion($version); - $this->createdPackageVersions[] = $packageVersion; + $createdPackageVersions[$key] = $packageVersion; } return $packageVersion; diff --git a/tests/Controller/Admin/AdminSmokeTest.php b/tests/Controller/Admin/AdminSmokeTest.php index c41e2534..d2d5ef1d 100644 --- a/tests/Controller/Admin/AdminSmokeTest.php +++ b/tests/Controller/Admin/AdminSmokeTest.php @@ -17,6 +17,7 @@ use App\Controller\Admin\OIDCCrudController; use App\Controller\Admin\PackageCrudController; use App\Controller\Admin\PackageVersionCrudController; +use App\Controller\Admin\SecurityContractCrudController; use App\Controller\Admin\ServerCrudController; use App\Controller\Admin\ServiceCertificateCrudController; use App\Controller\Admin\SiteCrudController; @@ -68,6 +69,7 @@ public static function crudControllerProvider(): iterable yield 'OIDC' => [OIDCCrudController::class]; yield 'Package' => [PackageCrudController::class]; yield 'PackageVersion' => [PackageVersionCrudController::class]; + yield 'SecurityContract' => [SecurityContractCrudController::class]; yield 'Server' => [ServerCrudController::class]; yield 'ServiceCertificate' => [ServiceCertificateCrudController::class]; yield 'Site' => [SiteCrudController::class]; diff --git a/tests/Controller/Admin/DashboardControllerTest.php b/tests/Controller/Admin/DashboardControllerTest.php new file mode 100644 index 00000000..1537bc7e --- /dev/null +++ b/tests/Controller/Admin/DashboardControllerTest.php @@ -0,0 +1,68 @@ +get('doctrine')->getManager() + ->getRepository(User::class)->findOneBy([]); + $client->loginUser($user); + + $client->request('GET', '/admin'); + + $this->assertResponseRedirects(); + + // Ask EasyAdmin what that URL should be rather than hard-coding the + // slug, so this keeps working if the route path changes. + $expected = static::getContainer()->get(AdminUrlGenerator::class) + ->setController(ServerCrudController::class) + ->setAction(Crud::PAGE_INDEX) + ->generateUrl(); + + $location = (string) $client->getResponse()->headers->get('Location'); + $this->assertSame( + parse_url($expected, \PHP_URL_PATH), + parse_url($location, \PHP_URL_PATH), + 'the redirect must still land on the Server index' + ); + } + + public function testTheRedirectTargetLoads(): void + { + $client = static::createClient(); + + $user = static::getContainer()->get('doctrine')->getManager() + ->getRepository(User::class)->findOneBy([]); + $client->loginUser($user); + + $client->request('GET', '/admin'); + $client->followRedirect(); + + $this->assertResponseIsSuccessful(); + } +} diff --git a/tests/Service/LeantimeServiceResetTest.php b/tests/Service/LeantimeServiceResetTest.php new file mode 100644 index 00000000..0a710e34 --- /dev/null +++ b/tests/Service/LeantimeServiceResetTest.php @@ -0,0 +1,81 @@ +assertInstanceOf( + ResetInterface::class, + new LeantimeService(new MockHttpClient()), + 'autoconfigure only tags kernel.reset when the service implements ResetInterface' + ); + } + + public function testTheDirectoryIsFetchedOncePerInstance(): void + { + $client = new MockHttpClient([self::directory(), self::directory()]); + $service = new LeantimeService($client); + + $this->assertSame(7, $service->findUserIdByEmail('someone@aarhus.dk')); + $this->assertSame(7, $service->findUserIdByEmail('someone@aarhus.dk')); + + $this->assertSame(1, $client->getRequestsCount(), 'the second lookup must come from the cache'); + } + + public function testResetForcesTheDirectoryToBeFetchedAgain(): void + { + $client = new MockHttpClient([self::directory(), self::directory()]); + $service = new LeantimeService($client); + + $service->findUserIdByEmail('someone@aarhus.dk'); + $service->reset(); + $service->findUserIdByEmail('someone@aarhus.dk'); + + $this->assertSame(2, $client->getRequestsCount(), 'without this a worker would never see directory changes'); + } + + public function testTheDirectoryIsRereadAfterReset(): void + { + $client = new MockHttpClient([ + self::directory(), + new JsonMockResponse(['result' => [ + ['id' => 9, 'firstname' => 'New', 'lastname' => 'Starter', 'email' => 'new.starter@aarhus.dk'], + ]]), + ]); + $service = new LeantimeService($client); + + $this->assertNull($service->findUserIdByEmail('new.starter@aarhus.dk'), 'not in the directory yet'); + + $service->reset(); + + $this->assertSame(9, $service->findUserIdByEmail('new.starter@aarhus.dk'), 'visible after the reset'); + } + + private static function directory(): JsonMockResponse + { + return new JsonMockResponse(['result' => [ + ['id' => 7, 'firstname' => 'Some', 'lastname' => 'One', 'email' => 'someone@aarhus.dk'], + ]]); + } +} diff --git a/tests/Service/ModuleVersionFactoryTest.php b/tests/Service/ModuleVersionFactoryTest.php new file mode 100644 index 00000000..c24ddfc7 --- /dev/null +++ b/tests/Service/ModuleVersionFactoryTest.php @@ -0,0 +1,174 @@ + */ + private array $persisted = []; + + protected function setUp(): void + { + $this->persisted = []; + } + + public function testRepeatedModuleIsCreatedOnce(): void + { + // Drupal delivers modules as an object keyed by machine name, so a + // repeat within one payload means the same key twice — which only the + // package differing can produce. + $this->factory()->setModuleVersions(new Installation(), (object) [ + 'views' => self::installed('drupal/views', '1.0.0'), + ]); + + $this->assertCount(1, $this->modules()); + $this->assertCount(1, $this->moduleVersions()); + } + + public function testSameModuleWithTwoVersionsCreatesTwoVersions(): void + { + $factory = $this->factory(); + + $factory->setModuleVersions(new Installation(), (object) [ + 'views' => self::installed('drupal/views', '1.0.0'), + ]); + $modulesAfterFirst = count($this->modules()); + + $factory->setModuleVersions(new Installation(), (object) [ + 'views' => self::installed('drupal/views', '2.0.0'), + ]); + + $this->assertSame(1, $modulesAfterFirst); + $this->assertCount(2, $this->moduleVersions(), 'distinct versions are distinct rows'); + } + + public function testTwoNewModulesSharingAVersionStringStayApart(): void + { + $this->factory()->setModuleVersions(new Installation(), (object) [ + 'views' => self::installed('drupal/views', '1.0.0'), + 'token' => self::installed('drupal/token', '1.0.0'), + ]); + + $this->assertCount(2, $this->modules()); + $this->assertCount(2, $this->moduleVersions(), 'a shared version string must not merge two modules'); + } + + public function testNothingCarriesOverBetweenCalls(): void + { + $factory = $this->factory(); + + $factory->setModuleVersions(new Installation(), (object) ['views' => self::installed('drupal/views', '1.0.0')]); + $factory->setModuleVersions(new Installation(), (object) ['views' => self::installed('drupal/views', '1.0.0')]); + + $modules = array_values($this->modules()); + $this->assertCount(2, $modules, 'each call starts from an empty buffer'); + $this->assertNotSame($modules[0], $modules[1]); + } + + public function testAFailedFlushLeavesNothingForTheNextCall(): void + { + $factory = $this->factory(throwOnFirstFlush: true); + + try { + $factory->setModuleVersions(new Installation(), (object) ['views' => self::installed('drupal/views', '1.0.0')]); + $this->fail('expected the failing flush to throw'); + } catch (\RuntimeException) { + // Expected. + } + + $this->persisted = []; + $factory->setModuleVersions(new Installation(), (object) ['views' => self::installed('drupal/views', '1.0.0')]); + + $this->assertCount(1, $this->modules(), 'the module is created afresh, not taken from a stale buffer'); + } + + /** + * ModuleVersion::getVersion() reports 'Unknown' for a null version, so the + * scan this replaced never matched a null-versioned module in the buffer and + * created a row per occurrence. That quirk is preserved deliberately — + * changing it would change which rows get written. + */ + public function testNullVersionsAreNotDeduplicated(): void + { + $this->factory()->setModuleVersions(new Installation(), (object) [ + 'views' => self::installed('drupal/views', null), + 'token' => self::installed('drupal/views', null), + ]); + + $this->assertCount(2, $this->moduleVersions(), 'documented pre-existing behaviour, not an endorsement'); + } + + private function factory(bool $throwOnFirstFlush = false): ModuleVersionFactory + { + $entityManager = $this->createStub(EntityManagerInterface::class); + $entityManager->method('persist')->willReturnCallback( + function (object $entity): void { + // Doctrine assigns the ULID during persist(), because + // UlidGenerator is a CUSTOM rather than a post-insert + // generator. The stub does the same, so entities here are in + // the state the factory actually meets them in. + if ($entity instanceof AbstractBaseEntity) { + $entity->setId(new Ulid()); + } + + $this->persisted[] = $entity; + } + ); + + $flushes = 0; + $entityManager->method('flush')->willReturnCallback( + function () use ($throwOnFirstFlush, &$flushes): void { + if ($throwOnFirstFlush && 0 === $flushes++) { + throw new \RuntimeException('flush failed'); + } + } + ); + + $moduleRepository = $this->createStub(ModuleRepository::class); + $moduleRepository->method('findOneBy')->willReturn(null); + $moduleVersionRepository = $this->createStub(ModuleVersionRepository::class); + $moduleVersionRepository->method('findOneBy')->willReturn(null); + + return new ModuleVersionFactory($entityManager, $moduleRepository, $moduleVersionRepository); + } + + private static function installed(string $package, string|int|float|null $version): object + { + return (object) [ + 'package' => $package, + 'version' => $version, + 'status' => 'Enabled', + ]; + } + + /** @return array */ + private function modules(): array + { + return array_filter($this->persisted, static fn (object $e): bool => $e instanceof Module); + } + + /** @return array */ + private function moduleVersions(): array + { + return array_filter($this->persisted, static fn (object $e): bool => $e instanceof ModuleVersion); + } +} diff --git a/tests/Service/PackageVersionFactoryTest.php b/tests/Service/PackageVersionFactoryTest.php new file mode 100644 index 00000000..54a476bb --- /dev/null +++ b/tests/Service/PackageVersionFactoryTest.php @@ -0,0 +1,169 @@ + */ + private array $persisted = []; + + protected function setUp(): void + { + $this->persisted = []; + } + + public function testRepeatedPackageIsCreatedOnce(): void + { + $this->factory()->setPackageVersions(new Installation(), [ + self::installed('acme/foo', '1.0.0'), + self::installed('acme/foo', '1.0.0'), + ]); + + $this->assertCount(1, $this->packages(), 'the same vendor/name twice must reuse one Package'); + $this->assertCount(1, $this->packageVersions()); + } + + public function testSamePackageWithTwoVersionsCreatesTwoVersions(): void + { + $this->factory()->setPackageVersions(new Installation(), [ + self::installed('acme/foo', '1.0.0'), + self::installed('acme/foo', '2.0.0'), + ]); + + $this->assertCount(1, $this->packages()); + $this->assertCount(2, $this->packageVersions(), 'distinct versions of one package are distinct rows'); + } + + /** + * Two brand-new packages sharing a version string must not collapse into one + * PackageVersion. Keying the buffer on object identity is what keeps them + * apart — neither package has an id yet, because nothing has been flushed. + */ + public function testTwoNewPackagesSharingAVersionStringStayApart(): void + { + $this->factory()->setPackageVersions(new Installation(), [ + self::installed('acme/foo', '1.0.0'), + self::installed('acme/bar', '1.0.0'), + ]); + + $this->assertCount(2, $this->packages()); + $this->assertCount(2, $this->packageVersions(), 'a shared version string must not merge two packages'); + } + + /** + * The point of making the service stateless: a second call cannot reuse the + * first call's entities, even on the same instance. + */ + public function testNothingCarriesOverBetweenCalls(): void + { + $factory = $this->factory(); + + $factory->setPackageVersions(new Installation(), [self::installed('acme/foo', '1.0.0')]); + $factory->setPackageVersions(new Installation(), [self::installed('acme/foo', '1.0.0')]); + + $packages = array_values($this->packages()); + $this->assertCount(2, $packages, 'each call starts from an empty buffer'); + $this->assertNotSame($packages[0], $packages[1]); + } + + /** + * The buffers used to be cleared after flush() rather than in a finally, so + * a failing flush left them populated for the next call — holding entities + * attached to an EntityManager that had since closed. Locals cannot do that. + */ + public function testAFailedFlushLeavesNothingForTheNextCall(): void + { + $factory = $this->factory(throwOnFirstFlush: true); + + try { + $factory->setPackageVersions(new Installation(), [self::installed('acme/foo', '1.0.0')]); + $this->fail('expected the failing flush to throw'); + } catch (\RuntimeException) { + // Expected. What matters is the state left behind. + } + + $this->persisted = []; + $factory->setPackageVersions(new Installation(), [self::installed('acme/foo', '1.0.0')]); + + $this->assertCount(1, $this->packages(), 'the package is created afresh, not taken from a stale buffer'); + } + + private function factory(bool $throwOnFirstFlush = false): PackageVersionFactory + { + $entityManager = $this->createStub(EntityManagerInterface::class); + $entityManager->method('persist')->willReturnCallback( + function (object $entity): void { + // Doctrine assigns the ULID during persist(), because + // UlidGenerator is a CUSTOM rather than a post-insert + // generator. The stub does the same, so entities here are in + // the state the factory actually meets them in. + if ($entity instanceof AbstractBaseEntity) { + $entity->setId(new Ulid()); + } + + $this->persisted[] = $entity; + } + ); + + $flushes = 0; + $entityManager->method('flush')->willReturnCallback( + function () use ($throwOnFirstFlush, &$flushes): void { + if ($throwOnFirstFlush && 0 === $flushes++) { + throw new \RuntimeException('flush failed'); + } + } + ); + + // Nothing is in the database, so every lookup misses and the factory is + // forced onto its in-call buffers — which is what we want to exercise. + $packageRepository = $this->createStub(PackageRepository::class); + $packageRepository->method('findOneBy')->willReturn(null); + $packageVersionRepository = $this->createStub(PackageVersionRepository::class); + $packageVersionRepository->method('findOneBy')->willReturn(null); + + return new PackageVersionFactory($entityManager, $packageRepository, $packageVersionRepository); + } + + private static function installed(string $name, string $version): object + { + return (object) [ + 'name' => $name, + 'version' => $version, + 'description' => 'A package.', + ]; + } + + /** @return array */ + private function packages(): array + { + return array_filter($this->persisted, static fn (object $e): bool => $e instanceof Package); + } + + /** @return array */ + private function packageVersions(): array + { + return array_filter($this->persisted, static fn (object $e): bool => $e instanceof PackageVersion); + } +}