search

silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
719 stars 819 forks source link

RFC: App object to encapsulate global state and services #6681

Closed chillu closed 7 years ago

chillu commented 7 years ago

Acceptance Criteria

Excludes

Notes

RFC Versions

Related

Tasks

WIP branches

chillu commented 7 years ago

Required for making manifest caches configurable through PSR-16: https://github.com/silverstripe/silverstripe-framework/issues/6647

tractorcow commented 7 years ago

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

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

nyeholt commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

Yes.

tractorcow commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

FunctionalTest doesn't use process isolation.

dhensby commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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.

sminnee commented 7 years ago

Alternatively SapphireTest::setUp() could reset the app object each time with App::set_inst(). I'm not sure what it would need to set it to, as it would either be creating a new app object each time (slow) or pulling global state from somewhere.

tractorcow commented 7 years ago

Ok, that's good with me :)

I would put this into SapphireTest::start() which is called only once at the start of testing to bootstrap test state.

tractorcow commented 7 years ago

Oh, that reminds me, I was thinking about moving is_running_tests() into app to remove reliance on SapphireTest from the rest of the app. Should I add this in to the RFC?

chillu commented 7 years ago

Regarding widely "injecting" a service locator object (App) via $this->app or __construct($app) into core classes: I think that's an anti-pattern - see the PSR-11 notes. We should aim for auto-wired DI on explicit dependencies instead (e.g. $this->db rather than $this->app), and until we can do that refactor on all core classes, rely on App::inst() and Injector::inst() to avoid polluting core APIs with $app instance vars and constructor args. This seems like the DataModel parameter all over again otherwise?

Assuming that DI containers can choose to return singletons, I'd imagine that you wouldn't call $this->app->getModuleManifest(), but rather $this->moduleManifest in core classes - and have that manifest singleton auto-wired. Only very few core classes should be aware that App or Injector objects even exist, since that's tight coupling.

My assumption is that you'd replace all uses of Injector::inst() with App::inst()->getInjector() or App::inst()->getService() for core classes where we're not auto-wiring dependencies yet?

One counterpoint to my argument is performance: $this->app allows services to be lazy loaded (see Symfony Service Container). When auto-wiring based on constructors or setters, we need to inject on construction time of the parent object, regardless if that particular dependency is ever used in this execution path.

Which brings me to nest() and unnest(): This feature relies on global references to App::inst() peppered around core classes to function, which I think is a harmful approach on the long run. If FunctionalTest doesn't isolate properly (reuses already initialised core classes with possibly stale state), we should fix that issue. I'd expect FunctionalTest to run a variation of Dan's startup script ($app = new SilverStripe(); $app->handleRequest()). It could choose to (deep) clone() of the current environment. But from my understanding, we're not trying to fix the whole HTTP boot process with the App object, right?

Regarding Symfony's HTTPKernel, we've got a related issue to Use PSR-7 for HTTPRequest/HTTPResponse. I agree with Sam that request handling is scope creep for this object implementation. @dhensby I haven't looked into HTTPKernel much. From your perspective, does the implementation of the App object make it harder to use HTTPKernel later on?

sminnee commented 7 years ago

Which brings me to nest() and unnest(): This feature relies on global references to App::inst() peppered around core classes to function, which I think is a harmful approach on the long run

I don't think it's feasible to do away with App::inst() in the initial PR and until we've completed that task we'll need to ensure that it returns the right value — so if we have inst(), we'll need either nest() or set_inst()

In time, we may even able to remove the inst() method altogether and not have a static instance. I don't think that's realistic for SS4.

micmania1 commented 7 years ago

Regarding widely "injecting" a service locator object (App)

Is App our service locator or is Injector? It seems like we want App to be but Injector is the thing that does most of the heavy lifting in userspace.

eg. Injector: manage user services (depends on config) App: manage core SilverStripe services (eg. Config, Injector, ClassManifest etc)

tractorcow commented 7 years ago

The way I see it is, App is more a container for state, whereas Injector is a container for injectable services. A few mid-level containers, such as module manifest, aren't injectable (due to boot order requirements) so are provided by the App object.

tractorcow commented 7 years ago

@kinglozzer @stevie-mayhew @unclecheese @wilr it'd be good to get your feedback on this RFC :) We are looking to close discussion on this next week in time to begin implementation.

kinglozzer commented 7 years ago

My general feedback is 👍.

Presumably we’d still keep Injector::nest() and Config::nest(), as they can still be useful on their own without nesting everything?

I assume this work wouldn’t affect Config::modify(), IMO it would feel a bit weird to recommend writing code like this due to the two completely different approaches:

// Updating a value
Config::modify()->update(Foo::class, 'bar', 'baz');

// Getting a value
App::inst()->getConfig()->get(Foo::class, 'bar');

// Wouldn't work (unless preceded by a Config::modify() call?)
App::inst()->getConfig()->update(Foo::class, 'bar');

I’d be okay with leaving Config::inst() alone as a shorthand version of App::inst()->getConfig() for that reason alone.

I’d also like to see examples of how this might be used to customise the bootstrap process, I’m still a little unclear on that.

stevie-mayhew commented 7 years ago

I am also a 👍 for this.

Like Loz, I think that some clear examples of how this should work to alleviate the dependency injection through the bootstrap would be great, although I have a pretty clear idea of how I would do it. I think moving to a application control system is going to allow us to move faster when we want to switch out components and start inheriting more PSR based components, so thats great news.

sminnee commented 7 years ago

Like Loz, I think that some clear examples of how this should work to alleviate the dependency injection through the bootstrap would be great, although I have a pretty clear idea of how I would do it.

This can probably be done at the PR stage, eh?

chillu commented 7 years ago

I'm still trying to get my head around why we need an intermediary App object that's not a kernel (= doesn't handle HTTP requests), and is also not a full service locator (we've still got Injector). And why Symfony doesn't need these contortions.

I've written up a deep dive into Symfony's kernel (highlevel docs and working example), might be of benefit to others:

Symfony boots via app.php or app_dev.php, which uses PSR autoloading to find a project specific AppKernel that extends the Kernel component. Most of the logic is in the Kernel base class. The AppKernel registers modules (based on getEnvironment()), and sets early boot config like cache and log directories. The Kernel also creates the service injection container. This calls a buildContainer() which looks like it would do the equivalent of our ConfigManifest compilation. Since this is all in a parent class of the project specific AppKernel, you can override buildContainer() in your own project for really deep customisations. The AppKernel has a HTTPKernel dependency through the service injection container, which takes care of the actual request handling. app.php calls $kernel->handle($request), which comes with a default ControllerResolver (the equivalent of our Director::handleRequest()).

So Kernel the closest equivalent to our App object, but without any setters or aliases, just a getContainer() method. From what I can tell, it doesn't get passed around deeper into the execution, but you can retrieve it via $this->container->get('http_kernel').

Differences of Kernel to the proposed App implementation:

I think we should strip down the proposed API to it's essentials. This acceptance criteria is the at the core of it: "There is a way to replace dependencies during early bootstrap (e.g. caching layers initialised before injector kicks in)". The value in this card isn't in creating a slightly less hacky global state repo, but rather a stable API that allows devs to customise the bootstrap without copying hundreds of lines from main.php and Core.php.

I think this can be achieved by implementing something similar to the KernelInterface with a project specific AppKernel. Since AppKernel can project specific, you can register custom services in a well structured fashion (accessing only instance state) through boot() (and utilities like initializeContainer()). If you consider that scope creep for this card, then maybe we shouldn't build the App object - because it sounds like a temporary API to me: It should really be a Kernel object which can handle requests.

There's a great tutorial series about Symfony's kernel as well - hit "next chapter" at the bottom for more content.

sminnee commented 7 years ago

My biggest concern with bundling too many HTTP-specific concerns into the App object is that a potential use-case I've had in mind is connecting the SilverStripe ORM to another controller stack such as Silex or Lumen. In this architecture, you'd likely still be making use of the SilverStripe App object to grab the database connection and instantiate DataObjects, etc. However, you would want to be able to easily side-step any HTTP concerns and hand control over to Lumen/Silex.

That said, if it's easy to ignore the HTTP concerns it provides, that's probably okay. But I think that taking HTTP/routing responsibilities from Director and putting them in App is probably be a bad idea.

chillu commented 7 years ago

Right, hadn’t considered that use case in the context of this card. Discussed a bit with Sam offline. New AC:

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.

Notes:

tractorcow commented 7 years ago

My biggest concern with bundling too many HTTP-specific concerns into the App object is that a potential use-case I've had in mind is connecting the SilverStripe ORM to another controller stack such as Silex or Lumen. In this architecture, you'd likely still be making use of the SilverStripe App object to grab the database connection and instantiate DataObjects, etc. However, you would want to be able to easily side-step any HTTP concerns and hand control over to Lumen/Silex.

One thing I would like to do, but haven't had much time to consider, is how we can remove reliance on super globals in director. My reasoning around wanting request to be a part of the app state is to provide a standard request state component that could be relied on instead.

An alternative to this is to move Director to a singleton, and have this hold the request instead. Then you could do App::getDirector()->baseURL(). This could allow, for instance, subclassing of Director (CLIDirector?).

One thing I'd like to consider is how this would work in the case @sminnee presented; Maybe request handling isn't handled by silverstripe. In which case, what would it look like?

The Symfony Kernel is pretty independant of HTTP processing other than it's handle() method and the HTTPKernel dependency. I still think it's conceptually very similar to our App object, and we would benefit from nudging our App API into this direction. Maybe even renaming App to Kernel?

I think we need to make the call if handle() becomes a part of this RFC, or is out of scope.

dhensby commented 7 years ago

What @chillu has articulated align with my point of view. Though it feels like what we thought the goals of the App object are and what the expectations of those implementing are not quite aligned.

If we're not looking at an equivalent of symfony's Kernel and that's to come "later" then I'm less concerned about the structure of the App object (which appears to be a global state holder).

chillu commented 7 years ago

One thing I would like to do, but haven't had much time to consider, is how we can remove reliance on super globals in director. My reasoning around wanting request to be a part of the app state is to provide a standard request state component that could be relied on instead.

Director (together with Core.php) is just a shitty static version of a Kernel with some unrelated global helpers on it. We should look into splitting up its responsibilities rather than promoting less-hacky-but-still-hacky approaches like App::getDirector()->baseURL(). I'd see request specific methods like baseURL() or is_https() move to the request object. Which should solve superglobal access like $_SERVER - the subset that's HTTP headers should be part of the request (Symfony has a ServerBag for this). If you don't have access to the request object (not in a controller context), you probably shouldn't worry about it. This might need deeper thinking into getCMSFields() which can be littered with Controller::curr() calls. But overall, I don't think moving Director into a singleton behind App::getDirector() is a sustainable API - I'd rather leave its current static mess and address it properly when we have time.

The main goal of this card for me is to make the boot process more of a predictable, terse, public API. require_once('Core.php') isn't an API. State isolation and removal of global helpers should be addressed only insofar it serves this goal.

micmania1 commented 7 years ago

My problem with this is that App is going to be a container regardless of what its API looks like. We'll end up with two containers in core with some services living in App and some services living in Injector. Just to backup what i'm saying here, Laravel has an Application object too which encapsulates its boot process and it implements ContainerInterface: https://laravel.com/api/5.1/Illuminate/Foundation/Application.html

If the goal is to improve bootstrapping there are much better ways to do this. The most simple way is by putting bootstrap.php in the users codebase instead of in framework. Make the entry point of the application index.php instead of framework/main.php. This gives the user control over what's happening before it even hits SilverStripe and seems to be how every other framework does it.

I think we do need an App like object, but we already have that with Injector. It just needs refactored to not rely on other core services and maybe renamed to represent its actual job which is the Application container.

The API of App being proposed (getSomeService()) isn't going to scale well going forward if we add more services or if the user wants to add another service. $app->get(MyServiceInterface::class) is much more scalable and is in line with what we already have with Injector.

tractorcow commented 7 years ago

So the way I see the boot process is that it involves a lot more than simply app state. A kernel object seems to me to be what should lie at the core of the boot process, however what we have scoped is not a kernel, but the state this kernel has.

For instance, this is what main.php could look like (two variants)

<?php
/** @var AppState $appState */
$appState = Core::buildState(); // replaces Core.php and most of main.php; Builds services, constructs HTTPRequest.
$app = new AppKernel($appState); // kernel
$app->direct(); // replaces Director::direct(), ErrorControlChain, etc

Alternatively, if the request is NOT a part of the state:

<?php
/** @var AppState $appState */
$appState = Core::buildState(); // replaces Core.php
$request = Core::buildRequest(); // replaces most of main.php
$app = new AppKernel($appState); // kernel
$app->direct($request); // replaces Director::direct(), ErrorControlChain, etc

In the above we have a model (app state, the goal of this RFC).

We have also discussed AppKernel (a good idea, not the goal of this RFC).

Essentially what I would propose is one of two directions:

chillu commented 7 years ago

In terms of goals for this RFC, I don't want to create an API that conceptually overlaps with existing APIs (Injector), or will be rendered obsolete or inconsistent once we get around to an AppKernel implementation and a revised request handling. It would be a shame if AppState exists in our API simply because of short-term goals like minimising the scope of a card. It should be an API that makes sense in the bigger picture of where we're headed.

Alternatively, if the request is NOT a part of the state:

I feel pretty strongly that it shouldn't be part of the state. The request is the base unit for customising your SilverStripe install, e.g. with middlewares or PHP-based request caching layers. It shouldn't be hidden away in a buildState().

In my opinion the AppKernel from Damian's examples is the app state (environment, path config, bootstrap services). In terms of isolation and testability, a few setters for state overrides make sense on the AppKernel instance. But overall, I would assume that this object will be instanciated in userland code eventually, and hence can be subclassed to achieve customisations. For example, Symfony doesn't provide a setCacheDir(), but rather asks you to override getCacheDir(). The kernel is booted at some point, at which point paths would've been passed on to service instances. So it's more consistent than a setCacheDir() in my opinion.

As Sam noted, a kernel should be bootable without concerning itself with HTTP requests (that might be taken care of by Lumen/Silex etc). We can split out those concerns into a separate Application object.

So following Damian's examples, I propose:

<?php
require(dirname(__DIR__) . '/vendor/autoload.php'));
$kernel = new Kernel(); // replaces Core.php, builds the "app state". Can be subclassed as a project specific AppKernel later

// For web requests
$request = HTTPRequest::createFromGlobals(); // replaces most of main.php
$app = new Application($kernel);
$app->handle($request); // replaces Director::direct(), ErrorControlChain, etc.

// For command line
$app = new ConsoleApplication($kernel);
$app->run($input);

My proposal mostly varies in naming from Damian's: AppState becomes Kernel, AppKernel becomes Application.

Since a lot of global state is in main.php, I think HTTPRequest::createFromGlobals() is in scope for this RFC. But: $app->handle($request) could just be a light wrapper around Director::direct() for now. And that whole code block would be placed in framework/main.php. We can look into moving it into a userland index.php later. At this point we could also use a default userland Appkernel extends Kernel. A handle($request) method will create more API surface for our own HTTPRequest and HTTPResponse classes, which would need to change for PSR-7 compliance (RFC). But this new method doesn't make this any easier or harder.

Since I've just looked into this, here's Laravel's bootstrap for reference: A userland index.php, which includes a userland app.php. Then the core Application is instanciated, which implements Symfony's HTTPKernelInterface. As opposed to Symfony, Laravel uses the Application object to make a Kernel service, which then takes care of request handling. This Kernel gets passed the $app as a constructor arg, which seems messy. Laravel has both Console/Kernel and Http/Kernel.

Note that Symfony's HTTPKernelInterface only defines a handle() method, so the surface is a lot smaller than the KernelInterface which has methods like getBundles() and getCacheDir(). Laravel does not implement the KernelInterface. The HTTPKernelInterface also requires a PSR compliant HTTPRequest (see https://github.com/silverstripe/silverstripe-framework/issues/6514).

Symfony's has an Application class as well for console use, which has a kernel dependency in its constructor (instanciated through bin/console).

sminnee commented 7 years ago

Perhaps, rather than an application object, it's better to think of the goal here as an easily-used Injector factory that builds and contains all state, including the module manifest and config.

All it needs is the base-path, which I've supplied as a constructor argument, but it could also be an argument to create().

Then you might have:

sapphire/main.php

use SilverStripe\Core\DefaultInjectorFactory;
use SilverStripe\Control\Director;

require __DIR__ . '/../vendor/autoload.php';

$factory = new DefaultInjectorFactory(dirname(__DIR__));
$factory->create()->get(Director::class)->direct();

Or a custom project index.php

use SilverStripe\Core\DefaultInjectorFactory;
use SilverStripe\Control\Director;
use Me\MyApp\MyCustomCache;

require __DIR__ . '/vendor/autoload.php';

$factory = new DefaultInjectorFactory(__DIR__);
$factory->setConfigCache(new MyCustomCache());
$factory->create()->get(Director::class)->direct();

In my example I've deliberately avoided refactoring Director::direct() except to make it a service, as I'd argue that's beyond the scope of the card.

tractorcow commented 7 years ago

I've had an extensive talk with @chillu regarding architecture, and I've better understood and changed my whole mind about my preferred approach.

Please see this (~400 line) gist for a suggested updated RFC.

https://gist.github.com/tractorcow/8be54d6b019412c037049c8caddf39eb

@sminnee @chillu @micmania1 @dhensby please leave your opinion here. :)

Initially I was highly opposed to injecting $obj->container into objects, but if it can be done in a safe and user-friendly way then I think it's an acceptable mechanism to remove reliance on static getters.

@sminnee I've also taken on board and better understood your previous suggestions regarding the refactor of Cookies / Session etc into HTTP request. My updated RFC includes this enhancement.

40 points.

chillu commented 7 years ago

Thanks for the discussion this morning, for staying open minded and for writing this up Damian :)

based on https://github.com/symfony/symfony/blob/3.4/src/Symfony/Component/HttpKernel/KernelInterface.php

Does that mean you're planning to implement all methods defined by this interface? It seems like we need a subset (e.g. getCacheDir(), but not getBundles()).

interface Application - why do we have an interface when it's empty?

Auto-setting properties in Injector::get() hard coded only for Injectable and Configurable adds magic to a system that's already difficult to understand in my opinion. We already have property-based injection in the Injector API, and many devs will go hunting for the behaviour in YAML config because of this (until they end up finding those two special cases in Injector itself).

Aside: Any auto-setting of properties (through existing Injector API or the new Configurable trait) also relies on this class being instanciated through Injector itself. new GridField() fails, $this->container->create('GridField') works. We currently don't give enough guidance on when to use Injector instead of new, which will increasingly become a problem as we refactor our core APIs to declare injectable dependencies.

trait Injectable - this trait already exists and is defined as "A class that can be instantiated or replaced via DI". That's already misleading: Any class can be instanciated via DI, the whole point of DI is to reduce coupling - and hence most classes shouldn't know about DI (or implement a trait). It's just a shortcut which relies on Injector::inst(). Adding a $container property to this adds to this confusion: Just because a class is considered "injectable" (which should be any class) doesn't mean it needs to have access to a DI container. It also promotes bad practices: See Symfony's advice on avoiding your code becoming dependant on the container and PSR-11 notes for details.

Injector::inst() will work with a deprecation notice. No core code that isn't itself deprecated will invoke this.

There's 100+ uses of Injector::inst() in framework/src. I think it's too early to deprecate it's use, and we should successively replace it with actual direct dependencies, rather than just shifting our badly coupled architecture from Injector::inst() to $this->container (which adds API surface that'll be harder to remove later).

Long story short, can we please stick to the Kernel and Application objects for this RFC? We should be able to make this work with the current Injector::inst(), right? I didn't try to slide in a complete DI refactor into this discussion, but the original direction of adding AppState::inst() lead to this (I'm opposed to adding any more ::inst() shortcuts). You can use Injector::inst()->get(Kernel::class) to retrieve the kernel (e.g. for getEnvironment() calls), or just add a dependency on Kernel explicitly through existing the Injector API. My hope is that most of the time you don't need access to the kernel, and have your code properly encapsulated with explicit properties.

Here's an example to illustrate my point:

<?php
use SilverStripe\Core\Kernel;
use MyThing;

// Assumes MyController is instanciated via Injector
class MyController extends Controller {

    private static $dependencies = [
        'kernel' => '%$' . Kernel::class,
        // If MyThing has it's own required dependencies,
        // they should be added via constructor args and wired up via its own $dependencies property
        'myThing' => '%$' . MyThing::class,
    ];

    /** @var Kernel */
    protected $kernel;

    /** @var MyThing */
    protected $myThing;

    function index($request) {
        // MyThing doesn't need a kernel dependency, the behaviour is made explicit.
        // Also doesn't require any use of Injector::inst() or $this->container
        $this->myThing->setDebug($this->kernel->getEnvironment() !== 'live');

        // MyThing doesn't need to know about the request, it just takes an array of parameters
        $this->myThing->doSomething($request->getVars());
    }
}

HTTPApplication->direct($request) should be HTTPApplication->handle($request), which means we can technically implement HttpKernelInterface once we've got PSR-7 sorted. It's a convention which both Symfony and Laravel have. Consequently, CLIApplication->direct() should be CLIApplication->handle($input).

40 pts.

Can you please outline where you see the majority of work here?

tractorcow commented 7 years ago

Does that mean you're planning to implement all methods defined by this interface? It seems like we need a subset (e.g. getCacheDir(), but not getBundles()).

No, just use it as a guide.

interface Application - why do we have an interface when it's empty?

Identification, like we do with TestOnly as an identifier interface.

Auto-setting properties in Injector::get() hard coded only for Injectable and Configurable adds magic to a system that's already difficult to understand in my opinion.

I justify it by Injectable and Configurable being within the same namespace; So it's not a coupling I'm scared to make. It adds more value than it takes away, and doesn't violate the main theme of each trait.

Aside: Any auto-setting of properties (through existing Injector API or the new Configurable trait) also relies on this class being instanciated through Injector itself.

I'd like to encourage this pattern.

new GridField() fails,

new GridField has before and will always bypass DI, unless you want to add a post-constructor hook to Injectable users. Even then, you'd still need to pass in the injector instance to the constructor for EVERY object. We have something similar for Extensible, but forgetting to add the call silently forgets to construct extensions, so it's not a pattern I'm keen to repeat. (see Extensible::constructExtensions())

trait Injectable - this trait already exists and is defined as "A class that can be instantiated or replaced via DI"

I agree it's poorly described. It's intended to mean a class that has ::create() and ::singleton() helper shortcuts.

For then container injection, I'd rather change it's purpose and description than avoid using it though; Make it a new trait, if needed. ContainerAware if you will, and leave current Injectable untouched.

All classes are injectable, and MOST of the time, should declare dependencies that are injected, rather than having a reference to injector itself. However, for classes such as FixtureFactory, it makes sense for them to have the container itself injected, for the production of other DI objects.

There's 100+ uses of Injector::inst() in framework/src. I think it's too early to deprecate it's use, and we should successively replace it with actual direct dependencies, rather than just shifting our badly coupled architecture from Injector::inst() to $this->container

I was against this originally, and it was not until after lots of discussion with you and @sminnee that I was brought around to agreeing with adding this. So... I'm confused by you not wanting the container property anymore.

HTTPApplication->direct($request) should be HTTPApplication->handle($request), which means we can technically implement HttpKernelInterface once we've got PSR-7 sorted. It's a convention which both Symfony and Laravel have. Consequently, CLIApplication->direct() should be CLIApplication->handle($input).

Sure, I'm not fixed on any of the API naming. It was more for the sake of outlining the purpose of each class.

However, back to the discussion on injected core services, do you agree with injecting config via Configurable, or is that the same mistake? Do you want to go back to Config::inst() as well?

I want to go full "add container/config, deprecate old static references", or "purely rely on the existing API, and no container/config" at all. I don't want to mix and match two concurrently / undeprecated approaches.

If this is the case, I want to go back to the RFC and re-draft it without $this->container and $this->config, change the deprecation, and re-implement nesting (it went away, but it's going to come back, and I'll need time to consider how it works).

This will drop the points down to 13-ish, since no longer re-writing the 100+ usages of config/injector::inst(). The new 5 points are due to re-work of request and other static accessors to move into it.

Let's talk tomorrow about this change, and maybe you can explain to me why I think you've changed your mind, or what it was I mis-understood! Thank you.

chillu commented 7 years ago

I was against this originally, and it was not until after lots of discussion with you and @sminnee that I was brought around to agreeing with adding this. So... I'm confused by you not wanting the container property anymore.

I think $this->container is better than Injector::inst(), but worse than explicitly declared dependencies (check my links to Symfony's and the PSR explanations). And that’s more or an architectural pattern and direction in core than something that needs to be solved in this RFC. It just surfaced because you originally proposed AppState::inst(). The "benefit" of Injector::inst() over $this->container is that it doesn't introduce additional API on the component that we'd might want to remove later.

Adding a $container argument on Injectable->create(Injector $container, ...$args) kind of defeats the purpose as a shortcut, MyClass::create($this->container) just as much code as $this->container->create(MyClass::class).

However, back to the discussion on injected core services, do you agree with injecting config via Configurable, or is that the same mistake? Do you want to go back to Config::inst() as well?

In order of decreasing personal preference:

  1. $this->config injected as a class-specific dependency through Config_ForClass with the class as a constructor arg (usage: $this->config->get('myProperty')). This is the cleanest way, done through normal Injector. But it would require additions to the Injector YAML notation (pass name of parent class as constructor arg)
  2. $this->config injected as a singleton dependency through Config (usage: $this->config->get(self::class, 'myProperty')). Easier to inject, less magic, but more writing for devs.
  3. $this->config() added through Configurable trait based injected config dependency (usage: $this->config()->get('myProperty')). Close to current solution, but using DI rather than globals
  4. $this->config() added through Configurable trait based on Config::forClass() (usage: $this->config()->get('myProperty')). Current solution
  5. Config::inst() (usage: Config::inst()->get(self::class, 'myProperty')). Still used nearly 100x in core.

1, 2 and 3 rely on the class being injected (not using new). Making that standard practice is a high impact change to the way SilverStripe apps are written. You'd have to expect that every object (core and project code) needs injected dependencies, or be prepared to pass them in manually. As you say, that should be done in one consistent API change if required, not through a mixed approach like in 3.x.

I want to go full "add container/config, deprecate old static references", or "purely rely on the existing API, and no container/config" at all. I don't want to mix and match two concurrently / undeprecated approaches.

Agreed, but that doesn't block the RFC implementation of App and Kernel, right?

tractorcow commented 7 years ago

Ok, I'm going to revert the parts of the RFC regarding config and injector, as well as configurable and injectable traits; We can keep the current static accessors (at least for 4.0). As per discussion we had on slack, I'll continue with shifting of other statics into HTTPRequest as non-static getters, and will re-implement nesting.

tractorcow commented 7 years ago

Updated RFC without deprecation of Injector::inst() / Config::inst()

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

chillu commented 7 years ago
tractorcow commented 7 years ago

Need to think about that. Maybe talk tomorrow

HTTPResponse->getRequest() ... $request->getCookies()->set(...) - Shouldn’t that be on the response?

I want to get rid of Cookie::get() Session::get(), so making them available via the request, and the request via the response, means they are also available from the response.

E.g. where you would do

Cookies::set('key', 'value');

you would do

$response->getRequest()->getCookes()->set('key', 'value');

I can add getCookies() to response as well, but we in order to ensure both request / response use the same cookies object, I simply made response contain the request this response was generated for.

If we split cookies and session into read / write components, then we don't need that anymore.

If you like I can scope this out, so that $request->getCookies() returns cookies input container, and $response->getCookies() returns cookies output container. This is a larger scope to this refactor though.

chillu commented 7 years ago

$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.