Fixes updating locale when app instance changes in Octane

[nunomaduro]

When an Octane server is starts, we boot the framework once. After, every time Octane serves a request, it uses a "clone" of that application instance used to boot framework to serve that request.

This is done, so any change made to that "cloned" application instance does not impact the original application instance and specially does not get propagated to the next request - similar to a regular PHP FPM implementation where requests are served always using a fresh application instance.

Now, currently, Carbon's service provider is capturing a reference to the original application instance, the one we use to "clone" an application for every new request. Meaning the following:

Before this pull request:

// Users do:
app()->setLocale("pt"); // Change the state of the request application instance...

// Carbon listens the event, and changes the state of the original application instance, used to and performs:
$locale = $this->app->getLocale(); // Returns the original locale: "en";
Carbon::setLocale($locale); // Locale won't change on the application instance used on the request...

After, with this pull request:

// Users do:
app()->setLocale("pt"); // Change the state of the request application instance...

// Carbon listens the event, and changes the state of the original application instance, used to and performs:
$locale = app()->getLocale(); // Returns the new locale... 
Carbon::setLocale($locale); // Carbon locale will change with the new locale... 

Note, with this change, another issue happens: The global locale of Carbon, CarbonImmutable, CarbonPeriod, CarbonInterval gets changed to the new locale set on the request. For this, we are going to update Octane, so this state gets "resetted" when Octane finishes to serve the request.

So, once laravel/octane#552 is merge and tagged, we can put this pull request "Ready To Review".

Fixes laravel/octane#551.

[kylekatarnls]

In this case does it really make sense to boot auto-discovered service providers with the original app? Likely other libraries might try to listen events, it's kind and get current locale or other states of the app.

Also, I'm fine with some if (octane) then use some other instance, but I'm kinda bothered with the trick where we first try app() then fallback to $this->app this is almost good, but IMO, what a service provider receive in $this->app should be the default as it's what you should legitimately assume as the source of truth (prefer injected dependencies over outside singleton) and so app() should rather be a fallback.

That's why I would rather check if Octane shouldn't ensure service providers receive proper app before you boot them for a cloned app instance. Don't you think so?

[mcolominas]

From what I understand, the way Laravel Octane works is that when it starts, which only happens once, no matter how many requests are made, it starts with the original application (which includes providers and some more internal elements, for example, the routes, configuration, ...) and it is stored in the ram without closing, then when it receives a request, that is when the application is cloned.

So using $this->app inside the provider returns the original app, however using app() returns the cloned app, which is where the modified language is located.

[kylekatarnls]

Thanks. However it's not my main concern. I'm quite confident you tested this change it gets the issue fixed in Octane, but to allow such change, I need to be sure it's not a breaking change for all the users already using ServiceProvider.

And very likely it is, as it replaces $this->app with app() for everybody, there are likely many custom Laravel (non-Octane) installations where they are not the same instance (and app() is not necessarily the one meant to be the reference for current locale for all of them).

I don't need to be convinced the change helps Octane installation, I need it to be somehow only a fallback scoped to Octane, so the behavior stays the same for all other people.

And on the other note, it's not because problem disappears that the solution is correct. That's why I'm first challenging the initial issue: service providers audo-discovered in Laravel will receive an incorrect app instance auto-injected. And this point is not specific to Carbon, it would cause issues potentially in all libraries using $this->app in third-party, the fact that all those would have to consider injected app unreliable and would have to adapt with similar hack to be compatible with laravel/octane let me a bit skeptical.

So there are 2 things:

• Merge-request to support Octane will be accepted gladly, it just need to ensure unchanged behavior for existing non-Octane apps even if they are customized. Or it can be a config the user need to opt-in maybe.
• The need to use a singleton over injected dependency is a code smell IMHO and would worth to be investigated on the Octane side. But this one is just a suggestion.

[mcolominas]

The third party providers, I know some that have also had to be modified to adapt to how work Laravel Octane.

On the other hand, in my understanding, for non-octane Laravel, $this->app, app() and Container::getInstance() are the same and there should be no problem.

On the other hand, if laravel lumen is used, I wouldn't know how to tell you, since I've never used it, but I think it's also the same.

[kylekatarnls]

You can now try to install composer require nesbot/carbon:dev-master and see if you can call ->setAppGetter(fn() => $app)

Let me know if Octane is able to access this setter this way.

[nunomaduro]

@kylekatarnls I confirm that our change, in this pull request, work great with this following changes: laravel/octane#557. That been said, can you ping me back once you release carbon?

[kylekatarnls]

2.60.0 published wit the new setters.