rdlowrey/Auryn — pull request #64 by Danack (2014-06-01 12:10:14+00:00)
Title: Plugin based provider info.

The PR still has a small amount of tidying up to do, but it's in a state where it should be reviewed.

Levi, previously you have criticized me both for changing too much and having PR's that make the code ugly. Please bear in mind while you review this PR that I have chosen to err on the side of doing the minimal changes possible, but that has left some places that I think ought to be cleaned up further e.g. the method AurynInjector::makeClass is defintely not what I consider clean code, but I avoided changing it to read better, to minimise the lines changes in the PR.

Also I do try to avoid changing stuff at random. If you think I've changed something for no good reason, we will have a more productive conversation if you ask me why I've changed something rather than straight away criticizing me for doing something that you don't instantly like.

The commits for the PR are in the following chunks:

1) 2dd3 - Splitting the holding of the information about what classes/params to use from the Injector into a separate ProviderPlugin class. Regardless of your views of the rest of the changes, I think this should be done as currently the Provider class is just doing too damn much.

To maintain backwards compatibility for people who are already using Auryn:

* The actual injection class is now called AurynInjector.
* Provider is now a facade that makes AurynInjector and a StandardProviderPlugin and passes the method calls through to the appropriate class, so people shouldn't see any change in how they call Auryn unless they want to upgrade to creating the exact ProviderPlugin to use and initializing the injector with it.

2) bad4 - Adding class chain constructor information. This has no external effect but it refactors the property `beingProvisioned` into an ordered array that holds the hierarchy of things being constructed by the injector. That array is passed to the StandardProviderPlugin, which completely ignores it, as that plugin doesn't resolve what to inject based on class hierarchy.

3) 2fa6 - Added Class Contructor Chain plugin. This is what the two commits above were leading up to. This ProviderPlugin _does_ use the chainClassConstructor information to allow you to do things that I think a fully featured DIC needs to be able to do.

## Customising log level for certain classes

```php
class Logger {
function __cosntruct($logLevel) {...}
}

class SomeClassThatIsWorkingFine {
funciton __construct(Logger $logger){...}
}

class SomeClassThatNeedsToBeDebugged {
funciton __construct(Logger $logger){...}
}

$providerPlugin->define('Logger', [':logLevel' => 'warn']);
$providerPlugin->define('Logger', [':logLevel' => 'info'], ['SomeClassThatNeedsToBeDebugged']);

$classThatIsWorkingFine = $provider->make('SomeClassThatIsWorkingFine');
$classThatNeedsToBeDebugged = $provider->make('SomeClassThatNeedsToBeDebugged');

```

The above code creates two loggers, one set to 'warn' one set to 'info' for the two classes.

## Being able to use more than on DB connection

```php
class DBConnection {
function __construct($server, $username, $password){...}
}

class ArchiveDBProxy {
function __construct(DBConnection $dbConnection){...}
}

class LiveDBProxy {
function __construct(DBConnection $dbConnection){...}
}

//Moves stuff safely from the liveDB to the archiveDB
class DBArchiver {
function __construct(ArchiveDBProxy $archiveDB, LiveDBProxy $liveDB) {...}
}

$providerPlugin->define(
'DBConnection',
[ ':server' => $archiveServerIP,
':username' => 'someusername',
':password' => 12345,
],
['ArchiveDBProxy']
);

$providerPlugin->define(
'DBConnection',
[ ':server' => $liveServerIP,
':username' => 'someusername',
':password' => 12345,
],
['LiveDBProxy']
);

$injector->make('DBArchiver')
``

I know you both feel the injector is getting more complicated than you would like. However I think the code needs to be just complex enough to support how users are trying to use it. The above two use-cases are both valid cases that Auryn doesn't currently support.

Obviously the nice thing about having the ProviderPlugin separate from the actual injector is that we don't have to have it like that (i.e. could leave the ClassConstructorChainPlugin out of the repo, if you really hate it) and also don't have to support peoples crazy ideas about how to define the information used by the injector. So instead of either having to support PR's to allow custom definition, we can just tell people to make their own plugin.

There are small tidy up commits after that one, just to improve some naming, and comments.

Things I know I need feedback on/have questions about myself:

1. I put all the methods that the injector uses when it creates objects into ProviderInjectionPlugin rather than putting them in ProviderPlugin which currently contains the methods that are used by users when setting up the plugin. I think all of the methods in that interface should be using the `normalizedClass` name, but as I said, i avoided tidying some things up to make the PR be as minimal as possible.

Question, is there any point to splitting them like that? It seems like it might be a good idea, but also falls under YAGNI. And yes I also need to document those methods.

Also, I think the names for these interfaces suck, but I can't think of good names, so suggestions please.

2. There are are currently some poor method names. I think we should change the following with some suggetsions of better name for them

prepare => setInitCallable
getPrepareDefine => getInitCallable
getInterfacePrepares => getInterfaceInitCallables
getDelegate => getDelegation
isDelegated => hasDelegation

isDelegated isn't actually required - the code should be able to be refactored to just call getDelegate; it's only the getParamDefine that requires the `is` function as null is a valid define to use as a param, but it's not for delegation. (Also I'm pretty sure that there's a bug in getInterfacePrepares.)

It may be worth me just doing the tidy up of stuff I consider crap code/naming in a separate branch and then you guys can pick and choose the cleanups you approve of and reject the ones you don't.

3. I haven't deeply checked that the InjectorBuilder against the refactored separation of the injector/plugin. The tests for it pass so I hope it's fine.

4. The checking of whether something is a valid callable when delegation is setup is not a good idea. At best it means that the code for every class that is used as a delegate gets pulled into the script and has reflection done on it, which is not great for speed. It also means that you can't setup a function delegate that hasn't been loaded yet - which seems a dumb limitation.

Is there a really good reason for it being there that I've missed? I realise detecting errors early is nice, but it seems a bad tradeoff here.

5. I realise that the code coverage on the ClassConstructorChain plugin isn't at 100%. Also it doesn't use the CCC for prepares or aliases. I will try to get it to closer to 100% soon. Also I guess this might need some documenatation.