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

Custom routes option #938

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

danielpuglisi
Copy link

@danielpuglisi danielpuglisi commented Mar 6, 2018

Hey kaminari team,

I'm building a project which requires dynamic routes that go through a wildcard domain and are handled by a custom Rack application that acts as the router. E.g.:

match '(*path)', to: WebsiteRouter.new, via: :all

Because of this it's not possible to use url_for and I've built my own _path methods. Or at least I haven't found a way to get it to work with url_for.

Now because I can't use url_for, kaminari pagination doesn't work for those custom routes. Inspired by #115 I've added an option that would add support for other methods than url_for:

<%= paginate @posts, route: :custom_posts_path %>

How those methods build the path is up to the application itself.

The vulnerability issue discussed in #115 isn't affected anymore because the route option is no longer passed along with the params option.

Maybe this use case is to special for my own use case to implement as a feature. In this case I'll just go with monkey patching. Let me know what you think.

@yuki24
Copy link
Member

yuki24 commented Mar 21, 2018

Thanks for your PR, and my apologies for not getting back to you sooner. Although, at this point I can not promise that we will be able to merge this in. I have experimented with a large refactor that basically breaks up the big Paginator class into smaller pieces. Once this is done, you should be able to swap a router in a nicer way than the way currently is. I don't think we will be able to merge it any time soon, but we are aware of the fast that the current design is not as ideal as it should be, and I'm very open to suggestions as to how we could improve the code base.

@ruurd
Copy link
Contributor

ruurd commented Dec 20, 2020

That seems weird to accomodate one persons problem with url_for - I would say yes interesting as long as the PR adds to kaminari. If it's just a solution for one problem then I would decline the PR really. Besides: if @danielpuglisi is having trouble with url_for he will have that same trouble outside of kaminari also so why not fix the url_for problem instead?

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

Successfully merging this pull request may close these issues.

None yet

3 participants