"actingAs" helper is broken in unit tests

[timoschwarzer]

• Laravel Version: 5.5.29
• PHP Version: 7.0, 7.1, 7.2
• Database Driver & Version:

Description:

In unit tests, the user is reset to null even after using the actingAs method when performing a simulated request with the get method.
See: #22649

[abler98]

I think we need two properties for the setUser and request instance. But this may be cause new issues. Perhaps we should revert my changes.

[timoschwarzer]

Yes, this commit should be reverted soon as it likely breaks many unit tests.

[crynobone]

Could someone create a failing tests for this (Laravel does have Integration tests) to mimick the issues. Would really benefit to see when and why it broken. I'm using actingAs() without issue on the latest stable.

[abler98]

@crynobone default guards are not using RequestGuard. Those who use the Passport (or other guards based on RequestGuard) have this issue.

When sending an internal request from the tests, Laravel creates HttpKernel instance and send request to it:

trait MakesHttpRequests
{
    public function call($method, $uri, $parameters = [], $cookies = [], $files = [], $server = [], $content = null)
    {
        $kernel = $this->app->make(HttpKernel::class);

        ...

        // This invoke rebind `request` in app container and RequestGuard set user to null
        $response = $kernel->handle(
            $request = Request::createFromBase($symfonyRequest)
        );

        ...

        return $this->createTestResponse($response);
    }

[crynobone]

So wouldn't it make more sense to try to fix actingAs() to support this scenario since it doesn't change actual app behaviour, correct?

[abler98]

@crynobone in this case, we need to come up with a new way to authorize in tests. I need to write tests and have to come up with something to solve this problem.
P.S. Sorry for my bad English

[crynobone]

I already writen a testcase for actingAs, and bases on Auth::viaRequest() test which uses RequestGuard I still was unable to replicate above claim.

Without proper tests that cover above scenarios anyone might redo the same mistake of sending similar PR and it still going to return green.

To bad if you guys don't see the benefits of adding test to prevent regression bug.