Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Boot providers on app clone #556

Closed

Conversation

kylekatarnls
Copy link

@kylekatarnls kylekatarnls commented Jul 23, 2022

Replace (new CarbonServiceProvider($event->app))->updateLocale(); with a re-boot of all providers after the new app has been re-attached to them.

The point of this change is to have a generic handling of providers that would work the same as in typical installas (expect 1 boot() for each request) instead of handling specifically each provider.

It would be safer that providers that don't need to be re-booted would expose some interface/flag so Octane would skip them (so it can still be optimized as much as possible) + a manual exception list that Octane can have for the basic stuff, than the other way round where booting is skipped by default (which might fall unexpected for most providers) and each provider having to adapt their code to support Octane.

Suggestion for interface can be:

interface OctaneServiceProvider
{
  public function bootWithOctaneApp(Illuminate\Foundation\Application $application);
}

If present Octane would call $provider->bootWithOctaneApp($app) instead of $app->bootProvider($provider) and it would be up to the provider implementing the interface to decide which callbacks/events/etc. to do or keep the method void to do nothing.

The annoying part is the Closure used to manipulate protected properties of Application and ServiceProvider but if we agree on the principle first, we might add methods to Illuminate\Foundation\Application and Illuminate\Support\ServiceProvider to allow this without entering the protected scope.

@kylekatarnls kylekatarnls force-pushed the fix/boot-provider-on-app-clone branch from fee1931 to 889530a Compare July 23, 2022 15:22
@taylorotwell
Copy link
Member

I really don't think I can make this change on a patch release.

@kylekatarnls
Copy link
Author

I don't mean to release in patch, here is the general idea: service providers would expect to be booted for each request I got the report that locale sync is lost in octane. So here is some PoC of how it could remain without having octane specific code in providers nor specific handling for each provider in octane.

@nunomaduro
Copy link
Member

@kylekatarnls No need to send this changes to master.

We understand your feedback. Yet, at this time, it's a big "architectural" change, that we think is too risky of a "breaking change".

That been said, regarding the "locale" issues, we are going to make two fixes:

@kylekatarnls
Copy link
Author

It sounds good 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants