Skip to content

fix(Router): fix back-reference regex in replaceBackReferences() for 9+ placeholders - #10762

Open
banghamid wants to merge 3 commits into
codeigniter4:developfrom
banghamid:fix/router-replace-back-references
Open

banghamid wants to merge 3 commits into
codeigniter4:developfrom
banghamid:fix/router-replace-back-references

Conversation

@banghamid

Copy link
Copy Markdown
Contributor

Description
When Config\Routing::$multipleSegmentsOneParam is true, Router::replaceBackReferences() previously built the regex pattern as '/$([1-' . count($matches) . '])/u'.

When a route had 9 or more placeholders (count($matches) >= 10), the character class became [1-10] (or [1-11], etc.), which in PCRE matches only the range '1'–'1' (or '0'). As a result:

  • Back-references $2 through $9 were not replaced at all and remained literal '$2'..'$9' strings.
  • Multi-digit back-references such as $10 had only the $1 prefix replaced, corrupting the parameter value.

This PR changes the pattern in Router::replaceBackReferences() to '/$([1-9][0-9]*)/u' so both single-digit ($1–$9) and multi-digit ($10+) back-references are properly matched and replaced.

Checklist:

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

@carson-codeigniter4 carson-codeigniter4 Bot added the bug Verified issues on the current code behavior or pull requests that will fix them label Nov 11, 2026
@gr8man

gr8man commented Nov 11, 2026

Copy link
Copy Markdown
Contributor

Nice catch! That was a subtle PCRE quirk — [1-10] was indeed evaluated as the character range '1'–'1' plus the character '0', effectively dropping $2–$9 and corrupting $10+.

The fix /\$([1-9][0-9]*)/u is clean, minimal, ReDoS-safe, and independent of count($matches). Also, using [0-9] instead of \d under the /u flag correctly avoids matching Unicode non-ASCII digits.

The regression test clearly captures the bug and fails without this change.

Optional non-blocking suggestions:

  • Consider adding a test case (or data provider row) for an out-of-range reference (e.g. $11 on a 10-placeholder route) to lock in the behavior that non-existent groups resolve to '' via $matches[$index] ?? ''.
  • Testing the exact boundary threshold (9 placeholders, where count($matches) === 10 first caused the bug) alongside 10+ could also be nice to have.

LGTM! 👍

@banghamid

Copy link
Copy Markdown
Contributor Author

Thanks for the review @gr8man! Fair point on those edge cases — just pushed a commit adding the 9-placeholder boundary and an out-of-range $11 check to lock in the '' behavior.

@gr8man

gr8man commented Nov 11, 2026

Copy link
Copy Markdown
Contributor

You need to fix the tests. composer cs-fix

@banghamid

Copy link
Copy Markdown
Contributor Author

Thanks @gr8man! Renamed the data provider to match PHP CS Fixer's naming convention in 31dfc6e, and all 56 CI checks are now passing.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Verified issues on the current code behavior or pull requests that will fix them

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants