Repository navigation
chore: adopt the crazy-goat repository standard - #246
Merged
Merged
Conversation
…NG.md; add composer lint scripts
This was referenced Oct 2, 2026
Open
Open
s2x
added a commit
that referenced
this pull request
Oct 3, 2026
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 and holds nothing, 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 and Rector need `PhpToken` from the tokenizer extension, which the CI image provides through a conf.d ini, and `-n` drops those — the first push of this branch failed `lint` with `Class "PhpToken" not found`. conf.d is still scanned, so extensions installed that way stay available. What the replaced php.ini took with it is added back explicitly: `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, the analysis no longer resolves the extension's classes, tokenizer and FFI are still available, and tests/test_docs_consistency.phpt passes with the CHANGELOG entry. shellcheck clean.
s2x
added a commit
that referenced
this pull request
Oct 3, 2026
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.
s2x
added a commit
that referenced
this pull request
Oct 3, 2026
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.
s2x
added a commit
that referenced
this pull request
Oct 3, 2026
…ni (#259) Closes #258. ## What changed `bin/lint.sh` started PHPStan and Rector 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 `-n` (#188). What the tools see then depends on the machine instead of on the working tree. `AGENTS.md` mentioned `php -n` for the tests only. **This changes what the lint gate runs.** Both tools now run with `PHPRC` pointing at an ini that is generated per run and holds nothing, so the machine's `php.ini` is not read. No step, level or rule changed: - the `phpstan` and `rector` steps, and the `rector` call in the `--fix` branch, go through one `php_analysis` helper; - `memory_limit=-1` replaces the built-in 128M, because the replaced `php.ini` is where the real limit usually is; - `extension_dir` is carried into the generated ini: 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 would fail to load (round 3 of the review caught exactly that, and the resulting xdebug warning); - FFI is added back only where the current PHP has none in that same environment, as `extension=ffi` in the generated ini — every one of those processes loads it, so it is not also passed as `-d`, or PHP would load it twice. `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 asks `setup-php` for FFI, so the analysis has it however the runner provides it; - PHP-CS-Fixer is untouched: it works on the token stream and never resolves the project's classes. ## Why not `php -n` `php -n` was the first attempt and it does not work here, for two separate reasons. 1. **It drops what the tools need.** PHPStan needs the `Phar` class and Rector needs `PhpToken` from the tokenizer extension, which the `ubuntu-24.04` setup-php image provides through conf.d inis. The first push of this branch failed `lint` with `Class "Phar" not found` and `Class "PhpToken" not found` — see the comment below. 2. **It does not reach the processes PHPStan starts for itself.** PHPStan restarts with `PHP_BINARY -d memory_limit=… -d phpstan.restarted=1 …` and starts parallel workers; neither carries `-n`, so those processes loaded the machine's `php.ini` again. Verified with `ps -A` while `php -n vendor/bin/phpstan analyse` runs. `PHPRC` is an environment variable and *is* inherited, so it replaces `php.ini` in the restart and in the workers too. A probe file calling a method only the loaded extension could know confirms it: PHPStan resolves the class without the isolation and does not resolve it with it. conf.d is still scanned, so an extension installed that way (tokenizer on CI, FFI, xdebug) stays available — which is why the generated ini carries the machine's `extension_dir` over: on a Homebrew/pecl setup that is itself set in `php.ini`. 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. That residual gap is stated in `AGENTS.md`. ## What this does not claim The extension does not currently shadow `src/`: `php-ext/` registers its 15 classes in the **global** namespace (`ZVecSchema`, `ZVecDoc`, …) while `src/` is namespaced `CrazyGoat\ZVec\`, so there is no name collision. The real shadowing in #188 is a runtime one — `src/ZVec.php:9` returns early when `extension_loaded('zvec')`, so the barrel file never loads the FFI-only classes — and that is what breaks the *test suite*. PHPStan and Rector parse `src/` and never run that guard. So this is not a fix for an observable wrong result today; it removes the machine dependency that made the #246 finding (a Rector change in CI that the local run did not report) possible, and it produces the same verdict either way on this machine. That is also why no regression test comes with it: there is no assertion that would fail before the change and pass after it. `findings.md` records the rest, including that `composer.lock` is gitignored here, so the local tool versions float and have to be ruled out before blaming an extension. ## Verified - `bin/lint.sh` and `bin/lint.sh --fix` pass on PHP 8.5.10 with the legacy `zvec` extension loaded, and `--fix` changes no file. - Under the isolation: `zvec` not loaded and PHPStan does not resolve the extension's classes, while `Phar`, `tokenizer`, FFI and xdebug are available and `memory_limit` is `-1`; no startup warning is printed. - `tests/test_docs_consistency.phpt` passes with the CHANGELOG entry. - `shellcheck` and `bash -n` clean. - The full `.phpt` suite cannot run in a fresh worktree (it needs `sdk/` and `ffi/build/`, built by `bin/worktree-setup.sh`); the change touches no `src/`, `ffi/` or `tests/` file.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #245
Rolls out the shared crazy-goat standard: docs/workflow.md, docs/release-workflow.md, AGENTS.md, bin/ scripts, bin/lint.sh (PHP-CS-Fixer, Rector, PHPStan level 2, clang-format, shellcheck, hadolint), build.yml with changes/docs/lint jobs and ci-ok, release notes from CHANGELOG, Dependabot.
Formatting commits are behaviour-neutral (PHP compared token-wise; C/C++ differ only in whitespace and macro line continuations).