From 013d5610be4ea2ea9d7e5a348bd14322a77b8bd0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotr=20Ha=C5=82as?= Date: Sat, 3 Oct 2026 09:48:59 +0200 Subject: [PATCH] chore(lint): run PHPStan and Rector without the machine's php.ini MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit bin/lint.sh started both tools with the normal `php`, so every php.ini on the machine was loaded into the process that does the analysis. This repository also builds a PHP extension (php-ext/, the legacy `zvec`), and a contributor can have a copy of it installed and enabled in php.ini — the same reason the test suite needs `php -n` (#188). What the tools see then depends on the machine instead of on the working tree, which is how a Rector finding in #246 showed up in CI but not locally. AGENTS.md mentioned `php -n` for the tests only. The tools now run with `PHPRC` pointing at an ini that is generated per run, which replaces the machine's php.ini for the process bin/lint.sh starts and for the ones PHPStan starts for itself: `PHPRC` is an environment variable and is inherited, while `-n` is not — PHPStan restarts with `PHP_BINARY -d ...` and starts parallel workers, both of which load the machine's php.ini again (verified with `ps` while it runs). `php -n` on its own is not usable here: PHPStan needs the Phar class and Rector needs `PhpToken` from the tokenizer extension, and on the CI image those come from conf.d, which `-n` drops — the first push of this branch failed `lint` with `Class "Phar" not found` and `Class "PhpToken" not found`. conf.d is still scanned instead, so extensions installed that way stay available. What the replaced php.ini took with it is put back explicitly: `extension_dir` (on a Homebrew/pecl setup it is set in php.ini while the conf.d entries name their extension without a path, so every one of them would otherwise fail to load), `memory_limit=-1` instead of the built-in 128M, and FFI (`-d extension=ffi` plus `extension=ffi` in the generated ini) only where the current PHP has none in that same environment — phpstan.neon ignores undefined FFI methods and properties, not a missing FFI class, so without FFI the analysis would report errors on src/. The CI lint job now asks setup-php for FFI, so the analysis has it however the runner provides it. No step, level or rule changed, and PHP-CS-Fixer is untouched: it works on the token stream and never resolves the project's classes. Verified locally with the legacy zvec extension loaded: bin/lint.sh and bin/lint.sh --fix pass without a warning, the analysis no longer resolves the extension's classes, and Phar, tokenizer, FFI and xdebug are all still available. tests/test_docs_consistency.phpt passes with the CHANGELOG entry. shellcheck clean. --- .github/workflows/build.yml | 3 +++ AGENTS.md | 45 ++++++++++++++++++++++++++++++++ CHANGELOG.md | 8 ++++++ bin/lint.sh | 52 ++++++++++++++++++++++++++++++++++--- 4 files changed, 105 insertions(+), 3 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index ddde596..3274e8c 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -46,6 +46,9 @@ jobs: with: php-version: '8.4' coverage: none + # bin/lint.sh runs PHPStan and Rector with no php.ini, so the analysis needs + # FFI back explicitly (phpstan.neon analyses FFI\CData); see AGENTS.md. + extensions: ffi tools: composer:v2 - name: Install dependencies diff --git a/AGENTS.md b/AGENTS.md index 9760a10..b5f50e3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -266,6 +266,51 @@ hadolint on Dockerfiles (none today). A missing tool is a failure. Needs `hadolint`. Rector skips are documented in `rector.php`; do not "fix" them (for example `ZVecDocIterator::$collection` is never read on purpose). +PHPStan and Rector are run with the machine's `php.ini` replaced by an empty one, for the +same reason the test suite needs `-n` (#188): this repository also builds a PHP extension +(`php-ext/`, the legacy `zvec`), and a copy of it can be installed locally and enabled in +`php.ini`. Both tools work inside a PHP process, so whatever that process has loaded is +what they see — without the isolation the lint result depends on the machine instead of on +the working tree, which is how a Rector finding in #246 showed up in CI but not locally. + +`php -n` is **not** how this is done, for two reasons: + +- PHPStan needs the `Phar` class and Rector needs `PhpToken` from the tokenizer + extension. On the CI image those come from conf.d inis, which `-n` drops: the first + push of #259 failed `lint` with `Class "Phar" not found` and + `Class "PhpToken" not found`. +- `-n` does not reach the processes PHPStan starts for itself. It restarts with + `PHP_BINARY -d …` and starts parallel workers, and those load the machine's + `php.ini` again — verified with `ps` while it runs. `PHPRC` is an environment + variable, so it is inherited. + +conf.d is still scanned, so an extension installed that way stays available — **as long +as the generated ini carries `extension_dir`**, because those entries name their +extension without a path and on a Homebrew/pecl setup `extension_dir` is set in +`php.ini`. `bin/lint.sh` copies it, which is also what makes the FFI fallback work for a +shared `ffi.so`. + +An `extension=zvec.so` line in `php.ini` — the #188 case of a `make install` plus a manual +line — no longer reaches the tools; one in conf.d still would. + +What `bin/lint.sh` does, and how to repeat it by hand: + +```bash +# The ini PHP is told to load instead of the machine's php.ini. +i="$(mktemp)"; printf 'extension_dir="%s"\n' "$(php -r 'echo ini_get("extension_dir");')" > "$i" + +env PHPRC="$i" php -d memory_limit=-1 vendor/bin/phpstan analyse --no-progress +env PHPRC="$i" php -d memory_limit=-1 vendor/bin/rector process --dry-run +``` + +`-d memory_limit=-1` replaces the built-in 128M, because the replaced `php.ini` is where +the real limit usually is. Where the analysis would have no FFI at all, `extension=ffi` +goes into the generated ini — `phpstan.neon` ignores undefined FFI methods and properties, +not a missing FFI class, so without FFI the analysis reports errors on `src/`. The ini is +what PHPStan's restarted processes load too, which is why FFI is not also passed as `-d`: +PHP would load it twice and warn. The current PHP is asked whether it has FFI in exactly +this environment first, so nothing is loaded twice on any machine. + Setting up a fresh worktree: `bin/worktree.sh ` runs `bin/worktree-setup.sh` (`composer install`, then `./build_zvec.sh` for the FFI adapter the tests load). Nothing binds a host port, so worktrees do not collide. diff --git a/CHANGELOG.md b/CHANGELOG.md index 992df29..ff30f3f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -115,6 +115,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **PHPStan and Rector no longer see the machine's `php.ini`** (#258) + - `bin/lint.sh` starts both with `PHPRC` pointing at an ini generated per run, so a copy of the `zvec` PHP extension installed on the machine — the #188 case of a `make install` plus an `extension=` line — can no longer change what the analysis sees. conf.d is still scanned, so an extension installed that way stays available. + - `php -n` was tried first and does not work here: PHPStan needs the `Phar` class and Rector needs `PhpToken` from the tokenizer extension, which the CI image provides through conf.d (`lint` failed with `Class "Phar" not found` and `Class "PhpToken" not found`), and `-n` does not reach the processes PHPStan starts for itself anyway — it restarts with `PHP_BINARY -d …` and starts parallel workers, both of which load the machine's `php.ini` again. `PHPRC` is an environment variable and is inherited. + - What the replaced `php.ini` took with it is added back explicitly: `extension_dir` (on a Homebrew/pecl setup it is set in `php.ini`, and the conf.d entries name their extension without a path), `memory_limit=-1` instead of the built-in 128M, and FFI (`extension=ffi` in the generated ini, which every one of those processes loads) only where the current PHP has none in that same environment. `phpstan.neon` only ignores undefined FFI methods and properties, not a missing FFI class, so without FFI the analysis would report errors on `src/`. + - No step, level or rule changed. PHP-CS-Fixer is untouched: it works on the token stream and never resolves the project's classes. + - The CI `lint` job now asks for FFI explicitly, so the analysis has it whether the runner provides it compiled in or through an ini. + - `AGENTS.md` documents the flags, and why `php -n` is not the answer here. + - **Repository standard rolled out** (process and tooling, no API change) - `LICENSE` (MIT, Crazy Goat Software), `docs/workflow.md`, `docs/release-workflow.md`, `CONTRIBUTING.md` and `bin/pick-issue.sh`, `bin/worktree.sh`, `bin/worktree-done.sh`, `bin/worktree-setup.sh`. - `bin/lint.sh` (also `composer lint`) runs PHP-CS-Fixer, PHPStan (level 2), Rector, clang-format, shellcheck and hadolint. The PHP sources were reformatted to PSR-12 and the C++ sources with clang-format (whitespace only), plus a few Rector dead-code simplifications. diff --git a/bin/lint.sh b/bin/lint.sh index 3fa9f47..aff3e4a 100755 --- a/bin/lint.sh +++ b/bin/lint.sh @@ -33,15 +33,61 @@ shell_scripts() { done < <(git ls-files -z) } +# Isolation for PHPStan and Rector. Both run inside a PHP process, so they see whatever +# that process has loaded — and this repository also builds a PHP extension (php-ext/, +# the legacy `zvec`), which a contributor can have installed and enabled in php.ini. The +# lint result would then depend on the machine instead of on the working tree, which is +# what happened in #246. +# +# `-n` is not usable here. PHPStan needs the Phar class and Rector needs `PhpToken` from +# the tokenizer extension, and on the CI image those come from conf.d inis, which `-n` +# drops: `lint` failed in #259 with `Class "Phar" not found` and `Class "PhpToken" not +# found`. So instead of dropping every ini, the machine's php.ini is replaced by one that +# is generated per run and holds what the analysis needs. `PHPRC` is an environment +# variable and is inherited, so this also reaches the processes PHPStan starts for itself +# — it restarts with `PHP_BINARY -d ...` and starts parallel workers, which drop `-n`. +# +# What the replaced php.ini took with it has to be put back explicitly: +# - extension_dir: on a Homebrew/pecl setup it is set in php.ini, while the conf.d +# entries name their extension without a path, so without it every extension +# installed that way fails to load. +# - memory_limit: without it the limit is the built-in 128M, far less than what the +# analysis gets today. +# - FFI, but only where the analysis would otherwise have none: phpstan.neon ignores +# undefined FFI methods and properties, not a missing FFI class, so without FFI the +# analysis reports errors on src/. Probed in the same isolated environment the tools +# run in, and then written to the ini — which every one of those processes loads, so +# it is not also passed as `-d`, or PHP would load it twice and warn. +ANALYSIS_INI="$(mktemp "${TMPDIR:-/tmp}/zvec-lint-ini.XXXXXX")" +trap 'rm -f "$ANALYSIS_INI"' EXIT +: >"$ANALYSIS_INI" +php_extension_dir="$(php -r 'echo (string) ini_get("extension_dir");' 2>/dev/null || true)" +# Quoted, so a path with a space is fine; a quote or a newline cannot be expressed in an +# ini value at all, and such a path is left out rather than written out broken. +case "$php_extension_dir" in + *'"'* | *$'\n'*) php_extension_dir="" ;; +esac +if [ -n "$php_extension_dir" ]; then + printf 'extension_dir="%s"\n' "$php_extension_dir" >"$ANALYSIS_INI" +fi +PHP_ANALYSIS_FLAGS=(-d memory_limit=-1) +if ! env PHPRC="$ANALYSIS_INI" php -r 'exit(extension_loaded("FFI") ? 0 : 1);' 2>/dev/null; then + printf 'extension=ffi\n' >>"$ANALYSIS_INI" +fi + +php_analysis() { + env PHPRC="$ANALYSIS_INI" php "${PHP_ANALYSIS_FLAGS[@]}" "$@" +} + if [ "$FIX" = 1 ]; then - vendor/bin/rector process --no-progress-bar || true + php_analysis vendor/bin/rector process --no-progress-bar || true vendor/bin/php-cs-fixer fix || true # exits non-zero when it changed files cpp_sources | xargs -0 -r clang-format -i || true fi step "php-cs-fixer" vendor/bin/php-cs-fixer fix --dry-run --diff -step "phpstan" vendor/bin/phpstan analyse --no-progress -step "rector" vendor/bin/rector process --dry-run --no-progress-bar +step "phpstan" php_analysis vendor/bin/phpstan analyse --no-progress +step "rector" php_analysis vendor/bin/rector process --dry-run --no-progress-bar step "clang-format" bash -c "$(declare -f cpp_sources); cpp_sources | xargs -0 -r clang-format --dry-run --Werror" step "shellcheck" bash -c "$(declare -f shell_scripts); shell_scripts | xargs -0 -r shellcheck" step "hadolint" bash -c "git ls-files -z '*Dockerfile*' | xargs -0 -r hadolint"