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 #2553

Closed
askvortsov1 opened this issue Jan 19, 2021 · 1 comment · Fixed by #2577
Closed

Allow overriding routes #2553

askvortsov1 opened this issue Jan 19, 2021 · 1 comment · Fixed by #2577
Assignees
Milestone

Comments

@askvortsov1
Copy link
Sponsor Member

Feature Request

Refs flarum/issue-archive#345

Is your feature request related to a problem? Please describe.
Once a route is added to FastRoute's RouteCollector, it can't really be overriden or removed. As discussed in nikic/FastRoute#111, that is not something that is planned to be changed. With #2425, a middleware could override the handler for a route, but this is hacky and not immediately intuitive.

Describe the solution you'd like
Extensions should be able to override or remove routes. While we can't do this on the FastRoute level, we actually already wrap FastRoute's route registration system in our RouteCollection class (https://github.com/flarum/core/blob/master/src/Http/RouteCollection.php). In order to allow overriding routes, we could adjust RouteCollection to only pass routes into FastRoute once all extensions have booted. addRoute would just add routes to an internal data structure, and a new method (commit()? apply()?) would actually add them to the route collector. In the meantime, if extensions register a new route with the same name as an existing one, the existing one would be overriden.

Alternatively, we could explicitly require that a route be removed, and then the replacing route be added, to avoid extensions accidentially overriding routes if they copy-paste or otherwise reuse route names.

That being said, the recommended way to insert custom logic into the execution of a controller should be listening to events, and the recommended way to redirect / wrap routes should be middleware. Replacing routes completely has its use cases, but the aforementioned approaches shouldn't be ignored in favor of just swapping the route out completely, which is more difficult for extensions to maintain.

Describe alternatives you've considered
See #2133 for some other ideas

@askvortsov1 askvortsov1 modified the milestones: 0.1, 0.1.0-beta.16 Jan 19, 2021
@SychO9 SychO9 self-assigned this Jan 26, 2021
@askvortsov1
Copy link
Sponsor Member Author

If core's routes are applied before extension routes (which seems sensible if we go with my proposal above), this should also fix https://github.com/flarum/core/issues/2239

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants