Conversation
📝 WalkthroughWalkthroughAdds a dedicated routing configuration loader that reads rewrites and controller_paths from routing.yaml (falling back to neuron.yaml). Application route APIs accept an optional route name and attribute-based route loading uses the configured controller paths via the Registry. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Config as Config Loader
participant Router as Router
participant Registry as Registry
participant AttrScanner as Attribute Scanner
App->>Config: loadRoutingConfig()
Config->>Config: Check for routing.yaml
alt routing.yaml exists
Config->>Config: Parse YAML (rewrites, controller_paths)
Config->>Router: Register rewrites
Config->>Registry: Store controller_paths
else
Config->>Config: Log fallback to neuron.yaml
Config->>Registry: Load controller_paths from neuron.yaml
end
App->>AttrScanner: loadAttributeRoutes()
AttrScanner->>Registry: Get controller_paths
AttrScanner->>AttrScanner: Scan controller paths and extract attributes
AttrScanner->>App: addRoute(method, route, controllerMethod, request, filters, name)
App->>Router: Register route (with optional name)
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Mvc/Application.php (2)
135-146: DocBlock is missing the@paramfor the new$nameparameter.The method signature now includes
?string $name = null, but the PHPDoc block doesn't document this parameter. Add the missing parameter documentation for completeness.📝 Suggested documentation update
/** * @param string $method * @param string $route * @param string $controllerMethod * @param string $request * @param string|array $filters + * @param string|null $name Optional route name for named routes * @return \Neuron\Routing\RouteMap * * @throws BadRequestMethod * @throws Exception */
496-518: Consider adding a type check for the parsed YAML result.
Yaml::parse()can returnnullfor empty files or a scalar value for non-mapping YAML content. Whileisset()handlesnullgracefully, adding an explicit array type check improves defensive coding.🛡️ Suggested defensive check
try { $config = \Symfony\Component\Yaml\Yaml::parse( $content ); + + if( !is_array( $config ) ) + { + Log::warning( "routing.yaml does not contain a valid configuration array" ); + return; + } // Configure URL rewrites if( isset( $config['rewrites'] ) && is_array( $config['rewrites'] ) )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Mvc/Application.php
🧰 Additional context used
🧬 Code graph analysis (1)
src/Mvc/Application.php (1)
tests/Mvc/MockRouter.php (1)
get(21-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (4)
src/Mvc/Application.php (4)
150-206: LGTM!The
$nameparameter is consistently passed to all router methods (PUT, GET, POST, DELETE), and the switch statement properly handles all request method types withUNKNOWNthrowing aBadRequestMethodexception.
458-462: LGTM!The ordering is correct—
loadRoutingConfig()is called beforeloadAttributeRoutes()to ensure controller paths are available in the Registry before route scanning begins.
528-550: LGTM!The fallback logic is well-structured: Registry (from routing.yaml) is checked first, then neuron.yaml for backward compatibility. The distinction between
null(not configured) and empty array (explicitly disabled) is correctly handled with=== null.
593-603: LGTM!Passing
$def->namedirectly toaddRoute()is cleaner than setting it afterward on the RouteMap. This enables the router to handle named routes and duplicate detection during registration.
| catch( \Symfony\Component\Yaml\Exception\ParseException $e ) | ||
| { | ||
| Log::error( "Failed to parse routing.yaml: " . $e->getMessage() ); | ||
| } |
There was a problem hiding this comment.
Registry value persists across Application instances
Medium Severity
The loadRoutingConfig() method only sets Routing.ControllerPaths in the singleton Registry when routing.yaml exists and contains controller_paths. When the file doesn't exist or parsing fails, the Registry key is never cleared. In loadAttributeRoutes(), the code checks Registry::getInstance()->get('Routing.ControllerPaths') and only falls back to neuron.yaml when the value is null. If a previous Application instance set this Registry key, subsequent instances without routing.yaml will use the stale configuration instead of falling back to their own neuron.yaml settings. This affects test isolation and scenarios with multiple Application instances.
Additional Locations (1)
1 participant
Note
Separates routing concerns and enables URL rewrites plus named routes.
config/routing.yamlsupport:Application::loadRoutingConfig()readsrewritesandcontroller_paths; falls back toneuron.yamlif absent, withrouting.yamltaking precedenceApplication::addRoute()now accepts$nameand passes it to routerget/post/put/delete;registerAttributeRoute()forwards attributenamedirectlyloadAttributeRoutes()reads controller paths from Registry (set by routing.yaml) with backward-compatible fallback, and respects empty arraysneuron.yamlnotes routing config locationtests/Mvc/MockRouterupdated to accept$nameinget/postWritten by Cursor Bugbot for commit 69def15. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.