Yii2: Request object is not properly reset if it's singleton

[ddinchev]

This bug effects codeception:2.4.1+ and concerns Yii2 connector module and Yii's DI container, particularly for functional tests that perform multiple requests (or if the code redirects) within one test. If yii\web\Request has been configured to be a singleton, it is not reset between requests, specifically this line always return the first yii\web\Request ever created in the test.

This is my Yii configuration that triggers the bug:

return [
    'components' => [
        'request' => \yii\web\Request::class,
    ],
    'container' => [
        'singletons' => [
            \yii\web\Request::class => \yii\web\Request::class,
        ]
    ],
];

The reason I want to use singleton for request object is that in my actions, controllers and other components, I prefer to use dependency-injection for the Request object instead of service locator and given that the lifetime of a PHP application is single call - having a singleton Request for that lifetime makes perfect sense. However, if the code redirects, the tests will fail - as the request object will have the old state (actually will be the original Request instance) after the redirect.

Acceptance tests and actual code work just fine. As a workaround, right now I've changed my functional tests config to override the default config by the following:

return [
    ...
    'container' => [
        'singletons' => [
            \yii\web\Request::class => new \yii\helpers\UnsetArrayValue(),
        ],
        'definitions' => [
            \yii\web\Request::class => [
                'class' => \yii\web\Request::class,
                'enableCsrfValidation' => false,
                'enableCookieValidation' => false,
            ],
        ],
    ],
];

This way the \yii\web\Request is correctly recreated between requests, as it's no longer singleton.

Debugging this caused me tons of trouble (out of 400 tests that passed in codeception:2.4.0, more than 120 would fail in codeception:~2.4.1). IMHO the way Request object is reset should be fixed so it works even for singleton container definition or at least detect that the Request object is defined as singleton and proper warning is displayed so users know how to deal with it.

@SamMousa I tried to figure out elegant way to reset singleton request objects, but the public interface of \yii\di\Container does not allow it. However, it does have ::hasSingleton() method which might be used to generate a clear warning for the user.

[SamMousa]

Singletons are bad for testing; as you've just discovered. We do not reset the DI container during a test on purpose.

One thing you could do for testing is use a different definition.

I have the same issue with components and DI. For actions and controllers I use DI via a trait without requiring a singleton.

One solution is to subclass the container to allow you to clear the singleton.

In my opinion this is not something that should be fixed in the library; I do however like your idea for a check and warning to prevent the debugging.

Note that your solution to use singletons to mimic DI could simply be replaced by using a non singleton closure as the definition. This closure then uses the yii app instance to retrieve the request object. This keeps your components clean.

[ddinchev]

Thanks for the quick reply!

While I generally agree with "Singletons are bad for testing" - I'm not sure if the argument applies well here. They make perfect sense with DI and the reason this particular case causes trouble is due to the way the Yii2 module resets the request/response components. The module just assumes these objects are always accessed statically through the service locator - which is essentially a singleton that is easier to reset.

I've already shared one of the workarounds you proposed ("One thing you could do for testing is use a different definition.") and while you have suggested other valid workarounds, they are still workarounds. I think the least we should do is to warn the user that yii\di\Container request/response singletons are not properly reset (similar to the warning that Request behaviours will be lost on recreation) so that users don't waste time debugging this.

[SamMousa]

You are right about the service locator part; but that's the design of the framework. So I've assumed you use the framework as designed. Note that your use is not better, just a necessary evil when you want to use DI with yii2

[ddinchev]

Don't get me wrong, I don't want to argue. I really want to find a solution for the fact that 30% of tests that worked in 2.4.0 wouldn't in 2.4.1+ - bc has been violated, even if it's a non-obvious case. Again, just showing a warning should be fine.

DI seems encouraged and I haven't seen any documentation opposing what I do right now. In fact yii:2.1 and yii:3 it is even possible that request/response might not be accessible through service locator at all - only via DI. So it's hard to accept that "I'm not using the framework as designed" - I just don't use it as most people do, to make it easier to unit test my controllers/actions/components.

Actually even you seem to agree with this.

[SamMousa]

No worries, I love a good debate. For 3 we definitely want to improve the use of DI within the framework.

For now does my proposed workaround not solve your problem without making it harder to test your components?

Also I agree on the warning, it would be a good improvement. Regarding the BC break you are right and we are watching that more closely now. One of the issues was a lack of a contract and tests so it was hard to change anything without breaking BC in some way.

[ddinchev]

Well, I already posted in my original post that I've solved it (unsetting the singleton for tests from the container config). My issue was posted so that hopefully it will be addressed somehow, and so that other users with the same issue hopefully could find this thread. If we come to an agreement how to address the issue, I can give it a try and open a PR.

[SamMousa]

Hmm, but not making it a singleton makes it so multiple components can get injected with a different request object. So tests can definitely fail because of that, I think...

[ddinchev]

This is correct - different Request objects actually can be injected. But it seems that the Request objects are initialised from super-globals every time correctly - it's less efficient but seems to work fine (before this change, many tests of my already huge test suite would fail but work correctly after the change).

I still think this is kinda irrelevant to the main issue - which is that functional tests could fail when the Request or Response are defined as singleton in DI container, without any hint why they failed, which leads to frustration and debugging. I think we should do one of the following: 1) Correctly reset singleton objects somehow for the purpose of functional tests. 2) Warn the user that singleton Request/Response could have unpredictable effect on functional tests, as they are not reset correctly.

Again, I'm happy to try to present a PR, if there is an agreement which one to pursue.

[SamMousa]

I would review & accept a PR for 2.