Allow overriding routes #2577
Allow overriding routes #2577
Conversation
There was a problem hiding this comment.
Should we allow directly overriding, or might it be clearer to require that a route be somehow unset? This would prevent accidentially overriding routes by copying route names.
We should also test that this fixes https://github.com/flarum/core/issues/2239
src/Http/RouteCollection.php
Outdated
| } | ||
|
|
||
| public function getRouteData() | ||
| { | ||
| if (empty($this->reverse)) { |
There was a problem hiding this comment.
Should we allow this (and the same with getPath)? Perhaps we could throw an exception instead to further discourage early resolution?
There was a problem hiding this comment.
so, we would essentially apply the routes on the boot phase by manually resolving the route collection, instead of on first demand. That would technically work, I'm just unsure if it's a good idea to take early resolution into account and explicitly throw an exception just to discourage extension devs.
There was a problem hiding this comment.
That's a good point... Plus, now that:
- Classes registered via extenders will be resolved when needed instead of at subscribe time
- Subscribers will be resolved after boot
early resolution should be much, much less frequent. Yay extenders!!!
Yup, that's why it's a draft, I have yet to add the ability to remove a route, which means we'll have to throw an exception if a route is overridden but already exists. Wasn't sure at first, but I do favour the more explicit way of overriding a route. (by having to remove it first) |
|
one thing that's bothering me, is that adding an integration test for not being able to add an already existing route didn't work, there seems to be an issue with expecting the exception. |
| @@ -59,6 +60,13 @@ public function route(string $path, string $name, $content = null) | |||
| return $this; | |||
| } | |||
|
|
|||
| public function removeRoute(string $name) | |||
There was a problem hiding this comment.
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.
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.
|
|
||
| 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.
Shouldn't we have unique names regardless of method?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
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.
by applying the routes to FastRoute late, late enough for extensions to be able to override the routes.
SychO9
force-pushed
the
sm/allow-overriding-routes
branch
from
2359a46
to
9785d63
Compare
[ci skip] [skip ci]
Fixes #2553
Changes proposed in this pull request:
Store registered routes in an array before applying them to FastRoute, thus allowing extensions to replace them (as described in the issue).
Reviewers should focus on:
The default route
/is a special route, it is dynamically set inForumServiceProviderafterflarum.forum.routeshas been resolved, at which point, extensions have already registered their routes, which means it's the one route they cannot override.That said, I don't think there's a problem with that, they should not be directly overriding this route, instead they should be adding an option for a homepage route like tags.
TODO
Confirmed
composer test).