Skip to content
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

Merged
merged 7 commits into from
Feb 28, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion src/Extend/Frontend.php
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ class Frontend implements ExtenderInterface
private $css = [];
private $js;
private $routes = [];
private $removedRoutes = [];
private $content = [];

public function __construct(string $frontend)
Expand Down Expand Up @@ -59,6 +60,13 @@ public function route(string $path, string $name, $content = null)
return $this;
}

public function removeRoute(string $name)
Copy link
Member

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?

Copy link
Member Author

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.

{
$this->removedRoutes[] = compact('name');

return $this;
}

/**
* @param callable|string $callback
* @return $this
Expand Down Expand Up @@ -141,7 +149,7 @@ function (Saved $event) use ($container, $abstract) {

private function registerRoutes(Container $container)
{
if (empty($this->routes)) {
if (empty($this->routes) && empty($this->removedRoutes)) {
return;
}

Expand All @@ -151,6 +159,10 @@ function (RouteCollection $collection, Container $container) {
/** @var RouteHandlerFactory $factory */
$factory = $container->make(RouteHandlerFactory::class);

foreach ($this->removedRoutes as $route) {
$collection->removeRoute('GET', $route['name']);
SychO9 marked this conversation as resolved.
Show resolved Hide resolved
}

foreach ($this->routes as $route) {
$collection->get(
$route['path'],
Expand Down
14 changes: 13 additions & 1 deletion src/Extend/Routes.php
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ class Routes implements ExtenderInterface
private $appName;

private $routes = [];
private $removedRoutes = [];

public function __construct($appName)
{
Expand Down Expand Up @@ -62,9 +63,16 @@ private function route($httpMethod, $path, $name, $handler)
return $this;
}

public function remove(string $method, string $name)
{
$this->removedRoutes[] = compact('method', 'name');

return $this;
}

public function extend(Container $container, Extension $extension = null)
{
if (empty($this->routes)) {
if (empty($this->routes) && empty($this->removedRoutes)) {
return;
}

Expand All @@ -74,6 +82,10 @@ function (RouteCollection $collection, Container $container) {
/** @var RouteHandlerFactory $factory */
$factory = $container->make(RouteHandlerFactory::class);

foreach ($this->removedRoutes as $route) {
$collection->removeRoute($route['method'], $route['name']);
}

foreach ($this->routes as $route) {
$collection->addRoute(
$route['method'],
Expand Down
4 changes: 2 additions & 2 deletions src/Forum/ForumServiceProvider.php
Original file line number Diff line number Diff line change
Expand Up @@ -203,8 +203,8 @@ protected function setDefaultRoute(RouteCollection $routes)
$factory = $this->app->make(RouteHandlerFactory::class);
$defaultRoute = $this->app->make('flarum.settings')->get('default_route');

if (isset($routes->getRouteData()[0]['GET'][$defaultRoute]['handler'])) {
$toDefaultController = $routes->getRouteData()[0]['GET'][$defaultRoute]['handler'];
if (isset($routes->getRoutes()['GET'][$defaultRoute]['handler'])) {
$toDefaultController = $routes->getRoutes()['GET'][$defaultRoute]['handler'];
} else {
$toDefaultController = $factory->toForum(Content\Index::class);
}
Expand Down
55 changes: 50 additions & 5 deletions src/Http/RouteCollection.php
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,16 @@ class RouteCollection
*/
protected $routeParser;

/**
* @var array
*/
protected $routes = [];

/**
* @var array
*/
protected $pendingRoutes = [];

public function __construct()
{
$this->dataGenerator = new DataGenerator\GroupCountBased;
Expand Down Expand Up @@ -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])) {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we have unique names regardless of method?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.
Here's an example: https://github.com/v17development/flarum-seo/blob/master/extend.php
Search: https://github.com/search?l=PHP&p=3&q=Flarum+Extend+Routes+get+-repo%3Aflarum%2Fapi-docs&type=Code

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)

Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Alternatively, we could do this in phases: method + name unique for this release, just name unique for next release?

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

Expand All @@ -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];
Expand Down
35 changes: 35 additions & 0 deletions tests/integration/extenders/RoutesTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,41 @@ public function custom_route_can_be_added_by_extender()
$this->assertEquals(200, $response->getStatusCode());
$this->assertEquals('Hello Flarumites!', $response->getBody());
}

/**
* @test
*/
public function existing_route_can_be_removed()
{
$this->extend(
(new Extend\Routes('api'))
->remove('GET', 'forum.show')
);

$response = $this->send(
$this->request('GET', '/api')
);

$this->assertEquals(404, $response->getStatusCode());
}

/**
* @test
*/
public function custom_route_can_override_existing_route_if_removed()
{
$this->extend(
(new Extend\Routes('api'))
->remove('GET', 'forum.show')
->get('/', 'forum.show', CustomRoute::class)
);

$response = $this->send(
$this->request('GET', '/api')
);

$this->assertEquals('Hello Flarumites!', $response->getBody());
}
}

class CustomRoute implements RequestHandlerInterface
Expand Down
47 changes: 47 additions & 0 deletions tests/unit/Http/RouteCollectionTest.php
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'));
}
}