feat: support Laravel :column binding hint in localized route URLs - #960
Open
jordyvanderhaegen wants to merge 2 commits into
Open
feat: support Laravel :column binding hint in localized route URLs#960jordyvanderhaegen wants to merge 2 commits into
jordyvanderhaegen wants to merge 2 commits into
Conversation
Routes defined with {param:column} syntax (e.g. {post:slug}) were not
resolved correctly when generating localized URLs, as the binding hint
was treated as part of the parameter name.
Refactor substituteAttributesInRoute to use a single preg_replace_callback
that strips the :column hint before looking up the attribute, mirroring
how Laravel's RouteUri::parse() normalises the URI during route compilation.
Also fixes a pre-existing bug in the optional-param cleanup regex
([^)] → [^}]).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Walkthrough
ChangesCustom route key localization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant LaravelLocalization
participant substituteAttributesInRoute
participant UrlRoutable
LaravelLocalization->>substituteAttributesInRoute: localized route template and attributes
substituteAttributesInRoute->>UrlRoutable: request route key
UrlRoutable-->>substituteAttributesInRoute: localized or standard key
substituteAttributesInRoute-->>LaravelLocalization: generated localized URL
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Registers view_with_slug and view_post_comment routes in setRoutes() and
adds HTTP dispatch tests to verify that Laravel correctly matches routes
using the {param:slug} syntax and extracts the parameter value from the URL:
- Single binding: {post:slug} parameter is correctly extracted
- Optional binding provided: {post:slug}/{comment:slug?} both extracted
- Optional binding omitted: route still matches with optional segment absent
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/LaravelLocalizationTest.php`:
- Around line 686-691: Update
testRouteWithOptionalColumnBindingMatchesWhenOmitted to verify the
view_post_comment route is matched, either by asserting the matched route name
or by changing the request URI to one that cannot match view_with_slug, while
preserving validation of the omitted optional comment segment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 412a8210-11d8-47d1-a3e8-55414f20c2ac
📒 Files selected for processing (5)
src/Mcamara/LaravelLocalization/LaravelLocalization.phptests/LaravelLocalizationTest.phptests/ModelWithCustomRouteKey.phptests/lang/en/routes.phptests/lang/es/routes.php
Comment on lines
+686
to
+691
| public function testRouteWithOptionalColumnBindingMatchesWhenOmitted(): void | ||
| { | ||
| $response = $this->get(self::TEST_URL.'posts/my-post'); | ||
| $response->assertStatus(200); | ||
| $this->assertEquals('my-post', $response->getContent()); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '(^|/)LaravelLocalizationTest\.php$|route|routes|webpack|migrations|phpunit|composer' || true
echo
echo "Relevant test section:"
if [ -f tests/LaravelLocalizationTest.php ]; then
wc -l tests/LaravelLocalizationTest.php
sed -n '50,100p' tests/LaravelLocalizationTest.php
sed -n '660,705p' tests/LaravelLocalizationTest.php
fi
echo
echo "Search route definitions in tests:"
rg -n "view_with_slug|view_post_comment|optional|comment|posts/my-post|Route::|fallback" tests -S || true
if command -v git >/dev/null 2>&1; then
echo
echo "Git diff summary (if available):"
git diff --stat
echo
echo "Git diff for file (if available):"
git diff -- tests/LaravelLocalizationTest.php | sed -n '1,240p'
fiRepository: mcamara/laravel-localization
Length of output: 9483
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Candidate source files:"
git ls-files 'src/*.php' | sed -n '1,120p'
echo
echo "Search translateRoutes/transRoute:"
rg -n "function (translateRoutes|transRoute)|translateRoutes\(|transRoute\(" src -S
echo
echo "Relevant route/translation method bodies:"
for f in $(rg -l "translateRoutes|transRoute" src); do
echo "--- $f"
ast-grep outline "$f" --match 'translateRoutes' --view expanded || true
ast-grep outline "$f" --match 'transRoute' --view expanded || true
done
echo
echo "Route collection refresh definition:"
rg -n "refresh_router_lookups|class LaravelLocalizationRouter|function getDefaultRouterLoader" tests src -SRepository: mcamara/laravel-localization
Length of output: 2442
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "LaravelLocalization.php sections:"
wc -l src/Mcamara/LaravelLocalization/LaravelLocalization.php
sed -n '1,120p' src/Mcamara/LaravelLocalization/LaravelLocalization.php
sed -n '640,735p' src/Mcamara/LaravelLocalization/LaravelLocalization.php
sed -n '735,820p' src/Mcamara/LaravelLocalization/LaravelLocalization.php
echo
echo "Tests/LaravelLocalizationTest.php header/setup:"
sed -n '1,100p' tests/LaravelLocalizationTest.php
echo
echo "Language stub route generation/transRoute implementation searches:"
rg -n "translateRoutes|transRoute|cachedRoutes|translate|Route::group|Locale::make|routes\[" src/Mcamara/LaravelLocalization/LaravelLocalization.php src -S || true
echo
echo "Composer deps:"
sed -n '1,120p' composer.jsonRepository: mcamara/laravel-localization
Length of output: 24487
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "setLocale definitions/usages:"
rg -n "function setLocale|public function setLocale|protected function setLocale|Locale:make|getNonLocalizedURL|getURLFromRouteNameTranslated|translations" src/Mcamara/LaravelLocalization/LaravelLocalization.php src/Mcamara/LaravelLocalization tests/tests src -S || true
echo
echo "Search route name assertions near optional binding tests:"
rg -n "assertStatus|assertRouteMatches|assertEquals\('my-post'|getContent\(\)|view_post_comment" tests/LaravelLocalizationTest.php -S || true
echo
echo "Framework doc evidence maybe via local docs if present:"
[ -d vendor ] && (find vendor/laravel/framework -maxdepth 4 -type f | rg 'Router|RouteCollection|Route' | head -20 || true) || trueRepository: mcamara/laravel-localization
Length of output: 8862
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant Laravel routing source sections:"
for f in \
vendor/laravel/framework/src/Illuminate/Routing/AbstractRouteCollection.php \
vendor/laravel/framework/src/Illuminate/Routing/RouteCollection.php \
vendor/laravel/framework/src/Illuminate/Routing/RouteUri.php \
vendor/laravel/framework/src/Illuminate/Routing/RouteParameterBinder.php \
vendor/laravel/framework/src/Illuminate/Routing/ImplicitRouteBinding.php \
vendor/laravel/framework/src/Illuminate/Routing/RouteDependencyResolverTrait.php
do
echo "--- $f"
rg -n "function find|findRoute|getRoute|parameter|binding|slug|dependencies|dependency|RouteParameterBinder|resolveDependencies|dependencies" "$f" || true
done
echo
echo "Extract route matching/resolution snippets:"
python3 - <<'PY'
from pathlib import Path
targets = {
"AbstractRouteCollection.php": [1, 180],
"RouteCollection.php": [1, 180],
"RouteUri.php": [1, 220],
"RouteParameterBinder.php": [1, 220],
"ImplicitRouteBinding.php": [1, 220],
"RouteDependencyResolverTrait.php": [1, 180],
}
for path, (start, end) in targets.items():
f = Path(f"vendor/laravel/framework/src/Illuminate/Routing/{path}")
lines = f.read_text(errors="replace").splitlines()
print(f"--- vendor/laravel/framework/src/Illuminate/Routing/{path}:{start}-{end}")
for i in range(start-1, min(end, len(lines))):
if any(tok in lines[i] for tok in ["function find", "function getRoute", "function resolveDependencies", "function getRouteKey", "function match", "parameter=", "dependency", "slug"]):
print(f"{i+1}: {lines[i]}")
PYRepository: mcamara/laravel-localization
Length of output: 6537
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "AbstractRouteCollection matching bodies:"
sed -n '60,140p' vendor/laravel/framework/src/Illuminate/Routing/AbstractRouteCollection.php
sed -n '180,230p' vendor/laravel/framework/src/Illuminate/Routing/AbstractRouteCollection.php
echo
echo "RouteCollection/CompiledRouteCollection dispatch body bodies:"
rg -n "function dispatch|function dispatchToRoute|protected function dispatch|public function dispatch" vendor/laravel/framework/src/Illuminate/Routing -S
case "$(rg -l "function dispatch\(" vendor/laravel/framework/src/Illuminate/Routing -S | head -1)" in
vendor/laravel/framework/src/Illuminate/Routing/Router.php) sed -n '200,280p' vendor/laravel/framework/src/Illuminate/Routing/Router.php ;;
esac
sed -n '190,230p' vendor/laravel/framework/src/Illuminate/Routing/CompiledRouteCollection.php
sed -n '180,230p' vendor/laravel/framework/src/Illuminate/Routing/RouteCollection.php
echo
echo "ImplicitRouteBinding and dependencies bodies:"
sed -n '1,110p' vendor/laravel/apimachinery/src/Illuminate/Routing/ ... 2>/dev/null || sed -n '1,130p' vendor/laravel/framework/src/Illuminate/Routing/ImplicitRouteBinding.php
sed -n '1,180p' vendor/laravel/framework/src/Illuminate/Routing/RouteDependencyResolverTrait.php
echo
echo "Standalone Laravel RouteParameterBinder dependency/parameter probe if testbench autoload is available:"
php -r '
if (!file_exists("vendor/laravel/framework/src/Illuminate/Routing/Route.php")) exit("NO ROUTE CLASS");
$suffix = basename(getcwd());
if ($suffix !== "vendor") exit("In sandbox, avoiding actual request; source read-only inspection was requested.");
' || trueRepository: mcamara/laravel-localization
Length of output: 15144
Make the omitted-optional binding test unambiguous.
/posts/my-post can match both view_with_slug and view_post_comment. This test currently asserts the response body from either matching route, so it does not prove the optional comment segment is handled by view_post_comment; assert the matched route name or use a URI that only targets the optional route.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/LaravelLocalizationTest.php` around lines 686 - 691, Update
testRouteWithOptionalColumnBindingMatchesWhenOmitted to verify the
view_post_comment route is matched, either by asserting the matched route name
or by changing the request URI to one that cannot match view_with_slug, while
preserving validation of the omitted optional comment segment.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Routes defined with Laravel's
{param:column}explicit binding syntax (e.g.{post:slug}) were not resolved correctly when generating localized URLs. The binding hint was treated as part of the parameter name, so lookups against the attributes array always failed and the placeholder was left unresolved in the URL.Solution
Refactor
substituteAttributesInRouteto use a singlepreg_replace_callbackthat strips the:columnhint before looking up the attribute value — mirroring what Laravel'sRouteUri::parse()does during route compilation.This also fixes a pre-existing bug in the optional-param cleanup regex (
[^)]→[^}]).What's changed
substituteAttributesInRoutenow handles{param:column}and{param:column?}placeholders correctlyUrlRoutablemodels, andLocalizedUrlRoutablemodels all work as beforeforceDefaultLocationcontinues to work with column-bound routesTests
Added
ModelWithCustomRouteKeyfixture and tests covering:{post:slug}binding with a data provider of 10 slug variants (hyphens, underscores, dots, numbers, unicode, accented characters){category:slug}/{post:slug}{post:slug}/{comment:slug?}forceDefaultLocation = trueSummary by CodeRabbit
New Features
Tests