Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: using resetServices() in tests cause named routes to fail #8018

Closed
michalsn opened this issue Oct 8, 2023 · 11 comments · Fixed by #8047
Closed

Bug: using resetServices() in tests cause named routes to fail #8018

michalsn opened this issue Oct 8, 2023 · 11 comments · Fixed by #8047

Comments

@michalsn
Copy link
Member

michalsn commented Oct 8, 2023

PHP Version

8.2

CodeIgniter4 Version

4.4.1 and develop

CodeIgniter4 Installation Method

Composer (using codeigniter4/appstarter)

Which operating systems have you tested for this bug?

macOS

Which server did you use?

cli

Database

irrelevant

What happened?

Using resetServices() in tests causes named routes to fail.

Steps to Reproduce

Routes.php

$routes->get('sample-url-1', 'HomeController::index', ['as' => 'sample1']);

Sample.php - sample library

namespace App\Libraries;

class Sample
{
    public function urlTo($route)
    {
        return url_to($route);
    }

    public function routeTo($route)
    {
        return route_to($route);
    }
}

SampleTest.php - sample test

use App\Libraries\Sample;
use CodeIgniter\Test\CIUnitTestCase;

/**
 * @internal
 */
final class SampleTest extends CIUnitTestCase
{
    protected function setUp(): void
    {
        $this->resetServices();

        parent::setUp();
    }

    public function testSampleUrlTo()
    {
        $sample = new Sample();
        $this->assertInstanceOf(Sample::class, $sample);

        $this->assertSame(
            'http://example.com/index.php/sample-url-1',
            $sample->urlTo('sample1')
        );
    }

    public function testSampleRouteTo()
    {
        $sample = new Sample();
        $this->assertInstanceOf(Sample::class, $sample);

        $this->assertSame('/sample-url-1', $sample->routeTo('sample1'));
    }
}

Expected Output

Passing tests.

Anything else?

If we comment out the line with $this->resetServices();, then all tests will pass.

@michalsn michalsn added the bug Verified issues on the current code behavior or pull requests that will fix them label Oct 8, 2023
@kenjis
Copy link
Member

kenjis commented Oct 8, 2023

In the constructor of RouteCollection, the loadRoutes() method should be called?

public function __construct(FileLocator $locator, Modules $moduleConfig, Routing $routing)

public function loadRoutes(string $routesFile = APPPATH . 'Config/Routes.php')

@michalsn
Copy link
Member Author

michalsn commented Oct 8, 2023

That would solve the problem, but I've never done anything related to routes, so I don't know how such a change would affect the rest of the code.

@kenjis
Copy link
Member

kenjis commented Oct 10, 2023

In normal use cases, there is no change because loadRoutes() should be called (and is called now) right after the instantiation of RouteCollection.

But the existing tests for the framework would fail, because some tests expect the RouteCollection does not have any route right after the instantiation.

@kenjis
Copy link
Member

kenjis commented Oct 10, 2023

I found a bug #8024 in a test and the framework code.
It may be better to change to call loadRoutes() in the constructor.
But many existing tests for the framework fail, so it seems better to change it in 4.5 just to be on the safe side

@michalsn
Copy link
Member Author

How about adding loadRoutes() to the resetServices()? Would that help with failing framework tests?

@kenjis
Copy link
Member

kenjis commented Oct 10, 2023

Oh, that might be better.
As for RouteCollection, I think it would be preferable to have no route at instantiation.

@kenjis
Copy link
Member

kenjis commented Oct 10, 2023

Oh, it doesn't work.
Services::routes()->loadRoutes() creates the Request instance because RouteCollection depends on Request.
And hack like $_SERVER['HTTP_HOST'] = 'adm.example.com'; for testing does not work in the test method,
because the Request is already created in setUp().

@kenjis
Copy link
Member

kenjis commented Oct 10, 2023

As you probably already know, the workaround is to call Services::routes()->loadRoutes() after resetServices() in the test code.

@michalsn
Copy link
Member Author

Yeah... maybe we should just promote this as a permanent solution. Not sure what others think about it.

@kenjis kenjis removed the bug Verified issues on the current code behavior or pull requests that will fix them label Oct 16, 2023
@kenjis
Copy link
Member

kenjis commented Oct 16, 2023

This is not a bug, and you need to call loadRoutes() if you need.
I sent a PR like that: #8047

@michalsn
Copy link
Member Author

I can live with that, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants