auraphp / Aura.Di

Dependency Injection System
MIT License
349 stars 63 forks source link

Add ability to pass object instance as config #114

Closed jakejohns closed 8 years ago

jakejohns commented 8 years ago

Firstly, I support the decision to lock the container.

However, I liked being able to write 'configuration that could be configured'. When the ContainerConfig objects were resolved via the DI, I could set constructor params for them, or call setters in previously defined configs. That no longer is the case.

In lieu of that, I kind of want the ability to pass an instantiated object to the builder. So I can do things like this

<?php

$MyConfig = new My\Config('/path/to/cache');

$MyConfig->configRoutes(true)
    ->setUrlPrefix('/my')
    ->setOtherOption(false);

$builder->newConfiguredInstance(
    [
        $MyConfig,
        'Some\OtherConfig',
        'More\Configs'
    ]
);

I also added some duck-typing to the method that i added to instantiate the classes if they are strings and not objects. Honestly, I'd prefer to just check if instance of ContainerConfigInterface, but I'd also prefer that interfaces (ContainerInterface and ContainerConfigInterface) be a separated too. But further I just prefer container-interop adopt that ContainerInterface... but I'm probably getting ahead of myself.

Thoughts?

harikt commented 8 years ago

I really don't know the use case. For a cache it can be project settings that can be taken from a getenv value ? Regarding the set prefix and all I am not sure. I haven't came across one.

One thing I came across recently is with Aura.Auth and different LoginService providers .

Eg : /login , /login/{oauth-provider} .

In that case how can we dynamically set the provider for the Adapter. I am still thinking about it though. But that seems to be a different story / issue.

I am ok with making the change provided it can be a check for instance of rather than is defined methods. I leave this for @pmjones choice .

jakejohns commented 8 years ago

The main reason I didnt use instance of ContainerConfigInterface is because it wasn't there before. The only other reason is because I could write a package that can easily integrate with the container, but not require it (since the interfaces are in the same package as the implementation).

That being said, I also think it might be useful to separate the two (define and modify) into separate interfaces, or check if method exists before each call, as I often have configuration that defines but does not modify or vice-versa.

As far as the use cases of 'configurable configuration', the idea is so that I can provide a configuration class with a package that integrates easily with Aura\Di (and more specifically, Radar), but has it's own methods for altering how that configuration.

As an example, the authentication package I'm working on has a config that wires up the dependencies, but i'd like to be able to tell it to configure the routes on radar/adr or not, or give it a prefix for those routes, etc.

eg:

class Config {
    $route = false;
    $login = '/login';

    //...
    //..setters and stuff
    //

    public function modify($di)
    {
        // ...
        if ($this->route) {
            $this->modifyRoutes($di);
        }
    }

    protected function modifyRoutes($di)
    {
        $adr = $di->get('radar/adr:adr');
        $adr->get('Login', $this->login, ...);
    }

}
harikt commented 8 years ago

Cool. Thanks for explaining. That reminds me of separate interface package.

pmjones commented 8 years ago

@jakejohns After looking this over, I don't have a strong objection, other than to the duck-typing-examination for method existence. Would you have a serious problem if, as a first pass, we checked for instanceof ContainerConfigInterface instead? (Your point about separating the interfaces from the package is well-taken, but not going to pursue it just yet.)

jakejohns commented 8 years ago

I've replaced the method_exists stuff with regular instance of stuff. Certainly I don't have a "serious problem" with it.

I do wonder if there should be: ContainrDefineInterfece which defines define, ContainerModifyInterface which defines modify and then ContainerConfigInterface which implements the previous two. Then before calling define and modify one checks for the discrete interfaces, allowing one to create a 'configurator' which does one but not the other, without needing the empty method. Obviously not a major concern, and perhaps just over complicates things. just a thought.

@pmjones, you say you "don't have a strong objection"... do you have a mild one? I'd certainly be interested in hearing any concerns you do have.

pmjones commented 8 years ago

How's a "merge" for "no strong objection" ? ;-)