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

Fix URL generation in various places #123

Closed
tobyzerner opened this issue Jun 20, 2015 · 5 comments
Closed

Fix URL generation in various places #123

tobyzerner opened this issue Jun 20, 2015 · 5 comments
Assignees
Milestone

Comments

@tobyzerner
Copy link
Contributor

Splitting up our front controllers is a great move, but URL generation is problematic in some cases:

  • Flarum\Core\Handlers\Commands\RequestPasswordResetCommandHandler
  • Flarum\Core\Handlers\Events\EmailConfirmationMailer
  • Flarum\Api\Actions\Discussions\IndexAction

The first two are run through api.php, but need to generate forum routes.
The third can be run through index.php (when data is preloaded), but needs to generate an API route for the pagination links.
The Mentions extension also needs to generate forum routes when run via api.php.

@franzliedke how can we handle this?

@amdad
Copy link

amdad commented Jun 25, 2015

I don't now if this issue embrace also utf-8 user names problem.
Like you can see in your Jérémie mentions here:
http://discuss.flarum.org/d/79/fluxbb-joins-forces-with-flarum/15

@tobyzerner
Copy link
Contributor Author

@amdad That's an artefact from esoTalk import. We only allow alphanumeric usernames in Flarum for now :)

@amdad
Copy link

amdad commented Jun 25, 2015

For now... ;)

@tobyzerner tobyzerner added this to the 1.0 Beta 1 milestone Jun 25, 2015
@tobyzerner tobyzerner modified the milestone: 1.0 Beta 1 Jul 30, 2015
@tobyzerner
Copy link
Contributor Author

Currently Flarum's routes are defined in ApiServiceProvider, ForumServiceProvider, and AdminServiceProvider, all of which are only registered on their respective front controller. Thus, on an API request (entry via api.php), we have no knowledge of forum routes. This is problematic because sometimes API requests need to generate URLs to forum routes (e.g. when sending notification/confirmation emails)

What needs to be done:

  • Work out how we can register all routes on all requests, but only route to a primary collection of them.
  • Fix various instances where URLs are hardcoded (mostly in emails).

@franzliedke
Copy link
Contributor

I'm planning to make all route collections available to the URL generator, but only parse those routes (for actual routing) that are needed in each section (API, forum, admin). Will do that early next week.

tobyzerner added a commit that referenced this issue Oct 2, 2015
Spent quite a while looking into the best solution here and ended up going with three separate classes. Thanks to @luceos for the PR that got this rolling (#518). My reasoning is:

- The task of routing and URL generation is independent for each section of the app. Take Flarum\Api\Users\IndexAction for example. I don't want to generate a URL to a Flarum route... I specifically want to generate a URL to an API route. So there should be a class with that specific responsibility.
- In fact, each URL generator is slightly different, because we need to add a certain prefix to the start (e.g. /api)
- This also allows us to get rid of the "flarum.api" prefix on each route's name.
- It's still DRY, because they all extend a base class.

At the same time, I could see no reason this needed to be "interfaced", so all of the classes are concrete.

Goes a long way to fixing #123 - still just a few places left remaining with hardcoded URLs.
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.

3 participants