Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Supported route prefix (or namespace) which was added in Rails 3.1. #322

Open
wants to merge 3 commits into from

21 participants

Harai Akihiro Yuki Nishijima Justin Coyne Marcelo Jacobus Andrea Dal Ponte David Chandek-Stark RonnieOnRails Michael J. Giarlo Jack Kinsella Zachary Scott Eito Katagiri cam156  清玩 Mike Utley Brian Maddy Semyon Pupkov Bian Ying Ben Turner Philip Arndt francirp Sue Richeson
Harai Akihiro

Supported mountable engine's route prefix (or namespace), which was added in Rails 3.1.

Refinery CMS 2, the most popular cms which supports Rails 3.2, requires this feature because it has highly modularized design and it is common to refer to other engine's url from view.

Yuki Nishijima
Collaborator

The option is reasonable, but we can't merge this Pull Request for now since we have tons of issues about url_for. And I have some commends:

I keep this PR open and will possibly consider this again after we solve other url_for/routing issues.

Harai Akihiro

Some specs failed on Travis: https://travis-ci.org/yuki24/kaminari/builds/3659060

This is because the method my_engine in the specs is not defined on Sinatra. This is not really a problem when using it on Sinatra, but the failure must be fixed somehow.

The name route_prefix implies that it is a String object, but it is an ActionDispatch::Routing::RoutesProxy object.

That might be the most difficult problem on this issue. As you say, the naming is not good. But I could not find a better name. will_paginate had the same issue before, and they decided to use scope. But in Kaminari, the word scope has been already used for other purpose. We have to find another name.

Does anyone have a good idea?

Harai Akihiro

I have fixed the both issues.
I don't know if the name route_set is the best, but at least it is better than the previous name.

Justin Coyne

@yuki24 Can you take another look at merging this pull request? I'm still having this trouble.

Marcelo Jacobus

I have a crud controller that extends MyEngine::Base controller. The view there is scoped to 'my_engine'. I know that because "main_app.customer_path(customer)" works and "customer_path(customer)" does not.

So, I am in the "my_route" context, and I would like to paginate using helpers of my main app. But the examples bellow wont work.

 # wont work: ActionController::RoutingError
<%= paginate(@customers, params: { route_set: :main_app }) %>

 # undefined method paginate (but I can use pagination in the main app
 # when it does not extend MyEngine controller)
<%= main_app.paginate(@customers) %>

So, I am also looking forward to that pull request being merged :)

Excelent job guys. Great plugin.

Andrea Dal Ponte

Great job! I'm waiting for the merge too =)

Justin Coyne

@yuki24 Any update?

David Chandek-Stark

+1. Fixes my problem. Thanks!

RonnieOnRails

Up @yuki24. This PR is really wanted. Thank you.

Jack Kinsella

+1 Necessary for another major modular Rails open source application, Spree

Yuki Nishijima yuki24 was assigned
Zachary Scott
Collaborator

@jackkinsella Does Spree maintain 3.1? I'm not sure that we do.

Jack Kinsella

@zzak - I'm not sure what you mean by the question. I'm using a relatively recent 1.3X version of Spree with Rails 3.2.X.

Justin Coyne

This is issue is also a problem with Rails 4. This patch is direly needed. As no action has been taken for 6 months, I'm considering forking and releasing a new version of this gem with this patch.

Zachary Scott
Collaborator

@jackkinsella I must have confused since with before.

Zachary Scott
Collaborator

Sorry but I must hold off on this until next version, we are working on url_for as @yuki24 mentioned.

Eito Katagiri

ping @zzak @yuki24

Can we merge this PR then work on issues on url_for?
We should release new version. The last release (0.14.1) was one year ago :fearful:

neversion neversion referenced this pull request in projecthydra-labs/curate
Closed

Can't work with rails 4.0? #211

Zachary Scott
Collaborator
Justin Coyne

@zzak Any update on what needs to be done? I've been using this fork/branch for 10 months, In production for 6. It's a really important feature for me.

cam156

+1 merge please

 清玩

+1 merge please

Yuki Nishijima yuki24 modified the milestone: 1.0.0
Mike Utley

@zzak @yuki24 First thank you for your hard work on this gem. Why has no action been taken on this it seems to be a very large issue spanning at least 4 duplicate issues from what I can see.

Yuki Nishijima yuki24 referenced this pull request
Closed

Add url_helper option #577

Zachary Scott
Collaborator

Since master is at alpha should we commit new feature requests?

Brian Maddy

+1 merge please--I just lost a few hours trying to figure this out also.

Bian Ying

+1

Jeff Parr jparr referenced this pull request in projecthydra/sufia
Open

ActionController::UrlGenerationError in My::Files#index #674

Justin Coyne jcoyne referenced this pull request from a commit in jcoyne/kaminari
Justin Coyne jcoyne Support for rails engines route prefix
This is merging the work on harai:route_prefix_prototype in PR #322
5d1bee6
Philip Arndt

I have this in config/initializers/kaminari_monkey_patch.rb

# Please see https://github.com/amatsuda/kaminari/pull/322 for an explanation.
# This fixes an issue when using Kaminari with engines/main_app.

Rails.application.config.to_prepare do
  Kaminari::Helpers::Tag.class_eval do
    def page_url_for(page)
      arguments = @params.merge(@param_name => (page <= 1 ? nil : page), :only_path => true)
      begin
        @template.url_for arguments
      rescue
        @template.main_app.url_for arguments
      end
    end
  end
end

Inelegant, as it requires knowledge of the internals, but a seemingly stable workaround.

francirp

@parndt thanks...I slightly modified your solution and it worked for me:

In my Rails Engine at lib/kaminari/helpers/tag.rb:

module Kaminari
  module Helpers
    class Tag

      def page_url_for(page)
        arguments = @params.merge(@param_name => (page <= 1 ? nil : page), :only_path => true).symbolize_keys
        begin
          MyEngineName::Engine.routes.url_helpers.url_for arguments
        rescue
          @template.main_app.url_for arguments
        end
      end

    end
  end
end

Make sure to require the file at lib/your_engine.rb with require "kaminari/helpers/tag"

Sue Richeson

+1 Merge please. This is costing many hours of developer time!

Yuki Nishijima
Collaborator

I was looking into Rails to figure out what we can do about this. Does _with_routes helper method work?

<% _with_routes routes_proxy do %>
  <%= paginate @comments %>
<% end %>

I know it seems like a private API but I would prefer this rather than changing kaminari. I can also send a pull request to Rails to make _with_routes a public API. WDYT?

Justin Coyne jcoyne referenced this pull request from a commit in jcoyne/kaminari
Justin Coyne jcoyne Support for rails engines route prefix
This is a rebase of the work done by @harai in PR #322
0799f31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
26 lib/kaminari/helpers/action_view_extension.rb
View
@@ -13,6 +13,7 @@ module ActionViewExtension
# * <tt>:params</tt> - url_for parameters for the links (:controller, :action, etc.)
# * <tt>:param_name</tt> - parameter name for page number in the links (:page by default)
# * <tt>:remote</tt> - Ajax? (false by default)
+ # * <tt>:route_set</tt> - namespace of router (current engine's namespace by default)
# * <tt>:ANY_OTHER_VALUES</tt> - Any other hash key & values would be directly passed into each tag as :locals value.
def paginate(scope, options = {}, &block)
paginator = Kaminari::Helpers::Paginator.new self, options.reverse_merge(:current_page => scope.current_page, :total_pages => scope.total_pages, :per_page => scope.limit_value, :param_name => Kaminari.config.param_name, :remote => false)
@@ -39,7 +40,9 @@ def paginate(scope, options = {}, &block)
def link_to_previous_page(scope, name, options = {}, &block)
params = options.delete(:params) || {}
param_name = options.delete(:param_name) || Kaminari.config.param_name
- link_to_unless scope.first_page?, name, params.merge(param_name => (scope.current_page - 1)), options.reverse_merge(:rel => 'previous') do
+ route_set = options.delete(:route_set)
+ url = (route_set || self).url_for params.merge(param_name => (scope.current_page - 1))
+ link_to_unless scope.first_page?, name, url, options.reverse_merge(:rel => 'previous') do
block.call if block
end
end
@@ -64,7 +67,9 @@ def link_to_previous_page(scope, name, options = {}, &block)
def link_to_next_page(scope, name, options = {}, &block)
params = options.delete(:params) || {}
param_name = options.delete(:param_name) || Kaminari.config.param_name
- link_to_unless scope.last_page?, name, params.merge(param_name => (scope.current_page + 1)), options.reverse_merge(:rel => 'next') do
+ route_set = options.delete(:route_set)
+ url = (route_set || self).url_for params.merge(param_name => (scope.current_page + 1))
+ link_to_unless scope.last_page?, name, url, options.reverse_merge(:rel => 'next') do
block.call if block
end
end
@@ -129,19 +134,14 @@ def page_entries_info(collection, options = {})
def rel_next_prev_link_tags(scope, options = {})
params = options.delete(:params) || {}
param_name = options.delete(:param_name) || Kaminari.config.param_name
+ route_set = options.delete(:route_set)
output = ""
-
- if !scope.first_page? && !scope.last_page?
- # If not first and not last, then output both links.
- output << '<link rel="next" href="' + url_for(params.merge(param_name => (scope.current_page + 1))) + '"/>'
- output << '<link rel="prev" href="' + url_for(params.merge(param_name => (scope.current_page - 1))) + '"/>'
- elsif scope.first_page?
- # If first page, add next link unless last page.
- output << '<link rel="next" href="' + url_for(params.merge(param_name => (scope.current_page + 1))) + '"/>' unless scope.last_page?
- else
- # If last page, add prev link unless first page.
- output << '<link rel="prev" href="' + url_for(params.merge(param_name => (scope.current_page - 1))) + '"/>' unless scope.first_page?
+ unless scope.last_page?
+ output << '<link rel="next" href="' + (route_set || self).url_for(params.merge(param_name => (scope.current_page + 1))) + '"/>'
+ end
+ unless scope.first_page?
+ output << '<link rel="prev" href="' + (route_set || self).url_for(params.merge(param_name => (scope.current_page - 1))) + '"/>'
end
output.html_safe
3  lib/kaminari/helpers/tags.rb
View
@@ -16,6 +16,7 @@ class Tag
def initialize(template, options = {}) #:nodoc:
@template, @options = template, options.dup
@param_name = @options.delete(:param_name)
+ @route_set = @options.delete(:route_set)
@theme = @options[:theme] ? "#{@options.delete(:theme)}/" : ''
@params = @options[:params] ? template.params.merge(@options.delete :params) : template.params
end
@@ -25,7 +26,7 @@ def to_s(locals = {}) #:nodoc:
end
def page_url_for(page)
- @template.url_for @params.merge(@param_name => (page <= 1 ? nil : page))
+ (@route_set || @template).url_for @params.merge(@param_name => (page <= 1 ? nil : page))
end
end
11 spec/fake_app/rails_app.rb
View
@@ -16,8 +16,19 @@
Rails.backtrace_cleaner.remove_silencers!
app.initialize!
+# fake engine
+module MyEngine
+ class Engine < ::Rails::Engine
+ isolate_namespace MyEngine
+ routes.draw do
+ resources :items
+ end
+ end
+end
+
# routes
app.routes.draw do
+ mount MyEngine::Engine, :at => '/mounted_engine'
resources :users
end
31 spec/helpers/action_view_extension_spec.rb
View
@@ -14,6 +14,13 @@
lambda { helper.escape_javascript(helper.paginate @users, :params => {:controller => 'users', :action => 'index'}) }.should_not raise_error
end
end
+
+ if defined? Rails
+ context 'using custom route set' do
+ subject { helper.paginate @users, :params => {:controller => 'my_engine/items', :action => 'index'}, :route_set => my_engine }
+ it { should match(%r(/mounted_engine/items)) }
+ end
+ end
end
describe '#link_to_previous_page' do
@@ -33,6 +40,12 @@
subject { helper.link_to_previous_page @users, 'Previous', :rel => 'external', :params => {:controller => 'users', :action => 'index'} }
it { should match(/rel="external"/) }
end
+ if defined? Rails
+ context 'using custom route set' do
+ subject { helper.link_to_previous_page @users, 'Previous', :params => {:controller => 'my_engine/items', :action => 'index'}, :route_set => my_engine }
+ it { should match(%r(/mounted_engine/items)) }
+ end
+ end
end
context 'the first page' do
before do
@@ -60,6 +73,12 @@
subject { helper.link_to_next_page @users, 'More', :rel => 'external', :params => {:controller => 'users', :action => 'index'} }
it { should match(/rel="external"/) }
end
+ if defined? Rails
+ context 'using custom route set' do
+ subject { helper.link_to_next_page @users, 'More', :params => {:controller => 'my_engine/items', :action => 'index'}, :route_set => my_engine }
+ it { should match(%r(/mounted_engine/items)) }
+ end
+ end
end
context 'the last page' do
before do
@@ -225,6 +244,15 @@
end
describe '#rel_next_prev_link_tags' do
+ shared_context 'using custom route set' do
+ if defined? Rails
+ context 'using custom route set' do
+ subject { helper.rel_next_prev_link_tags @users, :params => {:controller => 'my_engine/items', :action => 'index'}, :route_set => my_engine }
+ it { should match(%r(/mounted_engine/items)) }
+ end
+ end
+ end
+
before do
75.times {|i| User.create! :name => "user#{i}"}
end
@@ -237,6 +265,7 @@
it { should be_a String }
it { should match(/rel="next"/) }
it { should_not match(/rel="prev"/) }
+ include_context 'using custom route set'
end
context 'the middle page' do
before do
@@ -247,6 +276,7 @@
it { should be_a String }
it { should match(/rel="next"/) }
it { should match(/rel="prev"/) }
+ include_context 'using custom route set'
end
context 'the last page' do
before do
@@ -257,6 +287,7 @@
it { should be_a String }
it { should_not match(/rel="next"/) }
it { should match(/rel="prev"/) }
+ include_context 'using custom route set'
end
end
end
Something went wrong with that request. Please try again.