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

[Michael J.Rubinsky]

Quoting Michael M Slusarz <slusarz at horde.org>:

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

This really shouldn't be a binder though, the way I understand it, a  
binder class defines broad instructions on how to find how to obtain  
an object. Having a binder class extended to describe how to create a  
*specific* object isn't really what the injector binders are for. This  
is the job of a factory.  This should be moved to Core_Factory with  
the factory managing the caching of the single instance internally.

These types of binders are planned to be moved, but haven't all been done yet.

--mike

The Horde Project
http://www.horde.org