Harden core utilities, test state, and console compatibility#432
Merged
binaryfire merged 40 commits intoJul 13, 2026
Conversation
Higher-order Conditionable proxies accepted only boolean values even though when() and unless() use normal PHP truthiness. Property and method capture could therefore pass a valid integer, string, array, or object into a strict boolean parameter and fail with a TypeError.\n\nAccept unconstrained condition values at the proxy boundary and normalize them once into the existing typed boolean property. This preserves the Laravel-facing API and makes direct and captured conditions follow the same truthiness rules.
Cover every path affected by the proxy's previously narrow condition parameter. The regressions exercise direct integer conditions, higher-order property capture, higher-order method capture, truthy and falsy values, and the symmetric unless behavior.\n\nThe assertions observe the resulting fluent chain rather than implementation details, so they demonstrate the public behavior Laravel developers expect and fail against the old strict boolean boundary.
Declare void on the eight existing Conditionable test methods that omitted an explicit return type. This aligns both upstream-mirrored test locations with the repository's fully typed test convention without changing their behavior or structure.
The log package uses the Conditionable trait directly but relied on that package arriving transitively through another component. Add the direct split-package requirement so standalone log installations declare everything their source needs.\n\nThis matches the require-what-you-use convention followed by the other Hypervel Conditionable consumers and keeps the requirement in sorted order.
Remote Testbench CLI commands intentionally reuse the current ParaTest worker's disposable application copy. Booting those commands can rewrite bootstrap/cache/packages.php with root package providers, and the cache previously leaked into later tests assigned to the same worker.\n\nSnapshot the resolved manifest path after application setup and restore its exact existence and contents during the shared Testbench teardown lifecycle. Unchanged files are not rewritten, missing files are recreated atomically when required, and failed cleanup participates in the existing exhaustive first-exception-preserving teardown behavior.
Force a remote Testbench command to rebuild the worker-shared package manifest, prove that the child actually changed the cache, and then verify the base lifecycle restores the captured baseline before later teardown assertions run.\n\nThe regression covers both an originally present and absent manifest through the shared restoration contract, and documents the callback and setup ordering on which the observation relies.
The route-cache tests checked source route files and bootstrap configuration but not the raw package manifest consumed by their subprocess. A leaked provider manifest could therefore register package routes and produce a misleading successful cache result.\n\nRead the manifest through the checked filesystem boundary, validate its shape, and report any package providers before running the route assertions. Version-only package metadata remains valid, while route-producing contamination now fails immediately with the responsible provider names.
Route the framework-wide audit to Conditionable and record the verified package architecture, accepted findings, rejected concerns, cross-package implications, implementation, regression coverage, validation, performance impact, and review status.\n\nAlso record the independently exposed Testbench manifest-isolation defect under its owning package and add the Foundation revalidation dependency. The Conditionable checklist remains open until this work is integrated into the audit's 0.4 base, matching the plan's authoritative completion rules.
Make the Testbench subprocess regression exercise manifest restoration deterministically. The child first recreates a deleted manifest, after which the test writes a fixed valid probe and verifies that teardown restores the exact captured baseline.\n\nThis removes the ordering-dependent assumption that a child rebuild must produce bytes different from the original manifest while preserving coverage of the real shared-state lifecycle.
Bind closure macros using the legal instance and class scopes for each magic dispatch path. Static closures now follow Laravel's current behavior, while closures created from internal functions and first-class method callables retain their already-valid callable when PHP refuses rebinding.\n\nSuppress only the native binding warnings whose nullable results are checked immediately. Ordinary methods and non-Closure callables remain unchanged, and no reflection, callable registry, lock, or coroutine-local state is introduced.
Cover static and non-static closures through both instance and static macro dispatch, including protected class-scope access. Add regressions for internal-function closures, bound instance-method callables, static-method callables, and invokable-object controls.\n\nFlush both test fixture registries during teardown so the package's own macro state cannot contaminate later methods, and complete the required void return types while merging the current upstream coverage.
Extend the existing static reset hooks on Eloquent factories, HTTP client responses, and JSON resources to clear their Macroable registries. These classes already participate in centralized framework teardown, so their macros should follow the same test-lifetime boundary as their other mutable static state.\n\nImmutable per-class JSON resource metadata remains cached and untouched; only the mutable macro registries are reset.
Give each framework-owned HTTP class with a Macroable registry the standard flushState boundary. This allows the centralized test lifecycle to clear macros registered on HTTP factories, pending requests, response types, and uploaded files without package-local teardown duplication.\n\nThe hooks are pure static resets used between tests and add no work to request or response hot paths.
Add one authoritative Request flushState hook that clears macros and restores the inherited Symfony configuration still consumed by Hypervel: MIME formats, method-parameter override, allowed override methods, and the request factory.\n\nReset the private Symfony factory slot through its inherited setter while leaving coroutine-local trusted proxy and host state, immutable response metadata, and production request handling untouched. The hook runs only during framework test teardown.
Give the framework-owned notification fake a standard static reset hook for its Macroable registry. Macros installed by one test can otherwise survive for the PHPUnit worker lifetime and affect later notification tests.\n\nThe change is confined to centralized test cleanup and does not alter notification delivery or application runtime behavior.
Add every newly covered framework-owned Macroable class to AfterEachTestSubscriber's authoritative static reset list. This keeps cleanup centralized and exhaustive instead of requiring individual tests to remember which HTTP, request, or notification registries they touched.\n\nApplication and package-owned Macroable classes continue to use the existing test-state registration mechanism rather than being hardcoded into the framework subscriber.
Exercise the real framework reset boundary with macros registered on every previously omitted framework surface, including inherited JsonApi resource storage. Verify that one centralized cleanup clears every registry.\n\nAdd a companion regression that mutates all four inherited Symfony Request configuration surfaces Hypervel still consumes and proves teardown restores their visible behavior and defaults. Both tests retain local finally cleanup so failures cannot contaminate the worker.
Add hypervel/macroable to each split package that directly composes or invokes the trait: cookie, JWT, log, and notifications. This makes standalone subtree installations truthful instead of relying on unrelated transitive dependency paths.\n\nKeep every require block in package order and normalize the touched manifest endings. No application dependency graph changes because the monorepo already provides the same package versions.
Document that framework-owned collection, HTTP client, and response macros are cleared automatically by the test lifecycle. Remove stale instructions that made every test manually flush framework registries.\n\nDirect application and package Macroable classes remain responsible for registering their own cleanup through the documented test-state mechanism, preserving a clear ownership boundary.
Capture the verified callable, static-state, dependency, documentation, and test-isolation findings together with their accepted boundaries, rejected complexity, performance evidence, regression coverage, validation, and review status.\n\nRecord the inherited Request reset as shared http-01 for later HTTP and testing revalidation, mark Macroable complete, and route the next audit slice to Collections with the relevant completed ledger entries. Also retain the owner-selected multi-package branch and pull-request workflow established during this audit.
Align the package manifest with the PHP features and optional integrations the Collections component actually uses. Require the PHP 8.5 polyfill for PHP 8.4 runtimes, remove the unused PHP 8.6 polyfill, and advertise VarDumper for dump-oriented collection helpers. Keep dependency and suggestion ordering canonical so future upstream and Composer metadata reviews see the package's real runtime surface.
Make Traversable-aware first, last, every, and some operations preserve iteration semantics without unsafe materialization, including duplicate keys and short-circuiting. Narrow Arr::push() to the array boundary its nested by-reference dot mutation genuinely requires. Strengthen the generic and conditional PHPDoc for array transforms, random values, sorting, CSS compilation, filters, and wrapping. Add PHPStan fixtures that pin the resulting key, value, list, and conditional inference without adding production overhead.
Cover Traversable first and last selection with duplicate keys, callback short-circuit behavior, default handling, and iterable every and some semantics. These cases reproduce the materialization and key-collision failures fixed in Arr. Pin the truthful array-only Arr::push() signature and complete the test declarations so the suite documents both the runtime behavior and public contract.
Correct enum_value() documentation so its fallback is described as either a zero-argument callable or a value, matching the helper's actual value() evaluation path rather than implying the enum value is passed into the callback. Keep focused enum helper coverage and explicit test return types around native values, backed enums, nulls, and lazy defaults.
Describe value() and when() closures with their real argument and return templates, including conditional return types for known truthy and falsey inputs. Runtime behavior remains unchanged. Add PHPStan fixtures for direct values, closures, defaults, dynamic conditions, and enum fallbacks so future annotation changes preserve the useful inference.
Add reduceInto() to the shared Enumerable surface, pass item keys to sum callbacks, and expose random key preservation through the contract because both eager and lazy implementations support it. Correct factory, aggregate, reducer, sorting, and conditional return types across the interface and shared implementation. Add PHPStan fixtures for the new operation and the strengthened inference so these public contracts remain variance-safe and behaviorally accurate.
Normalize backed enum keys without attempting invalid object string casts, align ArrayAccess parameter names with the inherited interface for named arguments, and describe random, nth, and split results with their real key and failure behavior. Remove Laravel's directly deprecated containsOneItem() and containsManyItems() aliases in favor of hasSole() and hasMany(), retaining an explicit removal marker for future upstream synchronization.
Accept plain Iterator sources wherever the lazy API advertises iterable input, normalize backed enum keys, and track outstanding requested keys so duplicate yields cannot falsely satisfy has(). Preserve keys in random selections when requested and reject a zero range step explicitly. Remove the deprecated item-count aliases, strengthen lazy factory and aggregate types, and use Carbon's direct timestamp accessor. The iterator and membership corrections add no per-item work beyond the operations that require them.
Exercise backed-enum keying, reduceInto(), keyed sum callbacks, and ArrayAccess named arguments through the shared eager and lazy collection provider. Remove tests for the intentionally omitted deprecated aliases. Complete explicit test and fixture return declarations across the shared suite so static analysis can check the full collection behavior surface without changing test semantics.
Cover ArrayIterator and Generator inputs through combine() and zip(), random key preservation, duplicate-key membership, and empty requested key sets. Remove obsolete coverage for the deprecated item-count aliases. Complete explicit return declarations throughout the lazy collection suite while retaining the existing scheduling and enumeration behavior.
Retain the package's operation-by-operation enumeration limits while adding explicit test and helper return declarations. Remove the obsolete laziness assertion for the deprecated containsOneItem() alias and keep hasSole() as the supported behavior. This keeps the suite focused on externally meaningful laziness guarantees after the public cleanup.
Declare the static-state regression test as void so its signature matches the framework test-suite convention. The test continues to verify that Collection and LazyCollection flush both registered macros and higher-order proxy state between tests.
Run the focused Collection tests through Hypervel's framework TestCase rather than the raw PHPUnit base. This gives the file the same lifecycle, cleanup, and coroutine-aware test environment as the rest of the component suite. No collection API behavior changes; this aligns test ownership with repository conventions.
Add explicit void return types to the higher-order proxy behavior tests. Keep the existing property access, method forwarding, and fluent target-return assertions unchanged while making the file fully analyzable under the suite's declaration standards.
Document reduceInto(), keyed sum callbacks, key-preserving random selections, and iterable lazy-collection inputs using the final public API. Remove references to the intentionally omitted deprecated item-count aliases. Keep examples Laravel-shaped and explain the new capabilities at their natural collection method sections rather than exposing internal implementation details.
Document the current enum_value(), value(), and conditional helper behavior, including lazy fallback evaluation and the relevant callable forms. Align the examples with the corrected helper contracts so application and package authors see the same behavior that static analysis now models.
Add a focused Differences From Laravel section for the package's intentional public divergences: omitted deprecated item-count aliases, key preservation on the Enumerable random contract, and Arr::push()'s truthful array-only mutation boundary. Keep the notes limited to user-visible API differences that matter when porting Laravel code or reviewing future upstream changes.
Record the verified Collections architecture, accepted defects and improvements, rejected lifecycle concerns, implementation boundaries, regression coverage, performance analysis, validation gates, and final review status in the audit ledger. Mark Collections complete, preserve the exact 71-package checklist, and advance the compact routing index to Reflection with no unresolved cross-package revalidation.
Declare typed all() and run() bridges directly on the Hypervel console application. Composer can preload an older Symfony Console implementation whose inherited methods have no return types, so relying on the repository Symfony version caused Hypervel\Console\Application to fail during class loading even though the public contract itself was accurate. Add a deterministic legacy-Symfony fixture and isolated subprocess regression that reproduces Composer method availability without depending on the installed Composer version. Reflection coverage also pins both methods to the concrete Hypervel class so the compatibility bridge cannot disappear silently. Move the remaining Console unit tests off raw PHPUnit and onto the framework test base, including the required parent setup call, so coroutine setup, Mockery verification, and framework teardown apply consistently. Record the shared console/contracts boundary in the framework audit plan and ledger. Validation: focused Console tests, the installed Composer PHAR preload probe, both PHPStan configurations, PHP CS Fixer, the full parallel component suite, Testbench contract tests, and Testbench dogfood tests all pass.
|
Warning Review limit reached
Next review available in: 12 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR updates collection APIs and typing, improves macro dispatch and framework static-state cleanup, restores testbench package-manifest state, adds typed console compatibility bridges, and records the corresponding audit and validation changes. ChangesFramework audit and lifecycle
Collections
Console compatibility
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Test
participant AfterEachTestSubscriber
participant FrameworkClass
participant Request
Test->>AfterEachTestSubscriber: finish test
AfterEachTestSubscriber->>FrameworkClass: flushFrameworkState()
AfterEachTestSubscriber->>Request: flushState()
Request-->>AfterEachTestSubscriber: reset inherited and macro state
FrameworkClass-->>AfterEachTestSubscriber: clear static macro state
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 corrects behavioral mismatches across several foundational surfaces — Conditionable proxies, Macroable closure dispatch, Collection/LazyCollection parity, Arr iterable contracts, test-state isolation, and console compatibility with older Composer-bundled Symfony versions — without introducing new abstractions or state models.
Confidence Score: 5/5Safe to merge. All changes correct pre-existing behavioral gaps at their owning boundaries without introducing new state models or breaking existing contracts. Each fix is traceable to a concrete mismatch between advertised contract and runtime behavior. The Macroable fallback chain handles all known PHP binding shapes. The collection parity changes are logically correct and covered by focused tests. The flushState() additions reset only pre-existing mutable static state. The console typed-override is a minimal fix for a real class-loading hazard. No files require special attention. The most behaviorally dense change is LazyCollection — the has() fix and makeIterator() Iterator branch — both with dedicated test coverage. Important Files Changed
Reviews (2): Last reviewed commit: "Refine macro and collection fallbacks" | 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 (2)
src/collections/src/Arr.php (1)
1261-1267: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd a "Tests only." warning to
flushState().Same concern as
UploadedFile::flushState()/Response::flushState(): flushing macros outside test cleanup silently removes every macro registered onArrfor the rest of the worker's life.📝 Proposed docblock update
/** * Flush all static state. + * + * Tests only. Calling this outside test cleanup removes every macro + * registered on this class for the remainder of the worker's life. */ public static function flushState(): voidAs per coding guidelines: "Add
Boot-only.,Tests only., orBoot or tests only.warnings to public mutators of worker-lifetime state when they are not intended for normal runtime use; name the concrete failure mode in the second sentence."🤖 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/collections/src/Arr.php` around lines 1261 - 1267, Add a “Tests only.” warning to the docblock for Arr::flushState(), and in the second sentence state that calling it outside test cleanup removes all macros registered on Arr for the remainder of the worker lifetime.Source: Coding guidelines
src/http/src/Client/Response.php (1)
540-547: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd
Tests only.warning toflushState()docblock.Now that
flushState()callsstatic::flushMacros(), it mutates worker-lifetime macro state. Per coding guidelines, this public static method should carry aTests only.warning naming the concrete failure mode.As per coding guidelines: "Add
Boot-only.,Tests only., orBoot or tests only.warnings to public mutators of worker-lifetime state when they are not intended for normal runtime use; name the concrete failure mode in the second sentence."📝 Proposed docblock update
/** * Flush all static state. + * + * Tests only. Calling this method at runtime would clear all registered response macros for every subsequent request in the worker. */ public static function flushState(): void { self::$defaultJsonDecodingFlags = 0; static::flushMacros(); }🤖 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/http/src/Client/Response.php` around lines 540 - 547, Update the docblock for the public static method flushState() to include a “Tests only.” warning and state that calling it during normal worker runtime mutates worker-lifetime macro state, potentially affecting subsequent requests.Source: Coding guidelines
🧹 Nitpick comments (5)
src/http/src/Client/Factory.php (1)
659-665: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd "Tests only." warning to
flushState()docblock per coding guidelines.Same as
Request::flushState()— this public static method mutates worker-lifetime macro state. The coding guidelines require aTests only.warning with the concrete failure mode. Other methods in this class (globalOptions,registerConnection,setConnectionConfig) already follow this pattern.📝 Proposed docblock update
/** * Flush all static state. + * + * Tests only. Calling this during normal request handling clears all + * registered macros for every subsequent HTTP client request on this worker. */ public static function flushState(): 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/http/src/Client/Factory.php` around lines 659 - 665, Update the docblock for Factory::flushState() to include a “Tests only.” warning describing that it mutates worker-lifetime macro state and must not be used in production, matching the wording and structure of Request::flushState() and the existing warnings on globalOptions(), registerConnection(), and setConnectionConfig().Source: Coding guidelines
src/http/src/Request.php (1)
1183-1195: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
Tests only.warning toRequest::flushState()
Request::flushState()resets worker-lifetime static state and is already used intests/Testing/PHPUnit/AfterEachTestSubscriber.php; calling it during normal request handling can leak state across concurrent requests on the worker.🤖 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/http/src/Request.php` around lines 1183 - 1195, Add a clear “Tests only” warning to the PHPDoc for Request::flushState(), stating that it resets worker-lifetime static state and must not be called during normal request handling. Keep the method’s reset behavior unchanged.Source: Coding guidelines
src/macroable/src/Traits/Macroable.php (1)
122-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider replacing the exception-for-control-flow pattern in
__callwith a conditional.Using
throw new RuntimeExceptionpurely to jump into thecatchblock works but is an anti-pattern. A conditional check is clearer and avoids constructing an exception object on every rebind failure.♻️ Proposed refactor
if ($macro instanceof Closure) { - try { - // PHP warns when this binding is unsupported; the checked fallbacks select the next valid form. - $macro = @$macro->bindTo($this, static::class) ?? throw new RuntimeException; - } catch (Throwable) { - try { - $macro = @$macro->bindTo(null, static::class) ?? $macro; - } catch (Throwable) { - // Keep the original first-class callable when PHP does not permit rebinding it. - } + $bound = @$macro->bindTo($this, static::class); + if ($bound !== null) { + $macro = $bound; + } else { + try { + $bound = @$macro->bindTo(null, static::class); + $macro = $bound ?? $macro; + } catch (Throwable) { + // Keep the original first-class callable when PHP does not permit rebinding it. + } } }🤖 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/macroable/src/Traits/Macroable.php` around lines 122 - 131, Replace the intentional RuntimeException control flow in __call’s first bindTo fallback with a conditional: only proceed to the second bindTo attempt when binding to $this succeeds or otherwise explicitly handle the failed result without throwing. Preserve the existing Throwable handling and original-callable fallback behavior for unsupported rebinding.src/collections/README.md (1)
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse setext heading style for consistency.
The file opens with a setext-style title (
===on line 2), but## Differences From Laravelon line 8 uses ATX style. markdownlint flags this as MD003.[source_static_analysis_hints]
♻️ Proposed fix
-## Differences From Laravel +Differences From Laravel +---🤖 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/collections/README.md` at line 8, Change the “Differences From Laravel” heading in the README from ATX syntax to setext syntax, using the same heading style as the opening title and preserving its heading level.src/collections/src/LazyCollection.php (1)
991-996: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
$preserveKeysdefault is unused/dead due tofunc_get_args()forwarding.Static analysis (PHPMD) flags
$preserveKeysas unused — correctly so:func_get_args()only contains arguments the caller actually supplied, so this method's own$preserveKeys = falsedefault is never actually applied; when omitted, behavior silently falls back toCollection::random()'s default instead. Forwarding explicitly makes intent clear and removes the redundant reliance on two independently-declared defaults staying in sync.♻️ Proposed fix
- public function random(callable|int|string|null $number = null, bool $preserveKeys = false): mixed - { - $result = $this->collect()->random(...func_get_args()); - - return is_null($number) ? $result : $this->newInstance($result); - } + public function random(callable|int|string|null $number = null, bool $preserveKeys = false): mixed + { + $result = $this->collect()->random($number, $preserveKeys); + + return is_null($number) ? $result : $this->newInstance($result); + }🤖 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/collections/src/LazyCollection.php` around lines 991 - 996, Update LazyCollection::random() to forward the resolved $number and $preserveKeys arguments explicitly to collect()->random(), rather than using func_get_args(). Preserve the existing return behavior, including wrapping non-null-number results with newInstance().Source: Linters/SAST tools
🤖 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`:
- Line 207: Escape the pipe characters in the `collections-11` table row’s
“Final decision” text so `int|float|TReturnType` is rendered as literal text and
the markdown table retains its six-column structure.
In `@src/http/src/Client/PendingRequest.php`:
- Around line 1857-1864: Update the flushState() method’s documentation to
include a clear “Tests only.” warning, stating that it clears worker-lifetime
macro registrations and must not be called during requests because it can remove
macros needed by later requests in the same worker.
In `@src/http/src/Response.php`:
- Around line 327-334: Update the docblock for Response::flushState() to include
a “Tests only.” warning and state that calling it during normal runtime clears
all registered Response macros for the remainder of the worker’s lifetime.
In `@src/http/src/UploadedFile.php`:
- Around line 132-139: Update the docblock for UploadedFile::flushState() to
include a “Tests only.” warning and state that calling it during normal Swoole
worker runtime removes registered macros for the remainder of the worker’s
lifetime.
---
Outside diff comments:
In `@src/collections/src/Arr.php`:
- Around line 1261-1267: Add a “Tests only.” warning to the docblock for
Arr::flushState(), and in the second sentence state that calling it outside test
cleanup removes all macros registered on Arr for the remainder of the worker
lifetime.
In `@src/http/src/Client/Response.php`:
- Around line 540-547: Update the docblock for the public static method
flushState() to include a “Tests only.” warning and state that calling it during
normal worker runtime mutates worker-lifetime macro state, potentially affecting
subsequent requests.
---
Nitpick comments:
In `@src/collections/README.md`:
- Line 8: Change the “Differences From Laravel” heading in the README from ATX
syntax to setext syntax, using the same heading style as the opening title and
preserving its heading level.
In `@src/collections/src/LazyCollection.php`:
- Around line 991-996: Update LazyCollection::random() to forward the resolved
$number and $preserveKeys arguments explicitly to collect()->random(), rather
than using func_get_args(). Preserve the existing return behavior, including
wrapping non-null-number results with newInstance().
In `@src/http/src/Client/Factory.php`:
- Around line 659-665: Update the docblock for Factory::flushState() to include
a “Tests only.” warning describing that it mutates worker-lifetime macro state
and must not be used in production, matching the wording and structure of
Request::flushState() and the existing warnings on globalOptions(),
registerConnection(), and setConnectionConfig().
In `@src/http/src/Request.php`:
- Around line 1183-1195: Add a clear “Tests only” warning to the PHPDoc for
Request::flushState(), stating that it resets worker-lifetime static state and
must not be called during normal request handling. Keep the method’s reset
behavior unchanged.
In `@src/macroable/src/Traits/Macroable.php`:
- Around line 122-131: Replace the intentional RuntimeException control flow in
__call’s first bindTo fallback with a conditional: only proceed to the second
bindTo attempt when binding to $this succeeds or otherwise explicitly handle the
failed result without throwing. Preserve the existing Throwable handling and
original-callable fallback behavior for unsupported rebinding.
🪄 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: 8fa63fa5-0f1b-440c-93af-349a60dda3c3
📒 Files selected for processing (58)
docs/plans/2026-07-12-framework-coroutine-state-lifecycle-audit-ledger.mddocs/plans/2026-07-12-framework-coroutine-state-lifecycle-audit.mdsrc/boost/docs/collections.mdsrc/boost/docs/helpers.mdsrc/boost/docs/http-client.mdsrc/boost/docs/responses.mdsrc/collections/README.mdsrc/collections/composer.jsonsrc/collections/src/Arr.phpsrc/collections/src/Collection.phpsrc/collections/src/Enumerable.phpsrc/collections/src/LazyCollection.phpsrc/collections/src/Traits/EnumeratesValues.phpsrc/collections/src/functions.phpsrc/collections/src/helpers.phpsrc/conditionable/src/HigherOrderWhenProxy.phpsrc/console/src/Application.phpsrc/cookie/composer.jsonsrc/database/src/Eloquent/Factories/Factory.phpsrc/http/src/Client/Factory.phpsrc/http/src/Client/PendingRequest.phpsrc/http/src/Client/Response.phpsrc/http/src/JsonResponse.phpsrc/http/src/RedirectResponse.phpsrc/http/src/Request.phpsrc/http/src/Resources/Json/JsonResource.phpsrc/http/src/Response.phpsrc/http/src/UploadedFile.phpsrc/jwt/composer.jsonsrc/log/composer.jsonsrc/macroable/src/Traits/Macroable.phpsrc/notifications/composer.jsonsrc/support/src/Testing/Fakes/NotificationFake.phpsrc/testbench/src/TestCase.phpsrc/testing/src/PHPUnit/AfterEachTestSubscriber.phptests/Conditionable/ConditionableTest.phptests/Console/ConsoleApplicationCompatibilityTest.phptests/Console/ContainerCommandLoaderTest.phptests/Console/Fixtures/LegacySymfonyApplication.phptests/Console/Scheduling/CacheEventMutexTest.phptests/Console/Scheduling/CacheSchedulingMutexTest.phptests/Console/Scheduling/FrequencyTest.phptests/Integration/Foundation/Console/RouteCacheCommandTest.phptests/Support/CollectionStaticStateTest.phptests/Support/CollectionTest.phptests/Support/HigherOrderProxyTest.phptests/Support/SupportArrTest.phptests/Support/SupportCollectionTest.phptests/Support/SupportConditionableTest.phptests/Support/SupportEnumValueFunctionTest.phptests/Support/SupportLazyCollectionIsLazyTest.phptests/Support/SupportLazyCollectionTest.phptests/Support/SupportMacroableTest.phptests/Testbench/Foundation/Process/RemoteCommandTest.phptests/Testing/PHPUnit/AfterEachTestSubscriberTest.phptypes/Collections/Arr.phptypes/Collections/Collection.phptypes/Collections/helpers.php
Replace exception-driven macro closure rebinding with the nullable binding contract provided by the supported PHP runtime. This preserves every callable form while avoiding exception construction when an instance call falls back to class-scope binding. Forward LazyCollection random arguments explicitly, keep Laravel differences in the package README rather than inline source annotations, and repair the audit ledger's Markdown table escaping.
Collaborator
Author
|
@greptileai @coderabbitai The review follow-up is in e03b8a6. Accepted findings:
Rejected findings:
The focused Macroable and LazyCollection regressions pass. The full formatter, both PHPStan configurations, parallel component suite, Testbench suite, and dogfood suite are green. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
git log --oneline -1 e03b8a637 2>/dev/null || echo "commit not found locally"
git show e03b8a637 --stat 2>/dev/null | head -50
echo "---Macroable---"
git show e03b8a637 -- src/macroable/src/Traits/Macroable.php 2>/dev/null
echo "---LazyCollection random---"
git show e03b8a637 -- src/collections/src/LazyCollection.php 2>/dev/null | head -80Length of output: 5737
All changes are minimal and consistent with what was described. Thanks for the detailed rationale on the rejected findings too — the PHP 8.4 baseline, AGENTS.md docblock rule, README heading convention, and docstring-noise reasoning all make sense and I have nothing further to push back on there. (´｡• ᵕ •｡`) ♡ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
|
Changes in e03b8a6 look correct across all four accepted items. Macroable: The nullable binding chain in both LazyCollection Inline annotations and ledger escaping: Both applied cleanly. The rejections are all well-grounded — PHP 7 compat would be unreachable dead code on PHP 8.4, the This is ready. |
|
The four accepted changes in e03b8a6 all look correct. Macroable nullable chain — the new shape is clean: // instance path
$macro = @$macro->bindTo($this, static::class)
?? @$macro->bindTo(null, static::class)
?? $macro;
// static path
$macro = @$macro->bindTo(null, static::class) ?? $macro;Every callable form is handled without exception construction: regular closures bind to LazyCollection::random forwarding — explicit args over Inline annotation removal — README as single owner is the right call. Inline divergence notes tend to drift independently of prose docs. Audit ledger escaping — table renders correctly now. The rejected findings are all sound. The PHP 7 |
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
Several foundational utility surfaces had accumulated mismatches between their advertised contracts and their runtime behavior:
This change corrects those inconsistencies at their owning boundaries. It does not add a second state model, compatibility registry, or request-time synchronization layer. The public surface remains Laravel-shaped where intended, with current collection capabilities added and obsolete Laravel-deprecated aliases removed.
For more details, see:
docs/plans/2026-07-12-framework-coroutine-state-lifecycle-audit.mdConditionable and Testbench isolation
Higher-order Conditionable proxies now accept the same truthy and falsy values as direct
when()andunless()calls. Values are normalized once into the existing boolean state, preserving the proxy design and avoiding any new lifetime or concurrency machinery.The log split package now declares its direct Conditionable dependency.
Testbench now preserves and restores the package manifest through its existing test lifecycle. Remote CLI operations can rebuild that manifest, and parallel tests must not leave the rebuilt contents visible to later tests. Route-cache coverage also asserts its package-discovery precondition directly so contamination fails at the source instead of surfacing as an unrelated route assertion.
Macroable and framework test state
Macroable closure dispatch now handles the legal PHP binding shapes explicitly:
Framework-owned Macroable classes now expose or complete their static reset hooks, and the centralized test subscriber clears those registries after each test. Notification fakes, HTTP client surfaces, responses, uploaded files, JSON resources, and existing collection/database reset owners all participate in the same cleanup model.
Hypervel\Http\Request::flushState()also restores the inherited mutable Symfony configuration that Hypervel still consumes: MIME formats, method-override settings, allowed override methods, and the request factory. This prevents one test from changing request behavior for the remainder of a worker.Cookie, JWT, log, and notifications now declare their direct Macroable dependency. The testing documentation consistently explains that framework-owned macro state is cleared centrally and that application- or package-owned macro state belongs in the supported test-state cleanup mechanism.
Collections correctness and parity
The Collections work aligns eager and lazy behavior while preserving their existing allocation and evaluation models:
LazyCollection::keyBy()supports backed enums consistently with eager collections;has()handles duplicate yielded keys and an empty requested-key set correctly;sum()callbacks receive both value and key, with truthful accumulated-result types;reduceInto()is available across the shared contract and implementations;random()support key preservation through a shared conditional return type;times()reports integer values instead of unresolved mixed generics;IteratorAggregateand plainIteratorsources, including generators;first(),last(),every(), andsome()honor their iterable contracts and preserve duplicate iterator occurrences;last()preserves reverse traversal, short-circuiting, and exception order;push()now advertises only arrays, matching the mutation and array return contract it can actually fulfill.The Laravel-deprecated
containsOneItem()andcontainsManyItems()aliases are removed in favor ofhasSole()andhasMany(). Intentional omission markers prevent later parity work from restoring them accidentally.Collection metadata now uses the PHP 8.5 polyfill required by the supported PHP floor, advertises the optional VarDumper integration, and keeps direct dependencies sorted. Public PHPDoc, named-argument parameter names, static-analysis fixtures, and the Collections and helpers guides now match the implemented behavior.
Composer-safe console contracts
Hypervel\Console\Applicationnow declares typedall()andrun()methods directly and delegates to Symfony. This keeps the accurate Hypervel contract independent of the Symfony version already resident in the process.This matters during Composer scripts: Composer can preload an older bundled Symfony Console application whose corresponding methods have no return types. Inheriting those methods caused PHP to reject the Hypervel class before the post-autoload script could run.
A deterministic isolated-process regression loads the literal Hypervel application against a fixture matching the relevant older Composer method surface. Reflection coverage separately pins both methods to the concrete class and their contract return types. The fixture deliberately excludes
addCommand(), matching the older parent and guarding against an invalid#[Override]assumption.The remaining Console unit tests now use the framework test base, so coroutine setup, Mockery verification, and framework teardown apply consistently.
Performance and complexity
The common paths retain their existing shapes:
last()uses constant memory;last()stores ordered pairs only when reverse callback semantics require them;Key forwarding in
sum()and checked closure binding perform the minimum work required for their corrected public behavior. No production lock, coroutine-context slot, dynamic registry, retry loop, or compatibility shim is introduced.Validation
Coverage includes focused runtime regressions, static-analysis fixtures, deterministic coroutine/test-state isolation, manifest lifecycle tests, and an isolated Composer-preload class-loading test. PHP CS Fixer, both PHPStan configurations, the complete parallel component suite, the Testbench package contract suite, and the Testbench dogfood suite pass.
Summary by CodeRabbit
New Features
reduceInto(), key-preserving random selection, improved iterable support, and callback keys for summation.Bug Fixes
Documentation
Tests