Skip to content

fix(Router): handle {locale} placeholder in redirect routes - #10761

Open
banghamid wants to merge 2 commits into
codeigniter4:developfrom
banghamid:fix/router-redirect-locale
Open

banghamid wants to merge 2 commits into
codeigniter4:developfrom
banghamid:fix/router-redirect-locale

Conversation

@banghamid

Copy link
Copy Markdown
Contributor

Description
In Router::checkRoutes(), {locale} in $routeKey is replaced with '[^/]+' while the original route key registered in RouteCollection is preserved in $matchedKey. Previously, $this->collection->isRedirect($routeKey) and $this->collection->getRedirectCode($routeKey) were called with the modified $routeKey instead of $matchedKey.

As a result, redirect routes containing {locale} were never recognized as redirects:

  • Redirecting to a URI treated the target path as a controller name instead of throwing RedirectException.
  • Redirecting to a named route left $handler as an array and threw a TypeError at str_contains($handler, '::').

This PR:

  • Evaluates {locale} detection before checking for a redirect so unsupported locales are still rejected and $this->detectedLocale is populated.
  • Checks isRedirect($matchedKey) and getRedirectCode($matchedKey) using $matchedKey.
  • Replaces {locale} in $redirectTo with $this->detectedLocale when redirecting from a localized route.

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

Thanks for fixing this, @banghamid! The root cause diagnosis is spot-on — using $matchedKey correctly resolves both URI and named route redirects, and evaluating {locale} detection before checking for redirects is the right approach.

The logic is clean and safe for develop. A couple of suggestions regarding the tests:

  1. Assert HTTP status code:
    testRedirectRoute() currently only checks expectExceptionMessage(). It would be great to also assert the status code (e.g., $this->expectExceptionCode(...)) so that the change to getRedirectCode($matchedKey) is explicitly covered.

  2. Additional test cases (optional but nice to have):

    • A case combining {locale} with dynamic placeholders, e.g.:
      ['{locale}/post/(:num)', '{locale}/article/(:num)', '{locale}/post/$1', '/en/article/1', 'en/post/1', 'alias']
    • A test verifying that useSupportedLocalesOnly(true) throws PageNotFoundException when an unsupported locale is accessed.

LGTM!

@banghamid

Copy link
Copy Markdown
Contributor Author

Thanks for the review @gr8man! Good catch on the status code assertion, didn't notice testRedirectRoute() wasn't checking it yet.

Just pushed an update to add:

$this->expectExceptionCode(302) in testRedirectRoute()
the {locale} + (:num) dynamic placeholder cases
a test for useSupportedLocalesOnly(true) throwing PageNotFoundException on unsupported locales

Let me know if anything else looks off.

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