Bug: IncomingRequest does not respect constructor param URI

[kenjis]

The following test fails on develop and 4.4.

Failed asserting that two strings are identical.
Expected :'http://example.com/foo/bar'
Actual   :'http://example.com'

<?php

namespace CodeIgniter;

use CodeIgniter\HTTP\IncomingRequest;
use CodeIgniter\HTTP\URI;
use CodeIgniter\HTTP\UserAgent;
use CodeIgniter\Test\CIUnitTestCase;
use Config\App;

final class TestTest extends CIUnitTestCase
{
    public function test()
    {
        $appConfig = new App();
        $request   = new IncomingRequest(
            $appConfig,
            new URI($appConfig->baseURL . 'foo/bar'),
            null,
            new UserAgent()
        );

        $this->assertSame(
            'http://example.com/foo/bar',
            (string) $request->getUri()
        );
    }
}

[iRedds]

I do not see anything supernatural.
You set the URI value in the constructor, and then the URI definition is called in the constructor according to the data from _SERVER, and since the array is an empty array, we get what we get.

CodeIgniter4/system/HTTP/IncomingRequest.php

Line 178 in 93006e3

$this->detectURI($config->uriProtocol, $config->baseURL);

CodeIgniter4/system/HTTP/IncomingRequest.php

Lines 269 to 273 in 93006e3

	protected function parseRequestURI(): string
	{
	if (! isset($_SERVER['REQUEST_URI'], $_SERVER['SCRIPT_NAME'])) {
	return '';
	}

In my opinion, the Request object should be created based on superglobal arrays without specifying a URI or UserAgent in the constructor. And as a consequence, the URI object must be created based on the Request object.

But if we need to specify a different URI, then PSR-7 has a withURI() method.

[kenjis]

You set the URI value in the constructor, and then the URI definition is called in the constructor according to the data from _SERVER, and since the array is an empty array, we get what we get.

The behavior is unexpected.
And every time we create a Request object, we must set superglobals.
It is a bad developer experience.

[iRedds]

I have a feeling that you made a mistake with the quoted fragment.

And every time we create a Request object, we must set superglobals.

Every time? But the Request object is created only once.

[kenjis]

No, we create many times when writing test code.
And the Request does not work as expected.

[iRedds]

Once I already showed an example of creating a Request object through a factory.

class Request
{
    protected function __construct(array $query = [], array $param = [], array $cookie = [], array $server = [])
    {
        // ...
       $this->uri = URI::createFromRequest($this);
    }

    public static function createFromGlobal(): Request
    {
        return new self($_GET, $_POST, $_COOKIE, $_SERVER);
    }
}

And as an example for tests it can look like this.

public function test()
{
    $appConfig = new App();
    $request   = Request::createFromGlobal()
        ->withUri(URI::createFromString($appConfig->baseURL . 'foo/bar'));

    $this->assertSame(
        'http://example.com/foo/bar',
        (string) $request->getUri()
    );
}

The difference is not too big.

[kenjis]

The test code is a bit better, but still is not good.
Because it depends on superglobals.
If you forget to reset superglobals, the state of the Response object wil be different.

The need to change the state with withUri() is also not good.

[iRedds]

If we forget to reset the superglobals, then most of our current tests will fail.

The constructor in my example is protected, but if you make it public or add a factory that does not pass any data, then there will be no dependence on superglobal arrays.

[kenjis]

On develop, the following tests pass:

$ vendor/bin/phpunit tests/system/HTTP/
PHPUnit 9.6.9 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.1.20
Configuration: /Users/kenji/work/codeigniter/official/CodeIgniter4/phpunit.xml

.................................................................................................................................. 130 / 711 ( 18%)
.................................................................................................................................. 260 / 711 ( 36%)
.................................................................................................................................. 390 / 711 ( 54%)
.................................................................................................................................. 520 / 711 ( 73%)
.................................................................................................................................. 650 / 711 ( 91%)
.............................................................                                                                      711 / 711 (100%)

Time: 00:36.190, Memory: 40.00 MB

OK (711 tests, 1463 assertions)

Change the comment:

--- a/tests/system/HTTP/IncomingRequestTest.php
+++ b/tests/system/HTTP/IncomingRequestTest.php
@@ -21,7 +21,7 @@ use InvalidArgumentException;
 use TypeError;
 
 /**
- * @backupGlobals enabled
+ * @ backupGlobals enabled
  *
  * @internal
  *

The test class resets the superglobals:

CodeIgniter4/tests/system/HTTP/IncomingRequestTest.php

Lines 34 to 41 in 93006e3

	protected function setUp(): void
	{
	parent::setUp();
	$this->request = new IncomingRequest(new App(), new URI(), null, new UserAgent());
	$_POST = $_GET = $_SERVER = $_REQUEST = $_ENV = $_COOKIE = $_SESSION = [];
	}

$ vendor/bin/phpunit tests/system/HTTP/
PHPUnit 9.6.9 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.1.20
Configuration: /Users/kenji/work/codeigniter/official/CodeIgniter4/phpunit.xml

.................................................................................................................................. 130 / 711 ( 18%)
.................................................................................................................................. 260 / 711 ( 36%)
.................................................................................................................................. 390 / 711 ( 54%)
.................................................................................................................................. 520 / 711 ( 73%)
.................................................................................................................................. 650 / 711 ( 91%)
.........................................FFF.................                                                                      711 / 711 (100%)

Time: 00:27.323, Memory: 40.00 MB

There were 3 failures:

1) CodeIgniter\HTTP\URITest::testBasedNoIndex
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'http://example.com/ci/v4/controller/method'
+'http://example.com/ci/v4/'

/Users/kenji/work/codeigniter/official/CodeIgniter4/tests/system/HTTP/URITest.php:1004
/Users/kenji/work/codeigniter/official/CodeIgniter4/vendor/bin/phpunit:122

2) CodeIgniter\HTTP\URITest::testBasedWithIndex
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'http://example.com/ci/v4/index.php/controller/method'
+'http://example.com/ci/v4/'

/Users/kenji/work/codeigniter/official/CodeIgniter4/tests/system/HTTP/URITest.php:1034
/Users/kenji/work/codeigniter/official/CodeIgniter4/vendor/bin/phpunit:122

3) CodeIgniter\HTTP\URITest::testForceGlobalSecureRequests
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'https://example.com/ci/v4/controller/method'
+'https://example.com/ci/v4/'

/Users/kenji/work/codeigniter/official/CodeIgniter4/tests/system/HTTP/URITest.php:1074
/Users/kenji/work/codeigniter/official/CodeIgniter4/vendor/bin/phpunit:122

FAILURES!
Tests: 711, Assertions: 1453, Failures: 3.

Frankly, I am not sure if the current test code is correct.

[kenjis]

This may be much clearer.

Reset the superglobals:

--- a/tests/system/HTTP/URITest.php
+++ b/tests/system/HTTP/URITest.php
@@ -26,6 +26,13 @@ use Config\App;
  */
 final class URITest extends CIUnitTestCase
 {
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $_POST = $_GET = $_SERVER = $_REQUEST = $_ENV = $_COOKIE = $_SESSION = [];
+    }
+
     public function testConstructorSetsAllParts()
     {
         $uri = new URI('http://username:password@hostname:9090/path?arg=value#anchor');

$ vendor/bin/phpunit tests/system/HTTP/URITest.php 
PHPUnit 9.6.9 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.1.20
Configuration: /Users/kenji/work/codeigniter/official/CodeIgniter4/phpunit.xml

.................................................................................................................FFF.........      125 / 125 (100%)

Time: 00:01.290, Memory: 16.00 MB

There were 3 failures:

1) CodeIgniter\HTTP\URITest::testBasedNoIndex
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'http://example.com/ci/v4/controller/method'
+'http://example.com/ci/v4/'

/Users/kenji/work/codeigniter/official/CodeIgniter4/tests/system/HTTP/URITest.php:1011

2) CodeIgniter\HTTP\URITest::testBasedWithIndex
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'http://example.com/ci/v4/index.php/controller/method'
+'http://example.com/ci/v4/'

/Users/kenji/work/codeigniter/official/CodeIgniter4/tests/system/HTTP/URITest.php:1041

3) CodeIgniter\HTTP\URITest::testForceGlobalSecureRequests
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'https://example.com/ci/v4/controller/method'
+'https://example.com/ci/v4/'

/Users/kenji/work/codeigniter/official/CodeIgniter4/tests/system/HTTP/URITest.php:1081

FAILURES!
Tests: 125, Assertions: 197, Failures: 3.

[iRedds]

Maybe I misunderstood you about resetting superglobals, but I meant @backupGlobals enabled.
That is, that superglobal arrays restore their state before each test.

[kenjis]

@backupGlobals enabled is bad practice in the past. We should remove the tag in the future.

Probably nobody knows the exact global state or superglobal values when running CI4 tests.
They may be backed up with @backupGlobals enabled in some tests, and not backed up in other tests.

It is very difficult to say that the global state before a test runs is really correct for the test.
And a new test code will change the global state.

[iRedds]

To opt out of the @backupGlobals tag, we need to opt out of manually populating the $_SERVER variable in tests.

[kenjis]

We need to remove all $_SERVER in framework classes.
The $_SERVER values should be injected to the class.
And of course we need to remove $_SERVER in tests.

[kenjis]

Closed by #7282