Bug: Running Feature Tests on multiple endpoints using filters

[dcaswel]

Describe the bug
I have a Feature Test class with multiple tests on the same endpoint. This endpoint uses a filter to handle authentication which then adds the authenticated user to the request so it can be used in the controller. When I run the tests individually, it works. However, if I try to run all of the tests in the test class, it gives me an error when trying to access the authenticated user that has been added to the request.

After doing A LOT of digging, I think I have found the issue. The filters functionality alters the request object by altering $this->request in the Filters object. This works most of the time because that property points to the same object as the request that later gets passed to the controller. However, in the use case that I described above, they do not. This is because in Codeigniter::handleRequest() when it gets the filters object using Services::filters(), it is getting a shared instance of Filters which already exists when the second test runs. So it gets back a Filters instance whose request parameter no longer points to the same request as what gets passed to the controller.

A simple solution to this would be to alter Codeigniter::handleRequest() so that when it finishes running the filters, after checking to see if $possibleRedirect is a response, you add this:

if ($possibleRedirect instanceof RequestInterface)
{
    $this->request = $possibleRedirect;
}    

CodeIgniter 4 version
v4.0.3

Affected module(s)

• CodeIgniter\CodeIgniter
• CodeIgniter\Filters\Filters

Expected behavior, and steps to reproduce if appropriate

1. Create a Feature Test class with 2 tests that are testing the same endpoint
2. Create a filter and register it to run on both endpoints. (In mine the filter is in general so it runs everywhere)
3. In your before() on the filter, add a new property to the request and return the request.
4. In your controller endpoint, do something to access the property that you added.
5. Run all the tests in the Feature Test class.
6. This should work but currently it will throw an error saying that the new property doesn't exist on the request object.

Context

• OS: CentOs7
• Web server: Apache 2.4.6
• PHP version: 7.2.24

[dcaswel]

I did discover this morning that if you add this to your test class, it will resolve this issue for now.

public function tearDown():void {
    parent::tearDown();
    Services::reset();
}

It might make sense to put this in \CodeIgniter\Test\FeatureTestCase. I would still consider this a bug but this at least gives a way to get around it until it can be fixed. I will try to create a PR for this if I can find time.

[MGatner]

I've just run into this myself. I see how you dig a lot of digging - it is quite elusive and cryptic. Unfortunately the solution seems to be too. Using Services::reset() will get the job done but also undoes any other mocking and prep from your fixtures. At this point we don't have a way to "reset" a single services so I'm going to suggest a bit of a hacky fix by mocking a fresh copy of the service. Down the road we might want to look at a more sophisticated way to handle this, especially if it turns out there are more service with unwanted "baggage" between tests.

[dcaswel]

I agree that the suggested fix of resetting the filters service will fix this particular use case. I am wondering, however, about my original suggestion for a fix by altering the Codeigniter::handleRequest() method so that if the $possibleRedirect that comes back from the filters is a request, you set the request to the request that came back from the filters instead of relying on the object instance being the same. It seems to me that this would be a more explicit way to make sure the request being used is the one the filters are intending to be used.

[MGatner]

@caswell-wc I'm not completely following. Services::filters will always use Services::response which should be a shared instance. If a filter returns a response that short circuits the filter process (https://github.com/codeigniter4/CodeIgniter4/blob/develop/system/Filters/Filters.php#L183), and otherwise "before" filters don't alter the Response object.

Maybe if you write it up as a PR or just an edit on your branch I would understand better.

[dcaswel]

@MGatner I'm not altering the response, I'm altering the request. In the documentation (https://www.codeigniter.com/user_guide/incoming/filters.html?highlight=filters#before-filters) it says that you can return an altered request to replace the current request. If you look at https://github.com/codeigniter4/CodeIgniter4/blob/develop/system/Filters/Filters.php#L178 you'll see that if you return a request from the filter's before method, the request in the Filters object gets replaced with that. Then after running all filters, if it is running the before filters, it returns the request in the Filters object (https://github.com/codeigniter4/CodeIgniter4/blob/develop/system/Filters/Filters.php#L210). But looking at the code where the filters are run (https://github.com/codeigniter4/CodeIgniter4/blob/develop/system/CodeIgniter.php#L382) It doesn't do anything with that unless it is a response or redirect. What I am proposing is that another check be put below line 391 that looks something like

if ($possibleRedirect instanceof RequestInterface)
{
    $this->request = $possibleRedirect;
}   

in order to make it more explicit that we are replacing the request that the controllers are going to use with the request that came back from the filters.

[MGatner]

Ah I see. Yes that’s a good point. I also notice we don’t have any before filters that return a Request nor tests for this scenario. I think it might be a hole.

Do you have time & capacity to submit a PR? You seem to have a good understanding of these mechanics.

[dcaswel]

I will have to get myself setup to be able to develop on the framework but I will try to find some time to do it. Should we open this issue again or create a new one?

[MGatner]

That would be awesome! FYI the “poorman’s method”:

1. Edit the code to how you want it in your project
2. In this repo click “Fork” and add it to your organization
3. In GitHub browse to each file, click Edit, paste your modified version and add a commit message

If you’re planning on developing more I recommend getting an actual environment, but that’s the quick-and-dirty way to get a PR done!