Skip to content

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

Merged
merged 4 commits into from
Aug 1, 2022

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Jul 24, 2022

Description
The current way is difficult to use.

  • add RouteCollection::loadRoutes()

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added refactor Pull requests that refactor code 4.3 labels Jul 24, 2022
@kenjis kenjis force-pushed the refactor-Routes.php-loading branch 3 times, most recently from f881a36 to 26b2c7b Compare July 24, 2022 10:23
Copy link
Member

@MGatner MGatner left a 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;
Copy link
Member

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.

*
* Loads only once unless reset.
*/
public function loadRoutes(string $routesFile = APPPATH . 'Config/Routes.php'): void
Copy link
Member

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().

Copy link
Member Author

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();.

@@ -1490,4 +1491,21 @@ public function shouldUseSupportedLocalesOnly(): bool
{
return $this->useSupportedLocalesOnly;
}

/**
Copy link
Member

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().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@kenjis kenjis added the breaking change Pull requests that may break existing functionalities label Jul 24, 2022
@kenjis kenjis force-pushed the refactor-Routes.php-loading branch from 96fef30 to bd03948 Compare July 24, 2022 11:39
Copy link
Member

@MGatner MGatner left a 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"?

@kenjis
Copy link
Member Author

kenjis commented Jul 24, 2022

Infinite loop occurs unless we remove:

$routes = Services::routes();

@MGatner
Copy link
Member

MGatner commented Jul 25, 2022

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.

@kenjis
Copy link
Member Author

kenjis commented Jul 25, 2022

If we remove $routes = Services::routes(); in Routes.php, we can load routes in the constructor.
All existing users must remove it, though.

@MGatner
Copy link
Member

MGatner commented Jul 26, 2022

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.

@kenjis
Copy link
Member Author

kenjis commented Jul 26, 2022

Yes, endless loop. Services::routes() calls Services::routes() and ...
Okay, let's wait for v5.

Since no one objects, I will add changelog to complete this PR.

@kenjis kenjis force-pushed the refactor-Routes.php-loading branch from bd03948 to 04cbdf1 Compare July 29, 2022 10:51
@kenjis
Copy link
Member Author

kenjis commented Jul 29, 2022

Added changelog.

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Splendid!

@kenjis kenjis merged commit 807a16b into codeigniter4:4.3 Aug 1, 2022
@kenjis kenjis deleted the refactor-Routes.php-loading branch August 1, 2022 01:12
@MGatner
Copy link
Member

MGatner commented Aug 1, 2022

🥳 Very excited to get this one in

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Pull requests that may break existing functionalities refactor Pull requests that refactor code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants