Repository navigation
Register back-end navigation, widgets and settings in a Laravel Octane worker - #1511
austinderrick wants to merge 10 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
edbff29 to
468913a
Compare
…oundary Under PHP-FPM every request gets a fresh process, so state left on a long-lived object never hurts anyone. Under Laravel Octane the same process serves many requests, and anything derived from one request stays visible to the next. Several of these leak one user's data to another. Nothing here requires the companion Storm changes to be present. Winter has to load on a Storm that predates them, and `implements` is resolved when a class is loaded, so naming a contract that a given Storm may not ship makes a class unloadable rather than merely unresettable. The managers therefore declare resetWorkerState() without naming ResetsWorkerState, and the boot-time gates reach runningInApplicationServer() through method_exists(). Where the Storm side is absent, the gates answer exactly as they do today, which is correct: there is no worker support to admit. Winter also takes no dependency on laravel/octane, not even for development. The package appears in suggest only, since a persistent worker is something an application opts into. Every Octane test skips where the package is absent, and the test bootstrap builds its no-op client as an anonymous class so that no file naming an Octane contract has to load when there is nothing to name. Registers Octane's service provider when the package is installed, since Winter replaces Laravel's provider discovery and the provider was never picked up. Adds System\Classes\Octane\ResetsRequestState, which clears request-derived state at the start of every operation rather than the end, because an exception escaping the HTTP kernel skips terminate() and RequestTerminated never fires. Rebuilds the permission-filtered contents of NavigationManager, WidgetManager, SettingsManager, MailManager and ComponentManager per request, while leaving their once-per-worker registrations alone. Clears the per-request caches that PreferenceMaker and the URL maker declare on traits, which needs the using classes resolved because PHP gives each of them its own copy. Keys the preference cache by user. Admits an application server in the boot-time gates of the Backend, System and Cms providers. Those gates test runningInBackend() once per worker, at boot, when no request exists, so back-end registration never ran at all under a worker. Loads a rebuilt CMS code cache file with require rather than require_once. The path is deterministic while the class name inside is random per rebuild, so in a reused process the load did nothing and left the class undeclared, which also disabled the recovery path in handleCorruptCache(). Closes the Twig include gate in a finally block, so a throw during template loading no longer leaves it open for every later request. Invokes the plugin reset on any plugin that implements ResetsWorkerState or simply declares resetWorkerState(), for the same loading reason as the managers above. Tests: 499 green against the companion Storm branch, and 499 green against stock wip/1.3 Storm with laravel/octane absent, where the Octane tests skip. Each fix was verified failing before its change. Also exercised end to end against a Winter based application of about thirty plugins running on Octane and FrankenPHP, whose Cypress suite of 36 groups passes on Octane and PHP-FPM alike.
468913a to
1be38b5
Compare
|
@austinderrick can Octane support be provided by a first party plugin (i.e. Winter.Octane)? |
Hey @LukeTowers : we could move some of this to a first party plugin, like the registerOctane/resetRequestState orchestrator, etc. A big chunk of the changes here are more specific to the Winter internals and how they hold onto variables, so we'll probably need to keep some of them. (And the partner wintercms/storm#241 too). I'll draft something up for it! |
The worker-safety primitives stay in core: resettable manager state, the ResetsWorkerState contract in Storm, and the boot-vs-request gating fixes. What moves to the plugin is the Octane-specific wiring — registering Laravel\Octane\OctaneServiceProvider (Winter disables package discovery), the request-boundary ResetsRequestState listener, and the tests that drive Octane's ApplicationGateway. The plugin declares itself elevated so the reset still attaches when PluginManager::$noInit skips ordinary plugin initialization. The composer suggestion now points at winter/wn-octane-plugin, which requires laravel/octane itself.
austinderrick
changed the title
The plugin's own composer.json requires laravel/octane; core does not need to advertise it.
|
@LukeTowers : I made a repo over here for the plugin and invited you, so we can move it to the Winter org: https://github.com/austinderrick/Winter.Octane Also on this PR and the related Storm one, I rewired them to work with the plugin instead. |
The property is untyped and starts life as null; the docblock claimed it was always a string.
|
@austinderrick if you make me an admin or an owner on the repo then I can transfer it to the Winter organization and you can continue working on it from the official namespace. |
@LukeTowers : Yep! Transfer headed your way. |
|
@austinderrick repository has been transferred and added to packagist: https://packagist.org/packages/winter/wn-octane-plugin |
The manager resets, the preference cache key and the docblock fix move to the Winter.Octane plugin. The CodeParser and Twig include gate fixes move to their own PRs. The gates now check Octane's client binding directly, so this PR no longer depends on a new Storm method.
austinderrick
changed the title
…eset methods (#1) The core PRs were reduced to the changes a plugin cannot make (wintercms/winter#1511 and wintercms/storm#241), so the plugin now clears the manager, auth, error mask and Halcyon state itself: - MANAGER_STATE lists the request-derived properties on the navigation, widget, settings, mail, component and back-end auth managers, cleared by reflection. - ErrorHandler's masks join STATIC_CACHES, and the Halcyon in-memory cache is flushed on every resolved store. - ResolveAuthIpAddress records the client address on the auth manager after the trusted-proxy middleware. - The ResetsWorkerState contract moves to Winter\Octane\Contracts. - Registration now checks only that Storm dispatches interface listeners, and boot() logs a warning when a worker has no back-end navigation (Winter without #1511). - The auth reset also turns sessions back on, covering Storm without the once() fix.
LukeTowers
left a comment
LukeTowers
left a comment
There was a problem hiding this comment.
Hmm, not wild about this approach, plugins make use of the same pattern. Is there another way we could achieve the desired end result without requiring an octane specific conditional in the core code?
|
Closing: this moved into the Octane plugin in wintercms/wn-octane-plugin#2, which makes these back-end registrations itself when a worker boots, so core needs no Octane-specific check. |
2 participants
Summary
Working toward Laravel Octane support. This PR is now only the change that has to live in core; the rest moved to the Winter.Octane plugin or to separate PRs.
The
Backend,CmsandSystemservice providers register back-end navigation, report widgets, form widgets and settings only whenrunningInBackend()is true. An Octane worker registers its providers once, against the synthetic request it boots with, so that check is false and none of the back-end registrations ever happen. Each gate now also passes when Octane's client contract (Laravel\Octane\Contracts\Client) is bound. Octane binds it in the worker before the providers register, and nothing else binds it, so PHP-FPM and artisan behave exactly as before. Registering in a worker costs little, because each call stores a callback and the menus and lists are built later, on first use.The check uses the binding directly, so this PR no longer depends on wintercms/storm#241.
What moved out
These changes moved to the Winter.Octane plugin, which clears the same state from outside core at the start of every Octane operation:
NavigationManager,WidgetManager,SettingsManager,MailManagerandComponentManagerPreferenceMakercache, which the plugin's reset of that static makes unnecessaryBackendController::$actiondocblock fixThese are bugs under PHP-FPM too, so each is now its own PR:
Testing
modules/backend/tests/ServiceProviderTest.phpchecks that back-end navigation is not registered outside the back end, and is registered when Octane's client is bound; that second test fails without the change. Since theCmsandSystemgates are the same one-line change, it covers only theBackendprovider.The Winter.Octane suite (44 tests) also passes against this branch and wintercms/storm#241. It drives requests through Octane's real
WorkerandApplicationGateway.