RFC: App object to encapsulate global state and services

[chillu]

Acceptance Criteria

• A kernel object exists, which can be easily overridden in user code, to control the boot process of any silverstripe application.
• An application object exists, which can be easily overridden in user code, to control the execution of any request in a silverstripe application.
• All components which make up the state of the Kernel object are available via Injector service, even if these components are initialised pre-config (including the Kernel itself).
• Kernel has no static members.
• The Kernel object can be nested (and un-nested), which internally nests all core services, as well as injector / config. However, this nesting is performed per-instance and not via static accessors as with config / injector.
• Cookie, Requirements, Config::inst() and Injector::inst() functionality remains unaltered.
• HTTPRequest is not a part of kernel, nor are any request-specific services.
• Session should accessible through the HTTPRequest (no longer static).
• Development experience should not be greatly adversely affected for classes which use the main core traits.
• If a non-current instance of Injector / Config is ever invoked, it must raise a logic error to enforce application consistency.
• The concept is applied to all core modules
• The upgrade path is clearly documented

Excludes

• Converting static methods on core classes to instance methods
• Converting core classes to interfaces
• Global variable state (e.g. $_SESSION, $_REQUEST) for initial implementation.

Notes

• PSR-11 discourages the direct use of service locators in implementations
• Symfony exposes services service locators (e.g. $this->app->get('my-dependency'))

RFC Versions

• V1 by @sminnee - https://gist.github.com/sminnee/7fd620a026d551730a088d5e82e542f0
• V2 by @tractorcow - https://gist.github.com/tractorcow/c0656a334c13eac8ae252610d226a1c6
• V3 by @tractorcow / @chillu -
  https://gist.github.com/tractorcow/8be54d6b019412c037049c8caddf39eb
• V4 by @tractorcow - https://gist.github.com/tractorcow/2bb76d12074b885b9b2923dafa07ae88

Related

• Issue: Tighten up usage of dependency injection in core
• RFC: Make Injector comply with PSR-11
• PSR-11 meta
• Issue: Injector factories should be passed the full service definition
• Use PSR-7 for HTTPRequest/HTTPResponse
• Config should be injectable
• Allow config-cache customisation via custom bootloader/app object

Tasks

• Agree on RFC and implementation goals
• Create new Kernel / Application interfaces and base classes
• Re-implement nesting and refactor injector / config manifests into CoreKernal
• Implement SilverStripe specific AppKernel to replace Core.php
• Refactor main.php into HTTPApplication (incl errorcontrolchain)
• Refactor cli-main.php to use new HTTPApplication (CLIApplication class is future-scope)
• Implement HTTPRequest::createFromEnviroment()
• Refactor Session instance to be a HTTPRequest member
• Ensure Director::test (or some equivalent) is still available
• Upgrading guide
• Documentation, including customising app in user code
• move ConfigureFromEnv.php into AppKernel::boot()
• Move is_running_tests() into Kernel
• lazy DB::connect()
• Upgrade unit tests

WIP branches

• [WIP] API App object implementation #7037
• [WIP] App object refactor silverstripe-cms#1854
• [WIP] Refactor session usage silverstripe-asset-admin#505
• [WIP] App object refactor silverstripe-admin#110
• [WIP] App object silverstripe-assets#25
• [WIP] App object silverstripe-versioned#16
• [WIP] App object refactor silverstripe-testsession#49
• Remove unneeded globals silverstripe-installer#183
• [WIP] Upgrade module for app object refactor silverstripe-reports#74
• [WIP] Fix module tests silverstripe-siteconfig#63
• Update for app object silverstripe-graphql#104
• Updated behat-extension to use app object silverstripe-behat-extension#163

[chillu]

Required for making manifest caches configurable through PSR-16: #6647

[tractorcow]

I have written up a new RFC covering the full interface for App and DefautAppFactory

Please see https://gist.github.com/tractorcow/c0656a334c13eac8ae252610d226a1c6

[nyeholt]

Don't have time to delve too deeply right now, but a few years back AJShort did some extensive work down this line. There's a full commit list still at https://github.com/ajshort/sapphire/commits/composer , with relevant code mostly in https://github.com/ajshort/sapphire/tree/composer/src/SilverStripe/Framework

[micmania1]

I don't really get the whole nest/unnest thing. I know that its a pattern used elsewhere but can't you just use clone? The only time I can see this being used would be in testing, but clone would work then too and you wouldn't have to call unnest() at the end. The only reason nest/unnest would be required is if we're making static calls to App::inst() in which case we'd need to track the current app.

This leads on to my next question...
App::inst() vs __construct($app) - For classes which need to know about the app object, what's the end goal?

Injector::inst() and Config::inst() - where does this leave these methods? Would they be deprecated in favour of $app->getConfig()?

[tractorcow]

Nesting is important to ensure that state is retained at the point of nesting. This means that nesting at the application level includes not only nesting of the various direct dependencies, but also the current state of each dependent nested chains.

E.g.

State A:
App (state1)
-  Config (state1)
-  Injector (state1)

Injector::nest()

State B:
App (state1)
-  Config (state1)
-  Injector (state2)

action:
App::nest();

State: C

App (state2)
-  Config (state1)
-  Injector (state2)

Config::nest()
Injector::nest()

State D:
App (state2)
-  Config (state2)
-  Injector (state3)

App::unnest() (note: resets entire chain of nesting for config / injector to point at original App::nest())

App (state1)
  - Config (state1)
  - Injector (state2)

[tractorcow]

The only time I can see this being used would be in testing, but clone would work then too and you wouldn't have to call unnest() at the end.

If you never called unnest, then how could you return to the original state? You would end up with a different config in your local variable clone vs App::inst()->getConfig().

[tractorcow]

App::inst() vs __construct($app) - For classes which need to know about the app object, what's the end goal?

Well, simply App::inst() is a getter, and the constructor is only used when creating the initial app. The end goal is to consolidate global state into an object.

[tractorcow]

Injector::inst() and Config::inst() - where does this leave these methods? Would they be deprecated in favour of $app->getConfig()?

Yes.

[tractorcow]

@nyeholt main class found at https://github.com/ajshort/sapphire/blob/composer/src/SilverStripe/Framework/Core/Application.php

It does look VERY similar doesn't it? :)

[tractorcow]

Also just to add, I dislike injecting everything with an app property. If you wanted to keep Object class and add setApp / getApp maybe, but we've already agreed that class is going.

[micmania1]

If you never called unnest, then how could you return to the original state? You would end up with a different config in your local variable clone vs App::inst()->getConfig().

If the use-case is for testing, then the variable (nested app) would only exist for the length of the test so would be destroyed and forgot about automatically. You would never actually nest.

eg.

protected function setUp() {
    $initialApp = App:inst();
    $this->app = clone $initialApp;
}

public function testApp() {
    $this->assertInstanceOf(App::class, $this->app);
}

This wouldn't work if you called App::inst() everywhere which is what triggered my question about static call vs injection via constructor.

[tractorcow]

I see your point.

If we did use an app property on every object, but that's very risk prone, given that it only takes a single object to have a mis-assigned app var for it to be running in an incorrect state.

This could easily come around when pre-existing objects are created at the point the application is nested; It'll only affect future objects, and we have no way to guarantee they won't interact with the prior ones. This is why I'm against per-object app assignment.

[micmania1]

What are we testing that would require the entire app to be cloned/nested anyway? Sounds like a pretty big coupling problem we have :p

[tractorcow]

FunctionalTest doesn't use process isolation.

[dhensby]

I feel like we need to be looking at https://github.com/symfony/http-kernel for a lot of inspiration here - perhaps even looking to use it - though that may be impossible because it can't accept our DI and so on.

But initially from looking at the proposals it's not clear to me how a request is handled.

I feel we should be aiming for a startup script with bare bones like this:

<?php

$request = HTTPRequest::createFromGlobals();
$response = HTTPResponse::create();

$app = new SilverStripe();

$app->handleRequest($request, $response);

$response->output();

At the moment it's not clear to me how this app object would be used? Or is it really just a holder of state and we are shifting our current global state onto the app object but still leaving a lot of the boot up as is?

[tractorcow]

is it really just a holder of state and we are shifting our current global state onto the app object but still leaving a lot of the boot up as is?

This is generally my feeling; It's a refactor not a radical behavioural change.

[sminnee]

At the moment it's not clear to me how this app object would be used? Or is it really just a holder of state and we are shifting our current global state onto the app object but still leaving a lot of the boot up as is?

The 3 pieces that aren't easily refactored are, module manifest -> config -> injector. The app object can provide these.

I don't think that the app object should provide handleRequest(), for now I'd say leave that responsibility to the director or possibly to a Director and provide a getDirector(). The App object would be constructed even if you were running a small support script that that didn't make use of the router, or somewhere where you were using SilverStripe with another controller stack.

[sminnee]

On nest() / unnest():

There's no longer a case where you need to run tests and have the parent global app object — we deleted the dev/tests/ URL. So you could probably build tests using App::set_inst().

The areas where it would be more likely to be of use would be things like staticpublisher where you want to craft an altered global state for a portion of execution. This is kind of an advanced case and could be handled as an API addition beyond the initial implementation, if needs be. I don't think we would need nest() in the first version, and if we don't implement it now it would give us the freedom to potentially design a better API when we have the use-case in front of us.

For example, rather than nest / unnest we might have something closer Member::actAs()

[tractorcow]

We could drop App::nest() and continue to use individual Injector::nest() / Config::nest() as-is for now. Would that be a fair compromise to get v1 of app done?

[sminnee]

We could drop App::nest() and continue to use individual Injector::nest() / Config::nest() as-is for now. Would that be a fair compromise to get v1 of app done?

No, I think that's worse than creating App::nest() as we'd want to remove the static instances of injector & config. Maybe add the nest() method but mark as experimental.

[chillu]

$response->getRequest() sounds like an API created to mask the deficiencies of another API (a Cookie object which is used for both read and write). Not every response will have a corresponding request, right? I think we should defer this refactor and concentrate on Kernel and Application. Let's see how far we get with the core of this RFC, and then decide if we can still fit the cookie handling into beta1.

Note that cookie reading is part of PSR-7, but writing isn't (only indirectly via Response->with Header()). Related:

https://laravel.com/docs/5.4/responses#attaching-cookies-to-responses
https://symfony.com/doc/current/components/http_foundation.html#setting-cookies
http://paul-m-jones.com/archives/6310
https://github.com/hansott/psr7-cookies
https://github.com/dflydev/dflydev-fig-cookies

$request->getSession() looks a bit more straightforward, that should only live on the request, not the response, right? Both Laravel and Symfony handle it like that.

[sminnee]

Do we have to fix cookies (and everything else related to the controller stack) as part of this ticket? It feels like it's turning into a cascade of "fix everything annoying" ;-)

If I were to refactor cookie, I'd probably have a middleware that

• builds a CookieJar object based on the cookie header of a request
• attaches it as an attribute to the request object
• after processing is compete, check for changes to the CookieJar object
• writes appropriate Set-Cookie headers to the response

However that's well beyond the scope of this card

[tractorcow]

I've had a discussion with @chillu and decided to update the scope of the request / response refactor.

https://gist.github.com/tractorcow/2bb76d12074b885b9b2923dafa07ae88

Cookies API remains unchanged, but we will move Session::inst() into $request->getSession().

[chillu]

Old ACs:

• There is clear documentation on how to instantiate SS core classes and their dependencies (when to use new vs. dependency injection)
• There is a way to replace dependencies during early bootstrap (e.g. caching layers initialised before injector kicks in)
• Interoperability with other libraries is maintained by minimizing use of SS-specific references like Injector and Config in core classes
• I can use smaller parts of SilverStripe without copying hundreds of lines of main.php and Core.php code in other controller stacks (e.g. Silex and Lumen). Note: This is an architectural goal, and doesn’t need to be operational as part of this card.
• Configuration can easily be mocked directly on core class instances (without relying on Config::nest() global state)
• It is clear which dependencies a class relies on without reading the entire class implementation
• There are no directly overlapping APIs (DataModel method arguments)
• The concept is applied to all core modules
• The upgrade path is clearly documented

New ACs:

• A kernel object exists, which can be easily overridden in user code, to control the boot process of any silverstripe application.
• An application object exists, which can be easily overridden in user code, to control the execution of any request in a silverstripe application.
• All components which make up the state of the Kernel object are available via Injector service, even if these components are initialised pre-config (including the Kernel itself).
• Kernel has no static members.
• The Kernel object can be nested (and un-nested), which internally nests all core services, as well as injector / config. However, this nesting is performed per-instance and not via static accessors as with config / injector.
• Cookie, Requirements, Config::inst() and Injector::inst() functionality remains unaltered.
• HTTPRequest is not a part of kernel, nor are any request-specific services.
• Session should accessible through the HTTPRequest (no longer static).
• Development experience should not be greatly adversely affected for classes which use the main core traits.
• If a non-current instance of Injector / Config is ever invoked, it must raise a logic error to enforce application consistency.
• The concept is applied to all core modules
• The upgrade path is clearly documented

[tractorcow]

Nice +1 thanks @chillu

[tractorcow]

Just so that everyone here is clear, we are going to close and accept this RFC pretty early next week. Please get in your concerns early if you have anything you would like to add.

[tractorcow]

Hello, the discussion seems to have tailed off, and given our general agreement on approach we will close this discussion and move onto implementation.

[tractorcow]

todo : update A/C and task.

[tractorcow]

I've just updated the tasks for this story. Quite a lot of progress made so far!

• no more ConfigureFromEnv.php
• No more SapphireTest::is_running_tests()
• Lazy-connect DB::getConn() (great for database-less requests)
• no more DataModel
• ErrorControlChain is now entirely optional middleware
• no more die() anywhere (throwing HTTPResponse_Exception is now preferred).
• no more Session::inst()
• no more phpunitshim

[tractorcow]

I've added WIP branches to the initial story body.

New progress:

• SapphireTest refactor (now boots custom application, injectable test state for restoring / setting initial state). Versioned / etc specific test state is now shifted into versioned module. See tests.yml for core config.
• Refactor of nesting: No longer do we need to count nest() / unnest() instances! just keep a reference to the initial object, and call ->activate() on it to revert all subsequent nesting. Works on injector, config, and the new app kernel. I've removed static variables from injector / config.
• Core tests are passing.

[chillu]

🎉🎉🎉