fix: refresh REQUEST after URI GET normalization by rahul05ranjan · Pull Request #10606 · codeigniter4/CodeIgniter4

rahul05ranjan · 2026-10-09T12:48:42Z

Description

Fixes #9872. SiteURIFactory can normalize $_GET after PHP has populated $_REQUEST, leaving stale query values for getVar() and Validation::withRequest(). This follows the direction agreed on in #10587: refresh REQUEST once during URI parsing while keeping getVar()'s existing lookup behavior.

Summary

SiteURIFactory parses the normalized query
  → update GET
  → refresh only REQUEST keys derived from GET, POST, or COOKIE
  → retain unrelated or independently changed REQUEST values

Superglobals::getRequestData() provides the merged source data in PHP's configured order. Both URI parsing paths use it before and after GET normalization. No general synchronization is added to setGetArray().

Evidence

Before: The two new SiteURI regressions fail on develop: the old query key remains in REQUEST, and code stays stale.
After: SiteURIFactoryDetectRoutePathTest.php passes (34 tests, 42 assertions), including a withRequest() validation check and preservation of application-set values.
SuperglobalsTest.php passes (56 tests, 115 assertions); SiteURIFactoryTest.php passes (8 tests, 32 assertions).
File-scoped PHPStan and PHP CS Fixer checks pass on the changed PHP files. Full CI remains with GitHub Actions.

Merge Danger

Door: Two-way. Reverting the commit restores the previous request initialization behavior.

Blast Radius: Request initialization in the two SiteURIFactory URI parsing paths. An application-defined REQUEST value that is identical to its old merged source value cannot be distinguished from that source value; the refresh treats it as source-derived.

Checklist:

Securely signed commit (verified by GitHub)
Component PHPDoc added where it explains the merge and refresh behavior
Unit testing with >80% coverage (focused tests pass; coverage not measured locally)
User guide changelog updated; the existing getVar() reference remains accurate
Conforms to style guide (file-scoped PHP CS Fixer passes)

rahul05ranjan · 2026-10-09T12:50:03Z

@paulbalandan The new PR's workflows are waiting for approval to run. Could you approve them when you get to this review? Here's the PHPUnit run; the focused local results are in the PR description.

michalsn

Overall, I think this looks good.

michalsn · 2026-10-09T13:08:13Z

+     *
+     * @return array<string, request_items>
+     */
+    public function getRequestData(?string $requestOrder = null): array


I think this method should be marked as "internal".

Done — getRequestData() is now marked @internal in 5d28859. Thanks for the review!

Copilot

🟡 Changes recommended

The new tests depend on the host’s request_order, and SiteURIFactory’s PHPDoc omits its new $_REQUEST side effect.

3 open findings

Avoid assuming GET is enabled in the request-order regression fixture · New
Make request-order test deterministic by passing GP explicitly · New
Update PHPDoc to document $_REQUEST mutations · New

What changed in this PR

Fixes stale $_REQUEST values after URI query normalization while preserving custom entries.

Changes:

Adds configured GET/POST/COOKIE merging.
Refreshes source-derived REQUEST entries during URI parsing.
Adds regression tests and changelog documentation.

Base branch was unavailable; develop compatibility policy was applied. A focused PHP runtime probe passed; full validation remains with CI.

File	Description
`system/Superglobals.php`	Adds merged request-data retrieval.
`system/HTTP/SiteURIFactory.php`	Refreshes REQUEST after GET normalization.
`tests/system/SuperglobalsTest.php`	Tests request merging semantics.
`tests/system/HTTP/SiteURIFactoryDetectRoutePathTest.php`	Tests URI normalization regressions.
`user_guide_src/source/changelogs/v4.7.5.rst`	Documents the fix.

🧠 Review effort: Balanced

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

rahul05ranjan · 2026-10-17T05:43:46Z

+
+        $factory = new SiteURIFactory(new App(), $superglobals);
+
+        $this->assertSame('ci/woot', $factory->detectRoutePath('QUERY_STRING'));


Addressed in c247f6e: both tests now skip when GET is not part of the effective request order. The check mirrors the request_order → variables_order → GP fallback that Superglobals::getRequestData() itself uses, so the fixtures no longer depend on the host configuration. It also covers the line 316 case you pointed out.

rahul05ranjan · 2026-10-17T05:43:56Z

+        $this->superglobals->setGetArray(['get_key' => 'get_value']);
+        $this->superglobals->setPostArray(['post_key' => 'post_value']);
+
+        $data = $this->superglobals->getRequestData();


Addressed in c247f6e: this test now passes 'GP' explicitly, like the other order-specific tests. I also passed 'G' explicitly in testGetRequestDataReflectsGetChanges(), which had the same environment dependency.

rahul05ranjan · 2026-10-17T05:44:06Z

        }

-        // Update our global GET for values likely to have been changed
+        // Refresh only the request values derived from superglobals after GET changes.


Addressed in c247f6e: the class-level PHPDoc and the PHPDoc of createFromGlobals(), parseRequestURI(), and parseQueryString() now document the $_REQUEST update as well. (For the class-level summary I added a separate $_REQUEST line rather than rewriting the existing sentence, because develop has since removed the adjacent @see line and rewriting it would make this branch conflict with develop — happy to merge the two lines into one if you prefer.)

rahul05ranjan · 2026-10-12T05:55:31Z

Rebased on the latest develop to drop the merge commit — that should sort out the no-merge-commits check.

The PHP 8.5 - SQLite3 failure in the previous run looks like a runner flake: the job log shows a transient autoload failure right before it, and there's no unterminated comment near system/CodeIgniter.php:568 on the head. The fresh CI run should settle it either way.

rahul05ranjan · 2026-10-12T06:00:52Z

The rebase dropped the commit signature, so the signed-commits check was failing on the rewritten commit. I've restored the original signed commit (7ff3dff) as the branch head — history is linear and signed again.

paulbalandan · 2026-10-12T08:23:37Z

@rahul05ranjan have you done the review comments? if yes, please comment on them. I can't see "Outdated" so I'm not sure if those were done.

…mutations Co-authored-by: rahul05ranjan <11793455+rahul05ranjan@users.noreply.github.com>

rahul05ranjan · 2026-10-17T05:44:19Z

Hi @paulbalandan, sorry for the slow response on this.

To confirm the status: the "mark as internal" comment was addressed earlier in 5d28859, and the three remaining review comments are now addressed in c247f6e:

SuperglobalsTest: getRequestData() now receives an explicit order (GP/G) so the tests no longer depend on the host's request_order.
SiteURIFactoryDetectRoutePathTest: the REQUEST-refresh fixtures now skip when GET is not part of the effective request order, instead of assuming it.
SiteURIFactory: the class-level PHPDoc and the docblocks of createFromGlobals(), parseRequestURI(), and parseQueryString() now document the $_REQUEST mutations too.

I've also replied on each of the three threads. Happy to adjust anything further.

fix: refresh REQUEST after URI GET normalization

7ff3dff

rahul05ranjan mentioned this pull request Oct 9, 2026

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

Closed

4 of 5 tasks

carson-codeigniter4 Bot added the bug Verified issues on the current code behavior or pull requests that will fix them label Oct 9, 2026

paulbalandan requested a review from michalsn October 9, 2026 12:50

michalsn approved these changes Oct 9, 2026

View reviewed changes

paulbalandan requested a balanced review from Copilot October 9, 2026 13:20

Copilot started reviewing on behalf of paulbalandan October 9, 2026 13:20

View session

Copilot AI reviewed Oct 9, 2026

View reviewed changes

rahul05ranjan force-pushed the fix/request-refresh-after-uri-normalization branch from 2cebb1e to ce9bd19 Compare October 12, 2026 05:55

rahul05ranjan force-pushed the fix/request-refresh-after-uri-normalization branch from ce9bd19 to 7ff3dff Compare October 12, 2026 06:00

Mark Superglobals::getRequestData() as internal

5d28859

carson-codeigniter4 Bot added the stale Pull requests with conflicts label Oct 17, 2026

This comment has been minimized.

Sign in to view

test: make request-order fixtures deterministic and document REQUEST …

c247f6e

…mutations Co-authored-by: rahul05ranjan <11793455+rahul05ranjan@users.noreply.github.com>

rahul05ranjan force-pushed the fix/request-refresh-after-uri-normalization branch from 28da7f9 to c247f6e Compare October 17, 2026 05:42

carson-codeigniter4 Bot removed the stale Pull requests with conflicts label Oct 17, 2026

Navigation Menu

Repository navigation

fix: refresh REQUEST after URI GET normalization - #10606

fix: refresh REQUEST after URI GET normalization#10606
rahul05ranjan wants to merge 3 commits into
codeigniter4:developfrom
rahul05ranjan:fix/request-refresh-after-uri-normalization

rahul05ranjan commented Oct 9, 2026

Uh oh!

rahul05ranjan commented Oct 9, 2026

Uh oh!

michalsn left a comment

Uh oh!

michalsn Oct 9, 2026

Uh oh!

rahul05ranjan Oct 14, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

rahul05ranjan Oct 17, 2026

Uh oh!

rahul05ranjan Oct 17, 2026

Uh oh!

rahul05ranjan Oct 17, 2026

Uh oh!

rahul05ranjan commented Oct 12, 2026

Uh oh!

rahul05ranjan commented Oct 12, 2026 •

edited

Loading

Uh oh!

paulbalandan commented Oct 12, 2026

Uh oh!

This comment has been minimized.

rahul05ranjan commented Oct 17, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants


		$factory = new SiteURIFactory(new App(), $superglobals);

		$this->assertSame('ci/woot', $factory->detectRoutePath('QUERY_STRING'));

Conversation

rahul05ranjan commented Oct 9, 2026

Summary

Evidence

Merge Danger

Uh oh!

rahul05ranjan commented Oct 9, 2026

Uh oh!

michalsn left a comment

Choose a reason for hiding this comment

Uh oh!

michalsn Oct 9, 2026

Choose a reason for hiding this comment

Uh oh!

rahul05ranjan Oct 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

rahul05ranjan Oct 17, 2026

Choose a reason for hiding this comment

Uh oh!

rahul05ranjan Oct 17, 2026

Choose a reason for hiding this comment

Uh oh!

rahul05ranjan Oct 17, 2026

Choose a reason for hiding this comment

Uh oh!

rahul05ranjan commented Oct 12, 2026

Uh oh!

rahul05ranjan commented Oct 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulbalandan commented Oct 12, 2026

Uh oh!

This comment has been minimized.

rahul05ranjan commented Oct 17, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rahul05ranjan commented Oct 12, 2026 •

edited

Loading