Avoid injecting whole app as dependency

Right now in many places usage of Yii::$app was replaced by injecting Application in the constructor and using $this->app instead. But this is far from perfect and sometimes creates more problems than it solves.

  1. Why Application is injected? IMO it should be a module or some more generic container/service locator. Requiring instance of Application creates unnecessary constraints and makes component less flexible - maybe I want to use it in module context and it should use components from my module? In Yii 2 modules supports tree traversal and already can work as a replacement of application, requiring an instance of Application looks like a regression.

  2. It hides dependencies. One of the main advantages of DI is that dependencies are explicit. This is not true if you’re injecting entire application (or container) since it can provide anything.

  3. This creates a big tree which may lead to serialization problems: https://github.com/yiisoft/yii-swiftmailer/issues/7

  4. This is unnecessary in most cases. Honestly, so far I didn’t see a valid case when this was actually required (except modules). And recent rush to make tests green created some ugly cases when injecting app was overused. Here app is used only to get request and response, while we could inject them directly (and skip this conditions for default values, which does not make much sense in the constructor). Here app is used only to access request and alias resolving. There are almost 100 files where app is injected or used as dependency, most of them do not look necessary at all, and some of them indicate design issues (why app is responsible for translation or alias resolving?).

I suggest reviewing all places when app is injected as dependency and limit such cases to an absolute minimum.

2 Likes

First of all, thanks for suggestion.

It is certainly wrong and was a quick-fix to get things running so things could be worked on while it is fixed a bit later.

Injecting container is wrong as well. A class should require what it needs without knowing how to get in from container.

So yes, we will remove all these before release.

1 Like

while it might not be perfect but it is way better than what we had before, I have seen codebases using crazy hacks to overcome dependency problem for testing, with this at least you can drain the container and populate it with test dependencies, this is much better IMO, it can clean up further by limit the use.

Just don’t return to CodeIgniter era where everything was pushed into $this.

What do you mean? I seem to start forgetting CodeIgniter times :slight_smile:

In CI you’d always need $this to access any component, it was just too tightly coupled.

Ah. Right right.