Fixes #17722

[SamMousa]

This draft PR implements action injection inspired by my implementation here: https://github.com/SAM-IT/yii2-magic/blob/master/src/Traits/ActionInjectionTrait.php

Since that implementation the core framework has improved action param handling with things like type validation when using scalar type hints in modern PHP versions.

This is why this implementation uses a different route; it alters the existing parameter iteration and checks for type hints that are NOT built in (thus class type hints).

TODO

• Add test for container exceptions, currently there is no exception handling.
• Add test for optional injection (IE nullable type).

Design decisions

1. First try to resolve the variable as a component of the module.
2. Don't put the injected stuff into actionParams (since this is used by the Url helper to create canonicalized URLs).
3. Put descriptions of the injections into \Yii::$app->requestedParams.

Motivation 1

Local-first injection allows us to override configuration for specific modules. It could even be argued that we should not fall back to the DI container at all. The issue I have with the DI container is that it will instantiate any class, even if it has no definition.
While some of my services live in the DI container (and are defined there) we should in my opinion definitely not promote code like this:

public function actionCreateCar(Car $newCar) {
    // Do stuff here
    $newCar->save()
}

Motivation 2

Other code than the URL helper might depend on this array, that code might not be able to (securely) handle action params. (For example serializing them could leak DB passwords)

Motivation 3

The in the current code base the only thing that uses \Yii::$app->requestedParams is the log panel. It tries to serialize this array and using a string description guarantees us that we can actually serialize it (a class containing a closure for example would cause exceptions). Also, when checking the log panel it shows sensible information:

Q	A
Is bugfix?	❌
New feature?	✔️
Breaks BC?	❌
Tests pass?	?
Fixed issues	#17722

[SamMousa]

@samdark I need your opinion on how to deal with exception handling.
If the DI container or module cannot provide a required service, it should lead to HTTP 500 type errors; no matter what the client does it will not be able to fix the request.

Given that we could just allow the generic exception handling to happen since the DI container will already throw exceptions.

Please let me know your thoughts.

[samdark]

I think it's a good idea to make it behave the same was as in Yii 3 Injector: https://github.com/yiisoft/injector/tree/master/docs/en#how-it-works

[samdark]

But, of course, there are Yii 2 specific things since not everything is in DI container...

[samdark]

Design decisions 1-2 make sense. Not sure what's meant by "descriptions of the injections".

[samdark]

If the DI container or module cannot provide a required service, it should lead to HTTP 500 type errors; no matter what the client does it will not be able to fix the request.

A runtime exception would do.

[SamMousa]

@samdark check the screenshot of the debugger; hmm from my phone the quote part didn't work.

Not sure what's meant by "descriptions of the injections".

One of the images above is from the debugger request panel. That panel shows the request params. To do this it serializes them at the end of the request and deserializes them to show them in the panel.
When we inject a service it would get serialized as a whole, this will not always work (service objects can contain closures). Regardless, when debugging request params it's not needed to inspect the state of the inspected services, you just want to know which ones were injected. Using a description will tell you that.
So instead of showing a dump from the user component object, it will tel you this:

Component: \yii\web\User $user

[samdark]

OK, that's fine about description.

[SamMousa]

This is ready, the code climate notice about npath complexity should (imo) be ignored.
The status of Yii2 means that we should not do large refactors since this code is unlikely to see (much) future development.

[schmunk42]

The method bindActionParams() has an NPath complexity of 384. The configured NPath complexity threshold is 200

@samdark Maybe you can adjust this value for yii2...

[samdark]

Or we can refactor it a bit to reduce complexity...

[samdark]

Reviewing it again I wonder if it's a good idea to apply it to console controllers as well.

[SamMousa]

I currently use it in both

[SamMousa]

And it makes sense in console definitely

[samdark]

@SamMousa do you have ready to apply code? If yes, would you please apply it? If no, I think I can handle it but that would take more time.

[SamMousa]

This trait works for both controllers; I used that as the basis, couldn't exactly copy paste it though.
https://github.com/SAM-IT/yii2-magic/blob/master/src/Traits/ActionInjectionTrait.php

[samdark]

Implemented it for console. Going to add tests for it.

[samdark]

Done.

[samdark]

@SamMousa would be great if you'll take a look at the code.

[dmotitsk]

@SamMousa Can you please explain why you added a commit that checks that there is a definition in the container for a injecting type. I think that there is a good reason for this, but I can see it for now. Thank you.

[SamMousa]

The reasoning is that you only inject services, so you shouldn't use it to create arbitrary objects, for example a new AR model. This implementation ensures that only things that you have explicitly defined, as a component in the app or a definition in the container, can be injected.
It also helps with debugging since injecting the wrong class will lead to a failure instead of getting some instance that has not been configured and then having your code fail later.

Imagine injecting a DB connection and due to misconfiguration getting a connection without configuration...

[dmotitsk]

Thank you for your explanation.

As for me it is a little bit confusing, because it differs from the way the container works in other cases. And it is a little bit inconvenient to define services each time I want to inject them into action. What about injecting objects other than services - no one deny people to do
Yii:$container->get(SomeClass::class) inside action. So those who want to do will do it anyway. In function declaration or inside it. So as for me it will be much more convenient to allow to inject objects (services) without the requirement to define them in the container. But I also understand your point and anyway thank you for this implementation.

[dmotitsk]

@SamMousa Do you mind if I create a new issue to see what the community thinks about it?

[SamMousa]

It's not up to me to mind, but I think it's a very bad idea...
There is absolutely no reason to misuse your container like a universal factory like that.
Instead if you need object construction, create factories and inject them...