[Solar-talk] Proposal: Remove namespace to Solar::loadClass()/factory()

[Travis Swicegood]

Howdy all,

One thing in Solar that's been bugging me is the concrete class 
dependencies that it has.  Code like this makes me cringe:

$sql = Solar::factory('Solar_Sql');

My whole problem with it is that it creates a dependency on Solar_Sql 
and keeps me to inserting my own Domain51_Sql code.  My solution, as 
Paul has heard many (he would add "many, many" I believe) times is some 
sort of Dependency Injection model where you don't request the class, 
but it's interface.  Paul's not a fan of this model, and it is 
definitely more esoteric and probably not the easiest to grok.  I have, 
however, come up with a model that I think will work quite well and is a 
step in that direction: namespaces.

$sql = Solar::factory('Sql');

Currently, the Solar_Controller_Front allows you to specify via a config 
value the "classes" that it will search.  What I'm proposing is that 
Solar::loadClass() start using its config['namespaces'] to load classes 
in a similar manner.  loadClass() would also need to start returning the 
name of the class that it found.

The reasoning for this is my work on the Domain51_Service_* classes the 
last few weeks.  Those are classes I would definitely like to see in 
Solar, however anything I do with them right now requests the objects as:
    $s3 = Solar::factory('Domain51_Service_Amazon_S3');

This is fine, until they make it to Solar.  If they do, all of my apps 
are going to be obsoleted unless I maintain dual versions of the Service 
code or change all of my factory() calls.  If the code were as follows 
instead, I wouldn't have anything to change:
    $s3 = Solar::factory('Service_Amazon_S3');


This is a step toward a DI-type system, without a change to 
__construct() for full on Constructor Injection (a la PicoContainer).  
This keeps Solar::loadClass() working as a Service Locator, but 
introduces real namespaces by removing them from the the requests.  
Obviously, I would like to see Interfaces introduced so you can always 
verify that the methods you need will always be available regardless of 
what object comes back, but that's not an immediate need for this 
particular change.


== Required code changes ==
To make this change happen - three things need to be done.  The first, 
and obvious is the change to loadClass().  It has to be able to know 
about its namespaces, apply them, and catch exceptions when the file 
isn't found instead of throwing them again.  It also needs to return the 
name of the real file that it expects.  That leads to a change I didn't 
foresee - throwing an exception in Solar::run() would cause an issue 
with namespacing because Solar::exception() relies on the loadClass() 
functionality.  You quickly hit an infinite loop that way on the first 
namespace.  I've changed it to allow you to throw a regular exception, 
but it could easily throw Solar_Exception since Solar should always know 
exactly where it is.  The last change that needs to be made is the 
factory() method.  It needs to know which class to instantiate, so that 
changes to instantiate the class based on what loadClass() returns.

There's another change in there that I found out (and is recorded in 
issue 30 on Trac).  Asking for a config value via Solar::config() that 
was false would return null even if a default value was provided.  This 
is used with a check against 'bc_compat:namespaces' to allow this code 
to work with the old style names by stripping the Vendor_ portion off of 
the requested class.

The attached patch works without issue with all of the Solar demo code.  
If you specify the following config values, it will work with any 
Solar-based code without modification thanks to the bc_compat:namespaces 
setting.

$config['Solar']['namespaces'] = array(
    'Solar',
    'VendorOne',
    'VendorTwo',
    'Domain51' // cause everyone uses my code, right? ;-)
);


Anyhow, this email has become long enough.  Thoughts, comments, 
suggestions?  Am I the only one that sees value in removing the 
namespace from the requested code?

-Travis

P.S.  I wrote the majority of this email yesterday, but let it sit 
through the night to see if I thought of anything else.  One additional 
change I think might be useful is to change the signature of loadClass() to:

public function loadClass($class, $concrete = false)

By specifying "true" as the second parameter, you could override the 
namespace code.  This would be necessary for sub-classes that need to 
insure the specific super-class they expect is loaded.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: namespaces.patch
Type: text/x-patch
Size: 4043 bytes
Desc: not available
Url : http://mail.killersoft.com/pipermail/solar-talk/attachments/20061107/3a5c2a7b/attachment.bin