-
Notifications
You must be signed in to change notification settings - Fork 1.9k
refactor: loading app/Config/routes.php #6293
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
f881a36
to
26b2c7b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! I've always found this clunky, and it makes static analysis on external packages very ornery to pull a magic $routes
variable out of nowhere. Good improvement, a few suggestions in review.
@@ -1393,6 +1393,7 @@ public function resetRoutes() | |||
} | |||
|
|||
$this->prioritizeDetected = false; | |||
$this->didDiscover = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it could be a breaking change, though I'm not sure why this wasn't the case before.
system/Router/RouteCollection.php
Outdated
* | ||
* Loads only once unless reset. | ||
*/ | ||
public function loadRoutes(string $routesFile = APPPATH . 'Config/Routes.php'): void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the calls you replaced look like this:
$routes = Services::routes();
$routes->loadRoutes();
Which makes me think this would be better off returning $this
so those can become: $routes = Services::routes()->loadRoutes()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since loadRoutes()
is essentially an initialization process, it is better to group it with Services::routes()->loadRoutes()
.
Actually, I would like to call loadRoutes()
in the constructor, but we can't because Routes.php
has $routes = Services::routes();
.
system/Router/RouteCollection.php
Outdated
@@ -1490,4 +1491,21 @@ public function shouldUseSupportedLocalesOnly(): bool | |||
{ | |||
return $this->useSupportedLocalesOnly; | |||
} | |||
|
|||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless you have a compelling reason for sticking this at the bottom I think it would make more organizational sense to be up above discoverRoutes()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
96fef30
to
bd03948
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that bootstrap.php does the initial load already, does it make sense to move loadRoutes()
into the Service so the returned version is always "loaded"?
Infinite loop occurs unless we remove: CodeIgniter4/app/Config/Routes.php Line 6 in c1bac7e
|
Gotcha. I think how you had it is good. I don't know why anyone would want the collection without loading the routes but it's there as an option - maybe would help with testing or something. |
If we remove |
What are the consequences if they don't? Endless loop? If that's the case it seems too risky a change. This is already a huge improvement - I am content taking this from a 2 to a 9 without having to reach 10, which can happen with version 5. |
Yes, endless loop. Services::routes() calls Services::routes() and ... Since no one objects, I will add changelog to complete this PR. |
loadRoutes() and discoverRoutes() are initialization.
bd03948
to
04cbdf1
Compare
Added changelog. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Splendid!
🥳 Very excited to get this one in |
Description
The current way is difficult to use.
RouteCollection::loadRoutes()
Checklist: