-
Notifications
You must be signed in to change notification settings - Fork 2k
feat(app): Allow turning off defined routes and/or auto routing #9749
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
base: 4.7
Are you sure you want to change the base?
Conversation
is it possible to do inheritance? get rid of the conditions? nothing like that:
This is an assumption, it may not work. |
Well, this was an attempt to do that to some extent, but the AutoRouter relies on the defined routes to ensure that any controllers used in defined routes can't be auto-routed. My preference would have been to separate them completely and have auto-router checked first, ignoring the defined routes. Then, if no route is matched it would move to checking defined routes. That would be nice and clean, but would also be a pretty big breaking change, and a potentially problematic one for lots of sites. So I did it this way hoping to keep things working for all sites as it does now. |
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.
Saved time with Auto Routing is nice - that's a useful enhancement.
Most of the suppressed PHPStan errors don't seem related to your code changes. I think it's time to sync the 4.7
branch with develop
. I'll try to do that tomorrow.
e7c65af
to
5de0a16
Compare
* refactor(app): Standardize subdomain detection logic * Update app/Config/Hostnames.php Co-authored-by: Pooya Parsa <[email protected]> * Update tests/system/Helpers/URLHelper/MiscUrlTest.php Co-authored-by: Pooya Parsa <[email protected]> * addressing review comments * cs fix * cs fix * cs fix * remove typo in docs ci-skip --------- Co-authored-by: Pooya Parsa <[email protected]>
5de0a16
to
40e9309
Compare
It looks like there are commits from a different PR included here. Could you please rebase and make sure only the commits relevant to this PR are included? |
@michalsn Which commits? I did pull down the latest 4.7 and rebased last night. Then cs-fix and rector. |
@lonnieezell this one: "refactor(app): Standardize subdomain detection logic" This is clearly a commit from a different PR. We should not see these changes here. However, I'm not sure what happened. You can try:
|
Description
This allows users to choose which type of routing they want to use - auto routing or defined routing, and turn the other one completely off. This is especially beneficial if you're using auto-routing exclusively because it previously loaded up the entire route collection anyway and dealt with that overhead, even if it was empty.
Estimated performance increases:
Checklist: