Yii2: 2.0.36 breaks Codeception tests

[DmLapin]

What steps will reproduce the problem?

After install 2.0.36 update all our codeception tests became broken. Reason: https://github.com/yiisoft/yii2/pull/18127.
If I remove

elseif (is_array($dependency)) {
    $dependencies[$index] = $this->resolveDependencies($dependency, $reflection);
}

from \yii\di\Container::resolveDependencies, tests become OK.

First of all, it IS breaking change a-priory because of Container::resolveDependencies behaviour change.

Problem starts here: vendor/codeception/codeception/src/Codeception/Lib/Connector/Yii2.php:75. Connector use Yii::createObject (and therefore dependency container) for Application creation. We have application config like this:

return [
...
'container' => [
    'definitions' => [
        common\interfaces\NewsletterEmailGeneratorInterface::class => [
            ['class' => common\components\newsletter\NewsletterEmailGenerator::class],
            [
                di\Instance::of(web\View::class),
                di\Instance::of('internalFilesystem'),
]
...

Recursive resolveDependencies call try get 'internalFilesystem' component, but it's not configured in DI Container on this stage! We got:
[yii\base\InvalidConfigException] Failed to instantiate component or class "internalFilesystem".

Example of stack trace:

 Test  unit/commands/news/news_domain/NewsBorrowCommandTest.php:testInvalidData

  [yii\base\InvalidConfigException] Failed to instantiate component or class "internalFilesystem".

.../vendor/yiisoft/yii2/di/Container.php:449
.../vendor/yiisoft/yii2/di/Container.php:447
.../vendor/yiisoft/yii2/di/Container.php:374
.../vendor/yiisoft/yii2/di/Container.php:159
.../vendor/yiisoft/yii2/di/Container.php:484
.../vendor/yiisoft/yii2/di/Container.php:491
.../vendor/yiisoft/yii2/di/Container.php:491
.../vendor/yiisoft/yii2/di/Container.php:491
.../vendor/yiisoft/yii2/di/Container.php:491
.../vendor/yiisoft/yii2/di/Container.php:395
.../vendor/yiisoft/yii2/di/Container.php:159
.../vendor/yiisoft/yii2/BaseYii.php:365
.../vendor/codeception/codeception/src/Codeception/Lib/Connector/Yii2.php:75
.../vendor/codeception/codeception/src/Codeception/Lib/Connector/Yii2.php:53
.../vendor/codeception/codeception/src/Codeception/Module/Yii2.php:185
.../vendor/codeception/codeception/src/Codeception/Subscriber/Module.php:56

| Q | A
| ---------------- | ---
| Yii version | 2.0.36
| PHP version | 7.4

[samdark]

it's not configured in DI Container on this stage!

Where/how is it configured?

[DmLapin]

Where/how is it configured?

Not sure, that understand question. When we create application in typical way ($application = new yii\web\Application($config);), container options from $config['container'] go to \yii\base\Application::setContainer(). But codeception creates application in another way:

/** @var \yii\web\Application $app */
$this->app = Yii::createObject($config);

See this part of stacktrace:

.../vendor/yiisoft/yii2/di/Container.php:395
.../vendor/yiisoft/yii2/di/Container.php:159
.../vendor/yiisoft/yii2/BaseYii.php:365
.../vendor/codeception/codeception/src/Codeception/Lib/Connector/Yii2.php:75

There is no configured application instance (and therefore no configured container) on line Container.php:395, but recursive dependency resolve starts.

[samdark]

I see. @SamMousa is it better to be adjusted in Codeception module or in the framework (not sure how yet)?

[SamMousa]

First of all: #18127 seems to be bad regardless of this specific issue:

• As @DmLapin noted it changes behavior, so it is a BC break
• It introduces (possible) infinite recursion without checks

Then there is the Yii2 issue of attempting to have everything in the configuration array. Things like the logger, errorhandlers and possibly the container get populated from the application config, but they are not part of the application instance.

The application in the codeception module is recreated for every test, but the container is not. As @DmLapin correctly notes, the module uses the container to ask for an application object given a configuration array. If this array refers to objects that are not yet configured, because they are configured by the application at some later stage this will cause issues.

I see several options to remedy the situation:

1. Revert the change from #18127
2. Refactor the config so DI container config lives outside the application config, which from my point of view will improve the code structure regradless of other solutions
3. Have the Yii2 Codeception module parse the configuration array and treat some keys specially

I think 3 is the worst solution; the whole point of the module is to NOT know how the configuration format works, it is not relevant if we let Yii's own code figure that out. I believe 2 is a bad thing to ask from a user even if it (subjectively, in my view) improves their code; especially considering Yii2 should not be releasing BC breaking versions.

The option 1 is best in my opinion, since we have only had the feature for a very short while there will not be a lot of applications that are not under active development that use it. Apps under active development that use such a new feature will likely be able to step away from it.
Upon further inspection, I think the resolveDependencies call should also be more secure in things like forwarding the $reflection variable, since it makes no sense to use that for nested array dependencies..

In conclusion, I propose reverting #18127 and for these advanced cases just relying on a \Closure for configuration of the DI container. It is cleaner and more readable for all PHP users instead of using some secret array language understood only by Yii developers. The array config should be limited to simple cases in all other cases it imo more sensible to use a (static) closure.

[samdark]

@hiqsol what do you think about reverting it?

[DmLapin]

Additional thoughts about Codeception connector. If recursive resolveDependencies will be remained (now or in future), it will be not good way to create application object through Yii::createObject. It will try to resolve all dependencies at start but not on-demand. Performance of tests will degrade.

[SamMousa]

It won't, the dependencies need to be instantiated anyway, so delaying it is not an option.
Note that optional dependencies still are delayed (not all components are instantiated for example). So performance wise it is not an issue.

[DmLapin]

It won't, the dependencies need to be instantiated anyway, so delaying it is not an option.

If one test use only one application component, we shouldn't resolve all Instance objects on all levels of application config - it's waste of time. See example in my first post: NewsBorrowCommandTest don't use internalFilesystem and don't even use NewsletterEmailGeneratorInterface.

[SamMousa]

Yeah I agree, but even resolving dependencies recursively shouldn't do that right? Since things like the components are array are not in the dependencies list so shouldn't be recursively resolved?

[DmLapin]

@SamMousa I agree with your statement above: for these advanced cases just relying on a \Closure. In our applications we stick with this approach. I even think that

vendor/yiisoft/yii2/di/Container.php:395
$config = $this->resolveDependencies($config);

is redundant. But it's outside of this ticket.

[hiqsol]

How do you use these references:

                di\Instance::of(web\View::class),
                di\Instance::of('internalFilesystem'),

Do you resolve them with explicit use of container in your code?
Which is bad practice.
Changes in #18127 are intended to avoid this practice and to allow create complex objects directly with container.
That's why I want #18127 to be saved.

Unfortunately closure is not a general solution cause closures cannot be merged in contrast to arrays.
While closure can be used for this specific case: those references can be redone as closures and they will not be touched by resolveDependencies.

[SamMousa]

Merging arrays is not a good practice since it is based on a lot of assumptions. If my base config changes the merged result could no longer be valid. (Services don't need to be in a class hierarchy and LSP does not need apply to the configuration side of a service).

This is however another discussion.

What kind of objects need arrays of services anyway? Why not just add those to the constructor with typed arguments and let them be resolved that way?

[hiqsol]

The example is given in the PR itself: https://github.com/yiisoft/yii2/pull/18127#issue-438065979

[hiqsol]

Merging arrays is not a good practice since it is based on a lot of assumptions

But that's the only advantage of array based Yii-style configs.
Otherwise builder-style configs looks better.

I appreciate array merging very much cause it allows me to split my applications in pluggable modules.
We have great experience with that.

E.g. in this example https://github.com/yiisoft/yii2/pull/18127#issue-438065979
additional formatters can be easily added in additional pluggable modules.

[SamMousa]

Yeah, I'm aware of the pros and cons from our discussion regarding Yii3 DI package ;-)

Anyway, for Yii2 it is a breaking change which to me means we shouldn't have applied it and therefore should revert it as soon as possible.

If you want to have your use case in a BC way, why not add support to arrays for Instance::of()?
Like this could be done in a subclass even:

So we would get:

return [
    ContentTypeMiddleware::class => [
        '__construct()' => [
            Instance::of(StreamFactory::class),
            MultipleInstance::of([
                'json' => JsonFormatter::class,
                'yaml' => YamlFormatter::class,
            ]),
        ],
    ],
];
/**
 * This class represents a string keyed list of dependency definitions that can be resolved by the DI container...
 */
class MultipleInstance extends Instance ... 

This makes it more explicit, while keeping the usability you mention.

[hiqsol]

Unfortunately, it will not work.
MultipleInstance::of evaluates to object instance which cannot be merged.

Anyway, for Yii2 it is a breaking change which to me means we shouldn't have applied it and therefore should revert it as soon as possible

I propose to not hurry up with the reverting.
I want to ask @DmLapin to use solution with Closure for this case. The change will be small and I can explain more if necessary.
And wait if there is going to be more complaints. I hope there will be not :)

[SamMousa]

But there were no complaints with the previous state either.. Breaking BC and then not reverting because there might not be (enough) complaints puts an unfair burden on the people relying on Yii to not break BC.

Also note, that in many cases even if the code doesn't break, it might become less efficient due to the recursion which causes early instantiation. This is way harder to catch and will likely not lead to complaints, but will lead to degraded performance.

What requests have there been made to support this specific use case? Why does @DmLapin have to change his code to use a Closure when your use case can also be implemented with a Closure?

The reasoning here is simply unfair, we break something then without sufficient logical arguments propose not to revert it?

Anyway, there are ways to solve your problem of array mergeability in user code without changing the framework:

return [
    // A service need not be an object
    'listOfFormatters' => [JsonFormatter::class, YamlFormatter::class],
    'formatterInstances' => static function(Container $container) {
         $result = [];
         foreach($container->get('listOfFormatters') as $key => $definition) {
             $result[key] = $container->get($definition);
         }
        return $result;
    },
    ContentTypeMiddleware::class => static function(Container $container): ContentTypeMiddleware {
        return new ContentTypeMiddleWare($container->get(StreamFactory::class), $container->get('formatterInstances'));
    }
]

But all these examples are besides the point; it cannot be the case that you place the burden of the argument on the party that notifies us of a BC break that we did not plan.
It should be reverted and then the burden is on you to:

• Implement the feature in a non BC breaking way

or

• Convince us that the BC break is acceptable

I'm pretty sure we can come up with a solution that does not break BC. We could even make this feature optional and have people explicitly enable it in their container config, that way it can be turned on and off.

[hiqsol]

The change is released so reverting is a great BC-break! at least for my team ;)

[SamMousa]

Yeah that's true. Hopefully my PR is a middle ground

[SamMousa]

That is acceptable :)

[hiqsol]

This feature was also implemented in Yii 3 container and it is considered good and helpful.
And it was understood that it is a potential BC-break. The risks were considered small and the changes were taken.

I believe this feature is a great step forward from bad practice of manual resolving Instance::of refs with explicit use of container. It allows simplify code and enrich config.
And you still demand to revert it to let to keep this bad practice.
It makes me sad :( I don't get the idea! BC-breaks just happen. It's not first and not the last one.

@DmLapin what do think?
If you use Instance::of in your config I believe you can use the feature to your benefit and improve your code.

[SamMousa]

It makes me sad :( I don't get the idea! BC-breaks just happen. It's not first and not the last one.

It should not happen in a framework that is no longer in active development and. Break anything or everything in Yii3, but don't break stuff in Yii2. Adding a new feature is fine, but breaking stuff definitely is not.

FYI. I do not use Instance::of() at all, for me that is just another symptom of the bad stuff that is array config. My components get their dependencies via their constructor meaning autowiring just works. I require interfaces that then get resolved by the container without me using the container anywhere in my code.
Also note that the Instance class has a get() function which actually abstracts away the dependency on the container. From a users' code perspective calling a public function ->get() on an injected object (the result of Instance::of()) is perfectly valid.

Consider this object:

class X extends Component {
    public $depA;

    public function init() {
        if ($this->depA instanceof Instance) {
            $this->depA = $this->depA->get();
        }
    }

This is perfectly valid code:

• The DI container is not used as a service locator
• We do not inject the container or reference it's global singleton instance

The fact that Instance internally does those bad things is none of my concern, if you implement Instance in another way as long as you expose a get() that allows me to resolve it, this code is clean.

You are not actually solving a real issue of bad code, all you are doing is adding one more way in which Yii configuration code is not readable for non-Yii developers.

[hiqsol]

My components get their dependencies via their constructor meaning autowiring just works

Me too. I hope everybody does this basics. But it still doesn't help with more complex cases.
Use of Instance::of and the feature gives even more freedom of moving burden of building of objects outside of the code.

The fact that Instance internally does those bad things is none of my concern, if you implement Instance in another way as long as you expose a get() that allows me to resolve it, this code is clean.

Agree. With known limitations of hidden global variable though, including problems with testing ;)

You are not actually solving a real issue of bad code, all you are doing is adding one more way in which Yii configuration code is not readable for non-Yii developers.

Partially agree... I've definitely improved my code by moving complexity from code to config.
The Yii-style config is not perfect but I just don't know any better :)

[SilverFire]

class X extends Component {
    public $depA;

    public function init() {
        if ($this->depA instanceof Instance) {
            $this->depA = $this->depA->get();
        }
    }

This is perfectly valid code:

For classes that extend Component – yes, this is a valid example as they are already bound to the framework. When I configure a DIC for a class that is not related to Yii, this example becomes invalid.

In PHP-DI, they have DI\Get(Some::class) that works pretty the same way as @hiqsol pushed.

[DmLapin]

How do you use these references:

                di\Instance::of(web\View::class),
                di\Instance::of('internalFilesystem'),

Do you resolve them with explicit use of container in your code? Which is bad practice.

I have bad news for you :) It'is officialy recommended syntax (see example with Instance::of('tempFileStorage')) and it's resolved automatically by Container when we need the instance of NewsletterEmailGenerator (see closer my example). It's completely clean and correct config because dependencies are injected in NewsletterEmailGenerator constructor and we don't resolve them manually. We use this pattern in many places of several applications and now we are locked to get any framework update.

Some words about https://github.com/yiisoft/yii2/pull/18127#issue-438065979. I agree with @SamMousa. In my opinion, good OOP practice could be something like this:

ContentTypeMiddleware::class => [
    '__construct()' => [
        Instance::of(StreamFactory::class),
        Instance::of(TypeResolverInterface::class),     
    ],
],

where TypeResolverInterface is responsible for actual formatters and configured separately.

[SilverFire]

I do not use Instance::of() at all, for me that is just another symptom of the bad stuff that is array config. My components get their dependencies via their constructor meaning autowiring just works. I require interfaces that then get resolved by the container without me using the container anywhere in my code.

As for me, it's not about autowiring at all.

Say I have 3rd party UrbanDictionary, CambridgeDictionary, and a few more. They all depend on Psr\Http\Client\ClientInterface, but it should be pre-configured with the correct URI, headers, and so on. The container will never autowire them correctly!

I will either declare 3 aliases, e.g.

'UrbanDictionaryHttpClient' => [
    '__class' => HttpClient::class,
    'uri' => 'https://urbandictiionary.com',
],
// ...

and then highlight which one should I use for each Dictionary:

UrbanDictionary::class => [
    '__construct()' => [
        Instance::of('UrbanDictionaryHttpClient'),
    ],
],

Or I should configure it as imperative dependency creation (that is worse as for me)

UrbanDictionary::class => function () {
    $httpClient = new HttpClient(); // configure
    return new UrbanDictionary($httpClient);
},

[SamMousa]

Or I should configure it as imperative dependency creation (that is worse as for me)

What about this:

UrbanDictionary::class => static function (Container $container) {
    return new UrbanDictionary($container->get('UrbanDictionaryHttpClient'));
},

Note that this use of the container is valid since we are configuring the container.
It could even be the case that this works, if not we could make it work:

UrbanDictionary::class => static function (HttpClientInterface $urbanDictionaryClient) {
    return new UrbanDictionary($urbanDictionaryClient);
},

Since for closures as config we can use param name based resolution. Not saying I endorse this specifically, just showing some alternatives.

[hiqsol]

It'is officialy recommended syntax

Oh yes :facepalm:
That's this syntax. The feature must be fixed for it. And test added.
I hope that will close the question.

[SilverFire]

What about this:

UrbanDictionary::class => static function (Container $container) {
    return new UrbanDictionary($container->get('UrbanDictionaryHttpClient'));
},

Note that this use of the container is valid since we are configuring the container.

This one is valid, agreed. And we've used it for a long time. But the config merging is not possible in this way

[SamMousa]

I'm aware; whether that is a bad thing or not is an interesting but different discussion.

So towards a resolution:

That's this syntax. The feature must be fixed for it. And test added.

Does that mean merging my PR is okay with you? It fixes this issue immediately while making the new feature available at the flip of a boolean.

[hiqsol]

I need to take a closer look.
I hope to fix the issue without making the feature optional.
Is it ok?

[SamMousa]

Definitely fine with

[hiqsol]

Sorry for long delay. I had no time.
I can't reproduce the issue. I'm trying to create a test for this issue.
Please see #18270 .
What needs to be added to reproduce the problem?

[samdark]

@DmLapin, @SamMousa would you please help with some additional info?

[samdark]

@SamMousa would it solve the issue to create application with new in Codeception module same way as it is done in framework bootstrap file itself?

[SamMousa]

Sure but that will break tests for other people. The issue with this is that it breaks BC and that simply means should revert it...

[SamMousa]

It would break for people who have subclassed the application object..

[samdark]

Added a test for it. Likely it can be simplified.