Add interfaces for main application components

[ngumilyov]

As far as I understand right now we can not use DI container for returning application components that are not abstract classes or don't implement an interface. Let me explain what I mean.

We can not do something like this (somewhere in bootstrap):
\Yii::$container->setSingleton(\yii\db\Connection::class, function () { return \Yii::$app->db; });

but we can do this:
\Yii::$container->setSingleton(\yii\mail\MailerInterface::class, function () { return \Yii::$app->mailer; });

So we can inject mailer (via MailerInterface) but can not inject Connection.

So we are forced to use Service Locator in our code. Or we can move DB configuration from application component to container but it doesn't look well as for me.

Would not it be better to add interfaces to concrete classes (like Connection or Session) and add possibility to inject via these interfaces?

interface ConnectionInterface {
  // Here is declaration for Connection public methods
}

class Connection extends Component implements ConnectionInterface {
 // Here is the same code for Connection class
}

\Yii::$container->setSingleton(ConnectionInterface::class, function () { return \Yii::$app->db; });

[samdark]

Yes. It's a good idea.

@yiisoft/core-developers ?

[njasm]

I just realised that today!
Not to be able to inject Yii components in my classes constructors either by type hinting an interface or implementation using the already in place di infrastructure.

[klimov-paul]

I am against the idea.
Covering every component with the interface will greatly increase code-parsing time as it practically double the amount of entities involved in program flow.
This will also double amount of source files.
Following this path eliminates the simplicity, which Yii is built upon.

\Yii::$container->setSingleton(\yii\mail\MailerInterface::class, function () { return \Yii::$app->mailer; });

I can not understand the meaning of such construction. What is the case of registering a singleton in the way it should be an application component?
Application itself already applies singleton pattern, thus all its components share singleton status with it.

I do not understand why it is better to use:

Yii::$container->get('component');

instead of

Yii::$app->get('component');

what is the benefit?

[ngumilyov]

We can use type hinting for DI. So we can inject dependency into constructor without explicitly calling global objects (container or app). It can be easier to test classes that uses DI instead of $app.

[cebe]

We can not do something like this (somewhere in bootstrap):
\Yii::$container->setSingleton(\yii\db\Connection::class, function () { return \Yii::$app->db; });

could you explain why you think you can not do it? Just tried to configure it on the basic app and it works as expected. Of course its pretty useless to make the container depend on the service locator, this way you create a circular dependency. You should configure an object inside the container:

Yii::$container->setSingleton(\yii\db\Connection::class, [
    'dsn' => 'sqlite::memory:',
    // more properties here
]);
Yii::$app->set('db', \yii\db\Connection::class); // or in application config components: 'db' => \yii\db\Connection::class

echo Yii::$container->get(\yii\db\Connection::class)->dsn; // sqlite::memory:
echo Yii::$app->db->dsn; // sqlite::memory:

[ngumilyov]

Yes, it is possible to do this. You moved DB configuration from component to container. As for me this a little bit differs from documentation. But it is the best option right now. And I use it for my projects.

[njasm]

Being able to do stuff like this would be, in my point of view:

1 - a huge leap forward in framework's flexibility
2 - much easier to decouple framework from domain
3 - much easier to unit test domain objects
4 - don't be forced by the framework to use the locator pattern

class SomeController extends Controller
{
    protected $service;
  
    public __construct(\some\namepace\SomeService $service)
    {
        $this->service = $service;
    }

    public function actionSomething()
    {
        $this->service->doSomething();
        // ...
    }
}

class SomeService implements ServiceInterface
{
    protected $app ;

    public __construct(\yii\web\ApplicationInterface $app)
    {
        $this->app = $app;
    }

    public function doSomething()
    {
        return $this->app->...();
    }
}

[SilverFire]

I am against the idea.
Covering every component with the interface will greatly increase code-parsing time as it practically double the amount of entities involved in program flow.
This will also double amount of source files.

With Opcache and realpath cache, interfaces will not impact performance, especially in PHP 7.0+, where classes are dramatically faster than in PHP 5.*

Application itself already applies singleton pattern, thus all its components share singleton status with it.

Is is a double-edged sword. On one hand, it is easy to understand for beginners, because it works explicitly: I write Yii::$app->accountService and I mean Hey, $app, I want to take something that I've named "accountService" in my application configuration.
For advanced developers it means coupling to framework and dependency on naming in application configuration, so it's harder to write more reusable code.

I do not understand why it is better to use:

Yii::$container->get('component');

instead of

Yii::$app->get('component');

what is the benefit?

Indeed, there's absolutely no benefit in using Dependency injection Container as Service Locator. But it makes sense to use DiC for autowiring, as @njasm showed in his example above.

@cebe example it pretty good to start using autowiring right now, but subset of Liskov Substitution Principle says to depend on abstractions, not on concretes.

Yii has really good components that deserve to be really reusable, even without framework itself. That's why I've recently extracted CacheInterface from Cache class and use it in my projects. I've seen positive feedback on this change is community chats.

Introducing interfaces itself will not make existing code neither slower, nor more complex to understand for beginners. But it will give more space to growth (without framework switching) for those, who need more than service locator and application as a singleton.

We also have an issue Request/response should be injected in controller, actions and related classes via constructor opened by @samdark and it is one of the most upvoted issues for 2.1 milestone, so I make conclusion that a lot of people agree with the idea of switching from Service Locator to DI.

I'm voting strictly for. Who else?

[samdark]

I am for it.

[dynasource]

me too

[sergeymakinen]

I know the question was for core developers and there’s +1 ability but I must say I welcome it and I believe that’s exactly the right way forward.

[cebe]

I know the question was for core developers

everyone is welcome to give their opinion, not only core developers :) The @ mention is just to highlight people so they get notification.

[njasm]

I believe I already gave my opinion with the example above, but I'll reinforce it and make my opinion explicit. let's go for it!

[njasm]

so when do we start? were to start? any plan already?

[samdark]

Anywhere in small pieces. One by one.

[njasm]

@samdark ok, my idea is to extract interfaces from the current public methods of each component's implementation.

I'll start with the yii\db\Connection and create yii\db\ConnectionInterface.

and we discuss from there, does it sound like a plan?

[samdark]

Yes.

[njasm]

I didn't extracted yii\db\Connection EVENT_* constants to the interface yet, I think we should, but would like to know other opinions about it first.

Since Connection is uses and/or returns Command, DataReader and Transactionclasses. Should we discuss the possibility to extract also interfaces or is it too premature at this stage and we should move to other core components first?

/cc @samdark @cebe @klimov-paul @SilverFire @sergeymakinen @dynasource @ngumilyov

[samdark]

Other components first, I guess...

[samdark]

Moving constants makes sense.

[njasm]

@samdark , @sergeymakinen have a different view where to put those constants, the discussion is underway on the pull request #15277

with this in mind, the push already have this component interface's extracted:

1. DB (yii\db\Connection) 
2. Logger (yii\log\Logger)

Next in line:

1. [] View (yii\web\View)
2. Formatter (yii\i18n\Formatter)
3. l18N (yii\i18n\I18N)
4. UrlManager (yii\web\UrlManager)
5. Mailer (yii\swiftmailer\Mailer) exists
6. AssetManager (yii\web\AssetManager)
7. Security (yii\base\Security)

Question:

1. Any Component that we should not extract an interface from the list above? If so which ones?
2. What other components should be extracted an interface that's is missing from the list above?
3. Any missing Component in the list above, that should be on the list?
4. I would like to keep pushing to the same pull request, since this is more of a copy/past task than anything else, but if you prefer I can create a different pull request for each component?

[samdark]

1. It's OK to have interfaces for all these even if it's unlikely that components such as Security will be replaced ever.
2. Separate PRs are better. Easier to review.

[samdark]

Already in progress for 3.0.