[dev] [commits] Horde branch master updated. 0729ad4e199098207fec994fbac9d34c72e9b436

[Michael M Slusarz]

Quoting Michael Rubinsky <mrubinsk at horde.org>:

> Quoting Michael M Slusarz <slusarz at horde.org>:
>
>> Quoting Chuck Hagenbuch <chuck at horde.org>:
>>
>>> commit 0729ad4e199098207fec994fbac9d34c72e9b436
>>> Author: Chuck Hagenbuch <chuck at horde.org>
>>> Date:   Sun Oct 3 10:41:27 2010 -0400
>>>
>>>   Revert "Add ondemand injector binders." These are equivalent to  
>>> factories.
>>>
>>>   This reverts commit e27690055fdcf4896b1a11cdecec1de304c1cffc.
>>>
>>> framework/Injector/lib/Horde/Injector.php               |   55  
>>> +--------------
>>> framework/Injector/package.xml                          |    2 +-
>>> framework/Injector/test/Horde/Injector/InjectorTest.php |    8 --
>>> 3 files changed, 4 insertions(+), 61 deletions(-)
>>>
>>> http://git.horde.org/diff.php/framework/Injector/lib/Horde/Injector.php?rt=horde-git&r1=e27690055fdcf4896b1a11cdecec1de304c1cffc&r2=0729ad4e199098207fec994fbac9d34c72e9b436
>>> http://git.horde.org/diff.php/framework/Injector/package.xml?rt=horde-git&r1=e27690055fdcf4896b1a11cdecec1de304c1cffc&r2=0729ad4e199098207fec994fbac9d34c72e9b436
>>> http://git.horde.org/diff.php/framework/Injector/test/Horde/Injector/InjectorTest.php?rt=horde-git&r1=e27690055fdcf4896b1a11cdecec1de304c1cffc&r2=0729ad4e199098207fec994fbac9d34c72e9b436
>>
>> Please revert this -- ondemand injector binders are NOT equivalent  
>> to factories.  Instead, they are a way to reduce overhead during  
>> initialization of the injector.  There is a not-insignificant  
>> amount of overhead in reading all the binder class files from disk  
>> (even if using a cache), creating the PHP data class objects, and  
>> storing these class objects in memory.  Considering that most of  
>> the binders are not used on any given pageload, this is unncessary  
>> overhead.  Additionally, it makes reading debug logs much easier -  
>> instead of having to scroll through several pages of binder  
>> print_r() class string output, there is only a single line for each  
>> declared binder.
>>
>> Additionally, it can be argued that the current behavior of  
>> addBinder() is wrong.  Say, for example, some configuration value  
>> used in a binder changes during the page load.  In the current  
>> system, the old value of the configuration will *always* be used.   
>> Using ondemand binders, the new value of the configuration may be  
>> used if the binder is used after the configuration value changes.
>>
>> I would be all for allowing addBinder() to accept either an object  
>> or a string and then auto-determining how to handle.
>
> Perhaps a summary of the discussions that led to this will help  
> illuminate. The bigger picture here is that our current use of  
> binders is wrong. The majority, if not all, of the classes that  
> exist in horde/core/binders should actually be factories, and that  
> is what we are currently working towards. In this way, the on-demand  
> binder *is* essentially the same as a factory binder...it creates  
> the object on-demand, not when registered.
>
> In the case of complex factories that require configuration  
> parameters to be passed to the create() method, we should be  
> requesting a factory directly from the injector, and not an instance  
> of the object we want e.g.,  
> $injector->getInstance('Horde_Core_Factory_Foo')->create($someParameters)  
> instead of incorrectly adding a new binder that returns a factory.

I understand this part - I had brought up the idea of  
constructors/binders with variable arguments a while back.  Thanks to  
those who worked on this last weekend - I now see the proper use of  
factories in these instances.

But what I stated above still stands - since there remains a number of  
binders that won't be converted to factories.  Example: the  
Horde_Token binder.  This binder's sole purpose is to configure  
Horde_Token in a single location based on the Horde configuration  
(e.g. there is a singleton Horde_Token instance for all Horde apps).

However, this binder is not used on every page load.  It is  
unnecessary overhead to pass an object to addBinder() when we could  
just as easily pass a (string) classname instead, and then only  
instantiate the binder if needed.  It may not be a huge performance  
boost, but the code I previously added has been proven to be work, be  
stable, and it does not add much complexity to the Injector code.

michael

-- 
___________________________________
Michael Slusarz [slusarz at horde.org]