Order command routes most-specific-first so literal paths win over placeholders#31
Conversation
…aceholders
The command-route directory loader registered routes in filename-sort order,
and CommandRouteClassLoader builds every route with empty requirements, so a
placeholder segment such as {id} compiles to [^/]++ and also matches a sibling
literal segment (for example a /resource/{id} route swallowing /resource/current).
Resolution then depends on registration order rather than path specificity.
RouteSpecificitySorter reorders the scanned command routes most-specific-first
(fewer placeholders, then longer static prefix, stable otherwise) and is applied
in AttributeDirectoryLoaderDecorator before the routes are merged, so concrete
paths are matched before templated ones per the OpenAPI path-precedence rule.
Per-class $routes->import(...) ordering is unchanged.
|
Warning Review limit reached
More reviews will be available in 50 minutes and 41 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds ChangesRoute Specificity Sorting
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Routing/RouteSpecificitySorter.php (1)
55-58: Consider preserving route priorities when rebuilding the sorted collection.While the sorter rebuilds a
RouteCollectionwithout carrying forward route priorities viaadd(), this is currently not a practical concern since no routes in the bundle have explicit priorities set. However, for defensive consistency with the Symfony RouteCollection API contract,getPriority()and the three-parameteradd($name, $route, $priority)form could be used to ensure priorities are preserved if routes with non-default priorities are encountered in the future.Suggested patch
public function sort(RouteCollection $routes): RouteCollection { $all = $routes->all(); $positions = []; + $priorities = []; $position = 0; - foreach ($all as $name => $route) { + foreach ($all as $name => $route) { $positions[$name] = $position; + $priorities[$name] = $routes->getPriority($name); ++$position; } @@ $sorted = new RouteCollection(); foreach ($names as $name) { - $sorted->add($name, $all[$name]); + $sorted->add($name, $all[$name], $priorities[$name]); } return $sorted; }🤖 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 `@src/Routing/RouteSpecificitySorter.php` around lines 55 - 58, The RouteSpecificitySorter rebuilds a RouteCollection using the two-parameter add() method, which does not preserve route priorities from the original routes. To ensure defensive consistency with the Symfony RouteCollection API contract and future-proof the code for routes with non-default priorities, modify the loop that adds routes to the sorted collection to also retrieve and pass the route priority. Use the getPriority() method on each route from the all array and provide it as the third parameter to the add() method when adding routes to the sorted collection.
🤖 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.
Nitpick comments:
In `@src/Routing/RouteSpecificitySorter.php`:
- Around line 55-58: The RouteSpecificitySorter rebuilds a RouteCollection using
the two-parameter add() method, which does not preserve route priorities from
the original routes. To ensure defensive consistency with the Symfony
RouteCollection API contract and future-proof the code for routes with
non-default priorities, modify the loop that adds routes to the sorted
collection to also retrieve and pass the route priority. Use the getPriority()
method on each route from the all array and provide it as the third parameter to
the add() method when adding routes to the sorted collection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d727ec78-d636-48a0-960e-34d228b6894c
⛔ Files ignored due to path filters (2)
tests/Mock/Routing/src/CollectionItemCommand.phpis excluded by!tests/Mock/**tests/Mock/Routing/src/CollectionLiteralCommand.phpis excluded by!tests/Mock/**
📒 Files selected for processing (4)
src/Routing/Loader/AttributeDirectoryLoaderDecorator.phpsrc/Routing/RouteSpecificitySorter.phptests/Unit/Routing/Loader/AttributeDirectoryLoaderDecoratorTest.phptests/Unit/Routing/RouteSpecificitySorterTest.php
RouteCollection::add() carries a route's priority as its third argument; the specificity rebuild now forwards RouteCollection::getPriority() so an explicit non-default priority survives the sort. Behaviour is unchanged for the default priority 0 (not stored, so the collection stays in specificity order); when a priority is set it dominates, with specificity as the stable tiebreak.
1 participant
Summary
The command-route directory loader registers routes in filename-sort order, and
CommandRouteClassLoaderbuilds every route withrequirements: []. A placeholder segment such as{id}therefore compiles to[^/]++and also matches a sibling literal segment — e.g. a/resource/{id}route can swallow/resource/current— with the winner decided purely by registration order rather than path specificity.RouteSpecificitySorterreorders the scanned command routes most-specific-first (fewer placeholders, then longer static prefix, stable otherwise) and is applied inAttributeDirectoryLoaderDecoratorbefore the routes are merged. Concrete paths are now matched before templated ones — the OpenAPI path-precedence rule — regardless of class/file name.Changes
src/Routing/RouteSpecificitySorter.php— new, pure sorter over aRouteCollection.src/Routing/Loader/AttributeDirectoryLoaderDecorator.php— apply the sorter to the scanned command routes (one line).tests/Unit/Routing/RouteSpecificitySorterTest.php— ordering rules (placeholder count, static-prefix length, stability).tests/Unit/Routing/Loader/AttributeDirectoryLoaderDecoratorTest.php— integration test asserting the decorator emits the literal route before the colliding placeholder. The fixture filenames deliberately scan placeholder-first, so this test fails without the sort.tests/Mock/Routing/src/CollectionItemCommand.php,CollectionLiteralCommand.php— colliding fixtures (/api/items/{id}vs/api/items/featured).Verification
composer cs-fixer— clean.composer phpstan(level max) — no errors.composer test— 201 tests, 596 assertions, green. Confirmed the new integration test fails without the fix.Notes
$routes->import(Command::class, 'attribute')ordering is unchanged: the precedence sort applies to the directory scan the bundle controls; explicit per-class import order remains the application's responsibility.Summary by CodeRabbit