[dev] Injector musings

[Michael M Slusarz]

Ugh... finally getting around to responding.

Quoting Chuck Hagenbuch <chuck at horde.org>:

> Quoting Michael M Slusarz <slusarz at horde.org>:
>
>> 2.) How do you create an instance with an injector if that  
>> instance's constructor has varied arguments?  This seems to be a  
>> limitation of Horde_Injector... is this by design or simply because  
>> nobody has implemented it yet.
>
> The latter, if I'm following. What I want to add to the injector  
> (that came from a completely separate conversation with one of the  
> folks who wrote the initial code) is setter injection. In the case  
> of the Mime_Viewer objects, different viewers would have setFoo()  
> methods for the dependencies that aren't in the constructor (common  
> to all Mime_Viewers). The injector would then satisfy those  
> dependencies as well on object creation.
>
>> For example, Horde configuration needs to be decoupled from  
>> Horde_Mime_Viewer.  However, every Horde_Mime_Viewer instance does  
>> not need every single configuration option passed to it that might  
>> potentially be used by the drivers.  On the other hand, the drivers  
>> may create other Horde_Mime_Viewers internally that may need config  
>> options not needed by the original driver.
>
> For this last case the viewer should have a reference to the  
> injector. Using setter injection this could be accomplished with a  
> setInjector(Horde_Injector $injector) method; the injector would  
> know to pass itself to this when creating the object.
>
>> Regardless, it is very useful to keep the instance around since it  
>> might be used elsewhere in the code.  A better example of this  
>> might be IMP_Contents.  Once initialized, it may be used in various  
>> locations around the code that are not accessible to each other.   
>> How does this work with injector usage?  Or is this the motivation  
>> to retain usage of singletons in this usage pattern?
>
> I'm not sure I entirely follow the question. If the object  
> instantiation/initialization is complicated, then a binder or  
> factory for it seems appropriate. You can then call getInstance()  
> wherever you need it, and the object will be created and initialized  
> only once.

Not sure if I am following here.  Maybe a better example is  
IMP_Contents.  We only want to instantiate one IMP_Contents object for  
each message.  So, for UID 1 in INBOX, today we have the following call:

$contents = IMP_Contents::singleton(1 . IMP::IDX_SEP . 'INBOX');

However, we will most likely use this same object in various locations  
throughout IMP.  We don't want to create a new object - this is just a  
waste of resources.

Currently, the best we can do (I think) is as follows:

class IMP_Contents {
   ...
   function foo(Horde_Injector $injector) {
     return new IMP_Contents($injector->getInstance('UID'));
   }
   ...
}

$injector->bindFactory('IMP_Contents', 'IMP_Contents', 'foo');
$injector->setInstance('UID', 1 . IMP::IDX_SEP . 'INBOX');
$contents = $injector->getInstance('IMP_Contents');

That's not optimal since we are removing the UID definition from the  
actual getInstance() call.  But that's not a deal breaker.

Where things get a bit hackish is when we need to load UID 2 in the  
same session access.  This won't work:
$injector->setInstance('UID', 2 . IMP::IDX_SEP . 'INBOX');
$contents2 = $injector->getInstance('IMP_Contents');

Because $contents2 will be the object containing UID 1.  And I don't  
want to use $injector->createInstance() since I want this newly  
created instance to be available later (the whole point of a singleton).

Thus, the need for something like:
$contents = $injector->getInstance('IMP_Contents', 1 . IMP::IDX_SEP .  
'INBOX');
$contents2 = $injector->getInstance('IMP_Contents', 2 . IMP::IDX_SEP .  
'INBOX');

Then later on, when I call
$injector->getInstance('IMP_Contents', 1 . IMP::IDX_SEP . 'INBOX');

I will get the $contents object that was previously instantiated.

>> 3.) Configuration buried deep within core library functions is  
>> troublesome.  Example - Horde_Auth::setAuth(), which contains a  
>> call to Net_DNS_Resolver, which may potentially be configured via  
>> Horde's config file.  setAuth() is called from a variety of  
>> locations, and is also called internally by a variety of other  
>> library calls.  Passing the resolver object as a parameter isn't  
>> really doable and would be a giant step back in usability.  And it  
>> isn't possible to pass the resolver object to Horde_Auth since it  
>> is not setup to be OO for this kind of use.  Even if we convert to  
>> an OO-model, Horde_Auth is called from so many places that if we  
>> have to add additional parameters to 10's or 100's of calls, this  
>> seems like a step back.
>
> It seems like the static usage of Horde_Auth is a barrier here. If  
> Horde_Auth were used non-statically then you could get a reference  
> to it from the injector and then do what you needed without worrying  
> about objects. We can also make things like the current username  
> (getAuth) available as properties where appropriate so that they  
> don't require accessing an object and a method call.
>
> As an aside, I think one of the reasons there are potentially 100's  
> of static Horde_Auth calls to worry about here is because of the  
> kind of boilerplate code you've been arguing for  
> removing/consolidating. In short: I agree. :)
>
> At that point it doesn't seem unreasonable to pass the  
> Net_DNS_Resolver object to Horde_Auth through either setter or  
> constructor injection.

Yeah - Horde_Auth should probably be fully non-static.  It would  
facilitate passing it around in the future.

>> My solution was to set a static public member variable to a  
>> Net_DNS_Resolver object via the Horde_Registry.  It seems a bit  
>> hackish, but also seems like the cleanest/easiest way to decouple  
>> the configuration from the library.  Comments/concerns on this  
>> approach is much appreciated.
>
> Why not have a binder or factory to create the Net_DNS_Resolver  
> object? I know this answer might not matter if Horde_Auth is still  
> used statically, but see above.

It probably will work.  My hackish solution was just substantially  
easier to implement in the midterm. :)

michael