Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementing a Dependency Injection Container. #596

Closed
rjd22 opened this issue Jan 11, 2015 · 14 comments
Closed

Implementing a Dependency Injection Container. #596

rjd22 opened this issue Jan 11, 2015 · 14 comments
Labels

Comments

@rjd22
Copy link

rjd22 commented Jan 11, 2015

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?

@rjd22 rjd22 added the Idea label Jan 11, 2015
@shadowhand
Copy link
Contributor

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
Copy link
Contributor

enov commented Jan 12, 2015

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
Copy link
Contributor

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
Copy link
Member

kemo commented Jan 12, 2015

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

@acoulton
Copy link
Member

@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
Copy link
Author

rjd22 commented Jan 12, 2015

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
Copy link
Member

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
Copy link
Author

rjd22 commented Jan 12, 2015

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
Copy link
Author

rjd22 commented Jan 16, 2015

@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
Copy link
Contributor

enov commented Jan 17, 2015

@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
Copy link
Member

@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
Copy link
Author

rjd22 commented Jul 29, 2015

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
Copy link
Contributor

enov commented Jul 29, 2015

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

Cheers 👍

@rjd22
Copy link
Author

rjd22 commented Jul 29, 2015

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

@rjd22 rjd22 closed this as completed Mar 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants