Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #225 +/- ##
============================================
Coverage 100.00% 100.00%
- Complexity 129 192 +63
============================================
Files 13 15 +2
Lines 400 524 +124
============================================
+ Hits 400 524 +124 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
PR SummaryThis pull request introduces numerous updates aimed at better organizing and streamlining how routes are handled in the system:
These changes all contribute to a more robust and efficient route handling system within the software, aligning the codebase with modern best practices in PHP coding. |
We can mention it in docs or drop the current syntax.
Yes, doesn't fix directly. Since there is no exception, there is no need for it. |
But what goal of syntax change? |
vjik
left a comment
vjik
left a comment
There was a problem hiding this comment.
Can we make Route and Group constructors public and keep BC?
I think, GroupBuilder and RouteBuilder are not needed.
Did you mean builders? Route and Group constructors are public in this PR.
Will we abandon the current syntax? |
No, I mean
Mark as deprecated, then remove in major version. |
Let's abandon right away since the PR has major changes. |
…ray_is_list, and apply readonly properties
What major changes are needed? |
# Conflicts: # composer.json # src/Debug/DebugRoutesCommand.php # src/Route.php
# Conflicts: # src/Group.php # src/Route.php # src/RouteCollection.php # src/RouteCollector.php # tests/ConfigTest.php # tests/Debug/DebugRoutesCommandTest.php # tests/Debug/RouterCollectorTest.php # tests/Debug/UrlMatcherInterfaceProxyTest.php # tests/GroupTest.php # tests/RouteTest.php
…ove method consistency
There was a problem hiding this comment.
Pull request overview
This PR introduces a new routing syntax by adding immutable builder classes (RouteBuilder, GroupBuilder) while refactoring core Route/Group to mutable objects with constructors + getters/setters. It also updates routing collection/dispatch/debug code and rewrites tests to match the new APIs (breaking BC as noted).
Changes:
- Add
RoutableInterfaceplus newBuilder\RouteBuilderandBuilder\GroupBuilderthat convert to coreRoute/GroupviatoRoute(). - Refactor
Route,Group,RouteCollector, andRouteCollectionAPIs (getters/setters; middleware collection rename; host→hosts changes). - Update tests and debug tooling to use builders/getters, and bump dev dependencies.
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/RouteTest.php | Reworked tests for new Route constructor + setters/getters and validation. |
| tests/RouteCollectorTest.php | Updated to builder aliases and getMiddlewares() API. |
| tests/RouteCollectionTest.php | Updated for new route/group APIs and enabled middleware retrieval via getters. |
| tests/Middleware/RouterTest.php | Adjusted matcher to return built routes via toRoute(). |
| tests/MatchingResultTest.php | Updated to instantiate Route directly with methods/pattern. |
| tests/HydratorAttribute/RouteArgumentTest.php | Updated to convert builder route to core route with toRoute(). |
| tests/GroupTest.php | Reworked tests for new Group constructor + setters/getters and validation. |
| tests/Debug/UrlMatcherInterfaceProxyTest.php | Updated to instantiate Route with methods/pattern. |
| tests/Debug/RouterCollectorTest.php | Updated expectations for collected keys (hosts) and new route creation patterns. |
| tests/Debug/DebugRoutesCommandTest.php | Updated to use RouteBuilder in fixtures. |
| tests/CurrentRouteTest.php | Updated CurrentRoute API host→hosts and new Route constructor usage. |
| tests/ConfigTest.php | Updated config test to use RouteBuilder and toRoute(). |
| tests/Builder/RouteBuilderTest.php | New test coverage for builder-based fluent syntax and immutability expectations. |
| tests/Builder/GroupBuilderTest.php | New test coverage for group builder syntax, CORS behavior, and immutability expectations. |
| src/RouteCollectorInterface.php | Accept RoutableInterface and rename middleware getter to getMiddlewares(). |
| src/RouteCollector.php | Implement RoutableInterface support and rename internal middleware storage/getter. |
| src/RouteCollection.php | Convert RoutableInterface via toRoute(), switch to setter-based middleware/host/pattern/name updates. |
| src/Route.php | Make Route mutable with constructor + getters/setters; action separated from middleware list; add validations/caching. |
| src/RoutableInterface.php | New interface to unify builder→route/group conversion. |
| src/Middleware/Router.php | Dispatch using Route::getEnabledMiddlewares() instead of getData(). |
| src/Group.php | Make Group mutable with constructor + getters/setters; add validation and enabled middleware caching. |
| src/Debug/RouterCollector.php | Switch debug output to new getters; host→hosts key changes; middleware/action extraction updated. |
| src/Debug/DebugRoutesCommand.php | Replace custom array list detection with array_is_list(). |
| src/CurrentRoute.php | Update to use route getters and host→hosts API. |
| src/Builder/RouteBuilder.php | New immutable fluent builder for routes, converting to core Route. |
| src/Builder/GroupBuilder.php | New immutable fluent builder for groups, converting to core Group. |
| composer.json | Bump dev dependencies (PHPUnit, Rector, Psalm, yiisoft packages). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public function setDefaults(array $defaults): self | ||
| { | ||
| $route = clone $this; | ||
| if ($this->actionAdded) { | ||
| /** | ||
| * @psalm-suppress PropertyTypeCoercion Keys in the replacement array are not preserved. | ||
| * @infection-ignore-all | ||
| */ | ||
| array_splice( | ||
| $route->middlewares, | ||
| offset: count($route->middlewares) - 1, | ||
| length: 0, | ||
| replacement: $definition, | ||
| ); | ||
| } else { | ||
| array_push( | ||
| $route->middlewares, | ||
| ...array_values($definition), | ||
| ); | ||
| /** @var mixed $value */ | ||
| foreach ($defaults as $key => $value) { | ||
| if (!is_scalar($value) && !($value instanceof Stringable) && null !== $value) { | ||
| throw new InvalidArgumentException( | ||
| 'Invalid $defaults provided, indexed array of scalar or `Stringable` or null expected.', | ||
| ); | ||
| } | ||
| $this->defaults[$key] = (string) $value; | ||
| } | ||
|
|
||
| $route->enabledMiddlewaresCache = null; | ||
|
|
||
| return $route; | ||
| return $this; | ||
| } |
There was a problem hiding this comment.
setDefaults() merges into $this->defaults but never clears existing defaults. If callers expect a typical setter semantics (replace), old defaults will persist when passing a smaller/empty defaults array. Consider either resetting $this->defaults first, or renaming the API to indicate merging behavior.
| public function setHosts(array $hosts): self | ||
| { | ||
| foreach ($hosts as $host) { | ||
| if (!is_string($host)) { | ||
| throw new InvalidArgumentException('Invalid $hosts provided, list of string expected.'); | ||
| } | ||
| $host = rtrim($host, '/'); | ||
|
|
||
| if ($host !== '' && !in_array($host, $new->hosts, true)) { | ||
| $new->hosts[] = $host; | ||
| if ($host !== '' && !in_array($host, $this->hosts, true)) { | ||
| $this->hosts[] = $host; | ||
| } | ||
| } | ||
|
|
||
| return $new; | ||
| return $this; | ||
| } |
There was a problem hiding this comment.
setHosts() appends into $this->hosts without resetting it first. That means calling setHosts() multiple times (or calling it with an empty array) won't replace existing hosts, which is inconsistent with Route::setHosts() and with typical setter semantics. Consider resetting $this->hosts = [] at the start and then applying the same trim/unique logic.
| $action = array_pop($middlewares); | ||
|
|
||
| return [$middlewares, $action]; | ||
| return [$route->getMiddlewares(), $route->getAction()]; |
There was a problem hiding this comment.
getMiddlewaresAndAction() now returns getMiddlewares() + getAction(), which ignores disabled middleware filtering and diverges from the previous behavior that reported the effective middleware stack for the matched route. For debugging output it’s usually more accurate to use getEnabledMiddlewares() (then separate the action if non-null) so the panel reflects what will actually execute.
| return [$route->getMiddlewares(), $route->getAction()]; | |
| $middlewares = $route->getEnabledMiddlewares(); | |
| $action = $route->getAction(); | |
| return [$middlewares, $action]; |
| public function getName(): string | ||
| { | ||
| return self::methods([Method::OPTIONS], $pattern); | ||
| return $this->name ??= (implode(', ', $this->methods) . ' ' . implode('|', $this->hosts) . $this->pattern); |
There was a problem hiding this comment.
getName() caches the autogenerated name via ??=. With the new mutable setters (setPattern(), setHosts(), setMethods()), this can leave the route name stale if getName() was called before a subsequent mutation (the cached computed name will no longer reflect the current methods/hosts/pattern). Consider either (1) not persisting the computed name (return $this->name ?? <computed> without assignment), or (2) tracking whether the name was explicitly set and clearing/recomputing it when relevant setters are called.
| return $this->name ??= (implode(', ', $this->methods) . ' ' . implode('|', $this->hosts) . $this->pattern); | |
| return $this->name ?? (implode(', ', $this->methods) . ' ' . implode('|', $this->hosts) . $this->pattern); |
| $route = clone $this; | ||
| $route->override = true; | ||
| return $route; | ||
| $this->action = $action; |
There was a problem hiding this comment.
setAction() does not invalidate $enabledMiddlewaresCache. If getEnabledMiddlewares() is called before setAction(), subsequent calls will keep returning the cached list without the action appended. Clearing $enabledMiddlewaresCache (and possibly any derived name cache if applicable) in setAction() would prevent stale middleware stacks.
| $this->action = $action; | |
| $this->action = $action; | |
| $this->enabledMiddlewaresCache = null; |
| public function setMethods(array $methods): self | ||
| { | ||
| return $this->hosts($host); | ||
| if (empty($methods)) { | ||
| throw new InvalidArgumentException('$methods cannot be empty.'); | ||
| } | ||
| foreach ($methods as $method) { | ||
| if (!is_string($method)) { | ||
| throw new InvalidArgumentException('Invalid $methods provided, list of string expected.'); | ||
| } | ||
| $this->methods[] = $method; | ||
| } | ||
| return $this; | ||
| } |
There was a problem hiding this comment.
setMethods() appends to $this->methods but never resets it. Calling setMethods() more than once will accumulate/duplicate methods instead of replacing them, which is surprising for a setter and can also break the default name generation. Consider resetting $this->methods at the start and (if you keep name caching) clearing any cached autogenerated name.
Current syntax