Repository navigation
fix: refresh REQUEST after URI GET normalization - #10606
rahul05ranjan wants to merge 1 commit into
Conversation
|
@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
left a comment
There was a problem hiding this comment.
Overall, I think this looks good.
There was a problem hiding this comment.
🟡 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
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.
2cebb1e to
ce9bd19
Compare
|
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. |
ce9bd19 to
7ff3dff
Compare
|
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. |
|
@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. |
This comment has been minimized.
This comment has been minimized.
28da7f9 to
c247f6e
Compare
|
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:
I've also replied on each of the three threads. Happy to adjust anything further. |
paulbalandan
left a comment
There was a problem hiding this comment.
Please rebase this against latest develop, and a few finishing touches.
361237b to
bcbf67e
Compare
fb8692a to
451dc1c
Compare
Fixes codeigniter4#9872. SiteURIFactory can normalize $_GET after PHP has populated $_REQUEST, leaving stale query values for getVar() and Validation::withRequest(). Refresh only source-derived REQUEST entries once during URI parsing while preserving unrelated or custom REQUEST values.
451dc1c to
7837e0f
Compare
|
@paulbalandan @michalsn All review comments and suggestions have been addressed, the branch is rebased onto the latest develop with a verified signed commit, and all Carson checks are passing. Whenever you get a moment, this is ready for final review and merge. Thanks! |


Description
Fixes #9872.
SiteURIFactorycan normalize$_GETafter PHP has populated$_REQUEST, leaving stale query values forgetVar()andValidation::withRequest(). This follows the direction agreed on in #10587: refresh REQUEST once during URI parsing while keepinggetVar()'s existing lookup behavior.Summary
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 tosetGetArray().Evidence
develop: the old query key remains in REQUEST, andcodestays stale.After:
SiteURIFactoryDetectRoutePathTest.phppasses (34 tests, 42 assertions), including awithRequest()validation check and preservation of application-set values.SuperglobalsTest.phppasses (56 tests, 115 assertions);SiteURIFactoryTest.phppasses (8 tests, 32 assertions).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:
getVar()reference remains accurate