Share container-cached readonly services via #[ScopeShared]; exercise the suite on PHP 8.1 - #327
Open
roxblnfk wants to merge 13 commits into
Open
Share container-cached readonly services via #[ScopeShared]; exercise the suite on PHP 8.1#327roxblnfk wants to merge 13 commits into
roxblnfk wants to merge 13 commits into
Conversation
…hared] build(deps): require internal/container ^1.1 for the #[ScopeShared] attribute The container already keeps readonly services shared across scope() derivations by reflecting isReadOnly(), so this marks the same classes explicitly with #[ScopeShared]. The attribute states the sharing intent at the class and keeps the behaviour when readonly class is unavailable (PHP 8.1), where the reflection check no longer fires. Only classes that actually land in the container cache are marked, since scope() clones cache entries alone: the autowired runner chain, MessengerHub, ErrorReporter, ConsoleStreams, the ::class-registered interceptors (AttributesInterceptor, InjectInterceptor, AssertCollectorInterceptor, SuiteFilterInterceptor, BenchFinder and its BenchHandler dep), the bound Filter, the set() CoverageCollector, and the ApplicationConfig/RunConfiguration held in the container. make()-resolved services (SuiteLocator, SuiteFactory), new'd instances and attribute-made interceptors never enter the cache, so they are left untouched. The four plugins that now reference the attribute (assert, filter, bench, codecov) gain a direct internal/container ^1.1 requirement they previously pulled in only transitively. Assisted-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…grade The sources stay on 8.2 (they use `readonly class`), so the 8.1 job installs with `--ignore-platform-req=php`, rewrites `readonly class` into per-property `readonly` in place with rector-downgrade.php, and only then runs the suite. `readonly class` is the single 8.2-exclusive construct here; the affected container services keep their cross-scope sharing through the explicit #[ScopeShared] attribute once the class-level `readonly` is gone. 8.1 runs against the lowest dependency set only — the highest set pulls Symfony 8, which needs PHP 8.4 and cannot run on 8.1. The lowest set resolves to Symfony 6.4/5.4 and internal/path 1.3, all 8.1-compatible. Assisted-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
llm/skills registers a Composer plugin whose post-install hook loads code using `readonly class`, so it fatals during `composer install` on PHP 8.1 before the sources can be downgraded. It is dev-only tooling the suite never touches, so the 8.1 job removes it from the requirements before installing. Assisted-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the in-repo rector-downgrade.php step, which ran the project's own Composer-installed Rector and shared the project autoloader — so it eager-loaded the `readonly class` sources and fataled on 8.1 before it could downgrade anything. The php-testo/actions/downgrade-php action installs Rector in isolation and never boots the project autoloader, so the transform runs cleanly. Assisted-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
bridge/symfony-console/resources/stubs/testo.php is a template with syntax Rector can't parse, which aborted the downgrade. Pass it to the action's skip input. Assisted-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…asPrototype() hasPrototype() is a PHP 8.2 method, so on 8.1 (reachable via the downgrade job) the finder fataled with "Call to undefined method". Rector has no rule to rewrite the call, so a private methodPrototype() helper now resolves the overridden parent/interface method directly — parent first, then interfaces — returning null instead of relying on getPrototype()'s throw-on-absence. Behaviour is unchanged on 8.2+ and the self-test suite passes. Assisted-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fix(bench): guard memory_reset_peak_usage() for PHP 8.1 PHP 8.1 mis-fires the "never-returning function must not return" check on arrow functions whose body is a throw expression (fixed in 8.2), so the three `fn(): never => throw ...` helpers become regular closures that throw in a statement — behaviour is identical on 8.2+. memory_reset_peak_usage() is an 8.2 function with no 8.1 fallback and no Rector downgrade, so it is now called only when it exists; on 8.1 the per-iteration peak reset is simply skipped. Assisted-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
hirschbergUsesFarLessMemoryThanTheLcsTable measures per-call peak allocation, which needs memory_reset_peak_usage() (PHP 8.2+) to reset the peak between the two runs. Without it the second measurement reads the peak the first already raised, so the comparison is meaningless — mark the test skipped there rather than assert on noise. Assisted-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The hand-rolled parent-then-interface walk only approximated getPrototype() and could diverge on non-trivial hierarchies. Delegate to getPrototype() — the authoritative resolution, available since well before 8.2 — and just normalise its throw-on-absence to null, keeping the call sites unchanged. Assisted-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
test(output): restore the never-returning arrow-fn helpers The downgrade path now carries Rector rules for both `ReflectionMethod::hasPrototype()` and `fn(): never => throw ...` (rectorphp/rector-downgrade-php#397, #398), so these hand-written 8.1 workarounds are redundant and the source returns to its plain 8.2 form. The memory_reset_peak_usage() guard and the memory-comparison skip stay — no Rector rule covers those. Assisted-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The init command scaffolds testo.php from resources/stubs/testo.php, a template carrying __SRC_PATH__ and __SUITES__ placeholders that is deliberately not valid PHP. Its .php extension made every PHP tool treat it as code: Rector, psalm and php-cs-fixer all had to skip the path, and a downstream downgrade of the shipped package chokes on it. Rename it to testo.php.stub so no PHP tooling ever parses it, and drop the now-redundant Rector skip. Assisted-By: Claude Opus 4.8 (1M context)
Replace the inline downgrade (which lowered only the sources, leaving 8.2-only dependencies such as internal/container to fatal at runtime) with php-internal/actions/install-php. It pins the platform to 8.1, so Composer caps every dependency to its newest 8.1-compatible version and downgrades — via a throwaway Rector — both the sources and any dependency with no 8.1 version, including the path-mapped plugins and bridges. Drop the llm/skills plugin on this leg since the self-test does not need it. Assisted-By: Claude Opus 4.8 (1M context)
php-internal/actions/install-php was renamed to .../downgrade. The action installs with --no-scripts, so dropping llm/skills beforehand to avoid its post-install hooks is no longer needed — remove that step too. Assisted-By: Claude Opus 4.8 (1M context)
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.
🔍 What was changed
#[ScopeShared]and bumpinternal/containerto^1.1. The attribute makes their cross-scope sharing explicit instead of relying on the container reflectingreadonly class— which disappears once the class-levelreadonlyis downgraded away on 8.1.llm/skills, then runsphp-internal/actions/downgrade@v1, which resolves every dependency as if on 8.1 (capping each to its newest 8.1-compatible version, so Symfony falls to 6.x), relieves the packages that have no 8.1 version at all (the path-mappedtesto/*splits,internal/container, theroxblnfk/unpolymetapackage) into a local path repository, and Rector-downgrades both those and the project's owncore plugin bridge tests testo.phpto 8.1 before the suite runs.Why?
readonly class— and the implicit, reflection-based scope-sharing it gives the container — is unavailable. The explicit#[ScopeShared]on the cached services preserves today's sharing behaviour once the class-levelreadonlyis downgraded away.Review notes
scope()clones cache entries alone, sonew'd and attribute-made interceptors (created viainjector->make()) never enter the cache and are left untouched. The marked set is the 16 wired/cached services (runners,MessengerHub,ErrorReporter,ConsoleStreams, the::class-registered interceptors,Filter,CoverageCollector,ApplicationConfig/RunConfiguration), not every readonly class.lowestdependency set only —highestpulls Symfony 8, which needs PHP 8.4.Run testsfor now:core/Common/Reflection.phpcallsReflectionMethod::hasPrototype()(an 8.2 API) andrector ^2.6has no downgrade rule for it yet ([DowngradePhp82] Add DowngradeReflectionMethodHasPrototypeRector rectorphp/rector-downgrade-php#398). Everything before that — resolve, relief of the testo/container/unpoly packages, the Rector downgrade of the sources — runs. It goes green once the rule ships.Checklist
rangeSumbench)