MVC, DI (dependency injection) and creating Model instance from Controller

Question

My Dispatcher is "choosing" correct Controller; then creating Controller's instance (DependencyInjectionContainer is passed to Controller constructor); then calling some Controller's method...

class UserController extends Controller
{

  public function __construct(DependencyInjectionContainer $injection) {
    $this->container = $injection;
  }

  public function detailsAction() {
    ...
  }

}

DependencyInjectionContainer contains DB adapter object, Config object etc. Now let's see what detailsAction() method contains...

public function detailsAction() {

  $model = new UserModel();
  $model->getDetails(12345);

}

As you see I'm creating new instance of UserModel and calling getDetails methods. Model's getDetails() method should connect to db to get information about user. To connect to DB UserModel should be able to access DB adapter.

What is the right way to pass DependencyInjectionContainer to the UserModel? I think that this way is wrong...

public function detailsAction() {

  $model = new UserModel($this->container);
  $model->getDetails(12345);

}

Answer 1

Instead of injecting the entire DI Container into your classes, you should inject only the dependencies you need.

Your UserController requires a DB Adapter (let's call this interface IDBAdapter). In C# this might look like this:

public class UserController
{
    private readonly IDBAdapter db;

    public UserController(IDBAdapter db)
    {
        if (db == null)
        {
            throw new ArgumentNullException("db");
        }

        this.db = db;
    }

    public void DetailsAction()
    {
        var model = new UserModel(this.db);
        model.GetDetails(12345);
    }
}

In this case we are injectiing the dependency into the UserModel. In most cases, however, I would tend to consider it a DI smell if the UserController only takes a dependency to pass it on, so a better approach might be for the UserController to take a dependency on an Abstract Factory like this one:

public interface IUserModelFactory
{
    UserModel Create();
}

In this variation, the UserController might look like this:

public class UserController
{
    private readonly IUserModelFactory factory;

    public UserController(IUserModelFactory factory)
    {
        if (factory == null)
        {
            throw new ArgumentNullException("factory");
        }

        this.factory = factory;
    }

    public void DetailsAction()
    {
        var model = this.factory.Create();
        model.GetDetails(12345);
    }
}

and you could define a concrete UserModelFactory that takes a dependency on IDBAdapter:

public class UserModelFactory : IUserModelFactory
{
    private readonly IDBAdapter db;

    public UserModelFactory(IDBAdapter db)
    {
        if (db == null)
        {
            throw new ArgumentNullException("db");
        }

        this.db = db;
    }

    public UserModel Create()
    {
        return new UserModel(this.db);
    }
}

This gives you better separation of concerns.

If you need more than one dependency, you just inject them through the constructor. When you start to get too many, it's a sign that you are violating the Single Responsibility Principle, and it's time to refactor to Aggregate Services.

Answer 2

I'd use a singleton object for all config parameters : You set it up in your bootstrap, then choose to use it directly or pass it as parameter in your objects.

The idea being to have one method all around to retrieve your config data.

You may then provide an abstract class for db manipulation which uses your config. singleton. DependancyInjection can still be used to override your default data.

The above link in the comment (possible 'duplicate') concludes on using constructor injection : this is close to your current method.

However if I try to figure how your model works, I guess you will have many other model classes other than "userModel". Thus an abstract class using a config singleton might be a good solution : all your next model classes will just extend this abstract class, and you don't have to worry about your config.

On the other hand, your solution is good to me as long as your dependanceInjectionContainer changes often.