-
Notifications
You must be signed in to change notification settings - Fork 77
Convert routes to a node tree to improve route matching perfomance #208
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: 3.x
Are you sure you want to change the base?
Changes from 1 commit
ac02ed6
cbe8fc2
66560e6
b7bc2ce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -113,8 +113,12 @@ public function match(ServerRequestInterface $request) | |
| $this->failedScore = 0; | ||
| $path = $request->getUri()->getPath(); | ||
|
|
||
| foreach ($this->map as $name => $proto) { | ||
| $route = $this->requestRoute($request, $proto, $name, $path); | ||
| $possibleRoutes = $this->getMatchedTree($path); | ||
| foreach ($possibleRoutes as $proto) { | ||
| if (is_array($proto)) { | ||
| continue; | ||
| } | ||
| $route = $this->requestRoute($request, $proto, $path); | ||
| if ($route) { | ||
| return $route; | ||
| } | ||
|
|
@@ -131,20 +135,18 @@ public function match(ServerRequestInterface $request) | |
| * | ||
| * @param Route $proto The proto-route to match against. | ||
| * | ||
| * @param string $name The route name. | ||
| * | ||
| * @param string $path The request path. | ||
| * | ||
| * @return mixed False on failure, or a Route on match. | ||
| * | ||
| */ | ||
| protected function requestRoute($request, $proto, $name, $path) | ||
| protected function requestRoute($request, $proto, $path) | ||
| { | ||
| if (! $proto->isRoutable) { | ||
| return; | ||
| return false; | ||
| } | ||
| $route = clone $proto; | ||
| return $this->applyRules($request, $route, $name, $path); | ||
| return $this->applyRules($request, $route, $route->name, $path); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. routename is already available in the route itself |
||
| } | ||
|
|
||
| /** | ||
|
|
@@ -261,4 +263,27 @@ public function getMatchedRoute() | |
| { | ||
| return $this->matchedRoute; | ||
| } | ||
|
|
||
| /** | ||
| * Split the URL into URL Segments and check for matching routes per segment | ||
| * This segment could return a list of possible routes | ||
| * | ||
| * @param string $path | ||
| * @return \RecursiveArrayIterator | ||
| */ | ||
| private function getMatchedTree($path) | ||
| { | ||
| $node = $this->map->getAsTreeRouteNode(); | ||
| foreach (explode('/', trim($path, '/')) as $segment) { | ||
| if (isset($node[$segment])) { | ||
| $node = $node[$segment]; | ||
| continue; | ||
| } | ||
| if (isset($node['{}'])) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Group all kind of routes with dynamic properties into a generic group |
||
| $node = $node['{}']; | ||
| } | ||
| } | ||
|
|
||
| return new \RecursiveArrayIterator($node); | ||
| } | ||
|
Comment on lines
+268
to
+288
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion
Calling - $node = $this->map->getAsTreeRouteNode();
+ $node = $this->map->getTreeRoutesCached(); // new memoised accessorWith large route maps this will materially affect performance.
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -258,7 +258,6 @@ public function testLogger() | |
| $matcher->match($request); | ||
|
|
||
| $expect = [ | ||
| 'debug: /bar FAILED Aura\Router\Rule\Path ON foo', | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will not work anymore because the /foo route will not be checked anymore. |
||
| 'debug: /bar MATCHED ON bar', | ||
| ]; | ||
| $actual = $logger->lines; | ||
|
|
||
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.
See doctype description:
@return mixed False on failure, or a Route on match.so this should return false?