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

Eliminate per-request data in application state #115

Open
askvortsov1 opened this issue Apr 5, 2021 · 1 comment
Open

Eliminate per-request data in application state #115

askvortsov1 opened this issue Apr 5, 2021 · 1 comment

Comments

@askvortsov1
Copy link
Member

From Franz in flarum/framework#2307 (comment):

I've taken great care to ensure that Flarum can easily be used with modern approaches like application servers (ReactPHP, PHP-PM, ...). This requires a mental shift from the good old days when everything was always "shared-nothing": global state was global, but only for the current request. No leaks, no worries.

I want to keep the container and application classes free of per-request objects. They should only deal with truly application-wide state, whether the PHP process only handles one request or multiple. There is one object tree that can be long-living, and then there are object trees for each request. In the traditional world, there will be no difference between them, but once you run with a PHP application server, there is.

Laravel does this, true, and I don't particularly like that. 😉 The downsides can be circumvented, alright, but you have to be aware of them. Just look at some of the dark magic that the Swoole adapter for Laravel has to do. Having this clean separation can sometimes make specific things a bit more complicated (though not impossible, in my experience), but has the benefit of being compatible with the different ways to run PHP applications - by design.

As pointed out above, Flarum is doing a relatively good job so far in terms of keeping per-request state separated from application state. However, there are still several places where we could be doing better. For now, the goal of this issue will be to catalogue these places and discuss how they could be improved.

  • SetLocale middleware sets the current user's locale as a property on the global LocaleManager singleton
  • Discussion has a static setStateUser method. Should be eliminatable via https://github.com/flarum/core/issues/1321
@askvortsov1
Copy link
Member Author

askvortsov1 commented Apr 7, 2021

It wouldn't be in core, but now that Laravel Octane is in beta, we might want to look into it as a potential extension. WRT to core, one of the changes I've seen in Laravel packages is using the value of $container passed into closures as an argument as opposed to reusing $this->container in service providers.

@askvortsov1 askvortsov1 transferred this issue from flarum/framework Mar 10, 2022
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

No branches or pull requests

1 participant