-
-
Notifications
You must be signed in to change notification settings - Fork 834
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
Allow overriding routes #2577
Allow overriding routes #2577
Changes from all commits
bdc566f
42c4cdc
8f2e001
ce70285
902afdb
9785d63
30f176b
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 |
---|---|---|
|
@@ -30,6 +30,16 @@ class RouteCollection | |
*/ | ||
protected $routeParser; | ||
|
||
/** | ||
* @var array | ||
*/ | ||
protected $routes = []; | ||
|
||
/** | ||
* @var array | ||
*/ | ||
protected $pendingRoutes = []; | ||
|
||
public function __construct() | ||
{ | ||
$this->dataGenerator = new DataGenerator\GroupCountBased; | ||
|
@@ -63,19 +73,50 @@ public function delete($path, $name, $handler) | |
|
||
public function addRoute($method, $path, $name, $handler) | ||
SychO9 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
$routeDatas = $this->routeParser->parse($path); | ||
|
||
foreach ($routeDatas as $routeData) { | ||
$this->dataGenerator->addRoute($method, $routeData, ['name' => $name, 'handler' => $handler]); | ||
if (isset($this->routes[$method][$name])) { | ||
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. Shouldn't we have unique names regardless of method? 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. While it makes sense for the names to be unique, regardless of the method, and for extension developers to always use unique route names as it is good practice. FastRoute doesn't error out when same route names are used, or even same method and route names, it only errors out when the same path is provided. Which means, that any extensions that have used similar route names, but with different methods would break if we don't take the method into account. Although, if there are any extensions that used the same route name and method with a different path, they would break with this implementation as well, I just find it less likely for an extension to have done that, but I haven't it it up (I'm not how I'd look that up either) 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. Since the URL Generator is based on unique route names per-app, I would prefer that we enforced unique route names in each app. Any extensions that use the same route name are improperly implemented, and I'd prefer that we break this now than later. Alternatively, we could do this in phases: method + name unique for this release, just name unique for next release? 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.
I would be in favour of this, we would warn extension devs that non unique route names will no longer work in the next release. |
||
throw new \RuntimeException("Route $name on method $method already exists"); | ||
} | ||
|
||
$this->reverse[$name] = $routeDatas; | ||
$this->routes[$method][$name] = $this->pendingRoutes[$method][$name] = compact('path', 'handler'); | ||
|
||
return $this; | ||
} | ||
|
||
public function removeRoute(string $method, string $name): self | ||
{ | ||
unset($this->routes[$method][$name], $this->pendingRoutes[$method][$name]); | ||
|
||
return $this; | ||
} | ||
|
||
protected function applyRoutes(): void | ||
{ | ||
foreach ($this->pendingRoutes as $method => $routes) { | ||
foreach ($routes as $name => $route) { | ||
$routeDatas = $this->routeParser->parse($route['path']); | ||
|
||
foreach ($routeDatas as $routeData) { | ||
$this->dataGenerator->addRoute($method, $routeData, ['name' => $name, 'handler' => $route['handler']]); | ||
} | ||
|
||
$this->reverse[$name] = $routeDatas; | ||
} | ||
} | ||
|
||
$this->pendingRoutes = []; | ||
} | ||
|
||
public function getRoutes(): array | ||
{ | ||
return $this->routes; | ||
} | ||
|
||
public function getRouteData() | ||
{ | ||
if (! empty($this->pendingRoutes)) { | ||
$this->applyRoutes(); | ||
} | ||
|
||
return $this->dataGenerator->getData(); | ||
} | ||
|
||
|
@@ -88,6 +129,10 @@ protected function fixPathPart(&$part, $key, array $parameters) | |
|
||
public function getPath($name, array $parameters = []) | ||
{ | ||
if (! empty($this->pendingRoutes)) { | ||
$this->applyRoutes(); | ||
} | ||
|
||
if (isset($this->reverse[$name])) { | ||
$maxMatches = 0; | ||
$matchingParts = $this->reverse[$name][0]; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
<?php | ||
|
||
/* | ||
* This file is part of Flarum. | ||
* | ||
* For detailed copyright and license information, please view the | ||
* LICENSE file that was distributed with this source code. | ||
*/ | ||
|
||
namespace Flarum\Tests\unit\Http; | ||
|
||
use Flarum\Http\RouteCollection; | ||
use Flarum\Tests\unit\TestCase; | ||
|
||
class RouteCollectionTest extends TestCase | ||
{ | ||
/** @test */ | ||
public function can_add_routes() | ||
{ | ||
$routeCollection = (new RouteCollection) | ||
->addRoute('GET', '/index', 'index', function () { | ||
echo 'index'; | ||
}) | ||
->addRoute('DELETE', '/posts', 'forum.posts.delete', function () { | ||
echo 'delete posts'; | ||
}); | ||
|
||
$this->assertEquals('/index', $routeCollection->getPath('index')); | ||
$this->assertEquals('/posts', $routeCollection->getPath('forum.posts.delete')); | ||
} | ||
|
||
/** @test */ | ||
public function can_add_routes_late() | ||
{ | ||
$routeCollection = (new RouteCollection)->addRoute('GET', '/index', 'index', function () { | ||
echo 'index'; | ||
}); | ||
|
||
$this->assertEquals('/index', $routeCollection->getPath('index')); | ||
|
||
$routeCollection->addRoute('DELETE', '/posts', 'forum.posts.delete', function () { | ||
echo 'delete posts'; | ||
}); | ||
|
||
$this->assertEquals('/posts', $routeCollection->getPath('forum.posts.delete')); | ||
} | ||
} |
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.
I don't like that this is in both extenders, but I don't suppose we have a choice?
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.
well, Frontend adds this type of content routes, I thought it'd makes sense to be able to remove them here as well, especially since the method is always
GET
here, so it needn't be a parameter.But technically, using the
Routes
extender to remove these routes will still work I believe.