Repository navigation
[#462] Normalize router helper surface through Request proxies - #466
Conversation
…xies Move router metadata access behind Request proxy methods, migrate core internal usages from router helpers to request()-> methods, and remove dead helper functions while keeping public helper wrappers for DX compatibility. Made-with: Cursor
📝 WalkthroughWalkthroughCentralize router metadata access behind Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #466 +/- ##
============================================
- Coverage 82.83% 82.44% -0.40%
- Complexity 2857 2894 +37
============================================
Files 250 250
Lines 7614 7798 +184
============================================
+ Hits 6307 6429 +122
- Misses 1307 1369 +62 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Module/ModuleManager.php (1)
200-211:⚠️ Potential issue | 🟠 MajorVerify
request()->getModuleBaseNamespace()behavior in CLI scaffolding context.
ModuleManager.replacePlaceholders()callsrequest()->getModuleBaseNamespace()to set the{{MODULE_NAMESPACE}}placeholder in generated module files. The method returns different namespaces based onenvironment()->isTesting(): it returns'Quantum\Tests\_root\modules'when testing, and'Modules'otherwise.This is problematic: the test suite itself (ModuleManagerTest) runs module scaffolding under
environment()->isTesting() === true, creating test fixtures with the test namespace. If module scaffolding is invoked viaphp qt module:generatewhile the test environment is active (whether intentionally or by mistake), generated production modules would be created with theQuantum\Tests\_root\modules\*namespace instead ofModules\*, making them unusable.Please confirm:
- (a) Is
php qt module:generateprotected from running whenenvironment()->isTesting()is true, or- (b) Should the testing branch in
getModuleBaseNamespace()not apply to scaffolding operations?If (b),
ModuleManagerneeds a different resolver than the one used byMiddlewareManagerandRouteBuilderfor runtime namespace resolution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Module/ModuleManager.php` around lines 200 - 211, ModuleManager::replacePlaceholders currently uses request()->getModuleBaseNamespace() (which branches on environment()->isTesting()) to populate {{MODULE_NAMESPACE}}, causing CLI scaffolding (module generation) to pick up the test namespace; change ModuleManager to not rely on request()->getModuleBaseNamespace() for scaffolding: detect CLI scaffolding context (PHP_SAPI === 'cli' or the module:generate command flag) and resolve the production module base namespace instead (e.g., use a dedicated resolver or config value such as a ModuleNamespaceResolver or config('modules.base_namespace') rather than request()), or add an explicit parameter to replacePlaceholders()/the generator call to pass the intended namespace; update references in replacePlaceholders and any callers so scaffolding always uses the production namespace while runtime classes (MiddlewareManager/RouteBuilder) can continue to use request()->getModuleBaseNamespace().
🧹 Nitpick comments (7)
src/Paginator/Traits/PaginatorTrait.php (1)
220-233: Pagination param stripping works, but is fragile to URL shape changes.The regex
/([?&](page|per_page)=\d+)/followed bypreg_replace('/&/', '?', ..., 1)handles common layouts, but:
- Non-numeric values (
?page=abc, empty, URL-encoded) won't be stripped and will leak into generated links.- Order-sensitive:
?page=1&foo=bar→ strips?page=1→ leaves&foo=bar→ the&→?fixup rescues it, but any stray leading&before other logic touches$routeUrlwould break.- The first
preg_replacecan returnnullon failure; it's guarded by?? ''on the next line, but$urlat line 230 is only checked with?? ''insidestrpos, not consistently elsewhere.Consider parsing with
parse_url+parse_str, unsettingpage/per_page, and rebuilding withhttp_build_query. That's both more robust and self-documenting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Paginator/Traits/PaginatorTrait.php` around lines 220 - 233, The getUri method currently uses fragile regexes to strip page/per_page; replace that logic by parsing the current request URI with parse_url and parse_str to extract query parameters, unset 'page' and 'per_page' (and handle non-numeric/encoded values), rebuild the query with http_build_query, reconstruct the route part (path + rebuilt query if non-empty), then prepend $this->baseUrl when $withBaseUrl is true and finally compute the delimiter ('?' vs '&') consistently before returning; update references in getUri to avoid preg_replace and null-return pitfalls from preg functions and ensure empty-query cases are handled uniformly.src/Renderer/Adapters/TwigAdapter.php (1)
79-79: NullgetCurrentModule()produces a malformed path.When no route is matched (CLI, early errors, direct
render()calls),request()->getCurrentModule()returnsnull, producing a path like.../modules//Views/....$this->fs->exists()will return false and the code falls through to the shared view path, so behavior is preserved — but the intent is unclear and identical to the oldcurrent_module()foot-gun.Consider early-returning the shared path when the module is null, or guarding the concat:
- $moduleViewPath = modules_dir() . DS . request()->getCurrentModule() . DS . 'Views' . DS . $view . '.php'; + $module = request()->getCurrentModule(); + $moduleViewPath = $module + ? modules_dir() . DS . $module . DS . 'Views' . DS . $view . '.php' + : null;Same comment applies to
src/Renderer/Adapters/HtmlAdapter.phpline 86.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Renderer/Adapters/TwigAdapter.php` at line 79, request()->getCurrentModule() can be null and causes a malformed $moduleViewPath; update TwigAdapter (and similarly HtmlAdapter) to guard that call by checking request()->getCurrentModule() before concatenating into $moduleViewPath (or early-return/resolve to the shared view path when it is null) so $this->fs->exists() is only called with a valid path; locate the $moduleViewPath construction in TwigAdapter::... (and HtmlAdapter::...) and either skip building the module-specific path when getCurrentModule() === null or replace it with the shared view path.src/Http/Traits/Request/Route.php (3)
159-164:getModuleBaseNamespace()doesn't belong on the Request/Route trait and hardcodes the namespace.This method never touches request/route state — it only branches on
environment()->isTesting(). Putting it on theRequest\Routetrait couples unrelated concerns, and hardcoding'Modules'removes the configurability the priormodule_base_namespace()helper likely provided (e.g., via config). If a consumer ever customizes the root namespace of modules, this proxy silently ignores that.Consider (a) moving this to a dedicated module/namespace resolver service and (b) sourcing the default from config rather than a literal, e.g.
config()->get('modules.base_namespace', 'Modules').🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Http/Traits/Request/Route.php` around lines 159 - 164, getModuleBaseNamespace() is misplaced on the Request/Route trait and hardcodes the namespace; remove it from the Request\Route trait and implement a dedicated module namespace resolver (e.g., ModuleNamespaceResolver or ModuleNamespaceService) that encapsulates the environment check (environment()->isTesting()) and reads the default from config()->get('modules.base_namespace', 'Modules') instead of the literal 'Modules'; update any callers of getModuleBaseNamespace() to use the new resolver (or a module_base_namespace() helper that proxies to it) so customization via config is respected.
138-157:routeGroupExistsperforms a full scan on every call.For apps with large route tables this is O(n) per call and is invoked from hot paths like
OpenApiCommandand previouslyroute_group_exists(). Consider caching agroup => trueset on first access (invalidated whenRouteCollectionchanges), or exposing ahasGroup()helper onRouteCollectionitself that maintains an index atadd()time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Http/Traits/Request/Route.php` around lines 138 - 157, routeGroupExists currently scans the entire RouteCollection each call causing O(n) hot-path overhead; instead add a hasGroup(string $name, string $module): bool method to RouteCollection that maintains an index (e.g., map[module][group] = true) updated when routes are added/removed/loaded (update in RouteCollection::add(), remove(), and any bulk load/replace methods) and then change Request::routeGroupExists to delegate to RouteCollection::hasGroup (or use the new index cache) so lookups become O(1); also ensure the index is rebuilt or invalidated when the collection is replaced/cleared and adjust callers like OpenApiCommand/route_group_exists() to use the new helper if they call the collection directly.
117-157:findRouteByName()androuteGroupExists()duplicate the same lookup loop.Both methods differ only in which Route field they compare (
getName()vsgetGroup()) and what they return. Extracting a private iterator keeps the DI/collection guard and the case-insensitive module match in one place.Sketch
private function findRouteBy(callable $matcher): ?RouterRoute { if (!Di::isRegistered(RouteCollection::class)) { return null; } foreach (Di::get(RouteCollection::class)->all() as $route) { if ($matcher($route)) { return $route; } } return null; }Then
findRouteByName/routeGroupExistsbecome thin predicates overfindRouteBy(...).Also worth noting: these two methods reach into a DI-registered
RouteCollectionfrom theRequestobject, which blurs the HTTP request abstraction with the router registry. A dedicatedRouteRegistryservice injected where needed would be a cleaner long-term home than piggy-backing onRequest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Http/Traits/Request/Route.php` around lines 117 - 157, Extract the duplicated loop into a private helper like private function findRouteBy(callable $matcher): ?RouterRoute that performs the DI guard (Di::isRegistered(RouteCollection::class)), retrieves the collection via Di::get(RouteCollection::class)->all(), iterates routes and returns the first RouterRoute where $matcher($route) is true (keep case-insensitive module comparison by comparing (string)$route->getModule()), then update findRouteByName(string $name, string $module) to call findRouteBy(fn($route) => $route->getName() !== null && strcasecmp($route->getName(), $name) === 0 && strcasecmp((string)$route->getModule(), $module) === 0) and update routeGroupExists(string $name, string $module) to return (findRouteBy(fn($route) => $route->getGroup() !== null && strcasecmp($route->getGroup(), $name) === 0 && strcasecmp((string)$route->getModule(), $module) === 0) !== null); ensure unique symbols referenced: findRouteByName, routeGroupExists, findRouteBy, RouteCollection, Di, RouterRoute, getName, getGroup, getModule.src/Router/Helpers/router.php (1)
35-42:current_module()guard is inconsistent with the other route helpers.
current_module()still special-casesDi::has(Request::class)and returnsnull, but every other helper here (current_controller(),current_action(),route_callback(),current_route(),route_pattern(),route_params(),route_param(),route_cache_settings(),route_name(),route_prefix(), etc.) unconditionally callsrequest()->.... Either all helpers should short-circuit whenRequestis not registered, or none should — the asymmetry is easy to misread as an intentional nullability contract for module only.Since
request()itself will register/resolveRequestin DI in normal flows, the simplest direction is to drop the guard and letcurrent_module()delegate directly like the others; otherwise, extract a small helper and apply it consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Router/Helpers/router.php` around lines 35 - 42, The current_module() helper is inconsistently guarding DI with Di::has(Request::class) while all other helpers call request() unconditionally; remove that special-case by deleting the Di::has(...) check and have current_module() simply delegate to request()->getCurrentModule() like the other helpers (or alternatively implement a small shared helper used by all route helpers), ensuring the function signature stays function current_module(): ?string and the unique symbol current_module() is the only location changed.src/Console/Commands/OpenApiCommand.php (1)
127-138: Cache therouteGroupExistsresult to avoid redundant lookups.
request()->routeGroupExists('openapi', $module)is now invoked twice with identical arguments. Each call iterates the fullRouteCollection. Storing the result once keeps intent clearer and avoids the repeated scan.♻️ Proposed refactor
- if (request()->routeGroupExists('openapi', $module) && $this->fs->exists($modulePath . DS . 'resources' . DS . 'openApi' . DS . 'spec.json')) { + $openapiGroupExists = request()->routeGroupExists('openapi', $module); + + if ($openapiGroupExists && $this->fs->exists($modulePath . DS . 'resources' . DS . 'openApi' . DS . 'spec.json')) { $this->error('The Open API specifications already installed for `' . ucfirst($module) . '` module'); return; } - if (!request()->routeGroupExists('openapi', $module)) { + if (!$openapiGroupExists) { $routeContent = $this->fs->get($routes);Also note the path casing inconsistency: line 127 checks
resources/openApi/spec.jsonwhile line 140-141 createsresources/openapi(lowercase). This predates the PR but is worth a follow-up since case-sensitive filesystems will treat them as distinct directories.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Console/Commands/OpenApiCommand.php` around lines 127 - 138, Cache the result of request()->routeGroupExists('openapi', $module) into a local variable (e.g. $hasOpenapiGroup) and use that variable in both checks instead of calling request()->routeGroupExists twice to avoid scanning the RouteCollection twice; update the conditional that currently checks $modulePath . DS . 'resources' . DS . 'openApi' . DS . 'spec.json' to use a consistent lowercase 'openapi' directory name to match where resources are created, and then use $hasOpenapiGroup when deciding to early-return and when deciding whether to insert the openapi routes with openapiRoutes($module) into $routes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Http/Traits/Request/Route.php`:
- Around line 76-79: Change getCompiledRoutePattern() to match the other
accessors by returning null when there is no current route or when the route's
compiled pattern is missing; update the method signature from
getCompiledRoutePattern(): string to allow null (e.g., ?string) and return null
instead of '' in both the "no route" branch and when getCompiledPattern() yields
null/empty, so callers can distinguish "no route matched" from "route has no
compiled pattern"; modify the implementation around
$this->route->getRoute()->getCompiledPattern() accordingly and keep behavior
consistent with getCurrentRoutePattern(), getRouteName(), and getRoutePrefix().
In `@src/Router/RouteBuilder.php`:
- Line 317: Update the docblock for the build() method to reflect that it can
throw the same exceptions as addRoute() and the public verb helpers: add
DiException and ReflectionException (or their union type) alongside
RouteException, since build() invokes the route DSL closures and can propagate
request/DI reflection errors encountered during route registration; reference
the build() method and the related addRoute()/verb helper behavior when making
the docblock change so callers see the accurate throws contract.
In `@tests/Unit/Http/RequestTest.php`:
- Around line 95-112: The test is reusing a DI-registered RouteCollection across
runs which causes test pollution; fix it by always creating a fresh
RouteCollection and overwriting the DI entry instead of conditionally setting
it: remove the Di::isRegistered(RouteCollection::class) conditional and
unconditionally create $collection = new RouteCollection();
$collection->add($route); then call Di::set(RouteCollection::class, $collection)
so request()->findRouteByName(...) and request()->routeGroupExists(...) always
read from a fresh collection (refer to RouteCollection, Di::set,
request()->findRouteByName, request()->routeGroupExists).
---
Outside diff comments:
In `@src/Module/ModuleManager.php`:
- Around line 200-211: ModuleManager::replacePlaceholders currently uses
request()->getModuleBaseNamespace() (which branches on
environment()->isTesting()) to populate {{MODULE_NAMESPACE}}, causing CLI
scaffolding (module generation) to pick up the test namespace; change
ModuleManager to not rely on request()->getModuleBaseNamespace() for
scaffolding: detect CLI scaffolding context (PHP_SAPI === 'cli' or the
module:generate command flag) and resolve the production module base namespace
instead (e.g., use a dedicated resolver or config value such as a
ModuleNamespaceResolver or config('modules.base_namespace') rather than
request()), or add an explicit parameter to replacePlaceholders()/the generator
call to pass the intended namespace; update references in replacePlaceholders
and any callers so scaffolding always uses the production namespace while
runtime classes (MiddlewareManager/RouteBuilder) can continue to use
request()->getModuleBaseNamespace().
---
Nitpick comments:
In `@src/Console/Commands/OpenApiCommand.php`:
- Around line 127-138: Cache the result of
request()->routeGroupExists('openapi', $module) into a local variable (e.g.
$hasOpenapiGroup) and use that variable in both checks instead of calling
request()->routeGroupExists twice to avoid scanning the RouteCollection twice;
update the conditional that currently checks $modulePath . DS . 'resources' . DS
. 'openApi' . DS . 'spec.json' to use a consistent lowercase 'openapi' directory
name to match where resources are created, and then use $hasOpenapiGroup when
deciding to early-return and when deciding whether to insert the openapi routes
with openapiRoutes($module) into $routes.
In `@src/Http/Traits/Request/Route.php`:
- Around line 159-164: getModuleBaseNamespace() is misplaced on the
Request/Route trait and hardcodes the namespace; remove it from the
Request\Route trait and implement a dedicated module namespace resolver (e.g.,
ModuleNamespaceResolver or ModuleNamespaceService) that encapsulates the
environment check (environment()->isTesting()) and reads the default from
config()->get('modules.base_namespace', 'Modules') instead of the literal
'Modules'; update any callers of getModuleBaseNamespace() to use the new
resolver (or a module_base_namespace() helper that proxies to it) so
customization via config is respected.
- Around line 138-157: routeGroupExists currently scans the entire
RouteCollection each call causing O(n) hot-path overhead; instead add a
hasGroup(string $name, string $module): bool method to RouteCollection that
maintains an index (e.g., map[module][group] = true) updated when routes are
added/removed/loaded (update in RouteCollection::add(), remove(), and any bulk
load/replace methods) and then change Request::routeGroupExists to delegate to
RouteCollection::hasGroup (or use the new index cache) so lookups become O(1);
also ensure the index is rebuilt or invalidated when the collection is
replaced/cleared and adjust callers like OpenApiCommand/route_group_exists() to
use the new helper if they call the collection directly.
- Around line 117-157: Extract the duplicated loop into a private helper like
private function findRouteBy(callable $matcher): ?RouterRoute that performs the
DI guard (Di::isRegistered(RouteCollection::class)), retrieves the collection
via Di::get(RouteCollection::class)->all(), iterates routes and returns the
first RouterRoute where $matcher($route) is true (keep case-insensitive module
comparison by comparing (string)$route->getModule()), then update
findRouteByName(string $name, string $module) to call findRouteBy(fn($route) =>
$route->getName() !== null && strcasecmp($route->getName(), $name) === 0 &&
strcasecmp((string)$route->getModule(), $module) === 0) and update
routeGroupExists(string $name, string $module) to return (findRouteBy(fn($route)
=> $route->getGroup() !== null && strcasecmp($route->getGroup(), $name) === 0 &&
strcasecmp((string)$route->getModule(), $module) === 0) !== null); ensure unique
symbols referenced: findRouteByName, routeGroupExists, findRouteBy,
RouteCollection, Di, RouterRoute, getName, getGroup, getModule.
In `@src/Paginator/Traits/PaginatorTrait.php`:
- Around line 220-233: The getUri method currently uses fragile regexes to strip
page/per_page; replace that logic by parsing the current request URI with
parse_url and parse_str to extract query parameters, unset 'page' and 'per_page'
(and handle non-numeric/encoded values), rebuild the query with
http_build_query, reconstruct the route part (path + rebuilt query if
non-empty), then prepend $this->baseUrl when $withBaseUrl is true and finally
compute the delimiter ('?' vs '&') consistently before returning; update
references in getUri to avoid preg_replace and null-return pitfalls from preg
functions and ensure empty-query cases are handled uniformly.
In `@src/Renderer/Adapters/TwigAdapter.php`:
- Line 79: request()->getCurrentModule() can be null and causes a malformed
$moduleViewPath; update TwigAdapter (and similarly HtmlAdapter) to guard that
call by checking request()->getCurrentModule() before concatenating into
$moduleViewPath (or early-return/resolve to the shared view path when it is
null) so $this->fs->exists() is only called with a valid path; locate the
$moduleViewPath construction in TwigAdapter::... (and HtmlAdapter::...) and
either skip building the module-specific path when getCurrentModule() === null
or replace it with the shared view path.
In `@src/Router/Helpers/router.php`:
- Around line 35-42: The current_module() helper is inconsistently guarding DI
with Di::has(Request::class) while all other helpers call request()
unconditionally; remove that special-case by deleting the Di::has(...) check and
have current_module() simply delegate to request()->getCurrentModule() like the
other helpers (or alternatively implement a small shared helper used by all
route helpers), ensuring the function signature stays function current_module():
?string and the unique symbol current_module() is the only location changed.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec92b5ae-2794-4103-888a-d32245916560
📒 Files selected for processing (18)
src/App/Adapters/WebAppAdapter.phpsrc/App/Helpers/misc.phpsrc/Console/Commands/OpenApiCommand.phpsrc/Http/Request.phpsrc/Http/Traits/Request/Route.phpsrc/Lang/Factories/LangFactory.phpsrc/Lang/Translator.phpsrc/Loader/Setup.phpsrc/Middleware/MiddlewareManager.phpsrc/Module/ModuleManager.phpsrc/Paginator/Traits/PaginatorTrait.phpsrc/Renderer/Adapters/HtmlAdapter.phpsrc/Renderer/Adapters/TwigAdapter.phpsrc/ResourceCache/ViewCache.phpsrc/Router/Helpers/router.phpsrc/Router/RouteBuilder.phpsrc/View/View.phptests/Unit/Http/RequestTest.php
💤 Files with no reviewable changes (1)
- src/App/Helpers/misc.php
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d391843b5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Router/Helpers/router.php (1)
34-41: InconsistentDi::has(Request::class)guard incurrent_module()only.Every other helper in this file (
current_middlewares(),current_controller(),current_action(),route_callback(),current_route(),route_pattern(),route_params(),route_param(),route_cache_settings(),route_name(),route_prefix(),find_route_by_name(),route_group_exists(),module_base_namespace()) unconditionally callsrequest()->...with no guard. Meanwhile,current_module()alone checksDi::has(Request::class)before delegating.The
request()helper (insrc/Http/Helpers/http.php) auto-registersRequeston first call if not already registered, so all siblings will safely trigger registration if needed. This guard incurrent_module()prevents that auto-registration and returns null instead. Two paths forward:
- Add the same guard to all other helpers for uniform defensive behavior, or
- Remove the guard from
current_module()to align with siblings' unconditional delegation pattern.Pick one to ensure the helper surface behaves uniformly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Router/Helpers/router.php` around lines 34 - 41, Remove the special-case Di::has(Request::class) guard from the current_module() helper so it unconditionally calls request()->getCurrentModule() like the other helpers; update the current_module() function to delegate directly to request()->getCurrentModule() (relying on the request() helper's auto-registration behavior) to achieve consistent behavior across helpers such as current_middlewares(), current_controller(), current_action(), route_callback(), current_route(), route_pattern(), route_params(), route_param(), route_cache_settings(), route_name(), route_prefix(), find_route_by_name(), route_group_exists(), and module_base_namespace().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Router/Helpers/router.php`:
- Around line 34-41: Remove the special-case Di::has(Request::class) guard from
the current_module() helper so it unconditionally calls
request()->getCurrentModule() like the other helpers; update the
current_module() function to delegate directly to request()->getCurrentModule()
(relying on the request() helper's auto-registration behavior) to achieve
consistent behavior across helpers such as current_middlewares(),
current_controller(), current_action(), route_callback(), current_route(),
route_pattern(), route_params(), route_param(), route_cache_settings(),
route_name(), route_prefix(), find_route_by_name(), route_group_exists(), and
module_base_namespace().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62f4ab89-1eb7-4a13-a7ee-5a7e23e1c5c5
📒 Files selected for processing (4)
src/Http/Traits/Request/Route.phpsrc/Router/Helpers/router.phptests/Unit/Http/RequestTest.phptests/Unit/Router/Helpers/RouteHelpersTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/Unit/Http/RequestTest.php
- src/Http/Traits/Request/Route.php
Closes #462
Move router metadata access behind Request proxy methods, migrate core internal usages from router helpers to request()-> methods, and remove dead helper functions while keeping public helper wrappers for DX compatibility.
Summary by CodeRabbit