Add wiring-level test coverage for the Octane reset listener - #2966
Merged
Conversation
`PermissionServiceProvider::registerOctaneListener()` wires two listeners for `Laravel\Octane\Contracts\OperationTerminated`, resetting the permissions team ID on every request, and (opt-in via `permission.register_octane_reset_listener`) clearing the in-memory permissions collection, but neither had any test coverage. The registration code bails out early via `if ($this->app->runningInConsole() || ...)`, and since Pest/Testbench always runs under the CLI SAPI, `runningInConsole()` is always `true` in the test suite so this code path was silently never exercised. This PR adds real coverage without requiring a running Swoole/RoadRunner server: - Adds `laravel/octane` as a **dev-only** dependency: it's pure PHP with no extension requirement (swoole/roadrunner are only needed to actually *serve* traffic via `octane:start`, not to use Octane's `Contracts`/`Events` classes). - `tests/Integration/OctaneListenerTest.php` forces `Application::$isRunningInConsole` to `false` via reflection (the real, memoized gate Laravel checks — see `Illuminate\Foundation\Application::runningInConsole()`), invokes the real (protected) `registerOctaneListener()` method, and dispatches an actual `Laravel\Octane\Events\RequestTerminated` event to exercise the genuine listener wiring end-to-end. - Each test dispatches the event with a **cloned sandbox app distinct from the base app** (`clone app()` + `forgetInstance(PermissionRegistrar::class)`), so the assertions only pass if the listener acts on `$event->sandbox()` specifically, not `$event->app()`. The tests were confirmed to fail when the production code was temporarily changed to use `$event->app()` instead. **Also fixes a related correctness issue:** `registerOctaneListener()` accessed `$event->sandbox` as a property, which isn't part of the `OperationTerminated` interface (only `sandbox(): Application` is). Adding `laravel/octane` as a dependency let PHPStan finally resolve the class and catch this once the stale `@phpstan-ignore-next-line` suppressions were removed. Fixed by calling `$event->sandbox()` instead — no behavior change, but now type-checked. **Coverage added:** - Listener is not registered when `octane.listeners` is disabled. - `PermissionRegistrar::getPermissionsTeamId()` resets to `null` on the sandbox instance on operation termination, while the base app's instance is untouched. - The permissions collection is cleared on the sandbox instance only when `permission.register_octane_reset_listener` is enabled, and left untouched (on both instances) when it isn't. **Maintenance trade-off:** the tests reach into a protected framework property (`Application::$isRunningInConsole`) and a protected provider method via reflection. If Laravel renames that property, or Octane changes `RequestTerminated`'s constructor signature, these tests could break for reasons unrelated to an actual regression here ... but we keep in sync with Laravel releases already.
There was a problem hiding this comment.
Pull request overview
Adds integration-level coverage to ensure the package’s Octane “operation terminated” listeners are actually wired and act on the Octane sandbox container, while also tightening the listener implementation to use the OperationTerminated contract API.
Changes:
- Add a new Pest integration test that forces the non-console code path, registers the real Octane listeners via reflection, and dispatches a real
RequestTerminatedevent against a cloned sandbox container. - Fix
registerOctaneListener()to call$event->sandbox()(contract method) instead of accessing$event->sandbox(non-contract property), and remove PHPStan suppressions. - Add
laravel/octaneas a dev dependency to make the Octane contracts/events available in tests and for static analysis.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/Integration/OctaneListenerTest.php | Adds wiring-level integration tests validating the Octane listener registration gates and sandbox-only effects. |
| src/PermissionServiceProvider.php | Corrects Octane listener implementation to use OperationTerminated::sandbox() and removes now-unneeded PHPStan ignores. |
| composer.json | Adds Octane as a dev-only dependency to support tests and type resolution. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Collaborator
Author
|
Related: #2952 |
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
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.
Add wiring-level test coverage for the Octane reset listener
PermissionServiceProvider::registerOctaneListener()wires two listeners forLaravel\Octane\Contracts\OperationTerminated, resetting the permissions team ID on every request, and (opt-in viapermission.register_octane_reset_listener) clearing the in-memory permissions collection, but neither had any test coverage. The registration code bails out early viaif ($this->app->runningInConsole() || ...), and since Pest/Testbench always runs under the CLI SAPI,runningInConsole()is alwaystruein the test suite so this code path was silently never exercised.This PR adds real coverage without requiring a running Swoole/RoadRunner server:
laravel/octaneas a dev-only dependency: it's pure PHP with no extension requirement (swoole/roadrunner are only needed to actually serve traffic viaoctane:start, not to use Octane'sContracts/Eventsclasses).tests/Integration/OctaneListenerTest.phpforcesApplication::$isRunningInConsoletofalsevia reflection (the real, memoized gate Laravel checks — seeIlluminate\Foundation\Application::runningInConsole()), invokes the real (protected)registerOctaneListener()method, and dispatches an actualLaravel\Octane\Events\RequestTerminatedevent to exercise the genuine listener wiring end-to-end.clone app()+forgetInstance(PermissionRegistrar::class)), so the assertions only pass if the listener acts on$event->sandbox()specifically, not$event->app(). The tests were confirmed to fail when the production code was temporarily changed to use$event->app()instead.Also fixes a related correctness issue:
registerOctaneListener()accessed$event->sandboxas a property, which isn't part of theOperationTerminatedinterface (onlysandbox(): Applicationis). Addinglaravel/octaneas a dependency let PHPStan finally resolve the class and catch this once the stale@phpstan-ignore-next-linesuppressions were removed. Fixed by calling$event->sandbox()instead — no behavior change, but now type-checked.Coverage added:
octane.listenersis disabled.PermissionRegistrar::getPermissionsTeamId()resets tonullon the sandbox instance on operation termination, while the base app's instance is untouched.permission.register_octane_reset_listeneris enabled, and left untouched (on both instances) when it isn't.Maintenance trade-off: the tests reach into a protected framework property (
Application::$isRunningInConsole) and a protected provider method via reflection. If Laravel renames that property, or Octane changesRequestTerminated's constructor signature, these tests could break for reasons unrelated to an actual regression here ... but we keep in sync with Laravel releases already.