fix: make getVar() read merged superglobals instead of stale $_REQUEST

[rahul05ranjan]

Description

Fixes #9872. SiteURIFactory updates GET data after PHP initializes $_REQUEST, so getVar() can return stale values to Validation::withRequest().

This changes getVar() to read a fresh merged view of GET, POST, and COOKIE data, respecting request_order / variables_order, without mutating $_REQUEST. Explicit setGlobal('request', ...) overrides and JSON requests retain their existing lookup behavior.

The merge preserves numeric keys, merges nested arrays recursively, normalizes lowercase order letters, and processes each source only once. It reuses the existing fetchGlobal() filtering path and clears temporary merged data in finally, avoiding a new protected helper that could conflict with application subclasses. The user guide and changelog describe the updated data source.

Checklist:

• Securely signed commits
• Component(s) with PHPDoc blocks, only if necessary or adds value (without duplication)
• Unit testing, with >80% coverage
• User guide updated
• Conforms to style guide

Testing:

• PHP 8.2.34: IncomingRequestTest.php 116 tests, SuperglobalsTest.php 55 tests, RequestTest.php 40 tests, and the three affected Validation cases pass (214 tests total).
• Regression tests reproduced the explicit-override failure, lowercase-order failure, and subclass signature collision before the fixes. Additional tests cover repeated reads and cleanup after a filter throws.
• File-scoped PHPStan and PHP CS Fixer checks pass. Coverage was not measured locally, so the coverage checklist item remains unchecked.
• Full validation remains with GitHub Actions. File-scoped Rector proposes removing existing parent calls outside this change; the constructor proposal also occurs on the previous PR head.

[carson-codeigniter4]

Hi there, @rahul05ranjan! 👋

Thank you for sending this PR!

We expect the following in all Pull Requests (PRs).

• PRs must be sent to the appropriate branch
• All git commits must be GPG-signed
• Must follow our style guide
• Be commented in the PHP source file
• Be documented in the user guide
• Be unit tested
• Pass all checks in GitHub Actions

Important

We expect all code changes or bug-fixes to be accompanied by one or more tests added to our test suite to prove the code works.

If pull requests do not comply with the above, they will likely be closed. Since we are a team of volunteers, we don't have any more time to work
on the framework than you do. Please make it as painless for your contributions to be included as possible.

See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md

[neznaika0]

Hi. See #10205
We deliberately did not add synchronization. You could add a comment about this.

[rahul05ranjan]

Thanks for the pointer, @neznaika0 — I see now that #10205 took this same syncRequest() approach and was closed as "not the desired approach." I'll rework this PR accordingly.

Based on the discussion in #9872, the preferred direction is to avoid mutating $_REQUEST entirely and instead have getVar() return a merged view of $_GET, $_POST, and $_COOKIE (respecting request_order), or change withRequest() to stop relying on getVar(). That also moves us toward eventually deprecating getVar().

I'll drop the Superglobals::syncRequest() method and the SiteURIFactory changes, and rework the fix around getVar()/withRequest() instead. I'll update the PR shortly.

[rahul05ranjan]

Reworked as discussed. The syncRequest() approach is gone.

getVar() now returns a merged view of $_GET, $_POST, and $_COOKIE (respecting request_order/variables_order) instead of reading the stale $_REQUEST, so $_REQUEST is never mutated. This also moves us toward eventually deprecating getVar().

Changes:

• Superglobals::getRequestData() returns the merged data (no mutation).
• RequestTrait::fetchFromArray() extracts the shared filtering/index logic.
• getVar() uses getRequestData() + fetchFromArray().
• Reverted the SiteURIFactory syncRequest() calls.
• Updated tests accordingly.

@neznaika0 @paulbalandan — does this match the direction you had in mind from #9872?

[rahul05ranjan]

Update: the rework is complete and all commits are now GPG-signed.

Summary of the final state:

• getVar() reads a merged view of $_GET/$_POST/$_COOKIE (respecting request_order) instead of the stale $_REQUEST, so $_REQUEST is never mutated.
• Superglobals::getRequestData() returns the merged data; RequestTrait::fetchFromArray() extracts the shared filtering logic.
• Reverted the SiteURIFactory syncRequest() calls.
• Fixed the static-analysis issues (protected method, no short ternary) and updated the tests.

The Carson checks (signed-commits, no-merge-commits, pr-title-linter) are all passing. The remaining GitHub Actions workflows are showing action_required and need a maintainer to approve them to run.

@neznaika0 @paulbalandan — whenever you have a moment, could you take a look? Thanks!

[neznaika0]

I’m not very good at evaluating PR — I don’t know the exact direction in this matter. For myself, I’ve decided not to use $_REQUEST and getVar(). Do you know any specific cases where this is 100 % necessary? In most cases, it’s better to use an explicit data source (GET, POST, COOKIE).

Earlier, it was said that this is a system class (for tests), and for the user, you need to use Request.

Wait for the member answers.

[Copilot]

Copilot review overview

🟡 Changes recommended

Critical and moderate findings remain around request overrides and synchronizing $_REQUEST.

Review effort: Lite
Findings: 2

Open (2)

• Preserve explicit request overrides when merging superglobals · New
• Add cookie and precedence coverage for merged request variables · New

What changed in this PR

Updates request-variable retrieval so getVar() reflects current GET/POST/COOKIE data after URI parsing.

Changes:

• Adds merged request-data retrieval.
• Refactors reusable array-fetching logic.
• Updates request and validation tests.

File	Summary
tests/​system/​Validation/​ValidationTest.php	Adjusts validation fixtures to use POST data.
tests/​system/​SuperglobalsTest.php	Tests merged request-data behavior.
tests/​system/​HTTP/​IncomingRequestTest.php	Updates request-variable fixtures.
system/​Superglobals.php	Builds merged request data.
system/​HTTP/​RequestTrait.php	Extracts reusable array-fetching logic.
system/​HTTP/​IncomingRequest.php	Uses merged data in getVar().

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[rahul05ranjan]

@neznaika0 thanks for the review — and your instinct here is exactly right.

To be clear, this PR doesn't add any new reliance on $_REQUEST or getVar(). It fixes a real bug in existing code: Validation::withRequest() still calls $request->getVar(), and getVar() was reading the stale $_REQUEST. So the choice isn't "should we use getVar()" — it's "getVar() is still here and still used by withRequest(), so it must return correct data."

The direction matches what @paulbalandan suggested in #9872: instead of mutating $_REQUEST (which #10205 did and was rightly rejected), getVar() now returns a merged view of $_GET/$_POST/$_COOKIE respecting request_order. $_REQUEST is never touched. This also moves us toward eventually deprecating getVar(), since it no longer depends on $_REQUEST at all.

On "100% necessary" cases: I agree — there's no case where getVar() is strictly necessary, and explicit getGet()/getPost()/getCookie() are always clearer. But withRequest() is public API that currently routes through getVar(), and until that's changed it needs to work. Happy to open a follow-up switching withRequest() to explicit sources if maintainers prefer that direction.

[rahul05ranjan]

@paulbalandan — gentle ping on this one. The rework is complete and follows the direction you outlined in #9872: getVar() now returns a merged view of $_GET/$_POST/$_COOKIE (respecting request_order) instead of reading the stale $_REQUEST, and $_REQUEST is never mutated.

All Carson checks pass (signed-commits, no-merge-commits, pr-title-linter). The remaining GitHub Actions workflows are showing action_required and need a maintainer to approve them to run (first-time contributor). Would you be able to take a look when you have a moment? Thanks!

[paulbalandan]

Hi, thanks for picking this up. Just a few initial comments:

1. Since the approach changed, the PR title and body should be edited to reflect the new approach taken. This is because the PR would be squashed on merge with the PR title as the commit message.
2. The merging of superglobals seems to be not the way PHP would do it. First, when the data contains a numeric key, like when parsing 100=foo, the array_merge would just renumber it. Second, it is not recursive unlike what PHP do when it creates a $_REQUEST.
3. Docs should be updated to match new behavior. Plus a changelog line.

[paulbalandan]

Also, could you remove those backslashes? 😅

[rahul05ranjan]

@paulbalandan thanks for the review! All three points are addressed, and the backslashes are gone 😅

1. PR title/body — updated to reflect the new approach (no more syncRequest()). Title is now fix: make getVar() read merged superglobals instead of stale $_REQUEST.
2. Merge semantics — switched from array_merge() to array_replace_recursive(), which matches PHP's own $_REQUEST merge (php_autoglobal_merge): numeric keys are preserved and array values are merged recursively. Added testGetRequestDataPreservesNumericKeys and testGetRequestDataMergesRecursively to cover both.
3. Docs + changelog — updated the getVar() section in the user guide and added a "Bugs Fixed" entry to v4.7.5.rst.

Let me know if there's anything else you'd like adjusted. Thanks!

[paulbalandan]

Thanks for the changes. A few more comments.

[rahul05ranjan]

@paulbalandan thanks for the follow-up! All three points addressed:

1. Dropped the $requestOrder parameter — getRequestData() now takes no arguments and reads the ini settings directly. Tests use ini_set('request_order', ...) to exercise the different orders deterministically.
2. Cast ini_get() to string — both request_order and variables_order are now (string) ini_get(...) and compared against '' only.
3. Changelog — the entry is now alphabetically ordered and labeled **HTTP:** (referencing IncomingRequest::getVar()).

Let me know if there's anything else. Thanks!

[paulbalandan]

Can you check on the failing tests?

[paulbalandan]

Thanks for checking that ini_set won't work for request_order. Minor comments, otherwise looks good:

1. incomingrequest.rst line 173 still reads that getVar can fallback to $_REQUEST. it should now be at the merged array
2. the phpstan-ignore at the test demonstrated that the array shapes at Superglobals are incorrectly typed. out of scope of this PR though

[Copilot]

Copilot review overview

🟡 Changes recommended

Request-order parsing diverges from PHP semantics, and the new protected method introduces a patch-line compatibility risk.

Review effort: Balanced
Findings: 1 · 1

Open (2)

• Avoid adding protected helper that breaks existing subclasses · New
• Normalize and deduplicate request_order directives · New

Resolved since last review (2)

• Add cookie and precedence coverage for merged request variables
• Preserve explicit request overrides when merging superglobals

[michalsn]

I still have two concerns:

1. Calling fetchGlobal('request', ...) before getVar() populates the request cache. getVar() then treats it as an explicit override and returns stale REQUEST data instead of merged GPC data. We should distinguish cached data from an explicit setGlobal() override.
2. An explicitly empty request_order="" should produce an empty result. PHP falls back to variables_order only when request_order is unset, but this implementation treats both cases the same.

Could we add regression tests for these cases?

[rahul05ranjan]

@michalsn Thanks for flagging both cases. The cached-request case was real: fetchGlobal('request', ...) could fill the cache before getVar(). I now distinguish an explicit setGlobal('request', ...) override from that cache, and added a regression for the call order you described.

For the empty request_order case, I checked PHP's own php.ini documentation: an empty value falls back to variables_order; it does not make $_REQUEST empty. I kept that behavior and added a test for getRequestData('').

I also rebased away the merge commit that was failing Carson's check. All branch commits are signed. Focused PHP 8.2 tests pass: 117 IncomingRequest, 56 Superglobals, and 40 Request tests. Carson's no-merge-commits and signed-commits checks now pass. The remaining workflows on this new head are awaiting maintainer approval to run. Could a maintainer approve them when reviewing? PHPUnit run

[michalsn]

For the empty request_order case, I checked PHP's own php.ini documentation: an empty value falls back to variables_order; it does not make $_REQUEST empty. I kept that behavior and added a test for getRequestData('').

Ok, sounds good.

[rahul05ranjan]

The two Coding Standards jobs flagged the same one-line docblock on the new request-override property. I changed it to the layout PHP CS Fixer requested. The focused PHP 8.2 fixer check now reports 0 files to fix.

The updated branch is at 59ff334a2. The new workflows are waiting for maintainer approval again; Coding Standards run. Could a maintainer approve that run when convenient?

[michalsn]

I've spent quite a while thinking about this, and my main concern is the complexity of the proposed behavior.

getVar() now combines JSON handling, explicit REQUEST overrides, fresh GPC merging, and temporarily replacing and restoring the REQUEST cache. Each part has a reason, but together they make the method harder to understand and maintain.

This also introduces different data lifetimes within the same request object. After GET data changes through the Superglobals service, getVar() sees the updated value, while getGet() may still return a previously cached value. If setGlobal('request', ...) is called, getVar() uses that override instead of fresh GPC data. Understanding the result therefore requires knowing both which method is called and how the request object was previously modified.

The finally block handles cache restoration correctly, but future changes, custom fetchGlobal() implementations, and filter callbacks need to account for that temporary state.

I don't think these mechanisms are inherently wrong. I'm questioning whether this additional complexity is justified for a bug caused by GET normalization in SiteURIFactory. Could we reconsider a one-time REQUEST update at that point, while preserving application-defined REQUEST values? That would keep the fix closer to its cause and preserve getVar() existing lookup behavior.

The user guide says this method exists for backward compatibility, so I would prefer to keep this fix as narrow as possible.

If we decide that the behavior introduced by this PR is what we want long term, I think we should target the 4.8 branch instead of develop, since it changes the existing contract of getVar().

Thoughts?

[rahul05ranjan]

@michalsn Thanks for the careful review. I agree the current getVar() path has grown complicated for a fix to URI normalization. The temporary cache swap and explicit-override flag are a lot of machinery for a backward-compatibility method.

I avoided changing SiteURIFactory because #10205 was closed, but I shouldn't treat that as ruling out your narrower proposal. That PR rebuilt $_REQUEST after each setGetArray() call; a one-time update that preserves application-defined REQUEST values is a different approach.

I'd be happy to rework this around that normalization point, keeping regression tests for withRequest() and explicit REQUEST values. @paulbalandan, does that align with why #10205 was closed? If the team prefers the fresh merged behavior in this PR, I agree it should target 4.8.

[paulbalandan]

To answer directly: #10205 was closed because I preferred a merged view at the time, not because a one-time refresh was ruled out in #9872. Having seen where the merged view ends up, I agree with @michalsn. Let's do it as two steps. Please open a new PR against develop with the one-time $_REQUEST refresh in SiteURIFactory, refreshing only the keys that come from GET, POST and COOKIE so other values already in $_REQUEST stay untouched. Bring getRequestData() and its tests along since the refresh needs that merge, plus the withRequest() regression tests. Once that is up we can close this one. The remaining idea here, getVar() reading live data, is really a contract change, and the better 4.8 follow-up is to deprecate getVar() and have withRequest() read the merged data directly. Happy to review that as a separate PR if you want to take it.

[michalsn]

That sounds good to me. Once the one-time refresh fixes the bug, I would prefer to keep getVar() existing behavior rather than change its contract unnecessarily.

---

For 4.8, we could formally deprecate both getVar() and withRequest() while preserving their behavior. The user guide already discourages using both methods, and explicitly passing the data to validate seems like a clearer migration path. We should also consider Controller::validate(), since it uses withRequest() internally.

That said, I would prefer to keep these changes gradual and avoid changing too much at once. Deprecation should give users a clear migration path while keeping existing applications working.

I'm also wondering whether we would need replacement methods with clearly defined rules for selecting input data, or whether the existing alternatives are sufficient and we should focus on promoting them.

needs rework in a new direction

[rahul05ranjan]

Thanks, @paulbalandan and @michalsn. I opened #10606 from current develop with the one-time REQUEST refresh in SiteURIFactory. It carries getRequestData() and its merge tests, plus regressions for both URI parsing paths and withRequest(). getVar() itself is unchanged.

The focused tests and file-scoped checks pass, and the commit is verified. I'll close this PR in favor of #10606; the broader deprecation discussion can stay separate for 4.8.