kohana / core

Core system classes from Kohana
http://kohanaframework.org
633 stars 328 forks source link

Implementing a Dependency Injection Container. #596

Closed rjd22 closed 8 years ago

rjd22 commented 9 years ago

I've been toying with the idea of implementing a dependency injection container for kohana using pimple. I was wondering what you guys think.

The first option is to make a module implementing pimple. And the other is implementing it into kohana core.

I like the option of the module but it would be better if other modules could use it for their own dependency management. That would mean it should be in core or atleast always included and loaded first as a module.

@enov @acoulton @shadowhand what do you guys think?

shadowhand commented 9 years ago

To what end? Wouldn't having a DI container as a module cause unnecessary coupling of application code to a specific DI interface? Personally, I really like Aura.DI and we use it alongside Kohana without any issue...

enov commented 9 years ago

I played with PHP-DI at the time when version 4.1 was released. It was very easily configured to be used with Kohana's config system, and it felt home.

$builder = new \DI\ContainerBuilder();

$arr_config = array_reverse(
  Kohana::find_file('config', 'php-di', NULL, TRUE)
);

foreach ($arr_config as $config_file) {
    $builder->addDefinitions($config_file);
}

$dic = $builder->build();

Within the builder's addDefinition, configuration are merged and you end up with CFS config system similar to the Kohana's native CFS for configs.

To what end? Wouldn't having a DI container as a module cause unnecessary coupling of application code to a specific DI interface?

I guess @shadowhand is right. While implementing a DI pattern, Kohana core and modules' classes should not be dependent on a container, rather just the interfaces they need to make things done.

However, I feel some container-aware classes should exist, like the Controller, for example, even though Zend guys might not agree with me. This is to give users some freedom and help them build faster. But again, @shadowhand will not agree, see comment above.

By the way, our Config system, while not a full-blown DIC, already covers some of the jobs to be done by these containers.

Also @acoulton has a comment here to ship separate modules that have configs to be used along with any (or some common) existing DICs. See below:

No. And we shouldn't add our own DI container / config files / whatever. Make kohana/core etc just take collaborators as constructor arguments and ship separate packages with default configuration for common DI containers that already exist. We might recommend one in particular, but users should be able to choose whichever one they want.

shadowhand commented 9 years ago

While implementing a DI pattern, Kohana core and modules' classes should not be dependent on a container, rather just the interfaces they need to make things done.

Specifically, the dependency inversion principle should always be kept in mind, and no class should instantiate another class (or, at minimum, have a setter that allows modifying the child object). So long as that goal is met, any DI container can be used alongside Kohana.

kemo commented 9 years ago

I'd rather suggest using a dependency injector over a container; https://github.com/rdlowrey/Auryn

acoulton commented 9 years ago

@enov thanks for tracking down that comment of mine - still what I think. kohana/kohana would probably need to have the library and configuration package for a particular container/injector. None of the other packages should have any reference to a DI container.

In addition to those listed already, there's also https://github.com/zeelot/kohana-dependencies. Whatever we use with kohana/kohana would ideally support some way of loading the configuration from the CFS but I don't think it really matters too much if we're making it easy for users to pick and choose one that suits them.

rjd22 commented 9 years ago

The problem that I'm trying to solve is making a module that follows solid principles. Frameworks like symfony allow module developers to use the DIC making the modules way cleaner but Kohana doesn't help with that. I agree with a common interface but we should also make sure module developers atleast have the tools to make good modules and a similar pattern. We should also try to avoid the problem of having multiple DIC's in an application or a different one per module.

acoulton commented 9 years ago

We should also try to avoid the problem of having multiple DIC's in an application or a different one per module.

That's why there shouldn't be any "one" DI container in any of the core packages. DI and class loading is a separate responsibility from what any of the other packages provide. It's up to the end-user to pick one that suits their application (but we can signpost by choosing one for kohana/kohana).

rjd22 commented 9 years ago

The reason why it chose pimple is it's simplicity but also it's compatibility with kohana config files. I understand that forcing a single one is bad so I would gladly hear solutions for the problem I'm trying to solve. What would be the best of both sides?

rjd22 commented 9 years ago

@shadowhand @enov @acoulton I would still love to hear some opinions on this matter because this is kind of an important feature for the module implementations I'm planning to write.

enov commented 9 years ago

@rjd22, there is a container interoperability project here: https://github.com/container-interop/container-interop

When a package depends on this, it can be used with any container that implements the interfaces of container-interop.

acoulton commented 9 years ago

@rjd22 the cleanest way IMHO would be to provide separate DI configuration packages for each DI container. So for example, you'd have say kohana/core and kohana/core-pimple-config.

Then an end user application (and kohana/kohana) might have a composer.json requiring kohana/core, kohana/core-pimple-config, and kohana/kohana-pimple and would configure and initialise pimple in the bootstrap.

But another end-user application might require kohana/core, myvendor/kohana-symfonydi-config and symfony/di.

A slightly less good but maybe still OK alternative would be to provide config files for various supported DI containers within your package. So for example in kohana/core you might find directories like config/di/pimple and config/di/symfony. However you would only be providing config files - kohana/core would not have any composer or code dependency on any DI-related interface.

So long as you're using dependency injection rather than service locator, there shouldn't be any reason why a module needs to have any dependency (even an interop interface) on a DI container. It should be possible (if boring) to build an application using your module with a single .php file full of $something = new Whatever($something_else) if you wanted to.

rjd22 commented 9 years ago

I've finally got around creating a module for this: https://github.com/rjd22/kohana-pimple

I was wondering what you think @enov and @acoulton. It would be really great if kohana had a interface for the container and a ContainerAwareController that would use this interface to access the Container.

I think Kohana should not care how the dependencies are registered but should care about how to access them from a controller or anywhere else.

enov commented 9 years ago

Hey @rjd22,

Thank you for your work.

Defining dependency config files in config would be overkill. Why not define dependencies in Kohana config and feed Pimple directly and enjoy the merging of config files with CFS for free?

Since we do not have a native ContainerAwareController or anything like that, $this->container = Container::factory(); in the before() of the controller will recreate a new instance of the container from config files, disregarding any changes done, for example, in bootstrap. If recreating from config files is all we care, then I guess the container will not do anything more than config based factory methods that we usually have here in Kohana.

Also, maybe a ContainerFactory::createFromConfig would be a cooler approach. Then we are sure the Container class would not have any hidden dependencies - even though it is obvious that it does not depend on Kohana::$config except in the factory method.

Thanks again, has my :star:.

Cheers :+1:

rjd22 commented 9 years ago

@enov My reason for doing it like this is that I wan'ted to decouple the loading of the dependency configs from the framework.

My reasons for doing so is that I don't wan't to force users to put their dependency configs in a fixed spot. Maybe they like to store them in the src/Resources folder for example. Or some other place Kohana can't read.

I could make a improvement that will enable you to use a dependencies.php config file if you want to use it like that and also allow users to point to config files when they want to store them for example in the src/ folder.

Thank you for your comment. I really appreciate it.