zendframework / zend-servicemanager

ServiceManager component from Zend Framework
BSD 3-Clause "New" or "Revised" License
188 stars 89 forks source link

On servicemanager and dependency injection #178

Closed bartve closed 7 years ago

bartve commented 7 years ago

A while ago I was toying with an idea I had on dependency injection in Zend Framework and made this rough gist as a proof of concept: https://gist.github.com/bartve/679d2354ba3f1e08a20ad9a63943a494

It's based on some (personal) requirements I had:

Drawbacks/discussion:

So before I will go put some effort into making a proper pull request I'm interested in your opinions. I have to admit that even though I have already tested this concept on an actual large ZF2 project and am quite happy with it, I'm still unsure if zend-servicemanager is the right place for this as it focuses on DI rather than managing service instances in a container. Then again, with things like ReflectionBasedAbstractFactory.php popping up, why not?

Anyway, fire at will (remember, it's just a POC draft) ;)

Ocramius commented 7 years ago

First of all, the idea is orginal. It resembles what we could think of annotation-based property injection in containers like Spring's.

Still, I don't believe that this is useful, since PHP has an extremely simple, yet very powerful way to express dependency types: __construct(DependencyType $dependencyName)

Let me clarify step by step: I hope this critique is useful to you to think this further.

Keep it simple

__construct is pretty much that

Automatically inject dependencies, no getter/setter clutter for big services/controllers with many dependencies

Same as above

Support dependency inheritance

This is more of a problem than an improvement, since now the definitions are both in the parent and the child class. The resolution is simple with __construct and parent::__construct() calls, but it becomes complex here.

Easily see/get a class's dependencies without expensive reflection or endless config arrays

Besides first instantiation time (which is sub-millisecond for autoloaded classes), reflection is much faster than what you probably think. Introspecting $reflectionClass->getConstructor()->getParameters() is only a few method calls more than what you have in your API. In addition to that, it can generally be cached away.

It's also already provided out of the box by the reflection abstract factory, although you probably want a ReflectionFactory (non-abstract) and some caching, if you rely heavily on it. Still, I wouldn't even worry about performance here.

Solution should have minimal, or at best no, dependencies itself

You just introduced a DependenciesInterface dependency (interface in the framework) in userland code: this is a huge HUUUUUUGE drawback, as it couples userland code to the framework, making upgrades, migrations and reuse harder.

To recap:

bartve commented 7 years ago

Thanks for taking the time to respond and the insights provided :)

This surely beats a "neh, your idea sucks". Being familiar with the "purer" php alternatives like enforcing dependencies in the constructor rather than non-enforcing get/set or the method above I was already prepared for the fact that might be a bigger anti-pattern than it would bring advantages or make things more easily without too much magic .

This is more of a problem than an improvement, since now the definitions are both in the parent and the child class. The resolution is simple with construct and parent::construct() calls, but it becomes complex here.

the getDependencies() function gets the current class dependencies with or without inherited ones. With the constructor method you have to pass everything not knowing where it goes. The separation was intentional. I kind of liked the idea of querying declared dependencies statically from a class rather than using reflection.

couples userland code to the framework

One of my main gripes too, didn't know how to effectively put it in words though.

I'll leave this open for a bit so others can respond, but this post confirms my initial suspicions there was something "fishy" about this approach.

bartve commented 7 years ago

Statically self-configuring classes might be an idea for a php micro-framework, but not for ZF. Anyway, thanks for reading.