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.
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 {public $backend;}$injector->load(array(array('class' => 'Requirements','properties' => array('backend' => '#$OriginalRequirementsBackend'))));
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')),));
'type' => 'prototype',
'properties' => array(
'db' => '#$MySQLDatabase'
)
),
'type' => 'prototype',
'properties' => array(
'db' => '#$LDAPMemberDatabase' // This would still set DataObject#$db, not a separate Member#$db
)
)
'type' => 'prototype',
'properties' => array(
'db' => '#$MySQLDatabase' // How am I supposed to know this?
)
)
'id' => 'TheMainDatabase'
),
'type' => 'prototype',
'properties' => array(
'db' => '#$TheMainDatabase'
)
),
'type' => 'prototype',
'properties' => array(
'db' => '#$TheMainDatabase'
)
)
'id' => 'TheMainDatabase'
)
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.
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.
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.
> > 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
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));
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?
--
> 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.!
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
-------
--
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 intentrather 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 comparingit with potentially dozens of config files that could contain DI specs.
> 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?
--
> 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.
Does this mean that this proposed DI Framework could be removed
entirely from Sapphire, and perhaps live in a decorator or module
instead?
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.
> 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. :-)