Fix for support Instance::of in configuration

[antonovsky]

Using createObject with \yii\di\Instance::of in configuration does not work. Creating an instance of the application in the suggested way solves the problem.

[SamMousa]

This breaks instantiation using the DI container, therefore it is a BC break. What issue is this PR fixing? Do you have an issue number?

[antonovsky]

I didn't create an issue. I posted this PR instead. Should I create an issue?

[SamMousa]

No but some more explanation in here would be nice. This change breaks some existing behaviour so it needs to be motivated. Can it be fixed in another way without breaking instantiation via the container?

[antonovsky]

Why you think this breaks instantiation using the DI container? What do you mean? The tests in my project are still working correctly. DI work.

[SamMousa]

Why you think this breaks instantiation using the DI container? What do you mean?

I'm sorry I was unclear. It breaks instantiation of the application object itself via the DI container, because it no longer uses the DI container.

Your issue can actually be solved by putting this in the DI config, this wouldn't require any change to the module code.

\yii\web\Application::class => static function(Container $c, array $params): \yii\web\Application {
    return new \yii\web\Application($params);
}

[antonovsky]

My app config looks something like this. Did you mean this? If I place your code in singletons or definitions, the error does not disappear: Fatal error: Uncaught ReflectionException: Class MyComponentClassFromDI does not exist in /vendor/yiisoft/yii2/di/Container.php on line 449

[
    'container' => [
        'singletons' => [
            // singletons
            \yii\web\Application::class => static function(Container $c, array $params): \yii\web\Application {
                return new \yii\web\Application($params);
            },
            'MyComponentClassFromDI' => [
                'class' => app\components\MyComponentClassFromDI::class,
            ],
        ],

        'definitions' => [
            // definitions
        ],
    ],
    'components' => [
        'MyComponentClassFromDI' => \yii\di\Instance::of('MyComponentClassFromDI'),
    ],
  // other application configuration things
];

[antonovsky]

I could be wrong, but i think at the time of the creation of the application itself, the DI container does not exist at all. Hence the problem with Instance::of().

[SamMousa]

That depends on what is in your bootstrap. I create the container before anything else, the container is then used to create the application object.

So that is what will break with your change.

[antonovsky]

Thank you very much. If done in the way you suggested, everything will work.
Last question: Why not let the application itself do the instantiation of the container?

[SamMousa]

This is subjective, others might not share my opinion.

In my view the container consists outside the application and the method of configuring it via app config array is just a dirty hack so people can have all configuration in one place. https://github.com/yiisoft/yii2/blob/master/framework/Yii.php this file creates it as a side effect.

I don't use that file but instead use my own bootstrap that instantiates the container. Imo that's cleaner

[antonovsky]

@SamMousa , How do you like this new revision? I think it works for all options.

[SamMousa]

It doesn't, I think you've misunderstood my issue.

My DI container config tells the container how to create the application. So any creation that uses new Application will not take that into account.

[antonovsky]

@SamMousa , Have you looked at the new code? The new version uses createObject instead of new Application.
That is, instead of changing the way the application object was created, I added Yii::configure(Yii::$container, $config['container']);

[SamMousa]

Hmm, github shows just 1 file changed... and that is only the Yii::configure stuff... let me see if I can find a different diff...

[SamMousa]

Ah wait, now I get it, you have reverted your old change so this is the full feature.

Yes, that looks good to me!

You take out container config and use that directly; feels like a clean solution to me!

[antonovsky]

Yes, i change only 1 file (src/Codeception/Lib/Connector/Yii2.php):

        if (isset($config['container']))
        {
            Yii::configure(Yii::$container, $config['container']);
            unset($config['container']);
        }

[samdark]

Looks alright but we need to fix tests before merge to ensure it doesn't break things: https://github.com/Codeception/module-yii2/issues/22

[antonovsky]

Please rebase on master to get the tests passing.

done

[samdark]

Thanks.