Bug: `getVar()` behaves inconsistently with GET parameters

[michalsn]

PHP Version

8.4

CodeIgniter4 Version

4.6.4

CodeIgniter4 Installation Method

Composer (using codeigniter4/appstarter)

Which operating systems have you tested for this bug?

macOS

Which server did you use?

cli-server (PHP built-in webserver)

Environment

development

Database

What happened?

In some cases, using $validator->withRequest() will not work correctly. Let's consider this test:

CodeIgniter4/tests/system/HTTP/SiteURIFactoryDetectRoutePathTest.php

Lines 226 to 241 in 11f0130

	public function testQueryStringWithQueryString(): void
	{
	// /index.php?/ci/woot?code=good#pos
	$_SERVER['REQUEST_URI'] = '/index.php?/ci/woot?code=good';
	$_SERVER['QUERY_STRING'] = '/ci/woot?code=good';
	$_SERVER['SCRIPT_NAME'] = '/index.php';
	$_GET['/ci/woot?code'] = 'good';
	$factory = $this->createSiteURIFactory($_SERVER);
	$expected = 'ci/woot';
	$this->assertSame($expected, $factory->detectRoutePath('QUERY_STRING'));
	$this->assertSame('code=good', $_SERVER['QUERY_STRING']);
	$this->assertSame(['code' => 'good'], $_GET);
	}

As you may notice in the above test, SiteURIFactory updates some superglobals, here:

CodeIgniter4/system/HTTP/SiteURIFactory.php

Lines 162 to 175 in 11f0130

	if (trim($path, '/') === '' && str_starts_with($query, '/')) {
	$parts = explode('?', $query, 2);
	$path = $parts[0];
	$newQuery = $query[1] ?? '';
	$this->superglobals->setServer('QUERY_STRING', $newQuery);
	} else {
	$this->superglobals->setServer('QUERY_STRING', $query);
	}
	// Update our global GET for values likely to have been changed
	parse_str($this->superglobals->server('QUERY_STRING'), $get);
	$this->superglobals->setGetArray($get);

The problem is that while $_SERVER['QUERY_STRING'] and $_GET are being updated, the $_REQUEST isn't. And $_REQUEST is needed to make getVar() work correctly.

CodeIgniter4/system/Validation/Validation.php

Lines 505 to 527 in 11f0130

	public function withRequest(RequestInterface $request): ValidationInterface
	{
	/** @var IncomingRequest $request */
	if (str_contains($request->getHeaderLine('Content-Type'), 'application/json')) {
	$this->data = $request->getJSON(true);
	if (! is_array($this->data)) {
	throw HTTPException::forUnsupportedJSONFormat();
	}
	return $this;
	}
	if (in_array($request->getMethod(), [Method::PUT, Method::PATCH, Method::DELETE], true)
	&& ! str_contains($request->getHeaderLine('Content-Type'), 'multipart/form-data')
	) {
	$this->data = $request->getRawInput();
	} else {
	$this->data = $request->getVar() ?? [];
	}
	return $this;
	}

CodeIgniter4/system/HTTP/IncomingRequest.php

Lines 496 to 506 in 11f0130

	public function getVar($index = null, $filter = null, $flags = null)
	{
	if (
	str_contains($this->getHeaderLine('Content-Type'), 'application/json')
	&& $this->body !== null
	) {
	return $this->getJsonVar($index, false, $filter, $flags);
	}
	return $this->fetchGlobal('request', $index, $filter, $flags);
	}

Steps to Reproduce

public function testQueryStringWithQueryStringAndRequest(): void 
{ 
     // /index.php?/ci/woot?code=good#pos 
     $_SERVER['REQUEST_URI']  = '/index.php?/ci/woot?code=good'; 
     $_SERVER['QUERY_STRING'] = '/ci/woot?code=good'; 
     $_SERVER['SCRIPT_NAME']  = '/index.php'; 
  
     // these are always the same at the beginning
     $_GET['/ci/woot?code'] = 'good';
     $_REQUEST['/ci/woot?code'] = 'good'; 
  
     $factory = $this->createSiteURIFactory($_SERVER); 
  
     $expected = 'ci/woot'; 
     $this->assertSame($expected, $factory->detectRoutePath('QUERY_STRING')); 
     $this->assertSame('code=good', $_SERVER['QUERY_STRING']); 
     $this->assertSame(['code' => 'good'], $_GET); 
     // this will fail
     $this->assertSame(['code' => 'good'], $_REQUEST); 
} 

Expected Output

Validation method $validator->withRequest() should work correctly.

The most tempting solution is to add syncRequestAfterGetChange() to Superglobals and call it from SiteURIFactory whenever $_GET is modified. This would keep $_REQUEST synchronized with current values.

The downside is it introduces "magic" behavior that violates standard PHP semantics where $_REQUEST is populated once and never auto-updated. However, since we update $_GET as a "standard", behavior, then updating $_REQUEST may be acceptable?

I was thinking about something like this:

public function syncRequestAfterGetChange(): void
{
    $requestOrder = ini_get('request_order') ?: ini_get('variables_order');

    $this->request = [];

    foreach (str_split($requestOrder) as $type) {
        match ($type) {
            'G' => $this->request = array_merge($this->request, $this->get),
            'P' => $this->request = array_merge($this->request, $this->post),
            'C' => $this->request = array_merge($this->request, $this->cookie),
            default => null,
        };
    }

    $_REQUEST = $this->request;
}

But I'm unsure if this is the right direction.

Anything else?

https://forum.codeigniter.com/showthread.php?tid=93652

[neznaika0]

I don't think auto-synchronization is necessary. Since this is a test problem, it is worth calling the method only in certain cases. In production, everything should work as intended.

I'm not sure, but a similar behavior can be seen when merging ENV and SERVER. If we change the value, it may remain the same in another array.

[michalsn]

This is not a test-related problem. The test only serves as an example to reliably reproduce the issue.

We are modifying $_GET, but getVar() relies on $_REQUEST, which has not been modified.

The core issue is how getVar() is currently handled. While we already discourage users from using it, $validation->withRequest() is still available, and it may not work correctly when validating parameters that originate from $_GET.

I'm not sure, but a similar behavior can be seen when merging ENV and SERVER. If we change the value, it may remain the same in another array.

The difference with $_REQUEST is that this superglobal is expected to contain values combined from $_GET, $_POST, and $_COOKIE (depending on configuration). However, it is populated only once at the beginning of the request. Since we modify $_GET early in the request lifecycle, those changes are not reflected in $_REQUEST.

[neznaika0]

I understand the problem. That's why I mentioned the SERVER.

I'm worried about the reverse problem - we've changed the REQUEST or GET and expect the other array not to change as intended in PHP. As a result, we will get synchronization.

[paulbalandan]

Will getVar() be eventually get deprecated? If not, can we not just return a merged array from the 3 other superglobals that always get updated and leave $_REQUEST as is?

[michalsn]

I'm not proposing permanent synchronization, as that is not how superglobals are intended to work. The proposal is to synchronize $_REQUEST only once, at a single, well-defined point during the request lifecycle in SiteURIFactory:

CodeIgniter4/system/HTTP/SiteURIFactory.php

Lines 171 to 175 in 11f0130

	// Update our global GET for values likely to have been changed
	parse_str($this->superglobals->server('QUERY_STRING'), $get);
	$this->superglobals->setGetArray($get);

The alternative would be to change how $validation->withRequest() works by avoiding getVar() and replacing the call to it with the merged $_GET, $_POST, and $_COOKIE data.

However, a one-time synchronization seems like the better approach. It is more reliable and makes getVar() work correctly.

[michalsn]

@paulbalandan That is also a valid solution. This would eventually allow us to get rid of getVar(), which is probably a good idea.