(SS3) Dependancy Injection and SS3

75 views
Skip to first unread message

Hamish Friedlander

unread,
Mar 17, 2011, 12:54:33 AM3/17/11
to silverst...@googlegroups.com
Hi All,

There's been some discussion on this list about Dependancy Injection and I've spent some time recently thinking about this. So, even larger wall of text away!

=== A brief introduction to dependancy injection (skippable if you know it all already :) ===

Dependency Injection is a very simple pattern in concept - replacing "getting an object by calling new" with "using a reference to an instance of an object that's already been set up for you"

We already do something similar to basic dependency injection sometimes in SS2.x. For instance the requirements code does this:

class Requirements {
private static $backend = null;
public static function backend() {
if(!self::$backend) {
self::$backend = new Requirements_Backend();
}
return self::$backend;
}
}

The actual Requirements class has some methods for setting $backend, but lets ignore those for now. You can see that without extra code there's no way to use any other type of object in place of backend.

Let's tweak that code a bit

class Requirements {
public static $backend_class = 'Requirements_Backend';
private static $backend_singleton = null;

private $backend;
public function __construct() {
if (!self::$backend_singleton) {
$backendClass = self::$backend_class;
self::$backend_singleton = new $backendClass();
}

$this->backend = self::$backend_class;
}
}

Now we've made backend much more natural to reference ($this->backend instead of self::backend()), _and_ we've allowed the use of some other class instead of Requirements_Backend.

$this->backend remains a singleton however - every instance of Requirements shares the same instance of Requirements_Backend. Sometimes we want a new instance for every object. That's pretty easy too:

class Requirements {
public static $backend_class = 'Requirements_Backend';

private $backend = null;
public function __construct() {
$backendClass = self::$backend_class;
$this->backend = new $backendClass();
}
}

This is dependancy injection in it's simplest form. It's limits are clear - all the configuration and setup has to happen _before_ the injection (which in this case is === _construct). You can't change what backend_class is set to dynamically half way through a request and have all the $this->backend properties update to point to a different instance. You can't have $this->backend give different instances based on the state of the system.

Despite this it's a very powerful concept. I'll give some examples of why later.

A "Dependancy Injection framework" is just code that makes the above patterns easy to achieve without too much copy-pasta. It also adds methods for configuring the classes in a nicer manner that having to set a bunch of statics on every instance.

=== Marcus' Dependancy Injection Framework ===

A while ago Marcus posted to this list with his Dependancy Injection framework. This was timely as I'd been thinking about how hard it would be to add dependency injection to SilverStripe to ease some testing problems I had. We had a bit of a conversation about it, and I said I'd think about what I wanted from a dependancy injection framework, and then the thread dried up.

Marcus' framework wasn't developed to be SilverStripe specific, but can be used by SilverStripe. 

The advantages of Marcus' framework are

- It's already there
- It's in use
- It's mostly integratable with SilverStripe without issues

So one option is to just add this framework as is, hooking it into Object's __construct. However (Marcus, correct me if I'm wrong) it has an issue that might make it unsuitable as is for use as a system-wide component. When injecting, the name of the property is the only hook used to determine what class is injected there.

For example, let's rewrite the Requirements example above to use

class Requirements {
public $backend;
}

Done. That was easy. There's some configuration that needs to happen, but basically now if a class exists called "Backend", it'll get constructed and injected into $backend when Requirements is constructed. You can also override that class with some other class (glossing over the actual configuration needed to do that for now).

The problem is that any other object that happens to have a property called $backend will _also_ get the same instance. This is because the property name is tied to the class name of the type of object we're expecting. For example

class WebDownloader {
public $backend;
}

Will end up with an instance of Backend too. The obvious solution is to use a more specific property name

class Requirements {
public $requirementsBackend;
}

However everywhere else that wants that same object needs to also change their names. And if you want two different instances, or that name clashes with an existing property?  Problems.

And what if someone writing some other class just happens to declare a property called $requirementsBackend, not knowing that we'll be sticking a value in there because it happens to be a property name some other code has decided is special? 

A second issue that comes up which is less problematic is that the configuration structure of Marcus' framework is a little bit weird, at least to me. It doesn't integrate nicely with the proposed configuration system, or with _config.php. That's pretty easily over-comeable though.

=== Google Guice ===

Dependency Injection frameworks have been in use in more static languages like Java for a while. However, they've typically been pretty heavy, involving lots of XML and other nasties. Spring, probably the most famous DI framework is like this.

However a couple of years ago Google released Google Guice, which is an extremely light weight DI framework for Java.

In Java you can't just do (examples in psuedo-PHP, because Java is horrible)

public $foo;

And then jam any type of object you want into foo, because Java is statically typed.

But what you can do is define an interface, like

interface Foo { .... }

Then define a property like

public Foo $foo;

This ends up being a really useful hook. It now means that it doesn't really matter what the name of the property is, we've already got better metadata to use to decide what type of thing to inject in there - the type.

Google Guice then lets you do something like

class Bar implements Foo { ... }

Guice->bind('Foo')->as('Bar');

There's one last step that Guice makes you do. Since we don't nessecarily want every property that happens to be typed as an interface that is injectable to _be_ injected, we have to mark where we expect injection 

@Inject
public Foo $foo;

=== What I'd like ===

I'd like to see a blend of Marcus' system and Google Guice system, something like this:

interface RequirementsBackend { ... }
Injection::bind('RequirementsBackend')->as('StandardRequirementsBackend');

class Requirements {
/** @Inject RequirementsBackend */
public $backend;
}

I know people don't like that "annotations in comments" style, but we've got to store metadata somewhere. An alternative might be

class Requirements {
static $injections = array(
'backend' => RequirementsBackend
);

public $backend;
}

This gives us a more flexible hook system, and it shouldn't take too much work on top of what Marcus has already done to get this to work.

=== Where would we use this? ===

I promised some reasons why DI is powerful earlier, so here we go

** Getting the database connection instance

Currently DataObject does something like this

class DataObject {
function write() {
$con = DB::getConn(); $con->execute($sql);
}

Using dependancy injection, this would look like

class DataObject {
/** @Inject Database */
private $db

function write() {
$this->db->execute($sql);
}

Not only does this read nicer, it would let us _use different database backends based on the model's class_. This would finally let us use models stored in legacy databases, among other things.

** Making static classes like session & requirements replaceable in a generic way 

Currently there are lots of utility classes where most of the methods are called statically, like Session::get() and Requirements::javascript(). These then have had backends or instance systems added to them ad-hoc, but (a) this is ugly, and (b) they are a maintenance nightmare.

Better is something like:

class MyController extends Controller {
/** @Inject Session */
$session

function action(){ $this->session->get('Foo'); }
}

and

class MyController extends Controller {
/** @Inject Requirements */
$require

function action(){ $this->require->css('monkeys.css'); }
}

You can also replace the default session with a "more secure" implementation for your secure controllers, or the requirements backend with one that does javascript combining on live.

** Mocking a restful service for testing

Often you want to be able to swap one API providing class for another when testing. Accessing remote services is a good example of this.

class SomeModel extends DataObject {
/** @Inject InterfaceWithSomeRestfulService */
private $service;

function Name() { return $this->service->getUser()->Name; }
}

Then in a test you can replace $service with a mock that returns pre-recorded values

I'll end this there for now - this is already getting long enough that a lot of people will be TL;DNR. We haven't covered how to configure overriding what to inject, or specifying when you want an instance per object versus when you want a singleton. But that will come at least partially from the new config system work.

Does anybody have any thoughts & opinions? I'll probably start a fork of SS3 with some of this stuff in it fairly soon, so people can play with what's possible & what isn't.

Hamish Friedlander
SilverStripe

Sam Minnée

unread,
Mar 17, 2011, 1:17:02 AM3/17/11
to silverst...@googlegroups.com
Marcus, what do you think of Hamish's proposal?

Marcus Nyeholt

unread,
Mar 17, 2011, 2:29:21 AM3/17/11
to silverst...@googlegroups.com
I think I'm going to have a late night tonight finishing slides and writing a proper response! :)

From the sounds of things, the DI framework already supports a fair amount of what's proposed, just a little differently to how it's set out here, and probably because the configuration is not immediately obvious. I'll put together stuff that goes into how to achieve the above things (at least, as I understand them) a bit later this evening. 

Another thing to add - more as a reminder to myself than anything - is the concept of merging  service definitions instead of overriding them. I'll try and add some more info about what I mean in my longer-to-come post. 

Marcus

On Thu, Mar 17, 2011 at 4:17 PM, Sam Minnée <s...@silverstripe.com> wrote:
Marcus, what do you think of Hamish's proposal?

--
You received this message because you are subscribed to the Google Groups "SilverStripe Core Development" group.
To post to this group, send email to silverst...@googlegroups.com.
To unsubscribe from this group, send email to silverstripe-d...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/silverstripe-dev?hl=en.


Marcus Nyeholt

unread,
Mar 17, 2011, 8:21:02 AM3/17/11
to silverst...@googlegroups.com
Okay so here's my longer response. From the looks of things, the kind of functionality is either already supported (just in a different way), or took me 10 minutes to add in. There's probably still a few things that can be improved though. 

> The problem is that any other object that happens to have a property called $backend will _also_ get the same instance. This is because the property name is tied to the class name of the type of object we're expecting. 

This is only true if you're relying on automatic variable injection, either on a public var or via a setter. 

You can specify in configuration the explicit bean you want bound for a property - either via constructor or property setting. 

class Requirements {

protected $backend;

public function __construct($backend) {
$this->backend = $backend;
}
}


$injector->load(array(
array(
'class' => 'Requirements',
'constructor' => array(
'#$OriginalRequirementsBackend'
)
)
));

$requirements = $injector->get('Requirements');

To change the requirements backend being used, you'd just change the reference from #$OriginalRequirementsBackend to #$NewRequirementsBackend

This is pretty much the same as 


class Requirements {
protected $backend;

public function setBackend($backend) {
$this->backend = $backend;
}
}

// or

class Requirements {
public $backend;
}


$injector->load(array(
array(
'class' => 'Requirements',
'properties' => array(
'backend' => '#$OriginalRequirementsBackend'
)
)
));


I've updated the test cases with an example in the method "testRequirementsSettingOptions" - check https://github.com/nyeholt/injector/blob/master/testing/testcases/TestInjector.php

So, if the class you want to change the property for is something you create lots of (eg a DataObject subclass) and you want different items set for each type, you'd specify the config for each data object type, and make sure they're defined as prototype beans so new instances are created each time. Something like

$injector->load(array(
'Member' => array(
'type' => 'prototype',
'properties' => array(
'db' => '#$LDAPMemberDatabase'
)
),
));


One thing that the injector doesn't support is using type trees to determine bindings, and to be honest, I'd prefer to keep it that way. You'd still create other data object types using the injector->get() method, so they'd still be injected using the default lookup mechanism. This could be an area where a lookup for metadata on the type is done - and would be relatively simple to implement (actually, simple enough that I just added it in the last 10 minutes). 

> specifying when you want an instance per object versus when you want a singleton. But that will come at least partially from the new config system work.

Already covered by specifying 'type' => 'prototype' in a spec for when you want a new object returned each time. 


>  I'll probably start a fork of SS3 with some of this stuff in it fairly soon, so people can play with what's possible & what isn't.

I actually started playing around with it in my fork of sapphire a few weeks ago - of course at the time I completely forgot to branch it so I've had the joy of undoing and redoing and all sorts of fun. At least it was better than trying the same with SVN. https://github.com/nyeholt/sapphire/tree/injector_integration - you'll need to have the injector repo checked out  as a module. There's a bunch of new config stuff in sapphire/_config.php that should point you on a merry trail.... ;)


Brain hurts, and I have slides to finish, so stuff about merging service definitions will need to wait for another time. 

Cheers,

Marcus

Hamish Friedlander

unread,
Mar 17, 2011, 5:49:04 PM3/17/11
to silverst...@googlegroups.com
Okay so here's my longer response. From the looks of things, the kind of functionality is either already supported (just in a different way), or took me 10 minutes to add in.

I hoped so - I was planning on basing anything I did on top of yours :).
 
There's probably still a few things that can be improved though. 

> The problem is that any other object that happens to have a property called $backend will _also_ get the same instance. This is because the property name is tied to the class name of the type of object we're expecting. 

This is only true if you're relying on automatic variable injection, either on a public var or via a setter. 

You can specify in configuration the explicit bean you want bound for a property - either via constructor or property setting. 

<snipped first example>

Although this isn't relevant to the argument at hand, it's worth keeping in mind that if we bind injection into Object::__construct then you're not going to be able to use constructor arguments for injected objects.
 
Also, you keep saying bean. Is there something special you're meaning there, or just "an object with getters/setters & private properties"?

class Requirements {
public $backend;
}

$injector->load(array(
array(
'class' => 'Requirements',
'properties' => array('backend' => '#$OriginalRequirementsBackend')
)
));

That's cool, and something I didn't get to specifically in my design - how to specify on a class by class basis. But for instance in the case of $db, _most_ of the classes are going to want the same instance/type of object injected, and maybe only a couple, or none at all, will have it replaced with some other type.

So, if the class you want to change the property for is something you create lots of (eg a DataObject subclass) and you want different items set for each type, you'd specify the config for each data object type, and make sure they're defined as prototype beans so new instances are created each time. Something like

$injector->load(array(
'Member' => array(
'type' => 'prototype',
'properties' => array(
'db' => '#$LDAPMemberDatabase'
)
),
));
 
So using just the above syntax, we'd have to put a $db on DataObject, and then make the injector deal with having the injected class overridden by type (maybe it already does), like
 
$injector->load(array( 
'DataObject' => array( 
'type' => 'prototype', 
'properties' => array( 
'db' => '#$MySQLDatabase' 
),
'Member' => array( 
'type' => 'prototype', 
'properties' => array( 
'db' => '#$LDAPMemberDatabase'  // This would still set DataObject#$db, not a separate Member#$db
)
));

An example: lets assume I'm writing a class that also needs the normal database connection instance, but doesn't inherit from DataObject

class Foo extends Object {
private $db;
}

Now I have to explicitly specify that I expect the same type of database there too

$injector->load(array( 
'Foo' => array( 
'type' => 'prototype', 
'properties' => array( 
'db' => '#$MySQLDatabase'  // How am I supposed to know this?
)
));

Is that what the "#$Bar" syntax is supposed to do? I don't really get what the difference between that and just saying "Bar". Would it be something like this?:

$injector->load(array( 
'MySQLDatabase' => array( 
'id' => 'TheMainDatabase' 
),
'DataObject' => array( 
'type' => 'prototype', 
'properties' => array( 
'db' => '#$TheMainDatabase' 
),
'Foo' => array( 
'type' => 'prototype', 
'properties' => array( 
'db' => '#$TheMainDatabase' 
)
));

And if someone else is writing a test, and they want to suppress database writing by making TheMainDatabase be replaced with an instance of MemoryOnlyDatabase instead, how do they do that without knowing that Foo also now has an injection? Is it enough to do:

$injector->load(array( 
'MemoryOnlyDatabase' => array( 
'id' => 'TheMainDatabase' 
)
));

at some later stage?

One thing that the injector doesn't support is using type trees to determine bindings, and to be honest, I'd prefer to keep it that way. You'd still create other data object types using the injector->get() method, so they'd still be injected using the default lookup mechanism. This could be an area where a lookup for metadata on the type is done - and would be relatively simple to implement (actually, simple enough that I just added it in the last 10 minutes). 

We wouldn't be using injector->get() in normal code. I'm assuming that inject will be bound into Object::__construct. Replacing new with injector->get() everywhere you want a Model instance is (a) way too much of a backwards-incompatible break, and (b) at least IMO really ugly, and I'd be against it.

Adding it to __construct has a major benefit though - we can ignore access rights as far as setting properties goes, so we can just have a private $foo, without requiring a setFoo() for injection.

> specifying when you want an instance per object versus when you want a singleton. But that will come at least partially from the new config system work.

Already covered by specifying 'type' => 'prototype' in a spec for when you want a new object returned each time. 

That was talking about how I'd like to see configuration done, which I mostly skipped over to keep the email short. A weird thing from the above examples though: type looks like it's currently specified per injectee class (Requirements, DataObject), not per injection. Is that right?
 
>  I'll probably start a fork of SS3 with some of this stuff in it fairly soon, so people can play with what's possible & what isn't.

I actually started playing around with it in my fork of sapphire a few weeks ago - of course at the time I completely forgot to branch it so I've had the joy of undoing and redoing and all sorts of fun. At least it was better than trying the same with SVN. https://github.com/nyeholt/sapphire/tree/injector_integration - you'll need to have the injector repo checked out  as a module. There's a bunch of new config stuff in sapphire/_config.php that should point you on a merry trail.... ;)
 
Cool. I'll take a look now. I don't know if I'll neccesarily fork off from your branch, maybe cherry pick what I want in. If I want to play with annotations, I'm going to need to pull in AJ Short's stuff too. 
 
Brain hurts, and I have slides to finish, so stuff about merging service definitions will need to wait for another time. 

Having looked at your examples, and the id stuff (assuming I understand it right), it looks like we could code similar to the way I'd like with the Injector stuff as is.

We'd at the very least need to come up with a way for this stuff to be specified in the static config, whatever that ends up looking like.

I'm still prefer the annotation style though. It's basically the same as IDs, but expressed in a way I find more natural. I've had a hard-ish time understanding Injector's configuration system, and I mostly know how DI and AOP works already. I'd be worried that if we introduced Injector as is, it just wouldn't get used as most people would ignore it as being too hard.

So, a question to the community (excluding Marcus and I): Which do you find clearer?

class DataObject {
private $db;
}

class Foo {
private $db;
}

singleton('Injector')->load(array(
'MySQLDatabase' => array(
'id' => 'TheMainDatabase'
),
'DataObject' => array( 
'properties' => array('db' => '#$TheMainDatabase')
),
'Foo' => array( 
'properties' => array('db' => '#$TheMainDatabase')
)
));


OR:

class DataObject {
/** @Inject TheMainDatabase */
private $db;
}

class Foo {
/** @Inject TheMainDatabase */
private $db;
}

singleton('Injector')->load(array(
'MySQLDatabase' => array(
'id' => 'TheMainDatabase'
)
));

Note that this ignores the static configuration (since it might apply equally to either option) - we might end up with something like:

class DataObject {
/** @Inject TheMainDatabase */
private $db;
}

class Foo {
/** @Inject TheMainDatabase */
private $db;
}

--- 100-data-handler.yml
Injection:
TheMainDatabase: MySQLDatabase

Marcus, feel free to present an example equally biased towards your preference if you like :).

Hamish Friedlander
SilverStripe


Marcus Nyeholt

unread,
Mar 17, 2011, 8:12:47 PM3/17/11
to silverst...@googlegroups.com, Hamish Friedlander


> Although this isn't relevant to the argument at hand, it's worth keeping in mind that if we bind injection into Object::__construct then you're not going to be able to use constructor arguments for injected objects.

True, but using property/setter injection works much the same way. 
 
> Also, you keep saying bean. Is there something special you're meaning there, or just "an object with getters/setters & private properties"?

Nope nothing special, just a java hangover - just a plain old php object (POPO).

> That's cool, and something I didn't get to specifically in my design - how to specify on a class by class basis. But for instance in the case of $db, _most_ of the classes are going to want the same instance/type of object injected, and maybe only a couple, or none at all, will have it replaced with some other type.

> So using just the above syntax, we'd have to put a $db on DataObject, and then make the injector deal with having the injected class overridden by type (maybe it already does), like

Yep, assuming the type of the object being injected has specific configuration for it, it will just set those properties - even if the object doesn't explicitly define a variable, thanks to PHP letting you set whatever the hell you want :). If that var is defined in a parent class, it just sets that. 

The example below is a bit of an extension of the previous one where I've actually defined the dependencies. Basically, you'd never have to bother defining configuration for the base case (DataObject), because it would just automatically pick up the Db configured POPO through property discovery (either public prop or setter). It's ONLY when you want to do something different from the norm that you'd have to specify configuration

$injector->load(array( 
'Db' => array( 
'class' => 'MySQLDatabase',
),
'LdapMemberDb' => array(
'class' => 'LdapDatabase',
),
'LdapGroupDb' => array(
'class' => 'LdapDatabase',
),
'Member' => array( 
'type' => 'prototype', 
'properties' => array( 
'db' => '#$LDAPMemberDatabase' 
),
'Group' => array(
'type' => 'prototype', 
'properties' => array( 
'db' => '#$LdapGroupDb'  
)
));




> An example: lets assume I'm writing a class that also needs the normal database connection instance, but doesn't inherit from DataObject
> class Foo extends Object {
> private $db;
> }
> Now I have to explicitly specify that I expect the same type of database there too

Nope - just rely on auto discovery via property name or setter. Whatever you have configured as the 'Db' in the injector will be set. 

> Is that what the "#$Bar" syntax is supposed to do? I don't really get what the difference between that and just saying "Bar". Would it be something like this?:

the #$Name syntax tells the injector to resolve the name to a configured service - otherwise, it's just treated as a string (or other primitive). 


> And if someone else is writing a test, and they want to suppress database writing by making TheMainDatabase be replaced with an instance of MemoryOnlyDatabase instead, how do they do that without knowing that Foo also now has an injection? Is it enough to do:
> $injector->load(array( 
> 'MemoryOnlyDatabase' => array( 
> 'id' => 'TheMainDatabase' 
> )
> ));
> at some later stage?

Yes, though the array key is actually the ID, and you'd tell it the class to create like

$injector->load(array( 
'TheMainDatabase' => array( 
'class' => 'MemoryOnlyDatabase' 
)
));

This won't have any affect on objects that have already been created and injected before the new config is loaded. 

> We wouldn't be using injector->get() in normal code. I'm assuming that inject will be bound into Object::__construct. Replacing new with injector->get() everywhere you want a Model instance is (a) way too much of a backwards-incompatible break, and (b) at least IMO really ugly, and I'd be against it.

> Adding it to __construct has a major benefit though - we can ignore access rights as far as setting properties goes, so we can just have a private $foo, without requiring a setFoo() for injection.

On the one hand it's explicit and clear to an end user that there is some magic going on, but it definitely is backwards incompatible and users would probably forget it. By "adding it to __construct" do you mean actually implementing the injection logic in Object? Doing $injector->inject($this) from __construct will still need public $vars or settors for privately defined variables to be able to access them from the injector. 

> That was talking about how I'd like to see configuration done, which I mostly skipped over to keep the email short. A weird thing from the above examples though: type looks like it's currently specified per injectee class (Requirements, DataObject), not per injection. Is that right?

Yes, per type. 

* prototype - Will create a new instance of the required dependency each time
* singleton (the default) - Will re-use the same configured instance for each required dependency. Note that this is not quite the same as the singleton pattern - in that, the SingletonClass::instance() mechanism guarantees that only one instance of SingletonClass will ever exists; in the injector, singleton means that there will only ever be one instance of a class for a given ID. This means (as per the example above with the LdapDatabase) you can actually have many named instances of a class, but wherever referenced, it will be the same for that name. 
 
> Cool. I'll take a look now. I don't know if I'll neccesarily fork off from your branch, maybe cherry pick what I want in. If I want to play with annotations, I'm going to need to pull in AJ Short's stuff too. 

Yep, I'd expect you to start afresh, but it should hopefully provide some clarity around use. 


> We'd at the very least need to come up with a way for this stuff to be specified in the static config, whatever that ends up looking like.

Yep, having seen the info you've already posted about the new config stuff, it looks like it'll be straightforward to support. 

> I'm still prefer the annotation style though. It's basically the same as IDs, but expressed in a way I find more natural. I've had a hard-ish time understanding Injector's configuration system, and I mostly know how DI and AOP works already. I'd be worried that if we introduced Injector as is, it just wouldn't get used as most people would ignore it as being too hard.

The configuration is based on how Spring defines things, using an ID as the primary naming mechanism, that is bound to an instance (or prototype) of a particular class. All subsequent binding is done using that named ID. 

> So, a question to the community (excluding Marcus and I): Which do you find clearer?

Just to clarify the example a bit - due to the auto-resolution, you'd never actually specify any of the dependencies explicitly

class DataObject {
public $db;
}

class Foo extends DataObject {

}

class Bar {
private $db;
public function setDb($db) {
$this->db = $db;
}
}

singleton('Injector')->load(array(
'Db' => array(
'class' => 'MySQLDatabase'
),
'AlternateDb' => array(
'class' => 'OtherDatabaseType',
)
));


After injection, this would end up with 

DataObject::$db = instance of MySQLDatabase by public property setting
Foo::$db = instance of OtherDatabaseType by configured property setting
Bar::$db = instance of MySQLDatabase by setter injection

Rewriting the above using annotations, the only thing that would change is the Foo definition, as the other two are injected automatically based on the name resolution. 

class Foo extends DataObject {
@Inject AlternateDb
public $db;
}

singleton('Injector')->load(array(
'Db' => array(
'class' => 'MySQLDatabase'
),
'AlternateDb' => array(
'class' => 'OtherDatabaseType',
)
));


Another option would be to get rid of the configuration altogether and do something along the lines of what you had

class DataObject {
@Inject MySQLDatabase
public $db;
}

class Foo extends DataObject {
@Inject AlternateDb
public $db;
}

class Bar {
private $db;
@Inject MySQLDatabase
public function setDb($db) {
$this->db = $db;
}
}

which would imply the relevant service creation, but the ability to add additional config to the service definition is then lost. 

I guess in the end I'm against putting much metadata about how to inject something in the injectee class directly - the whole point is to remove dependency information from a class and externalise it in a container. Sticking too much data in there is not much better than manually creating the object directly. 

Marcus

Sam Minnée

unread,
Mar 17, 2011, 6:53:42 PM3/17/11
to silverst...@googlegroups.com

Although I was initially skeptical about annotations, they've come up in several examples now and I"m beginning to think that they might be the best solution to the problem of how to specify information like the injection meta-data here.

It seems that one of the key points of difference between Hamish's and Marcus' approach is what you normally use to define your injection points. By default, Marcus was injecting by property name and Hamish was injecting by Interface/Base-class. It seems as though Hamish's approach is better in that case: repeated instances of a interface/baseclass across the application are more likely to require the same injection than repeated instance of a property name. To put it another way: classes & interfaces have a semantic weight that property names do not: implementors of classes are free to use whatever property names they like, but the interface names they use *must* match up with the rest of the application.

This is allowed:
$this->myDBFieldHasADifferentName = new Database();

This is not:
$this->db = new IDecidedToCallDatabaseSomethingDifferentFromTheRestOfSapphire();

I should note, however, that this distinction doesn't apply to the example above: in both cases, the arbitrary ID "TheMainDatabase" has been used. I don't particularly like this for a default case: rather than saying "TheMainDatabase" I'd prefer for the main database to be defined as the *default* injection for a Database. I would expect 80% of injections to be against interface/baseclass names and 20% against IDs.

The thing that I like about Hamish's method is that the mapping of injection points is partially configured in the class. As the creator of the class, I can define my class's relationship to the standard injection points without

We *could* provide for this in Marcus' model, by having two $injector->load() calls: one to define TheMainDatabase and one to map TheMainDatabase to Foo->db. However, this is an extra piece of scaffolding which Hamish's system is free of (opting instead to bundle this information in the docblock comment of the property.

This grouping together of related information in one place seems to me to be better API design.

Sam Minnée

unread,
Mar 17, 2011, 9:40:52 PM3/17/11
to silverst...@googlegroups.com
> I guess in the end I'm against putting much metadata about how to inject something in the injectee class directly - the whole point is to remove dependency information from a class and externalise it in a container. Sticking too much data in there is not much better than manually creating the object directly.

At a minimum, the right balance is specifying the kind of interface that the populated properties should be loaded with (base classes, for the purposes of this discussion, also specify interfaces). Objects that indicate what kind of interface their properties are expected to match can only be a good thing.

"@Inject AlternateDatabase" does seem a little bit fishy, because like Marcus says, it should be up to the injector to decide things at that level.

All that said, I expect that we're unlikely to get into a good position if we try and solve this by going in one direction or the other 100% of the time.

This is my thinking, thus far:

- Injection points should be annotated to say that they're injection points, to make it easier to know when injection configuration is broken
- Injection points should be setters/public-properties, rather than constructor arguments
- Injection points should be annotated with an interface that the passed property is expected to conform to
- It should be possible to inject by a number of different criteria; you can choose what makes the most sense for your needs:
- All injection points expecting this interface
- All injection points expecting this interface on that class
- All properties with this name
- All properties with this name on that class
- Giving arbitrary names to injection points shouldn't be implemented until we have a use-case that demonstrates its need. In particular, we should confirm that "All injection points expecting this interface on that class" isn't a better way of injecting to these points.
- Many of the base injection points that we set up initially should be configured with "All injection points expecting this interface" - Database, Mailer, Configuration.
- The config system that Hamish spec'd out should be able to be used to specify injections.

Sam Minnée

unread,
Mar 17, 2011, 10:05:52 PM3/17/11
to silverst...@googlegroups.com
On 18/03/2011, at 1:12 PM, Marcus Nyeholt wrote:

> > Is that what the "#$Bar" syntax is supposed to do? I don't really get what the difference between that and just saying "Bar". Would it be something like this?:
>
> the #$Name syntax tells the injector to resolve the name to a configured service - otherwise, it's just treated as a string (or other primitive).


I find the #$ thing kind of ugly, and I'm wondering if it's overkill to completely separate the definition of services from the application of it to selected classes. Perhaps it would be simpler if the two were combined?

In this example, I've set up a default SS_mysite database for most objects, but Member, Group, and Permission are stored in a separate database.

$injector->load(array(
'applyTo' => 'Database',
'type' => 'singleton',
'class' => 'MySQLDatabase',
'params' => array('username', 'password', 'localhost', 'SS_mysite'),
));

$injector->load(array(
'applyTo' => array('interface' => 'Database', 'within' => array('Member', 'Group', 'Permisson)),
'type' => 'singleton',
'class' => 'MySQLDatabase',
'params' => array('username', 'password', 'localhost', 'SS_authentication'),
));

Alternatively, you could take a 'lots of little objects' approach like what happens in Forms.

$injector->applyToAll('Database',
new SingletonService('MySQLDatabase', array('username', 'password', 'localhost', 'SS_mysite')));

$injector->applyToSome('Database', array('Member', 'Group', 'Permission'),
new SingletonService('MySQLDatabase', array('username', 'password', 'localhost', 'SS_authentication'));

The advantage of this approach is that you could use PHP to do your service re-use:

$injector->applyToAll('Database',
new SingletonService('MySQLDatabase', array('username', 'password', 'localhost', 'SS_mysite')));

$authDBService = new SingletonService('MySQLDatabase', array('username', 'password', 'localhost', 'SS_authentication');
$injector->applyToSome('Database', array('Member', 'Group', 'Permission'), $authDBService);
$injector->applyToSome('Database', array('Group', 'Permission'), $authDBService);

If you want to take a step further, you could use a fluent API:

$injector->interface('Database')->apply(new SingletonService('MySQLDatabase', array('username', 'password', 'localhost', 'SS_authentication'));
$injector->interface('Database')->within('Member', 'Group', 'Permission')->apply(new SingletonService('MySQLDatabase', array('username', 'password', 'localhost', 'SS_authentication'));

And to remove excessive punctuation, you could use Object::create_from_string:

$injector->interface('Database')
->apply(new SingletonService("MySQLDatabase('username', 'password', 'localhost', 'SS_authentication')");

$injector->interface('Database')->within('Member', 'Group', 'Permission')
->apply(new SingletonService("MySQLDatabase('username', 'password', 'localhost', 'SS_authentication')");

-------
Sam Minnée | Chief Technology Officer
SilverStripe
http://silverstripe.com

Phone: +64 4 978 7334
Skype: sam.minnee

Michael Gall

unread,
Mar 17, 2011, 11:07:30 PM3/17/11
to silverst...@googlegroups.com
Hi guys,

I've been watching this conversation and thought I'd drop my 2c in.

I was the person that introduced the Requirements_Backend into the codebase, with the express idea that it would be easier test. I suppose that's the main goal of introducing this DI into the codebase, and it's a noble goal. Now bear with me here, I think DI is great to some extent but I think you need to take a step back and look at what you are creating.

What's wrong with a far more basic approach? The current discussion looks like something out of an enterprise java stack, that isn't going to be pleasant to use or hack on.


Something like,


class DataObject {
  function db() {
    return DIFactory::generate("db");
  }
}


class DIFactory {
  /**
  * @var Database
  */
  static public $db = null;
 
  function generate($var) {
    if(!is_null($this->$var)) {
      return $this->$var;
    } else {
      return $this->$var = new DefaultInferredByAnnotation.
    }
}

Then in the _config.php file you can do something as simple as,

DIFactory::db = new MySQLDatabaseWith whatever configuration we want.

Then to add a new DI you create the function in the object that needs injection, and a static public variable in the DIFactory class. Alternatively, the DI store could just use an associative array.

Cheers

Michael


--
Checkout my new website: http://myachinghead.net
http://wakeless.net

Marcus Nyeholt

unread,
Mar 17, 2011, 11:10:28 PM3/17/11
to silverst...@googlegroups.com, Sam Minnée
> It seems that one of the key points of difference between Hamish's and Marcus' approach is what you normally use to define your injection points.  By default, Marcus was injecting by property name and Hamish was injecting by Interface/Base-class.  It seems as though Hamish's approach is better in that case: repeated instances of a interface/baseclass across the application are more likely to require the same injection than repeated instance of a property name.  To put it another way: classes & interfaces have a semantic weight that property names do not: implementors of classes are free to use whatever property names they like, but the interface names they use *must* match up with the rest of the application.

It's actually not that different - my default-default approach is to inject based on class. The simplest injector config looks like the following 

$injector->load(array(
'SomeService',
'Database',
'FileSystem',
));

which is just a list of class names, which is effectively saying that in the injector is now registered an object of type Database that can be referenced by the name Database. These are automatically injected based on name, so the following would auto resolve stuff

class MyClass {
public $someService;
public $database;
public $fileSystem;

The key difference is how to go about resolving the issue where this naming doesn't match up, so Hamish's proposal for annotations

class MyClass {
/** @Inject Database */
public $db;
}

$injector->load(array(
'Database',
));

would be functionally the same as the existing functionality of

class MyClass {
public $db;
}

$injector->load(array(
'Database',
'MyClass' => array(
'properties' => array(
'db' => '#$Database',
)
)
));

the difference being that the existing functionality externalises the setting of the configuration. One benefit in the @annotated form is that if you were injecting a subclass of MyClass, the annotation would be inherited down, so the assignment would be the same, and not fall back to name based resolution (as per the current functionality - although the static::$injections functionality I just added would cover that case). 

Limiting injection points to only be able to inject by interface or type could be a bit problematic - how do you resolve which to use in the following situation? 

class SQLiteDatabase implements Database {

}

class MySQLDatabase implements Database {

}


class DataObject {
/** @Inject Database */
public $db;
}

// uses sqlitedb for some background storage of data or something like that
class BackgroundProcessor {
/** @Inject Database */
public $db;
}

It'd be fair to say that you wouldn't expect the same object to be injected to both, but if instead we used the type to resolve things (eg replace @Inject Database with @Inject SQLiteDatabase and @Inject MySQLDatabase), what happens when we have two different objects of the same type that are dependencies for different things? 

$injector->load(array(
array(
'class' => 'MySQLDatabase',
'constructor' => array(
'webuser', 'webpass', 'localhost'
),
),
array(
'class' => 'MySQLDatabase',
'constructor' => array(
'otherappuser', 'otherapppass', 'other.app.host'
),
)
))

class DataObject {
/** @Inject MySQLDatabase */
public $db;
}

// uses sqlitedb for some background storage of data or something like that
class BackgroundProcessor {
/** @Inject MySQLDatabase */
public $db;
}

which is then chosen? They're different databases used for different purposes, but without having some arbitrary way to name them, you can't rely on either the interface or type to make the choice in consumer objects. 


> We *could* provide for this in Marcus' model, by having two $injector->load() calls: one to define TheMainDatabase and one to map TheMainDatabase to Foo->db.  However, this is an extra piece of scaffolding which Hamish's system is free of (opting instead to bundle this information in the docblock comment of the property.

You wouldn't need two separate calls to load, just a single one that configures multiple items at once, but you're right, the @Inject annotation removes the need for explicitly stating this in config

> I find the #$ thing kind of ugly, and I'm wondering if it's overkill to completely separate the definition of services from the application of it to selected classes.  Perhaps it would be simpler if the two were combined?

It is kind of necessary though to have an identifier of injections vs scalar values. Take the following example

class MySQLDatabase {
public $cache;
public $user;
public $pass;
}

$injector->load(array(
'CacheService' => array(
'class' => 'ZendCacheWrapper',
),
'Database' => array(
'class' => 'MySQLDatabase',
'properties' => array(
'user' => 'username',
'pass' => 'password',
'cache' => '#$CacheService',
),
)
));

In this case the @Inject annotation would be enough to add on $cache to remove the need for it here, but for more complex use cases it becomes more needed. The example in https://github.com/nyeholt/sapphire/blob/injector_integration/_config.php is setting up a list of request filters, each of which needs to actually be resolved to the relevant object. It allows for much more complex dependencies to be defined, but I can't see it being used too regularly, especially as an annotation approach will cover the majority of situations where it'd be needed. 

>  wondering if it's overkill to completely separate the definition of services from the application of it to selected classes. 

From what I can gather from the example stuff, it's just some syntax and method calls that under the covers would just be setting configuration parameters the same as below

$injector->load(array(
'Database' => array(
'class' => 'MySQLDatabase',
'constructor' => array('user', 'pass', 'etc'),
)
'UserDatabase' => array(
'class' => 'MySQLDatabase',
'constructor' => array('other','user','andhost')
),
'Member' => array(
'class' => 'Member',
'properties' => array(
'db' => '#$UserDatabase',
)
)
));

class DataObject {
/** @Inject Database */
public $db;
}

This is a good example of the benefits of keeping the configuration of dependencies separate from the actual code; If you want to change how your Member objects are being saved, you're simply changing the configuration, the Member object itself never knows any different. Of course, that's probably more benefit in a compiled language. 



Sam Minnée

unread,
Mar 17, 2011, 11:25:48 PM3/17/11
to silverst...@googlegroups.com
> What's wrong with a far more basic approach?
> Something like,
>
> class DataObject {
> function db() {
> return DIFactory::generate("db");
> }
> }
>
>
> class DIFactory {
> /**
> * @var Database
> */
> static public $db = null;
>
> function generate($var) {
> if(!is_null($this->$var)) {
> return $this->$var;
> } else {
> return $this->$var = new DefaultInferredByAnnotation.
> }
> }
>
> Then in the _config.php file you can do something as simple as,
>
> DIFactory::db = new MySQLDatabaseWith whatever configuration we want.

Quite a few things:

- You need to define a custom function db() in DataObject instead of having it automatically populated for you. So it's more work for hackers - they need to write scaffold.
- You need to keep track of custom generate() keys instead of using something with real PHP meaning like interfaces or base classes.
- You need to modify the DIFactory class, creating a new static for each use of the DI system. Admittedly this could be fixed by populating an array or something.
- There's not really a good way of loading in an entirely new dependency mapping for testing - once again, you're going to be manipulating a bunch of statics, but this time it's on DIFactory.
- You can't populate db() with different values depending on the DataObject class, should you need to
- You need to instantiate every object passed to the DI system regardless of whether or not it's used. The current config system suffers heavily from this.
- You can't configure the injector to generate a new instance for each related object.

Sure, your suggestion is simpler, and it would probably do for simple use-cases, but to me it only really seems simpler from the point of view of the person writing the DI system. It seems no easier for SilverStripe developers to use, and is missing features that we'd really like to see.

> The current discussion looks like something out of an enterprise java stack, that isn't going to be pleasant to use or hack on.

Don't confused a complex discussion around *designing* a feature with a complex discussion around *use* of a feature. Although I didn't like the use of the word "bean", either. ;-)

To do the equivalent of what you've described in your example, would be this:

DataObject {
/** @inject Database */
public $db;
}

In the _config.php (and if you think this config line is too complex then that's up for discussion. I happen to like fluent syntax but it's not meant to be complex)

DI::inst()->interface("Database")->apply("MyDatabase('username',...)");

That seems reasonably simple to me. If you wanted backward compatibility with the existing $databaseConfig option, you could put this in the core code somewhere.

global $databaseConfig;
$params = $databaseConfig;
unset($params['type']);
DI::inst()->interface("Database")->apply(array('class' => $databaseConfig['type'], 'params' => $params));


Sam Minnée

unread,
Mar 17, 2011, 11:37:08 PM3/17/11
to SS Dev

> In this case the @Inject annotation would be enough to add on $cache to remove the need for it here, but for more complex use cases it becomes more needed. The example inhttps://github.com/nyeholt/sapphire/blob/injector_integration/_config.php is setting up a list of request filters, each of which needs to actually be resolved to the relevant object. It allows for much more complex dependencies to be defined, but I can't see it being used too regularly, especially as an annotation approach will cover the majority of situations where it'd be needed.

I think that maybe we're heading too far down the complexity rabbit-hole in trying to allow for these situations, *especially* because as a consequence it's making simple cases look more complicated. Given those examples, I can understand Michael Gall's concerns, although I don't think it's about the overall DI framework so much as a few specific features.

We *will* need to draw a line somewhere, where we say "this feature might theoretically be nice, but the additional complexity it adds to complex situations isn't justified".

In particular - I think it must be possible to avoid named services in 95% of situations.
I also think that using PHP is going to be better than making our own complex DSL in PHP arrays.

What did you think of the fluent & SingletonService approach in my previous email?

Marcus Nyeholt

unread,
Mar 17, 2011, 11:41:04 PM3/17/11
to silverst...@googlegroups.com, Michael Gall
That's essentially what singleton() lets you do now - there's just a lot of boilerplate code needed to support it

Ingo Schommer

unread,
Mar 18, 2011, 12:16:22 AM3/18/11
to silverst...@googlegroups.com
Sam wrote: "Don't confused a complex discussion around *designing* a feature with a complex discussion around *use* of a feature. "

This is an important point. I won't pretend to understand every aspect of this discussion,
but in general very much look forward to *using* dependency injection (alongside with an easy way to configure it).

For anybody being similarly lost in the matter, I can recommend Symfony's tutorial of building
out a hardcoded dependency into first a service, and then a builder for services.

Marcus/Hamish: Please let me know if that example is more misleading than helpful,
apart from the property=id linkage and class-specific configuration discussed here it looks
fairly similar to me.

@Sam: They've come up with a pretty cool fluent interface as well, see

My 2 cts: Even after reading Marcus' example in his DI module a couple of times,
I was absolutely confused by the property=id autolinking magic. Maybe its my lacking Java background? ;)
There's been numerous arguments against this since in this discussion, mainly around developers expressing intent
rather than unknowingly adding a DI to their code. I much prefer Hamish's annotations,
as it makes injection points clear reading through the actual code, rather than comparing
it with potentially dozens of config files that could contain DI specs.

Ingo

Marcus Nyeholt

unread,
Mar 18, 2011, 12:24:40 AM3/18/11
to silverst...@googlegroups.com
Ingo - yep, I think what was missing from my earlier email was that I think an annotation on the object is a much clearer way for people to understand what's going on, and that until people get a grasp on things, the biggest thing is to make it easier to become a consumer of a DI container as opposed to a provider. 

So based on a couple of skype chats, a summary of the kind of injection to support

class MyClass {
public $dbService;
}

will NOT be injected by anything

class MyClass {
/** @Inject DbService */
public $dbService;
}

will have $dbService set to an instance of the object configured at 'DbService' in the injector, assuming the following configuration

$injector->load(array(
'DbService' => array(
'class' => 'MySQLDatabase',
)
));


To provide for more advanced configurations, you could specify the binding via the configuration

$injector->load(array(
'DbService' => array(
'class' => 'MySQLDatabase',
)
'MyClass' => array(
'dbService' => '#$DbService'
)
));

This is more useful in something like

class Logger {
public $loggers;

public function log($message, $level = 'debug') {
$this->loggers[$level]->write($message)
}
}

with the following configuration 

$injector->load(array(
'EmailLogger',
'FileLogger' => array(
'properties' => array(
'outputFile' => '/var/log/silverstripe'
)
)
'Logger' => array(
'properties' => array(
'loggers' => array(
'debug' => '#$FileLogger',
'fatal' => '#$EmailLogger',
)
)
),
'MyClass' => array(
'logger' => '#$Logger'
'dbService' => '#$DbService'
)
));

So TODO out of that

- change the injector to NOT do property scans of objects being injected to figure out new injections
- add support for annotation specification for properties - there's already support for binding properties based on a static $injections array, this may be a point to add this in
- change configuration approach to support 'Identifier' => 'ClassName' instead of the overly explicit 'Identifier' => array('class' => 'ClassName') form

So to summarise the allowed configuration

array(
'Identifier', // Will create an instance of class Identifier that can be referenced by Identifier
'Identifier' => 'ClassName', // Will create an instance of ClassName that can be referred to by Identifier
'Identifier' => array( // Specify a complete specification
'class' => 'ClassName'
'properties' => array(
'name' => 'value', // will set a property called 'name' on the object
)
)
)


Generally, I'd imagine most cases will be just array('Identifier', 'OtherId') being used to get objects into the injector, then @Inject used on classes to specify what should be bound. The next most likely being cases of Identifier => array('properties' => array('username', 'password')) to provide default config. But this type of stuff I'd imagine will be read from the new configuration framework stuff. 

--

Sam Minnée

unread,
Mar 18, 2011, 12:32:45 AM3/18/11
to silverst...@googlegroups.com
On 18/03/2011, at 5:16 PM, Ingo Schommer wrote:

> I much prefer Hamish's annotations,
> as it makes injection points clear reading through the actual code, rather than comparing
> it with potentially dozens of config files that could contain DI specs.


I spoke with Marcus on Skype, and as I understood it, we got a point where everyone like injection points. The improvement that we suggested was not only using interface names for injection points, but allowing for named sub-instances of a given interface.

Here, we are binding to two different loggers, and 1 cache, but we're asking to give us a cache that doesn't need to be preserved between pageviews (a replacement of all those cache vars scattered throughout the system).

class MyThing {
/** @inject SS_Log.Error */
public $errorLog;

/** @inject SS_Log.Debug */
public $debugLog;

/** @inject SS_Cache.NonPersistent */
public $cache;

}

And here we're injecting the dependencies. In this example, the same logger will be used for both errors and debug, and an SS_MemoryCache object is being used for our non-persistent cache. The Zend_Cache_File isn't actually being used by MyThing, but could be used by others for their non-persistent cache.

singleton('Injector')->load(array(
array('interface' => 'SS_Log', 'class' => 'SS_FileLog', 'arguments' => array('everything.log')),

array('interface' => 'SS_Cache', 'class' => 'Zend_Cache_File'),
array('interface' => 'SS_Cache.NonPersistent', 'class' => 'SS_MemoryCache'),
));

Note: the example is a sketch; the precise argument structure is still being established. In fact, this will probably end up in config.yml files once Hamish is finished with that side of things.

YAY D.I.!

Sam Minnée

unread,
Mar 18, 2011, 12:35:43 AM3/18/11
to silverst...@googlegroups.com
What do you think about using Object::create_from_string() to provide a concise way of specifying arguments to a class?

On 18/03/2011, at 5:24 PM, Marcus Nyeholt wrote:

> - change configuration approach to support 'Identifier' => 'ClassName' instead of the overly explicit 'Identifier' => array('class' => 'ClassName') form

-------

Marcus Nyeholt

unread,
Mar 18, 2011, 12:56:37 AM3/18/11
to silverst...@googlegroups.com
A little bit icky, but Hamish has already stated that in many cases the injection would be done after an object is created already (to preserve some backwards compatibility with code that uses new Object() everywhere). 



--

Mark Stephens

unread,
Mar 18, 2011, 1:00:19 AM3/18/11
to silverst...@googlegroups.com
Hi.

On Fri, Mar 18, 2011 at 5:16 PM, Ingo Schommer <in...@silverstripe.com> wrote:

My 2 cts: Even after reading Marcus' example in his DI module a couple of times,
I was absolutely confused by the property=id autolinking magic. Maybe its my lacking Java background? ;)
There's been numerous arguments against this since in this discussion, mainly around developers expressing intent
rather than unknowingly adding a DI to their code. I much prefer Hamish's annotations,
as it makes injection points clear reading through the actual code, rather than comparing
it with potentially dozens of config files that could contain DI specs.

I agree. While I see some merit in making the injection transparent to the class, what I see in practice is confused developers who are not aware that magic is taking place, especially on project teams. The annotations make it clear within the the scope of a class that there is an intent to inject something.

I think if Ingo is confused by something, we're really going down the wrong path :-)

Mark

Sam Minnée

unread,
Mar 18, 2011, 1:05:38 AM3/18/11
to silverst...@googlegroups.com
On 18/03/2011, at 5:56 PM, Marcus Nyeholt wrote:

> A little bit icky, but Hamish has already stated that in many cases the injection would be done after an object is created already (to preserve some backwards compatibility with code that uses new Object() everywhere).

Hrm, your response suggests that I didn't communicate myself properly.

I meant that instead of:

singleton('Injector')->load(array(
'Database' => array('class' => 'MySQLDatabase', 'arguments' => array('username', 'password')),
));

You can go:

singleton('Injector')->load(array(
'Database' => "MySQLDatabase('username', 'password')",
));

All it means is that when you're instantiating the object to pass into the injection point, you create it by passing the string to Object::create_from_string.

It should be fairly simple to implement and I'm happy to make a patch once you get the rest of it done, unless you had some objection to the functionality existing... It *does* bind the DI stuff to Object, which might not be nice.

Or am I missing something?

Marcus Nyeholt

unread,
Mar 18, 2011, 1:52:17 AM3/18/11
to silverst...@googlegroups.com
Yeah, the 'icky' bit was the Object::create_from_string binding - but it can probably be done by passing in an 'ObjectCreator' or something that would be responsible for creating objects based on the defined class. SilverStripe could then pass in one that uses Object::create_from_string. I'll add to my list of things. 

The other bit was me just doing too many things at once and not reading what you wrote properly :D





--

Sam Minnée

unread,
Mar 27, 2011, 6:13:05 PM3/27/11
to silverst...@googlegroups.com
On 18/03/2011, at 6:52 PM, Marcus Nyeholt wrote:

> Yeah, the 'icky' bit was the Object::create_from_string binding - but it can probably be done by passing in an 'ObjectCreator' or something that would be responsible for creating objects based on the defined class. SilverStripe could then pass in one that uses Object::create_from_string. I'll add to my list of things.

The other thing we could do is migrate Object::create_from_string() into the DI framework, since there's nothing Sapphire-specific about it - it's just a way of extracting construction-signatures from strings using the PHP-tokeniser.

dalesaurus

unread,
Mar 28, 2011, 12:05:59 PM3/28/11
to SilverStripe Core Development
Does this mean that this proposed DI Framework could be removed
entirely from Sapphire, and perhaps live in a decorator or module
instead?

Sam Minnée

unread,
Mar 28, 2011, 3:50:44 PM3/28/11
to silverst...@googlegroups.com
On 29 March 2011 05:05, dalesaurus <dale....@gmail.com> wrote:
Does this mean that this proposed DI Framework could be removed
entirely from Sapphire, and perhaps live in a decorator or module
instead?

Probably not.  It's going to become an integral part of the way that we build SilverStripe 3.
 

dalesaurus

unread,
Mar 29, 2011, 4:04:23 PM3/29/11
to SilverStripe Core Development
Are you sure it is going to be integral? Because I have yet to see a
single concrete example of where or how any of this is going to be
used in practice. The hypothetical uses put forth here just don't
sell me on DI making SS3 easier to read, code, or especially debug
(and that is without a nod to performance).

This dialog makes it feel like DI is a foregone conclusion since we
are seeing implementation details directly from the Core SS team
before identifying the actual problems attempting to be solved. I
haven't seen much talk about improving existing Decorator or OnBefore/
OnAfter calls, which also address the same domains as DI. I believe
those are a better avenue as they are currently well understood, very
legible, modular, use minimal overhead, and can be traced by standard
completion/debug systems.

Yesterday in another thread, Sam, you threw out that a new feature
needed to be "implement[ed] it in a way that didn't cause confusion
for developers". There is nothing about any of the above proposals
that satisfy that requirement.

Keep in mind that new users to SS are typically going to be entry to
intermediate level PHP developers/designers on a bell curve of
skills. Without a background in Java/Ruby/Python the entire concept
of DI is foreign largely because PHP doesn't natively support the
inheritance/override features attempting to be mimicked from those
other languages.

DI is super trendy right now (at least in Zend and Symphony). Looking
over their respective documentation doesn't lend much to the case of
it being something easy to digest or critical for anything but
exceptional use cases. Particularly with regard to the added
complexity, overhead, and obscurity it brings versus existing Sapphire
solutions. I look forward to being proven wrong here.

I am assuming there have been a number of off-list discussions about
this since the idea seems pretty well formed in the context presented
here. Can you share how those resulted in the idealogical approach of
being 100% set on building a DI framework? That would be a good start
towards understanding the rationale behind the big push for this
feature (for me at least).


I'm not trying to be a jerk here, I just want to highlight some
counterpoints since the discussion has been largely one-sided so far.


On Mar 28, 2:50 pm, Sam Minnée <s...@silverstripe.com> wrote:

Sam Minnée

unread,
Mar 29, 2011, 4:27:43 PM3/29/11
to silverst...@googlegroups.com
The biggest reason is probably unit testing. Without DI, it's really hard to do testing. We have a bunch of static methods and properties and every single one of these needs to be carefully catered to by the testing framework. Through a tedious and unreliable process of trial and error, SapphireTest::setUp() is now littered with a bunch of reset() and other calls, which kind of works, unless you do anything new in your project code. It screams "there must be another way!"

And to respond to the potential criticism "shouldn't you design your code to be executed, not testing": The biggest advantage of Test Driven Development - the reason that it creates a measurable reduction in bugs that post-development automated-testing doesn't - is because it forces you do create a better design. So the very things that we're encouraged to do are the things that make SilverStripe a better software framework.

Another reason is that we have different half-arsed implementations of DI peppered throughout the code - whether it's Object::create() and Object::useCustomClass(), Requirements / Requirements_Backend, or, from a certain perspective, the Extension system itself. It's also a pattern I have ended up using in a limited way on about 2/3 of the projects I've worked on over the last year.

Finally, DI is really useful in situations where you're integrating with external systems or there are complex pieces of your application that you'd like to swap out with a simple mock-up, whether it's for testing, developing locally without being hooked into the necessary VPN, or just to keep the complexity of that other system from interfering with the task at hand.

We're not adopting DI because it's trendy, we're adopting DI because for the last 18 months many of us have been screaming "OH GOD I WISH WE HAD DEPENDENCY INJECTION!" every other week.

An admission: DI is most useful when you're working on more complex systems, and not everyone doing that. SilverStripe still needs to cater to those users who want to just get a nice, customisable CMS up and running quickly & easily. Therefore, although it would be integral to the core, it should be relatively easy to ignore. As an analogy, PHP-PEG is going to be an integral part of the core (it's the parser grammar that powers the SSViewer rewrite), but most people can ignore that.

One of the issues with that claim is that simplicity is on the far side of complexity: we need to establish the DI framework, figure out how it gets configured, and then set up some defaults using that configuration scheme so that people who don't care about it can ignore it.

So, time for a call to action: We're only going to have sensible, easy to use defaults if developers out there tell us that the defaults we've made are wrong. So, when the first versions of this start hitting the master branch, and it's not working well for the simple cases, tell us! Post your code samples and say "THIS IS TOO HARD". For the discussion to be productive & objective, I think it will need to be based around specific code samples.

Marcus Nyeholt

unread,
Mar 29, 2011, 9:03:46 PM3/29/11
to silverst...@googlegroups.com

@dalesaurus Generally, the usage of DI will be invisible to people unless they specifically want to know about it and use it. There may be a few changes to the way some bits of the system are interacted with, but the fact that it's DI behind that will be invisible to end users. 

So, where you might currently write code like

class Page extends DataObject {

public function Items() {
return DataObject::get('Page', '"RelationID" = ' . $this->ID);
}
}

you might instead write

class Page extends DataObject {

public function Items() {
return $this->dataSource->get('Page', '"RelationID" = ' . $this->ID);
}
}


The fact that dataSource has been injected into Page via a DI container (as opposed to created in a constructor or set explicitly) is completely transparent as far as the end developer is concerned. 

To elaborate on what Sam means about testing, consider the following which could be an example of how code is written with statics, or localised creates

class CustomPage extends Page {

public function ImportantPeople() {
$list = new ArrayObject();

// localised create
$ldap = new LdapServer('details');
$people = $ldap->find('cn=SomeBranch');

// or using a static
$people = LdapServer::find('cn=SomeBranch');

foreach ($people as $person) {
// filter it out or something
}

return $list;
}
}

Add to this the fact that your _config.php now has stuff like

LdapServer::$host = 'ldap.server.com';
LdapServer::$user = 'user';

etc

How do you write a test for that? You can't do a true 'unit' test, because you're relying on external data, which you have no control over. You can write an integration test, but it means that your ldap server needs to be running while testing which is a massive pain if you're wanting to use continuous integration to manage things. 

Instead, how about

class CustomPage extends Page {

public $ldapServer;

public function ImportantPeople() {
$list = new ArrayObject();
$people = $this->ldapServer->find('cn=SomeBranch');

foreach ($people as $person) {
// filter out plebs
}

return $list;
}
}


Not a huge change, but it means now that for testing purposes (either when writing actual test cases, or running on a developer machine), you can simply substitue $ldapServer with a dummy or mock instance. 

public function testImportantPeople() {
$page = new CustomPage(); 
$page->ldapServer = new MyDummyLdapServer();

// or, more correctly
$mockLdap = $this->getMock('LdapConnector');
$mockLdap->expects($this->once())
->method('find')
->with($this->equalTo('cn=SomeBranch')
->will($this->returnValue(array('fake, controlled ldap data')));
$page->ldapServer = $mockLdap;
}


But there's a bit of an issue - in SilverStripe, page objects are instantiated automatically by the framework, meaning you don't get a chance to set this*. Also, what if you had an authenticator using the same Ldap connection? Wouldn't it make sense to have both objects use the same connection? 

So, we define the objects like

class CustomPage extends Page {
/** @Inject LdapServer */
public $ldapServer;
}

class LdapAuthenticator {
/** @Inject LdapServer */
public $ldapServer;
}


The DI container takes care of injecting both these properties automatically with the class registered as LdapServer, which could be done via configuration as follows

$injector->load(array(
'LdapServer' => array(
'class' => 'LdapConnector',
'properties' => array(
'host' => 'ldap.server.com',
'user' => 'user',
'pass' => 'pass',
'dn' => 'dn=com,cn=silverstripe'
)
)
// or using a more straightforward approach
'LdapServer' => "LdapConnector('ldap.server.com', 'user', 'pass', 'dn=com,cn=silverstripe')"
));

Of course, this configuration will be a bit more simplified once the yaml based config stuff is available. 



* In this specific example, you could put some code in the constructor to create the ldapServer object, which would still allow later overrides, but in general this is a bad thing to do. 



To address some specific points

> "new users to SS are typically going to be entry to intermediate level PHP developers/designers on a bell curve of skills"

Which is precisely why we should have something like DI to instill proper development practices from the start, instead of picking up the pieces of convoluted junk later on. 

> Without a background in Java/Ruby/Python the entire concept of DI is foreign largely because PHP doesn't natively support the inheritance/override features attempting to be mimicked from those other languages.

There are no features of those languages that lend themselves more or less to DI, or that we're trying to mimic or replicate in PHP by using DI. The concept of DI itself (assigning a dependency from outside a class instead of creating it inside, or Inversion of Control as it's also known) is a simple development pattern, completely language agnostic and should be relatively simple to grasp. Using a DI container to simplify and centralise the process of performing DI is just the next logical step. 

>  Particularly with regard to the added complexity, overhead, and obscurity it brings versus existing Sapphire solutions.

The problem is there aren't really any existing solutions to the problems described above. Sure, you have a combination of singleton(), Object::create(), Object::useCustomClass(), but that's a hodgepodge mix of stuff that no-one really uses, or uses in a way that's actually helpful.  

I've posted previously as to what specific areas I think would benefit by having DI to better structure design, but to re-iterate

* Data interaction - eg dataService->getById, dataService->getOne etc
** Data providers - for interacting with different DB engines
** Query objects - query builder stuff - instead of new SQLQuery all over the place
* Transaction management - transactionManager->runInTransaction($someClosure)
* Search service - searchService->index($dataObject), searchService->query(). There's 3 or 4 different search mechanisms now, it'd be nice to make them conform somewhat to a standard interface
* Permission control - making permission checks a little easier, using AOP to add checks to method calls to prevent access. Different permission services depending on permission requirements of the site
* Template engine management - easily swap in and out different template engines
* WebServices - use AOP or a more explicit proxy pattern to make services available over the web, doing auto conversion to web-consumable data types
* Workflow interaction - light on or heavy weight workflow management depending on registered implementation
* Conversion/Transform Service - define a clearer interface to different content transformation options
** ImageTransform
** PdfTransform
** VideoTransform
* Versioning and staging - providing different implementations of these where necessary, have a cleaner interface than the existing static calls to an extension's class
* Email sending - Providing a cleaner developer interface for sending emails
* Authentication handling - clean up authentication and authorisation mechanisms to be at a higher level of the system, and a more unified method of defining new authentication providers. An example that shows how this could work can be seen at
* Configuration management - making it easier to get access to configuration options from all parts of the system
* Logging - Making logging simple for developers to effectively use
* Session management - make it easier to switch session implementations
* Cache management - help to control the configuration and management of cache engines






Jason Stratford

unread,
Mar 30, 2011, 5:01:10 AM3/30/11
to silverst...@googlegroups.com
Nice post Marcus, clarified a lot of points for me. I think that it is clear that the move to DI is based on solid reason and ground work. Probably the only thing that is missing to reduce general concern is some solid examples and documentation to clarify everything. I am sure that will be available in time.

Jason

dalesaurus

unread,
Apr 1, 2011, 12:40:47 PM4/1/11
to SilverStripe Core Development
Thanks to you both for your replies. Marcus, that was a Wiki-worthy
dissertation on the subject. I am particularly grateful for someone
finally putting some context around actual practical uses.

>There are no features of those languages that lend themselves more or less to DI
I should clarify my statement then, PHP is harder to bend in to
dynamic overrides because of it's single inheritance coupled with
explicit (and one way) traversal from parent to child.
Python has method decorators or method object storage, Ruby has
aliases, Java has many approaches but I am most familiar with dynamic
proxies. PHP offers no way to automatically traverse back up the
inheritance chain without explicit mechanisms...which leads to messy-
yet-required code for any classes implementing a tricky pattern like
DI. The fact that all implementations here have string-to-object
evaluation, external non-php configuration, or the totally hacky
"annotations" suggest that nobody has figured out how to solve this in
straight PHP in an elegant fashion.

To digress for a moment about Annotations, in the form of /** @Inject
LdapServer */ as shown, I have concerns about the referenced
implementations. These load all php code from disk as strings and run
the entire thing through regular expressions. At runtime...EVERY
CALL. This is a bad practice on so many levels. While it is the most
transparent/legible option presented it brings considerable overhead.
+1 for Sam's YAML configuration, along the lines of the current
_config.php situation.

One important thing for tracing will probably be logging Injections
somewhere when wiring them up and allowing them to be shown in stack
traces with SS's custom error handlers. Or perhaps namespacing them
if PHP 5.3 is going to be required (use getter/setter to switch
namespace, execute call, switch back. Exceptions should then be clear
about what/where it was executed).

I look forward to playing with implmentations and hopefully striking a
balance between explicit when you need it, magic when you don't...and
not scanning the entire codebase with regexes :)

Sam Minnée

unread,
Apr 1, 2011, 10:22:11 PM4/1/11
to silverst...@googlegroups.com
The concerns you raise are completely valid, so thanks for giving this issue the critical thought that you have. It's feedback like this that will help make the SilverStripe 3 codebase the best it can be. :-)

> The fact that all implementations here have string-to-object
> evaluation, external non-php configuration, or the totally hacky
> "annotations" suggest that nobody has figured out how to solve this in
> straight PHP in an elegant fashion.

Yes, we're trying to find the best way to work within PHP's limitations, however, I see DI as being *more* useful in less dynamic languages, and indeed I have seen arguments along the line of "Ruby is a language that is so flexible that you don't need a DI framework". My hope is that most of the scaffolding can be baked into the Object class so regular devs don't need to worry about it too much.

> To digress for a moment about Annotations, in the form of /** @Inject
> LdapServer */ as shown, I have concerns about the referenced
> implementations. These load all php code from disk as strings and run
> the entire thing through regular expressions. At runtime...EVERY
> CALL. This is a bad practice on so many levels. While it is the most
> transparent/legible option presented it brings considerable overhead.

Yes I agree, this would be a horrible way of doing things. :-) Hamish is "the man with the plan" on this one, but as I understand it, the approach will be to parse annotations and cache the parsed result, re-caching only when the relevant code file changes. This process would be tied into ManifestBuilder, I expect. Having seen Hamish's work on the SSViewer rewrite I am confident that he'll build this in a robust manner. I assume that PHP-PEG will be used again, rather than regexes.

> +1 for Sam's YAML configuration, along the lines of the current
> _config.php situation.

I see this as being used in conjunction with annotations, rather than being an alternative. Some of the configuration lived more naturally within the code (specification about what kind of thing needs to be injected there) and some of it lives more naturally outside of the code (exactly what needs to be injected). I do, however, think that in most cases the yaml config will be preferable to a PHP-based configuration, particularly since we're moving the rest of the config into yaml files.

> One important thing for tracing will probably be logging Injections
> somewhere when wiring them up and allowing them to be shown in stack
> traces with SS's custom error handlers. Or perhaps namespacing them
> if PHP 5.3 is going to be required (use getter/setter to switch
> namespace, execute call, switch back. Exceptions should then be clear
> about what/where it was executed).

I totally agree - good debugging tools can be the different between and unusable nightmare and a must-have tool. :-)

Denis 'Alpheus' Čahuk

unread,
Apr 5, 2011, 4:56:54 AM4/5/11
to SilverStripe Core Development
As far as configuration is concerned, a native (even if slightly
limited) approach could be to use ReflectionParameter (http://
www.php.net/manual/en/class.reflectionparameter.php) and use parameter
hinting to inject into constructors and pass lazy dependencies with
the use of setters. However, idyllic as this might seem, it is only
useful to pass around meta-data about *what* to inject, not *which*
(e.g. Guice's @Named injection points) or whether to inject at all.

But as you already touched the dynamic language debate - is dependency
provision something that should be handled on a native or a
documentation level? What I mean to say by this is that if for example
you have a class of type Foo whose constructor requires parameters
$bar and $baz, PHP allows to us to define this as __construct($bar,
$baz). It does not *require* type hinting nor PHPDoc nor
documentation-based annotations. This might make the intent of the
class/interface less clear, but it is still PHP.
How much of a coding structure would you require to enforce onto
developers to configure a dependency injector? With YAML or
documentation-based annotations probably none at all, coupled with
type hinting for setters and constructors to make the life of Unit
test authors a lot easier.
Reply all
Reply to author
Forward
0 new messages