Harden reflection, configuration, and container lifecycle semantics#435
Merged
Conversation
Make ClassMetadataCache the single worker-lifetime owner for cached class and method reflection metadata. Route the existing Reflection package, container, and DI consumers through that canonical cache while preserving object and class-string inputs. Remove the duplicate container and DI reflection managers, their dead tests and APIs, and the redundant test-state resets. Declare DI's direct reflection dependency so subtree installs retain the same valid dependency graph.
Inspect the closure's actual first parameter instead of skipping an invalid first parameter and inferring behavior from a later typed parameter. Preserve valid class unions while giving distinct errors for missing parameters and missing usable type hints. Reject unresolved parent return types alongside self and static so container and lazy-proxy consumers fall back to the declared parameter type instead of receiving an invalid abstraction name. Add regressions for each failure shape and the supported union path.
Validate callable arrays as an exact two-member tuple before either syntax-only or runtime inspection. Malformed arrays now return false instead of reaching class_exists with an invalid receiver or accepting trailing members. Narrow class metadata targets and helper return types to their reachable values, use the canonical metadata cache, and add conditional return documentation. Preserve Laravel's intentional magic-dispatch and class-string callable semantics with focused regression coverage.
Align the lazy and proxy helpers with the canonical reflection cache and accurately document their overloads, exception surface, and two-argument eager callback. Retain the lightweight function-local Reflector instance used by repeated helper calls. Add comprehensive runtime coverage for ghost and proxy initialization, constructor argument arrays, direct initialization, eager properties, callback arguments, error propagation, and relative return-type fallback.
Exercise PHPStan inference for explicit and closure-derived lazy and proxy classes, both eager callback arguments, and object or class-string metadata targets. Cover conditional attribute lookup returns so future annotation or overload regressions fail the dedicated type-analysis suite rather than silently weakening application inference.
Add the missing public guide for creating lazy ghost objects and lazy proxy objects, including class inference, direct initialization, constructor argument arrays, and eager properties. Explain the semantic difference between populating the original ghost and returning a wrapped proxy, and document both arguments supplied to proxy eager callbacks.
Declare the ReflectionException paths exposed while registering Eloquent mixins, constructing queued event listeners, and resolving route signature parameters. These annotations mirror the current upstream contract and let callers and static analysis see the real failure boundary without changing runtime behavior.
Capture the verified Reflection findings, accepted implementation boundaries, regression coverage, performance assessment, completed validation, independent review, and owner pre-commit approval. Mark Reflection complete in the framework-wide checklist and route the next audit cycle to Config while preserving all cross-package revalidation references.
Make Laravel's current public APIs, configuration, documented behavior, and conventional extension patterns the explicit default compatibility contract. Limit greenfield freedom to Hypervel-specific internals and directly upstream-deprecated surfaces, while allowing exceptional owner-approved divergences only for meaningful practical benefits. Require evidence-backed improvements to be surfaced as approval gates, reject style-only churn and performance noise, and clarify that deliberate lifecycle bypasses do not require parallel enforcement machinery. Balance that restraint with an explicit requirement to fix verified defects completely at their lowest owning boundary.
Replace the public single-callback mutation hook with narrow internal observer and whole-state replacement operations used by Foundation. This prevents external consumers from replacing framework tracking while preserving lock-free configuration reads. Normalize integer ArrayAccess keys, restore typed getter and collection exception documentation, remove the obsolete facade and contract surface, and drop the unused split-package Container dependency. Add focused repository regressions for observer behavior, silent replacement, and positive and negative integer keys.
Rebuild configuration against a temporary repository and copy the completed state into the existing repository only after bootstrap succeeds. Long-lived services therefore keep a current Config reference, while failed rebuilds restore the prior binding and preserve the original exception. Introduce a Foundation-owned mutation tracker that attaches at successful bootstrap, records boot-time mutation batches in exact order, and seals before one-time worker replay. Remove the listener static registry and its test cleanup, and add regressions for early mutations, parent and child write ordering, sibling preservation, stable identity, replay sealing, and failed rebuild rollback.
Drop the eager reload-listener resolution that Reverb previously needed to begin recording configuration writes. Foundation now attaches mutation tracking at the successful configuration bootstrap boundary, so package boot mutations are captured automatically and the fixture no longer needs ordering-dependent setup.
Document the verified Config and Foundation defects, final identity-preserving reload architecture, rejected complexity, regression coverage, performance impact, Laravel-facing result, validation, and review outcome. Mark Config complete, record the cross-package assumptions that Foundation, Testing, and Reverb must retain, and route the next audit work unit to Container with its required Reflection findings.
Coordinate cacheable first singleton and auto-singleton construction so concurrent coroutines converge on one fully resolved instance. Propagate owner failures, allow retry, reject coordinator-induced cycles, and prevent provisional resolving-callback state from escaping. Centralize binding lifecycle invalidation and correct null instance handling, callback rollback, scoped replacement, auto-singleton extension, alias cycles, resolution-depth cleanup, variadic dependencies, conditional Closure bindings, and closure diagnostics. Preserve fresh PHP object-default semantics in cached parameter recipes while retaining the fast path for immutable defaults. Declare the existing buildWith behavior on the Container contract and App facade, and accurately narrow contextual binding identifiers to strings.
Add focused coverage for lifecycle transitions across transient, singleton, scoped, explicit-instance, and auto-singleton registrations. Verify cache invalidation, null-valued instances, extenders, offset removal, and scoped restoration after temporary replacement. Reproduce indirect alias cycles, exact resolution-depth cleanup, concrete and contextual variadics, conditional Closure registration, closure resolution diagnostics, and fresh object-valued constructor defaults while preserving cached immutable defaults.
Exercise deterministic contention for explicit singletons and eligible auto-singletons, proving one completed instance is returned only after resolving callbacks finish. Cover owner failure fan-out and retry, descendant re-entry, multi-owner wait cycles, and canceled-waiter cleanup. Bound every failure path so a coordinator regression fails descriptively instead of hanging a test worker.
Verify failed resolving callbacks roll back only the exact singleton, scoped instance, or auto-singleton provisionally published by the failed resolution. Callback-installed replacement instances remain authoritative. Cover both coroutine and non-coroutine resolution paths, including retry behavior and cleanup of the original failure without masking it.
Build callable parameter recipes through the same canonical metadata path as constructors, preserving declaring-class diagnostics and contextual attribute parameter metadata without duplicating reflection parsing. Keep the zero-argument shortcut limited to truly anonymous closures so class-bound first-class callables retain their contextual build stack. Add callable regressions and complete the affected scheduling fixture signatures.
Accept unit and backed enum guard identifiers at the Auth manager boundary while preserving existing string and null behavior. Normalize integer-backed zero without allowing falsey values to fall through to defaults. Align the Auth contract and facade metadata with the current Laravel-facing API and cover middleware guard selection for string-, integer-, and unit-enum identifiers.
Normalize unit and backed enum store identifiers consistently across resolution, memoization, forgetting, purging, and driver access. Preserve integer-backed zero as a valid identifier. Replace the falsey default-store fallback with null-only selection so the valid configured store name '0' is not silently discarded. Update the Cache contract and facade metadata and add coverage for every manager path.
Add typed named-logger resolution for Monolog-backed channels without leaking the underlying Monolog instance past Hypervel's Logger wrapper. Clone the wrapper, replace only its Monolog backend, preserve event dispatch, and share the original channel-family coroutine context while keeping separate channels isolated. Normalize enum channel identifiers and reject unsupported non-Monolog named drivers clearly. Add regressions for logger identity, names, events, shared and isolated context, enum identifiers, and failure behavior.
Pass the active ReflectionParameter through container, method, and route attribute resolution so contextual attributes can use the parameter name and type without repeating reflection. Port current route-name inference, memoized cache selection, named logging, and enum identifier support across Auth, Authenticated, Cache, Log, RouteParameter, and Storage. Preserve explicit route names and existing string call shapes. Add comprehensive attribute and route regressions, including custom parameter-aware attributes and cached route reflection metadata.
Document inferred route parameters, request-scoped memo cache injection, named loggers, and enum-backed contextual identifiers. Update the custom contextual attribute example to receive ReflectionParameter metadata and explain when applications can use it to inspect the destination parameter.
Capture the verified Container findings, accepted boundaries, rejected speculative machinery, implemented corrections, regression coverage, performance impact, and completed validation and review. Mark Container complete, record the cross-package assumptions that later audits must retain, and route the next work unit to Context with its required Container lifecycle revalidation.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR consolidates reflection metadata, preserves configuration repository identity across worker reloads, adds coroutine-coordinated container resolution, expands enum-aware Auth/Cache/Log/Storage APIs and contextual attributes, introduces ChangesFramework lifecycle and container compatibility
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Worker
participant LoadConfiguration
participant Repository
participant ConfigMutationTracker
Worker->>LoadConfiguration: bootstrap()
LoadConfiguration->>Repository: build configuration
LoadConfiguration->>ConfigMutationTracker: observe(repository)
Repository->>ConfigMutationTracker: record ordered mutation
LoadConfiguration->>Repository: replaceItems() on stable repository
Worker->>ConfigMutationTracker: replay(repository)
ConfigMutationTracker->>Repository: apply mutations
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR consolidates reflection metadata ownership into
Confidence Score: 5/5
Important Files Changed
Reviews (2): Last reviewed commit: "Clarify configuration lifecycle boundari..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/reflection/src/ClassMetadataCache.php (1)
234-239: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a
Tests only.warning to this public mutator of worker-lifetime state.As per coding guidelines, public mutators of worker-lifetime state that are not intended for normal runtime use must have a
Boot-only.,Tests only., orBoot or tests only.warning in their docblock, naming the concrete failure mode in the second sentence.📝 Proposed fix
/** * Flush all static state. + * + * Tests only. Flushing state during runtime degrades performance and causes cache misses. */ public static function flushState(): void { static::$classes = []; static::$methods = [];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/reflection/src/ClassMetadataCache.php` around lines 234 - 239, Update the docblock for ClassMetadataCache::flushState() to add a “Tests only.” warning and, in its second sentence, state the concrete failure mode caused by clearing worker-lifetime static state during normal runtime use.Source: Coding guidelines
🧹 Nitpick comments (5)
tests/Support/ReflectionHelpersTest.php (1)
308-320: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
testProxyduplicatestestProxyCanAcceptShortClosureinstead of testing a full-closure callback.Both tests use the exact same short closure (
fn (ReflectionHelperLazyClass $proxy) => $factory()). Compare with thelazy()pair (testLazyfull closure vs.testLazyCanAcceptShortClosureshort closure) and thetestClosureOnlyProxy/testClosureOnlyProxyCanAcceptShortClosurepair, which both correctly differentiate full vs. short closure syntax. As written,proxy()with an explicit class string plus a regularfunctioncallback is untested, and this test adds no coverage beyond its sibling.♻️ Proposed fix to restore distinct coverage
public function testProxy(): void { $factory = fn () => new ReflectionHelperLazyClass('foo', 'bar'); $instance = proxy( ReflectionHelperLazyClass::class, - fn (ReflectionHelperLazyClass $proxy) => $factory() + function (ReflectionHelperLazyClass $proxy) use ($factory) { + return $factory(); + } );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Support/ReflectionHelpersTest.php` around lines 308 - 320, Update testProxy to use a regular function callback with the explicit ReflectionHelperLazyClass parameter, while retaining the existing factory behavior and assertions. Keep testProxyCanAcceptShortClosure as the short-closure counterpart so proxy() coverage distinguishes both callback syntaxes.src/foundation/src/Bootstrap/LoadConfiguration.php (1)
29-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider documenting the new rollback/identity-preservation contract on
bootstrap().The method's docblock is still a one-line title, but it now encodes a non-obvious contract: on failure it restores the previous repository binding and rethrows; on success it preserves the previous repository's identity (via
replaceItems()) so long-lived consumers keep observing the same object; and it wires the config mutation tracker for later replay. Based on learnings, this repository treats extended method-level PHPDoc as acceptable (and expected) for complex, non-obvious lifecycle contracts.📝 Proposed docblock expansion
/** * Bootstrap the given application. + * + * On failure, the previously bound config repository (if any) is restored + * before the exception is rethrown. On success, if a previous repository + * existed, its contents are replaced in place to preserve its identity for + * long-lived consumers, and mutation replay is (re)armed via + * ConfigMutationTracker. */ public function bootstrap(Application $app): void🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/foundation/src/Bootstrap/LoadConfiguration.php` around lines 29 - 30, Expand the PHPDoc for Bootstrap\LoadConfiguration::bootstrap() to document its lifecycle contract: restore the previous repository binding and rethrow on failure, preserve the previous repository instance identity via replaceItems() on success, and wire the configuration mutation tracker for later replay.Source: Learnings
src/container/src/SharedResolution.php (1)
52-73: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a bounded wait to avoid indefinite coroutine hangs.
await()'s$this->signal->pop()has no timeout. Throwable failures in the owner'sbuild()are already handled byContainer::resolve()'scatch/fail()path, but a non-Throwable termination (exit(), OOM, or any path that skipsresolve()'sfinally) would leave thisSharedResolutionunsettled forever, permanently blocking every waiting coroutine on this abstract.🛡️ Proposed direction
- public function await(): mixed + public function await(float $timeout = 30.0): mixed { if (! $this->settled) { - $this->signal->pop(); + $this->signal->pop($timeout); } if (! $this->settled) { throw new BindingResolutionException('Shared container resolution was interrupted before completion.'); }Since this relies on Swoole's channel/timeout semantics, could you confirm whether the framework already guarantees
complete()/fail()always run for every construction failure mode (including worker-level fatal terminations) in this deployment model?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/container/src/SharedResolution.php` around lines 52 - 73, Update SharedResolution::await() to use a bounded timeout when waiting on $this->signal, and treat timeout or interrupted waits as an unsettled resolution by throwing BindingResolutionException. Preserve the existing failure rethrow and value return behavior after completion, and use the framework’s established Swoole channel timeout semantics where available.tests/Auth/AuthenticateMiddlewareTest.php (1)
173-211: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a plain (non-backed)
UnitEnumregression test.New tests only cover string- and int-backed enums.
AuthManager::guard()/shouldUse()/setDefaultDriver()also accept a plainUnitEnum, which falls throughenum_value()'s->namebranch — a different code path not exercised here (only exercised indirectly via a mocked manager inContextualAttributeBindingTest).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Auth/AuthenticateMiddlewareTest.php` around lines 173 - 211, Add regression coverage for plain UnitEnum values, not only backed enums, in the AuthManager tests. Define or reuse a non-backed enum and verify AuthManager::guard(), shouldUse(), and setDefaultDriver() resolve its case name correctly, including guard identity where applicable; keep the existing backed-enum tests unchanged.src/auth/src/AuthManager.php (1)
65-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a shared
UnitEnum→string normalization helper instead of repeating it 5x across two managers. BothAuthManagerandLogManagerindependently repeatif ($name instanceof UnitEnum) { $name = (string) enum_value($name); }. A small shared trait (e.g.Hypervel\Support\Concerns\NormalizesEnumIdentifiers) would centralize this and reduce drift risk as more managers (e.g.CacheManager) adopt the sameUnitEnumsupport.
src/auth/src/AuthManager.php#L65-L70: replace the inline check inguard()with the shared helper.src/auth/src/AuthManager.php#L187-L192: replace the inline check inshouldUse()with the shared helper.src/auth/src/AuthManager.php#L205-L210: replace the inline check insetDefaultDriver()with the shared helper.src/log/src/LogManager.php#L112-L117: replace the inline check indriver()with the shared helper.src/log/src/LogManager.php#L542-L547: replace the inline check insetDefaultDriver()with the shared helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/auth/src/AuthManager.php` around lines 65 - 70, Extract the repeated UnitEnum-to-string conversion into a shared NormalizesEnumIdentifiers trait and reuse it in AuthManager and LogManager. Update AuthManager::guard(), AuthManager::shouldUse(), and AuthManager::setDefaultDriver(), plus LogManager::driver() and LogManager::setDefaultDriver(), to call the helper instead of their inline checks; apply the changes at src/auth/src/AuthManager.php ranges 65-70, 187-192, and 205-210, and src/log/src/LogManager.php ranges 112-117 and 542-547.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/plans/2026-07-12-framework-coroutine-state-lifecycle-audit-ledger.md`:
- Around line 246-249: Add a “Laravel-facing result” ledger item to the
reflection work unit, using an approved classification such as “Public API,
configuration, and documented behavior unchanged.” Keep it alongside the
existing Implementation, Regression tests, Performance and complexity, and
Validation and review entries.
In `@docs/plans/2026-07-12-framework-coroutine-state-lifecycle-audit.md`:
- Around line 1008-1018: The dependency index still describes completed
container and config audits as pending. Update the affected entries around
container-05, container-06, container-08, container-09, container-10, config-01,
and config-02 to remove container/config from “later full” or pending consumer
audit descriptions, unless an outstanding revalidation is explicitly documented.
- Around line 1045-1047: Revert the `config` checklist entry to unchecked in the
framework coroutine state lifecycle audit, and leave it unchecked until the
documented owner approval and final bookkeeping commit are complete; keep the
`reflection` and `container` entries unchanged.
In `@src/foundation/src/Configuration/ConfigMutationTracker.php`:
- Around line 26-48: Add “Boot-only.” warnings to the public `observe()` and
`replay()` docblocks. For `observe()`, state that calls outside
`LoadConfiguration::bootstrap()` silently hijack the mutation channel; for
`replay()`, state that calls outside `ReloadDotenvAndConfig::reloadConfig()`
permanently stop recording for the tracker’s lifetime.
---
Outside diff comments:
In `@src/reflection/src/ClassMetadataCache.php`:
- Around line 234-239: Update the docblock for ClassMetadataCache::flushState()
to add a “Tests only.” warning and, in its second sentence, state the concrete
failure mode caused by clearing worker-lifetime static state during normal
runtime use.
---
Nitpick comments:
In `@src/auth/src/AuthManager.php`:
- Around line 65-70: Extract the repeated UnitEnum-to-string conversion into a
shared NormalizesEnumIdentifiers trait and reuse it in AuthManager and
LogManager. Update AuthManager::guard(), AuthManager::shouldUse(), and
AuthManager::setDefaultDriver(), plus LogManager::driver() and
LogManager::setDefaultDriver(), to call the helper instead of their inline
checks; apply the changes at src/auth/src/AuthManager.php ranges 65-70, 187-192,
and 205-210, and src/log/src/LogManager.php ranges 112-117 and 542-547.
In `@src/container/src/SharedResolution.php`:
- Around line 52-73: Update SharedResolution::await() to use a bounded timeout
when waiting on $this->signal, and treat timeout or interrupted waits as an
unsettled resolution by throwing BindingResolutionException. Preserve the
existing failure rethrow and value return behavior after completion, and use the
framework’s established Swoole channel timeout semantics where available.
In `@src/foundation/src/Bootstrap/LoadConfiguration.php`:
- Around line 29-30: Expand the PHPDoc for
Bootstrap\LoadConfiguration::bootstrap() to document its lifecycle contract:
restore the previous repository binding and rethrow on failure, preserve the
previous repository instance identity via replaceItems() on success, and wire
the configuration mutation tracker for later replay.
In `@tests/Auth/AuthenticateMiddlewareTest.php`:
- Around line 173-211: Add regression coverage for plain UnitEnum values, not
only backed enums, in the AuthManager tests. Define or reuse a non-backed enum
and verify AuthManager::guard(), shouldUse(), and setDefaultDriver() resolve its
case name correctly, including guard identity where applicable; keep the
existing backed-enum tests unchanged.
In `@tests/Support/ReflectionHelpersTest.php`:
- Around line 308-320: Update testProxy to use a regular function callback with
the explicit ReflectionHelperLazyClass parameter, while retaining the existing
factory behavior and assertions. Keep testProxyCanAcceptShortClosure as the
short-closure counterpart so proxy() coverage distinguishes both callback
syntaxes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 091e7abd-822e-4512-a70b-7b8303ce84d2
📒 Files selected for processing (72)
docs/plans/2026-07-12-framework-coroutine-state-lifecycle-audit-ledger.mddocs/plans/2026-07-12-framework-coroutine-state-lifecycle-audit.mdsrc/auth/src/AuthManager.phpsrc/boost/docs/container.mdsrc/boost/docs/helpers.mdsrc/cache/src/CacheManager.phpsrc/config/composer.jsonsrc/config/src/Repository.phpsrc/container/composer.jsonsrc/container/src/Attributes/Auth.phpsrc/container/src/Attributes/Authenticated.phpsrc/container/src/Attributes/Cache.phpsrc/container/src/Attributes/Log.phpsrc/container/src/Attributes/RouteParameter.phpsrc/container/src/Attributes/Storage.phpsrc/container/src/BoundMethod.phpsrc/container/src/BuildRecipe.phpsrc/container/src/Container.phpsrc/container/src/ParameterRecipe.phpsrc/container/src/ReflectionManager.phpsrc/container/src/SharedResolution.phpsrc/contracts/src/Auth/Factory.phpsrc/contracts/src/Cache/Factory.phpsrc/contracts/src/Config/Repository.phpsrc/contracts/src/Container/Container.phpsrc/database/src/Eloquent/Builder.phpsrc/di/composer.jsonsrc/di/src/Aop/ProceedingJoinPoint.phpsrc/di/src/ReflectionManager.phpsrc/events/src/Dispatcher.phpsrc/foundation/src/Bootstrap/LoadConfiguration.phpsrc/foundation/src/Configuration/ConfigMutationTracker.phpsrc/foundation/src/Listeners/ReloadDotenvAndConfig.phpsrc/log/src/LogManager.phpsrc/log/src/Logger.phpsrc/reflection/src/ClassInvoker.phpsrc/reflection/src/ClassMetadataCache.phpsrc/reflection/src/Reflector.phpsrc/reflection/src/Traits/ReflectsClosures.phpsrc/reflection/src/helpers.phpsrc/routing/src/ResolvesRouteDependencies.phpsrc/routing/src/RouteSignatureParameters.phpsrc/support/src/Facades/App.phpsrc/support/src/Facades/Auth.phpsrc/support/src/Facades/Cache.phpsrc/support/src/Facades/Config.phpsrc/support/src/Facades/Log.phpsrc/testing/src/PHPUnit/AfterEachTestSubscriber.phptests/Auth/AuthenticateMiddlewareTest.phptests/Cache/CacheManagerTest.phptests/Config/RepositoryTest.phptests/Container/ContainerCallTest.phptests/Container/ContainerTest.phptests/Container/ContextualAttributeBindingTest.phptests/Container/CoroutineSafetyTest.phptests/Container/ReflectionManagerTest.phptests/Container/ResolvingCallbackNonCoroutineTest.phptests/Container/ResolvingCallbackTest.phptests/Foundation/Bootstrap/LoadConfigurationTest.phptests/Foundation/Listeners/ReloadDotenvAndConfigTest.phptests/Integration/Console/CallbackSchedulingTest.phptests/Integration/Console/CommandSchedulingTest.phptests/Integration/Console/Scheduling/SubMinuteSchedulingTest.phptests/Integration/Reverb/server.phptests/Log/LogLoggerTest.phptests/Log/LogManagerTest.phptests/Routing/ReflectionParameterCachingTest.phptests/Support/ClassMetadataCacheTest.phptests/Support/ReflectionHelpersTest.phptests/Support/SupportReflectorTest.phptests/Support/SupportReflectsClosuresTest.phptypes/Reflection/helpers.php
💤 Files with no reviewable changes (5)
- tests/Container/ReflectionManagerTest.php
- src/di/src/ReflectionManager.php
- src/container/src/ReflectionManager.php
- src/support/src/Facades/Config.php
- src/testing/src/PHPUnit/AfterEachTestSubscriber.php
Document the boot-only ownership of configuration mutation observation and replay, and explain how configuration reloads preserve the repository retained by long-lived services.\n\nComplete the Reflection audit's Laravel-facing result, record Config's completed owner approval, and prune dependency-index entries whose consumer revalidation is already complete. This keeps the audit routing accurate without changing runtime behavior.
Collaborator
Author
|
@greptileai @coderabbitai — review follow-up is in f954a4b. Changes made:
The runtime suggestions were investigated but deliberately not applied:
The remaining outside-diff and nitpick suggestions do not add useful coverage or correctness:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
git log --oneline -1 f954a4bee 2>/dev/null || echo "commit not found in shallow history"
git show --stat f954a4bee 2>/dev/null | head -50Length of output: 1342
Your rationale for the declined items is sound:
And the nitpicks/outside-diff items (docblock convention, Nothing further from me on these threads. |
|
The follow-up is clean and the justifications all hold up. Documentation changes (applied): The Runtime suggestions (not applied): All five rationales are correct:
The audit bookkeeping (Reflection result, Config approval, dependency-index pruning) is also consistent — marking completed revalidations as done rather than leaving them as false pending items is the right hygiene. No further changes needed from my side. LGTM. |
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.
Summary
This PR hardens three pieces of worker-lifetime infrastructure: reflection metadata, configuration reloads, and container resolution. The common problem is ownership. Metadata must have one stable cache owner, long-lived services must retain one configuration repository, and shared container entries must publish one complete result under concurrent first use.
The changes fix concrete correctness failures while preserving the existing Swoole performance model. Immutable reflection recipes remain statically cached, configuration reads remain lock-free, and warmed container resolution remains cache-first. Coordination and fresh reflection are restricted to the cold or semantically dynamic paths that require them.
For more details, see: docs/plans/2026-07-12-framework-coroutine-state-lifecycle-audit-ledger.md
Reflection metadata
Reflection metadata previously had overlapping worker-static owners in the Reflection, Container, and DI packages. That made invalidation and behavior easy to drift while retaining APIs that no caller needed.
This PR makes
ClassMetadataCachethe canonical class and method metadata owner and removes the duplicate Container and DI managers. It also corrects closure parameter inference, malformed callable-array handling, and relative return-type rejection. The current lazy and proxy helper behavior, PHPStan inference surface, failure contracts, and user documentation are included.The result is a net deletion in the metadata layer with one cache owner and no new synchronization on reflection reads.
Configuration identity
Configuration reloads previously replaced the repository object. Services that had already captured the original repository continued reading stale data after a worker reload. Boot-time mutations were also recorded too late and flattened by key, which lost writes made before listener resolution and changed parent/child write ordering.
Reload now evaluates configuration against a temporary binding and copies a successful result into the existing repository. A Foundation-owned tracker records boot mutations in exact order, replays them once, and then seals itself for the worker lifetime. Failed rebuilds restore the prior binding and preserve the original exception.
This also fixes integer ArrayAccess keys, removes the obsolete Reverb eager-resolution workaround, and replaces the former public Hypervel-specific setting callback with the narrower internal hooks that own the behavior.
Container resolution and lifecycle
Concurrent yielding first resolutions could construct different instances for the same singleton or expose an instance before resolving callbacks completed. The container now coordinates only cacheable first construction for explicit and eligible automatic singletons. Waiters receive the same completed value or failure, failed owners can be retried, and ancestry and wait cycles introduced by coordination fail deterministically instead of hanging.
The normal resolution path remains unchanged in shape: warmed instances use the existing caches and pay only one predictable empty-map check. Channels, ancestry traversal, graph checks, and yielding occur only during contended first construction.
The Container work also corrects:
issetchecks;Error;buildWith()is now represented by the Hypervel Container contract and App facade.addContextualBinding()now accurately accepts the string identifiers its implementation supports instead of advertising an unusable Closure form.Contextual attributes and managers
Contextual attribute resolution now receives the active
ReflectionParameterthrough constructor, method, and route call paths. This enables current Laravel behavior for inferred route parameter names while allowing custom attributes to inspect the destination parameter without repeating reflection.The existing contextual surface is completed with request-scoped memo cache injection, named loggers, and enum identifiers for Auth, Authenticated, Cache, Log, and Storage. Auth, Cache, and Log manager boundaries normalize unit and backed enums consistently, including integer-backed zero. Cache also preserves the valid store name
0instead of replacing it through a falsey fallback.Named loggers retain the Hypervel Logger wrapper rather than leaking the underlying Monolog instance. They continue to dispatch events and share the original channel-family coroutine context while separate channels remain isolated.
Compatibility
Current Laravel-facing container and contextual attribute usage remains compatible. Existing string identifiers, binding APIs, configuration structure, and documented behavior continue to work. New behavior either completes current Laravel APIs, exposes behavior the concrete Hypervel container already had, or removes call shapes that could never execute successfully.
The Swoole-specific architecture remains intact: automatic singletons, scoped coroutine storage, immutable static recipes, and manager caches are preserved.
Performance
The steady-state additions are intentionally small:
array_key_existscheck only after anissetmiss;Shared-resolution allocation and synchronization are cold-path work. Fresh ReflectionParameter access is retained only for contextual attributes and defaults that recursively contain objects; scalar, enum, and object-free defaults continue using cached recipes.
Validation
The affected Reflection, Config, Foundation, Container, Auth, Cache, Log, Routing, Console, Testing, Reverb, DI, Events, Database, and Support behavior has focused regression coverage. The complete formatter, static-analysis, parallel test, Testbench contract, and Testbench dogfood gates pass.
Summary by CodeRabbit
buildWith()for constructing instances with parameter overrides.lazy()andproxy()helper behavior.lazy()/proxy()usage details.