Allow to inject objects to actions without the requirement to define them in the container

[dmotitsk]

This PR #18032 added action injection. But currently there is a requirement to define objects that are to be injected in the container. If the container knows nothing about the class type the exception will be thrown.

So the following code will work without definition of the type MyService in the container:

public function actionIndex()
{
    $service = \Yii::$container->get(MyService::class);
    $result = $service->doSmth();
    $this->render('index', compact('result'));
}

and this code won't work without defining MyService in the container (it will throw an Exception):

public function actionIndex(MyService $service)
{
    $result = $service->doSmth();
    $this->render('index', compact('result'));
}

The author of this PR @SamMousa has arguments against allowing injecting arbitary objects. You can read them here https://github.com/yiisoft/yii2/pull/18032#issuecomment-727220098.

But I have several arguments in favor of this idea.

1. The behavior of the container differs from other parts of the framework. In any other part if there is no definition in the container about the type, it will just try to create the object of this type. In action injection it will throw an exception.

2. As we can see in the example above developers can create any objects inside the action through $container->get() and disallowing them direct injection via function arguments doesn't protect against misunderstanding of what should be injected and what should not be.

3. As far as I understand this feature (action injection) was postponed for a long time. And the main argument to implement it in Yii2 framework was that it would be allowed in the 3rd version. So it will be easy in future to migrate from Yii2 to Yii3.
   As I can see currently Yii3 allows to inject objects to the actions without the requirement to declare their types in the container (correct me if I'm wrong). So it would be more logical to allow the same thing in Yii2.

It can be fixed very easy. Just need to rollback this commit https://github.com/yiisoft/yii2/pull/18032/commits/03b7c845ca25745785bb4ff5ef5d2cbc760bcc74.

[SamMousa]

Arguments 1 and 2 are invalid.

The behavior of the container differs from other parts of the framework.

You are comparing a container misused as a service locator with dependency injection; these are not the same things.

As we can see in the example above developers can create any objects inside the action through $container->get() and disallowing them direct injection via function arguments doesn't protect against misunderstanding of what should be injected and what should not be.

The argument that people can write bad code in other ways is not an argument to add new ways for people to create bad code.

If argument 3 is factually correct than I would counter-propose to remove that functionality from Yii3. There is no reason to support this bad behavior out of the box, if people want to use their own implementation they are free to do so, but our core implementation should promote proper use of DI.

[dmotitsk]

Ok, let me add some more code to my example. Let's say I have my service class and it also has a dependency - a repository object.

class MyService
{
    private MyRepository $repository;
    
    public function __construct(MyRepository $repository)
    {
        $this->repository = $repository;
    }
}

If I skip the definition of MyRepository inside the container, it will be created for me by the container (with its dependencies if there are any). That's why I'm writing that the behavior differs from other parts of the framework. So when I'm injecting to the action I'm actually expecting the same behavior. If this behavior is undesirable we should force to define any class that is supposed to be injected (and their dependencies) inside the container.

It is also interesting how other frameworks implement action injection. May be it is not the main argument but it is also something to be considered. As far as I remember Symfony allows to inject objects from App namespace (with excludes set in the config).

[SamMousa]

But proper code wouldn't do that...

Instead you would have an interface that is implemented by your service, then your action requires the interface not a concrete implementation. This means you must configure that relation in your container since it won't be able to construct an interface.

In your example if you define MyService in the container it will still auto create the dependency, you just have to configure the dependency that you're injecting not the child dependencies.

[samdark]

@SamMousa in theory you're correct. In practice, you're not always creating an interface for every possible service especially if you're more than sure it won't be replaced ever.

[SamMousa]

True, but even then, configuration in the DI container is just one line. At the very least if we do it, it should become optional imo.

[samdark]

I think either we don't do it or we do it by default. Making it optional doesn't make sense to me since you'll have to take that into account when developing extensions.

[SamMousa]

In that case my vote is against it.. It doesn't do any favors to code quality and we shouldn't forget that this is mostly about injecting classes that have no configuration and therefore can just as well be constructed..

In most cases we will either inject a service defined in the container or a component defined in the application, both cases are supported.

[samdark]

Makes sense. Won't be implemented.

[dmotitsk]

we shouldn't forget that this is mostly about injecting classes that have no configuration and therefore can just as well be constructed.

If I understood you correctly you mean that we inject mostly services and they can be constructed with a contstructor call (new Service()). But services has a lot of dependencies, mostly components that can be injected into it instead of using service locator (for example a Mailer). So we can not just instantiate a service, we need to inject it. And this dependencies can be constructed automatically for us, so we can completely skip a service definition from the container.

Now I can use Yii::$container->get() to get any service from a container.

You are comparing a container misused as a service locator with dependency injection

You are right, it is not the proper way to do. But I can also inject dependencies via controller's constructor (overriding it). It is the proper way of using DI, isn't it? And I can do it without defining the service in the container. My main point is an illogical behavior that allows constructing dependencies in one place but denies it in another one. Why is an action injection a special case that we should always take care of?
The problem with injecting services to controller's contructor is that if I have many actions and each of them use it's own service I need to inject all the services inside the constructor. Despite of that I won't use all of them except the one. The alternative is to break the controller into separate action classes with their own constructors and map them by actions() method. But if action is very simple (create the form, validate the input, pass a dto to the service and render the view) then it can be too complicated to have a separate class for an action.
The action injection simplifies things a lot. But with current implementaion I need to do somewhere in container's configuration
Yii::$container->set(Service1::class);
Yii::$container->set(Service2::class);
Yii::$container->set(Service3::class);
...
These definitions can be omitted because they are needed only for inecting them in the actions. Services have dependencies themselves (the 3rd parameter) but I can skip defining them because they will be constructed automatically in most cases.

So my main argument that we have an illogical behavior that contradicts the rest parts of DI usage in the framework.
This feature (action injection) is a great way to reduce the code amount needed to define an action. But it can be improved by removing the redundant service definitions (like in my example above). They do not serve any other purpose except allowing action injection.
Also it is not working the same way as Yii3 will. And we missing one of the main purpose of implementing these feature.

[SamMousa]

You misunderstand. Services you want to inject. But services most likely have some configuration and therefore will already be defined in your components or in DI config itself (and thus you can already inject them)

[dmotitsk]

But services most likely have some configuration and therefore will already be defined in your components or in DI config itself (and thus you can already inject them)

Ok, I got it.

But they can have no explicit configuration (when all their dependencies can be resolved automatically by the container). In this case their definition in the container is redundant in my opinion. And in my personal workflow they do not have explicit configuration in most cases.

And what about my other arguments about an incosistent behavior and Yii3?

[SamMousa]

I don't think the Yii3 argument is relevant. The whole point of a major upgrade is to allow for changing behavior. Also, I'm not too deep into Yii3, but I'd probably argue against action methods as a whole. Action classes make more sense and solve the discussion by using pure constructor injection.

…

On Mon, 23 Nov 2020, 08:08 dmotitsk, ***@***.***> wrote: But services most likely have some configuration and therefore will already be defined in your components or in DI config itself (and thus you can already inject them) Ok, I got it. But they can have no explicit configuration (when all their dependencies can be resolved automatically by the container). In this case their definition in the container is redundant in my opinion. And in my personal workflow they do not have explicit configuration in most cases. And what about my other arguments about an incosistent behavior and Yii3? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#18388 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEFRTKJZSU3DAXTBAACEZ3SRIC75ANCNFSM4TXGYYJA> .

[dmotitsk]

I don't think the Yii3 argument is relevant. The whole point of a major upgrade is to allow for changing behavior.

I constatly refer to Yii3 because as I understand this post https://github.com/yiisoft/yii2/issues/17722#issue-535273746 the whole idea was to make it easier to migrate from Yii2 to Yii3. And it is not the situation when we have a feature for a long time in Yii2 and we rework and reintroduce it in the next major version. As for me if this was decided to be implemented after the feature freeze it should be done keeping Yii3 in mind.

Action classes make more sense and solve the discussion by using pure constructor injection.

I used action classes with constructor injection before action injection became available. It is OK but for the simple cases it can be an overkill. Either way if Yii3 completely disallow action methods of controller it will be too different from other frameworks.

As for me action injection is a great "framework-level sugar". You implemented a great feature, but as for me, you were too strict requiring service definition in the container. First time when I wrote an action after this feature was presented I was very surprised that my service wasn't injected and I got an exception. Because I was used to the fact that the container creates undefined classes automatically. And this is what I call "illogical behavior". I expected one thing from my experience with the framework, but got the opposite.

These feature is cool and it makes much convinient to define actions but I think we can move it one step further as it is done in Yii3 and other frameworks. Those who want to explicitly define services in the container can continue to do this. Those who want to ommit services with empty $params argument will be able to skip this. And it will not take much work to implement.

[SamMousa]

And it is not the situation when we have a feature for a long time in Yii2 and we rework and reintroduce it in the next major version.

We didn't have this for a long time, if you want an easy migration path you don't use the Yii2 version and wait for Yii3.

First time when I wrote an action after this feature was presented I was very surprised that my service wasn't injected and I got an exception.

I don't see an issue here, you get surprised but it fails hard so you learn and you can now write cleaner code because of it.

actions but I think we can move it one step further...

And I think that would hurt code quality instead of helping it. Regardless, I'm done discussing this, I think we've both have repeated our arguments and further discussion serves no purpose.
Note that I did not make the decision, I've just explained my reasoning and preference.
If you want you can easily use your own controller class and change the behavior however you want.

Don't get me wrong, I don't mind the input or criticism, but I'm not going to spend time in a debate when there are no new arguments coming forward :)

[papppeter]

Hi @dmotitsk,

i think i have a solution for you. I am missing it as well, it would be really nice to have proper RequestModel auto injected in action method, without setting it up manually.

so my solution is (still in work in progress, but it is already working):

i created an action filter, which makes possible to auto register missing class names just before the action run, and clear it after run. It uses the same logic as bindInjectedParams

namespace system\base;

use Yii;
use yii\base\ActionFilter;
use yii\base\Controller;
use yii\base\InlineAction;

class OnDemandClassDefinitionRegister extends ActionFilter
{
    protected string $registeredClass;

    public function beforeAction($action)
    {
        if ($action instanceof InlineAction) {
            $method = new \ReflectionMethod($action->controller, $action->actionMethod);
        } else {
            $method = new \ReflectionMethod($action, 'run');
        }

        foreach ($method->getParameters() as $param) {
            $name = $param->getName();

            if (
                ($type = $param->getType()) !== null
                && $type instanceof \ReflectionNamedType
                && !$type->isBuiltin()
            ) {
                $this->registerClassDefinition($action->controller, $type, $name);
            }
        }

        return parent::beforeAction($action);
    }

    final protected function registerClassDefinition(Controller $controller, \ReflectionType $type, $name)
    {
        $typeName = $type->getName();
        if (($controller->module->get($name, false)) instanceof $typeName) {
        } elseif ($controller->module->has($typeName) && ($controller->module->get($typeName)) instanceof $typeName) {
        } elseif (\Yii::$container->has($typeName) && (\Yii::$container->get($typeName)) instanceof $typeName) {
        } elseif ($type->allowsNull()) {
        } else {
            $this->registeredClass = $type->getName();
            Yii::$container->set($type->getName(), $type->getName());
        }
    }

    public function afterAction($action, $result)
    {
        if (isset($this->registeredClass)) {
            Yii::$container->clear($this->registeredClass);
        }
        return parent::afterAction($action, $result); // TODO: Change the autogenerated stub
    }
}

In controller following markup can be used:

public function actionEvents(EventRequest $request)
    {

Request object itself:

<?php

namespace modules\schedule\backend\requests;

use Illuminate\Support\Carbon;
use yii\web\Request;

class EventRequest {
    private Request $request;

    public function __construct(Request $request)
    {
        $this->request = $request;
    }

    public function start(): string
    {
        return Carbon::parse($this->request->get('start'))->toDateTimeString();
    }

    public function end(): string
    {
        return Carbon::parse($this->request->get('end'))->toDateTimeString();
    }
}

for this i did registered main request in DI container (only one request should exists)

\Yii::$container->set(Request::class, fn() => \Yii::$app->request);