Terminate method not being called on terminable middleware in test

[AndrewFeeney]

After upgrading a Laravel 6 application to Laravel 7 I noticed that a test which was making assertions against behaviour which is executed in the terminate() method of a custom middleware was failing.

After further inspection, I found that even though the application behaviour had not changed, the terminate() method was never being called in the test. The laravel module calls terminate() on the HTTP Kernel class, but passes in the original request object to the terminate() method.

module-laravel/src/Codeception/Lib/Connector/Laravel.php

Line 147 in a072611

$this->app->make(Kernel::class)->terminate($request, $response);

In order for the terminate() method to be called on the route middleware, the applicable middlewares are fetched from route instance returned from $request->route() if any. However, the route does not appear to be resolved on the request instance that gets passed into the terminate method.

It would appear to me that in the normal execution of the Laravel HTTP Kernel, the instance of the request that is being passed into the terminate method is being resolved out of the service container rather than being mutated by reference such as this test module code might expect. If you dump the request instance returned by request() after the call to handle() on the kernel you can see that the route has been resolved and bound to the request instance.

I haven't yet managed to isolate the specific change which is causing my test, which previously passed the original "laravel5" module and Laravel 6, and now fails with both Laravel 7, and 8 using this new "laravel" module. It might be a change in the Laravel framework, or it might be related to changes in the Codeception module.

Swapping the $request variable with the request() global helper function (which resolves the request out of the service container under the hood) fixed the issue. I'm not sure that this is the way you would want to fix it in a patch, since you might not want to rely on the request() global function or use implicit service location like that, but it did confirm that this appears to be the problem.

        $this->app->make(Kernel::class)->terminate(request(), $response);

I'd be happy to look at submitting a PR with a fix, but since the tests are in a different repo, I'll have to take a look at that repo and get my head around how the tests workflow goes. I wanted to leave this issue here in case anybody else hits this, and / or has a better idea of the cause or a solution.

[TavoNiievez]

@AndrewFeeney I went through all the changes I made in the migration from module-laravel5 to module-laravel and didn't find any significant changes that could've broken something with this flow.

Most of the changes were related to fixing the database related code that changed in Laravel 8 and adding strict typing.

So, i'm wondering if to get the request object like this...:

$this->app->make(Kernel::class)
-    ->terminate($request, $response); 
+    ->terminate($this->app['request'], $response);

... would help fix your issue too.

I have just released version 2.1.0 of this module, so it is a good time to submit a PR for this if you have time.

If you have any questions regarding the test that you should write in the test project, feel free to ping me.

[countless-integers]

@TavoNiievez thanks for this tip. I've noticed a similar behaviour: global middleware would run terminate, but not route middleware. I tracked it down to Illuminate\Foundation\Http\Kernel::gatherRouteMiddleware receiving a request object that it could not resolve a route from and hence "gather" route middleware for it. Whatever is happening with the request passed in the connector, when it arrives in Kernel::terminate (source) and then through terminateMiddleware in gatherRouteMiddleware (source) it cannot get/resolve the route there and causes terminate to only loop over global middlewares. Passing the request fetched from the app container changes this and correct middlewares are prepended and looped over.

[countless-integers]

Looks like last commit to main on that module was in February... 💀

[TavoNiievez]

Hi @countless-integers ,
I have reviewed your PR, but the reasons for not making such a change remain the same as in September 2021, I am not sure what possible effects it may have on existing tests or whether the request object should be replaced from line 112 or 113 instead of 114.
If you somehow manage to make a test/PoC in the test project where, when applying your changes the solution is evident I will gladly proceed to merge the changes.

[countless-integers]

I did make this PR.
Would that suffice?

[countless-integers]

Just checking in, have you (or any other maintainer) had a moment to check those PRs?

[countless-integers]

@TavoNiievez any chance this gets in? PR with a test is in the other repo too

[countless-integers]

Anyone?